Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Aligned trait #3319

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

Jules-Bertholet
Copy link
Contributor

@Jules-Bertholet Jules-Bertholet commented Sep 24, 2022

Rendered

Add an Aligned trait to core::marker, as a supertrait of Sized.

Implementation

@Jules-Bertholet Jules-Bertholet force-pushed the aligned-trait branch 3 times, most recently from 3a78602 to 7320191 Compare September 24, 2022 16:38
@joshtriplett joshtriplett added the A-alignment Proposals relating to alignment. label Sep 24, 2022
@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Sep 24, 2022
@programmerjake
Copy link
Member

programmerjake commented Sep 25, 2022

I also suggested having Aligned since it would allow offset_of! to work on struct fields that are slices/str (aligned but unsized):
#3308 (comment)
#3308 (comment)

@thomcc
Copy link
Member

thomcc commented Sep 25, 2022

Yeah, this would be nice for offset_of!. I don't know if it's worth the additional language complexity, but unlike most proposals that extend ?Foo traits, this seems fairly minimal

@clarfonthey
Copy link
Contributor

So, one slight callout that I think might be worth considering for a future edition.

I think that ?Sized + Aligned is rather weird, and I think that we should have this be the meaning of ?Sized and ?Aligned opt out of both. However, this would break a lot of existing code, which is why I recommend it happening at an edition boundary.

I also think that ?Aligned should work on all editions to be equivalent to ?Sized today.

However, this is just my opinion on the matter, and changing everyone's muscle-memory for ?Sized might not be worth it. In this case, we would just make ?Aligned have an auto-fix for ?Sized.

There may have also been discussion on this already that I didn't see in the RFC, in which case that's fine and we should include it. But I think we should at least have some stance on this.

@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Sep 25, 2022

Discussion of ?Aligned added to the RFC. As stated there, I don't think we should add ?Aligned to the current edition, it would just be redundant and cause even more confusion. I have no objection to doing so in a later edition, but I've left that as a future possibility for now.

@thomcc
Copy link
Member

thomcc commented Sep 25, 2022

I think having the "only aligned" version be ?Sized + Aligned is a nice way around the compatibility change. Using ?Aligned doesn't seem worth the churn that it would take to me.

@clarfonthey
Copy link
Contributor

Yeah, it's entirely reasonable to just say it's not worth the effort, although I think it might be worth considering for completeness. I personally think it would be a positive change as a way of pushing people to think more about what kinds of types they want to allow in their APIs, but it could also be another barrier to learning the language for others. I just want to make sure that it was mentioned in the discussion so people can think about whether they prefer ?Aligned and ?Sized versus ?Sized and ?Sized + Aligned.

@Jules-Bertholet
Copy link
Contributor Author

Another factor in this discussion is that new opt-out traits might be added in the future (like DynSized). That's why I don't think we should make decisions about syntax in future editions now; we don't know how the other proposed traits will play out and I want this RFC to remain independent of those discussions.

@Jules-Bertholet
Copy link
Contributor Author

Added note about offset_of: if Rust ever gets structs with multiple unsized fields, these could be Aligned but not fully support offset_of.

@madsmtm
Copy link
Contributor

madsmtm commented Sep 30, 2022

I'd like to see some consideration on how this interacts with the pointer metadata APIs? In particular, it is being considered in rust-lang/rust#81513 to add Thin as a supertrait of Sized.

@programmerjake
Copy link
Member

I'd like to see some consideration on how this interacts with the pointer metadata APIs? In particular, it is being considered in rust-lang/rust#81513 to add Thin as a supertrait of Sized.

I would expect Aligned to just be another supertrait of Sized and not be a super/subtrait of Thin, there are fat Aligned types such as [T] and Thin + Aligned types that we may get in the future:

#[repr(C)]
struct PascalString {
    len: u8,
    #[thin]
    data: [u8],
}

There are also fat !Aligned types such as dyn Send and Thin + !Aligned types such as extern types.

@Jules-Bertholet
Copy link
Contributor Author

Currently, this RFC leaves adding Aligned to a future edition's prelude as a future possibility. But Aligned has no associated methods or items, so I think adding it to every edition's prelude actually would not break any code. Or am I wrong about that?

@clarfonthey
Copy link
Contributor

Currently, this RFC leaves adding Aligned to a future edition's prelude as a future possibility. But Aligned has no associated methods or items, so I think adding it to every edition's prelude actually would not break any code. Or am I wrong about that?

Prior art for this: Unpin is in every prelude for presumably similar reasons. So, it's not off the table, but I'm sure some folks would like to do a crater run beforehand.

Also expand on back-compat issues in Future Possibilities
@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Oct 10, 2022

I've proposed that Aligned be added to the prelude immediately.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 11, 2022

@Jules-Bertholet can you elaborate on what is enabled by this trait? The RFC says

Data structures and containers that wish to store unsized types are easier to implement if they can produce a dangling, well-aligned pointer to the unsized type. Being able to determine a type's alignment without a value of the type allows doing this.

but I was hoping for a more detailed example, or a link.

@Jules-Bertholet
Copy link
Contributor Author

@nikomatsakis I've added a link to unsized-vec, which is the crate that originally motivated me to write this RFC. unsized-vec provides UnsizedVec<T>, which is essentially equivalent to Vec<T>, except that T has no Sized bound.

The specific "dangling pointer" use-case that the RFC originally cited turns out to have a much simpler solution that doesn't require Aligned, but unsized-vec uses (an imperfect library emulation of) Aligned in many other ways. Basically, non-Aligned types require a lot of complicated special handling, and being able to skip that when it's not necessary allows both better performance and better APIs.

