Add a DynSized trait #44469

plietar · 2017-09-09T21:41:22Z

(This depends on #44295)

The DynSized trait is implemented by all types which have a size and alignment
known at runtime. This includes every type other than extern types, introduced
in RFC 1861 and implemented in #44295, which are completely opaque.

The main motivation for this trait is to prevent the use of !DynSized types as
struct tails. Consider for example the following types :

extern {
  type foo;
}

struct A<T: ?Sized> {
    a_x: u8
    a_y: T
}

struct B<T: ?Sized> {
    b_x: u8
    b_y: T
}

Before this change, the type A<B<foo>> is considered well-formed. However,
the alignment of B<foo> and thus the offset of the a_y field depends on the
alignment of foo, which is unknown.

By introducing this new trait, struct tails are now required to implement
DynSized, such that their alignment is known. The trait is an implicit bound,
making A<B<foo>> ill-formed.

Just like the Sized trait, the default bound can be opted-out by using
?DynSized.

The trait also prevents the use of the size_of_val and align_of_val functions with extern types.

After discussion on IRC, the intention here is to include DynSized as an "implementation detail" for now just to fix extern types, and propose an RFC later to extend it / stabilize it. Until this is the case, generic functions/types can't be defined for extern types.

The implementation here is deliberately conservative. There's a number of open questions to be resolved by the future RFC. All the answers are no in the current implementation.

Should DynSized be in the prelude ?
Should all bounds in core/std be changed from ?Sized to ?DynSized (where applicable) ? For now I have only change the is_null and as_ref functions on raw pointers, since they should frequently be used with extern types.
Should !DynSized tails be allowed in structs, as long as they are never accessed ? struct A { x: u8, y: foo } could be accepted but a.y rejected, since getting the offset of y is the part that can't be done.
Should users be able to specify their own implementations of DynSized ? This would allow things like C strings (null terminated) and Pascal strings (length prefixed). This has some overlap with the "custom DST" proposal.

rust-highfive · 2017-09-09T21:41:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

plietar · 2017-09-09T22:10:06Z

src/test/compile-fail/const-size_of-cycle.rs

@@ -13,6 +13,7 @@
 struct Foo {
    bytes: [u8; std::mem::size_of::<Foo>()]
    //~^ ERROR unsupported cyclic reference between types/traits detected
+    //~^^ ERROR unsupported cyclic reference between types/traits detected


The error message is now thrown twice, once when checking if the field type is WF, and once when checking that it is DynSized.

I'm not sure how to prevent this, or whether it even matters.

plietar · 2017-09-09T23:53:25Z

Rustdoc output is not great with this (it displays the implicit DynSized bound everywhere, ie it will display <T: ?Sized> as <T: ?Sized + DynSized>).

I'm working on a fix for it, should I add it to this PR, or as a follow up one ?

retep998 · 2017-09-10T02:55:04Z

Personally I'd prefer if the last field of a struct could be a !DynSized type, which makes the struct itself !DynSized, ensuring that the struct cannot be moved or constructed. However, if given a pointer to such a struct, the user should still be able to access the fields of the struct. This is useful for COM for example, where the first field is known to be a pointer to a vtable, but the rest is a black box, and you can only ever work with an interface from behind a pointer.

arielb1 · 2017-09-10T09:32:25Z

@retep998

That would be a problem because of alignment concerns - you can't do the "DST align" thing without knowing the alignment.

However, we could allow it for repr(packed) structs (and potentially for repr(tail_align) structs). In which case you'll have to wrap your COM struct within a repr(packed) wrapper, which is "ugly, but works".

I don't want to add a new DynAligned trait (where Sized: DynAligned) trait in addition to DynSized, but I might have to.

retep998 · 2017-09-10T11:34:27Z

@arielb1 Why should it need to know the alignment if I have a pointer? The alignment of the !DynSized part can't lower the alignment of the pointer, it can only raise it, and accessing a pointer that is more aligned than you expect is completely safe.

EDIT: Hmmmmm, the alignment of the !DynSized part does affect its position in a struct that contains it however...

bors · 2017-09-11T01:08:47Z

☔ The latest upstream changes (presumably #44316) made this pull request unmergeable. Please resolve the merge conflicts.

plietar · 2017-09-11T16:54:31Z

@retep998 Yes, that's definitely a use case I'd like to support eventually. My goal was to keep the feature conservative for now, and expand with an RFC.

OTOH, there was already an RFC for extern types, so maybe opaque tails falls under that RFC. I'm happy to include it if there's consensus about it.

@arielb1 !DynSized tails are fine as long as you don't try to observe their address.

Something like

#[repr(C)]
struct Foo {
    x: u8,
    tail: Opaque, // OK
}

fn use_foo(foo: &Foo) -> &Opaque {
    &foo.tail // ERROR: Cannot access field `foo`, type `Opaque` doesn't implement `DynSized`
}

There are cases where it could be allowed, such as single field structs, #[repr(packed)] or with DynAligned, although I don't think it is very desirable.

pnkfelix · 2017-09-13T14:00:23Z

I'll try to look at this somewhat soon, but I assume the review should not be considered pressing until #44295 successfully lands, right?

eddyb · 2017-09-13T14:28:09Z

r? @arielb1

cc @rust-lang/compiler Does this need FCP?

arielb1 · 2017-09-13T14:31:58Z

I think this does

carols10cents · 2017-09-18T15:54:55Z

@arielb1 is this ready to be proposed to be fcped? if so, could you do that please? if not, what needs to be done first?

arielb1 · 2017-09-19T09:04:34Z

@rfcbot fcp merge

arielb1 · 2017-09-19T09:05:14Z

I don't think I can invoke rfcbot. I'll need to ask @nikomatsakis for help.

rfcbot · 2017-09-19T09:24:35Z

Zoxc · 2017-09-26T02:08:46Z

Note that this will likely break the spectral crate (and add more errors to some other crates). Here is a reduced test case:

use std::fmt::Debug;

trait DescriptiveSpec<'r> {}

impl<'r, T> DescriptiveSpec<'r> for &'r T {}

fn from_spec<'r, T: DescriptiveSpec<'r>>(spec: &'r T)  {}

fn matching_contains<'s, T: 's, I>(a: &mut &'s I) where &'s I: Debug {
    from_spec(a);
}

fn main() {}

See #21974 (comment)

It would also be a good idea to use crater on this.

arielb1 · 2017-09-26T17:07:05Z

@Zoxc

Did you check that spectral isn't fixed by the sized-constraint "hack"? This should still break your example, but I'm not sure how bad that is (that's why we want a crater run) - it "only" makes an existing bug worse (e.g. the Debug version can be fixed by having Debug: ?DynSized, and if Debug: Sized held it would have occurred even today)..

Zoxc · 2017-09-27T05:29:56Z

Also I suggest you remove the documentation on stage0, but keep it on not(stage0). This avoid duplication and problems with accidentally modifying the wrong one.

kennytm · 2017-09-28T14:53:57Z

@plietar Hi, could you resolve the merge conflict and fix the CI failure? Thanks.

[00:32:17] error[E0599]: no method named `as_str` found for type `syntax_pos::symbol::InternedString` in the current scope
[00:32:17]     --> /checkout/src/librustdoc/clean/mod.rs:1942:69
[00:32:17]      |
[00:32:17] 1942 |                 let path = external_path(cx, &cx.tcx.item_name(did).as_str(),
[00:32:17]      |                                                                     ^^^^^^
[00:32:17] 
[00:32:19] error: aborting due to previous error

Rustfmt needs support for extern types, and rls depends on rustfmt.

The DynSized trait is implemented by all types which have a size and alignment known at runtime. This includes every type other than extern types, introduced in RFC 1861 and implemented in rust-lang#44295, which are completely opaque. The main motivation for this trait is to prevent the use of !DynSized types as struct tails. Consider for example the following types : ```rust extern { type foo; } struct A<T: ?Sized> { a_x: u8 a_y: T } struct B<T: ?Sized> { b_x: u8 b_y: T } ``` Before this change, the type `A<B<foo>>` is considered well-formed. However, the alignment of `B<foo>` and thus the offset of the `a_y` field depends on the alignment of `foo`, which is unknown. By introducing this new trait, struct tails are now required to implement `DynSized`, such that their alignment is known. The trait is an implicit bound, making `A<B<foo>>` ill-formed. Just like the `Sized` trait, the default bound can be opted-out by using `?DynSized`.

aturon · 2017-09-29T20:00:28Z

Checking off for @nrc, who is away for some time.

@petrochenkov, are you OK with this landing?

petrochenkov · 2017-09-30T08:44:16Z

Oh, I didn't even notice it was pending on me.

rfcbot · 2017-09-30T09:05:31Z

🔔 This is now entering its final comment period, as per the review above. 🔔

bors · 2017-10-04T23:13:51Z

☔ The latest upstream changes (presumably #44901) made this pull request unmergeable. Please resolve the merge conflicts.

rfcbot · 2017-10-10T09:25:22Z

The final comment period is now complete.

shepmaster · 2017-10-15T15:26:52Z

Heads-up @plietar — you have merge conflicts that need to be addressed before this can progress.

alexcrichton · 2017-10-19T14:43:23Z

triage ping for @plietar! out of curiosity, any update on this?

carols10cents · 2017-10-30T15:49:39Z

I'm going to close this PR due to inactivity; looks like even though this has made it through FCP the merge conflicts need to be resolved and then a crater run needs to happen.

@plietar thank you for your contribution, please feel free to reopen this when you have a chance to resolve the merge conflicts! Let us know if you need any help!!

mikeyhew · 2017-11-04T20:01:25Z

Is there a discussion taking place somewhere on the bullet points @plietar outlined in the first comment on this PR?

rust-highfive assigned pnkfelix Sep 9, 2017

plietar commented Sep 9, 2017

View reviewed changes

plietar force-pushed the dynsized branch 2 times, most recently from e5c3032 to 4bf8bbb Compare September 9, 2017 23:36

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2017

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 2017

rust-highfive assigned arielb1 and unassigned pnkfelix Sep 13, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 19, 2017

arielb1 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2017

Zoxc mentioned this pull request Sep 27, 2017

Immovable types rust-lang/rfcs#1858

Closed

plietar added 2 commits September 29, 2017 17:21

Implement RFC 1861: Extern types

907c7a3

Mark rustfmt and rls as broken.

da549e1

Rustfmt needs support for extern types, and rls depends on rustfmt.

plietar force-pushed the dynsized branch from 4bf8bbb to 07b35cf Compare September 29, 2017 15:27

plietar added 4 commits September 29, 2017 20:24

Make <*const T>::is_null() and al work with T: ?DynSized.

2d3e2b5

Fix tests for DynSized

1eefab4

Add DynSized related tests

d51764d

plietar force-pushed the dynsized branch from 07b35cf to d51764d Compare September 29, 2017 18:24

Fix tidy errors

55c2522

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 30, 2017

pnkfelix added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 5, 2017

Zoxc mentioned this pull request Oct 8, 2017

Immovable types prototype where the Move trait is builtin #44917

Closed

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 9, 2017

carols10cents closed this Oct 30, 2017

carols10cents added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2017

mikeyhew mentioned this pull request Nov 20, 2017

Add DynSized trait (rebase of #44469) #46108

Closed

kennytm mentioned this pull request Dec 22, 2017

More implicit bounds (?Sized, ?DynSized, ?Move) rust-lang/rfcs#2255

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a DynSized trait #44469

Add a DynSized trait #44469

plietar commented Sep 9, 2017 •

edited

Loading

rust-highfive commented Sep 9, 2017

plietar Sep 9, 2017

plietar commented Sep 9, 2017

retep998 commented Sep 10, 2017

arielb1 commented Sep 10, 2017 •

edited

Loading

retep998 commented Sep 10, 2017 •

edited

Loading

bors commented Sep 11, 2017

plietar commented Sep 11, 2017 •

edited

Loading

pnkfelix commented Sep 13, 2017

eddyb commented Sep 13, 2017

arielb1 commented Sep 13, 2017

carols10cents commented Sep 18, 2017

arielb1 commented Sep 19, 2017

arielb1 commented Sep 19, 2017

rfcbot commented Sep 19, 2017 •

edited by petrochenkov

Loading

Zoxc commented Sep 26, 2017

arielb1 commented Sep 26, 2017 •

edited

Loading

Zoxc commented Sep 27, 2017

kennytm commented Sep 28, 2017 •

edited

Loading

aturon commented Sep 29, 2017

petrochenkov commented Sep 30, 2017

rfcbot commented Sep 30, 2017

bors commented Oct 4, 2017

rfcbot commented Oct 10, 2017

shepmaster commented Oct 15, 2017

alexcrichton commented Oct 19, 2017

carols10cents commented Oct 30, 2017

mikeyhew commented Nov 4, 2017

Add a DynSized trait #44469

Add a DynSized trait #44469

Conversation

plietar commented Sep 9, 2017 • edited Loading

rust-highfive commented Sep 9, 2017

plietar Sep 9, 2017

Choose a reason for hiding this comment

plietar commented Sep 9, 2017

retep998 commented Sep 10, 2017

arielb1 commented Sep 10, 2017 • edited Loading

retep998 commented Sep 10, 2017 • edited Loading

bors commented Sep 11, 2017

plietar commented Sep 11, 2017 • edited Loading

pnkfelix commented Sep 13, 2017

eddyb commented Sep 13, 2017

arielb1 commented Sep 13, 2017

carols10cents commented Sep 18, 2017

arielb1 commented Sep 19, 2017

arielb1 commented Sep 19, 2017

rfcbot commented Sep 19, 2017 • edited by petrochenkov Loading

Zoxc commented Sep 26, 2017

arielb1 commented Sep 26, 2017 • edited Loading

Zoxc commented Sep 27, 2017

kennytm commented Sep 28, 2017 • edited Loading

aturon commented Sep 29, 2017

petrochenkov commented Sep 30, 2017

rfcbot commented Sep 30, 2017

bors commented Oct 4, 2017

rfcbot commented Oct 10, 2017

shepmaster commented Oct 15, 2017

alexcrichton commented Oct 19, 2017

carols10cents commented Oct 30, 2017

mikeyhew commented Nov 4, 2017

plietar commented Sep 9, 2017 •

edited

Loading

arielb1 commented Sep 10, 2017 •

edited

Loading

retep998 commented Sep 10, 2017 •

edited

Loading

plietar commented Sep 11, 2017 •

edited

Loading

rfcbot commented Sep 19, 2017 •

edited by petrochenkov

Loading

arielb1 commented Sep 26, 2017 •

edited

Loading

kennytm commented Sep 28, 2017 •

edited

Loading