Add a DynSized trait

[plietar]

(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{typefoo;}structA<T: ?Sized>{a_x:u8
a_y:T}structB<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]

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]

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]

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]

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]

@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]

@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]

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

[plietar]

@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)]structFoo{x:u8,tail:Opaque,// OK}fnuse_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]

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]

r? @arielb1

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

[arielb1]

I think this does

[carols10cents]

@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]

@rfcbot fcp merge

[arielb1]

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

[rfcbot]

Team member @arielb1 has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Aatch
• @arielb1
• @eddyb
• @estebank
• @jseyfried
• @michaelwoerister
• @nikomatsakis
• @nrc
• @petrochenkov
• @pnkfelix

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Zoxc]

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;traitDescriptiveSpec<'r>{}impl<'r,T>DescriptiveSpec<'r>for&'rT{}fnfrom_spec<'r,T:DescriptiveSpec<'r>>(spec:&'rT){}fnmatching_contains<'s,T:'s,I>(a:&mut&'sI)where&'sI:Debug{from_spec(a);}fnmain(){}

See #21974 (comment)

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

[arielb1]

@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]

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]

@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

[aturon]

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

@petrochenkov, are you OK with this landing?

[petrochenkov]

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

[rfcbot]

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

[bors]

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

[rfcbot]

The final comment period is now complete.

[shepmaster]

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

[alexcrichton]

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

[carols10cents]

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]

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