@Jules-Bertholet
Copy link
Contributor Author

unsized-vec has been entirely rewritten since this RFC was first posted. Compare aligned.rs to unaligned.rs to see how the Aligned trait is used.

@ZhennanWu
Copy link

ZhennanWu commented Jan 19, 2023

This may be off-topic but I would still want point out a possible extension for the alignment of the unsized types.

Currently custom DST has this problem of having to dynamically calculate the offset for its unsized field due to its unknown alignment. (rust-lang/rust#106683 (comment)). However, from a zero-cost abstraction point of view, if we can somehow guarantee the alignment of the unsized type to be the same value or to be a large enough value (which is quite common), this offset calculation should not be necessary.

This would involve complex interactions with the type representations and I don't expect it to come with this RFC. But still worth to explore for custom DST.

@CAD97
Copy link

CAD97 commented Jan 19, 2023

See also the lang-team notes around DynSized (disclaimer: that I authored). In those notes I define four classes of types:

  • "T: Sized + MetaSized + DynSized", where the size and alignment are known statically;
  • "T: ?Sized + MetaSized + DynSized", where the size and alignment are known from the pointee metadata1;
  • "T: ?Sized + ?MetaSized + DynSized", where the size and alignment may require reading the pointee; and
  • "T: ?Sized + ?MetaSized + ?DynSized", where the size and alignment cannot be determined by (generic) code.

Aligned is interesting because it makes the correct observation that splitting discussion of knowing size from knowing alignment is useful. I am extremely uncertain whether making this split for the other two classes of knowable layout (requires metadata, requires reading) is ever useful, or if it's ever useful to make size easier to know than alignment, but this is something to consider in the future. Plus, as ZhennanWu notes, knowing constant alignment can potentially improve code generation for dynamically sized (but constant aligned) types (though, since Aligned isn't object-safe, it'd need to be Aligned<4> or similar to be used for trait objects, and constant folding handles the meta-sized-but-actually-constant just fine).

Aligned is also interesting because -- unlike DynSized -- it doesn't need to introduce a new ?Bound into the language. Instead, ?Sized serves to opt-out of both Sized and Aligned bounds, and if you want just Aligned you write ?Sized + Aligned. I believe MetaSized is the same way (size_of_val gets a readable reference, and the unstable size_of_val_raw is carefully future-proofed to unsafely restrict itself to MetaSized types (plus extern type's current MetaSized bodge)), but DynSized does relax further than ?Sized relaxes today.


I have a few suggestions for things I think the RFC should note:

  • Note ?DynSized as a future extension of talking about unsized types, even if only to note that Aligned has little/no implications on.
  • Note as an alternative making Aligned behave like any other not-object-safe bound and be future incompatible with dyn trait, rather than giving it the Sized magic which only applies to a Sized supertrait.
  • Note as a future/alternative allowing a type to be Sized + ?Aligned, or argue that this is a nonuseful capability2.

It's not necessary to add these, but I think it's an improvement to mention these explicitly.


off-topic

if we can somehow guarantee the alignment of the unsized type to be the same value or to be a large enough value (which is quite common)

This can't be done in general, because we allow increasing alignment arbitrarily high. However, it could be allowed to say trait MyTrait: Aligned<16> or similar, such that all implementers of MyTrait are known to be aligned to 16.

It's not possible to increase alignment for the trait object but not for the concrete type, though, or at least not without breaking the ability to actually create that trait object via unsizing, because it's necessary to be able to go from &WithTail<u8> to &WithTail<dyn MyTrait> that this doesn't change the pointee layout at all.


Footnotes

  1. The notes don't make a decision on whether MetaSized is allowed to know the address of the pointee. Given Rust types are generally address-insensitive, and using an unsized type as a tail field requires knowing alignment in order to even get the data pointer, I'm currently of the belief that giving access to the data pointer without permission to read it is an unnecessarily exotic use case. I can't think of a case which wouldn't make it unsound to own an instance of the type in a Rust allocated object.

  2. FWIW, I do think it's reasonable to say that if you know the size you should be able to know the required alignment as well. The only benefit I can see to knowing size without alignment is that SB retagging can retag all of the bytes only knowing size. Thus the fully complete DynSized graph would give a default (?OptOut) bound of Sized + DynSized for generics, a default bound of DynSized for traits, and Sized: Aligned + MetaSized, Aligned: MetaAligned, MetaSized: MetaAligned + DynSized, MetaAligned: DynAligned, DynSized: DynAligned, and DynAligned: {nothing}.

@Jules-Bertholet
Copy link
Contributor Author

@CAD97 I've adressed your comments.

@Jules-Bertholet
Copy link
Contributor Author

With the offset_of! RFC merged, I've moved support for said macro from a future possibility to the core proposal.

@WaffleLapkin
Copy link
Member

As a datapoint: we actually have an Aligned trait in the compiler internals:
https://github.com/rust-lang/rust/blob/f9a6b71580cd53dd4491d9bb6400f7ee841d9c22/compiler/rustc_data_structures/src/aligned.rs#L22
(it's a bit different from this rfc due to not having language support, but has the same role)

@nikomatsakis
Copy link
Contributor

@Jules-Bertholet we discussed this in the @rust-lang/lang planning meeting and we were concerned that the RFC didn't really have much for motivation. @workingjubilee mentioned that there was strong motivation, but it wasn't really present in the RFC. Do you think you could update it?

@Jules-Bertholet
Copy link
Contributor Author

I've added an explanation of how unsized-vec uses Aligned to the motivation section.

Co-authored-by: Yotam Ofek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-alignment Proposals relating to alignment. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
Status: Rejected/Not lang
Development

Successfully merging this pull request may close these issues.