-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Hierarchy of Sized traits #3729
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: León Orell Valerian Liehr <[email protected]>
imo having an associated type on practically every type will be annoying, so I think it might be useful to require writing |
the trait's unstable, if it's just a matter of bikeshedding we can rename the type |
I've added this as one of the unresolved questions.
I've rewritten this section, it should have been updated earlier with the addition of the alternatives which keep the
I haven't made these changes at the moment - I've had good feedback about the explanations in the RFC and would prefer not to make major changes to that until the proposal itself needs changing. |
I've updated the previous summary comment so it is still accurate. |
I've pushed one small change with a reference to the now-upstream const traits RFC in #3762. |
Linux has at its syscall boundary There is even more exotic case: Note: manpage says that size of last field is 256. This is a lie. In fact, this is DST, and it can be both bigger and smaller than 256. Also, the string in the last field is always proper null-terminated C string, so we have two different ways of determining struct size: by calling
Not supporting both features will lead to horrible hacks, such as this: https://github.com/rust-lang/rust/blob/c1132470a6986b12503e8000e322e9164c1f03ac/library/std/src/sys/pal/unix/fs.rs#L730-L768 (this is from standard library!) And again: to support |
In the latest version, this hack is mostly gone. What remains is not so bad: https://github.com/rust-lang/rust/blob/58d6301cad7ac20407e708cd9d1ec499d1804015/library/std/src/sys/pal/unix/fs.rs#L726-L753 IMO it is not worth contorting the language to natively express esoteric types such as those, as long as we have some way of interfacing with C code using such types (and we do). |
Another summary comment! Here are the previous update summaries copied below so you don't need to uncollapse GitHub comments to find it: 2024-11-25Summary comment: #3729 (comment) For those following along or catching up, these are the notable the changes to the RFC since this was posted:
And these are all the other smaller changes that don't materially impact what is being proposed:
At the moment, I prefer the following alternatives to the primary proposal of the RFC, and may re-write to incorporate these as the primary proposal:
2024-12-04Summary comment: #3729 (comment) These are the major changes since the last summary:
These are the minor changes since the last summary:
These are the alternatives described in the RFC that I think are worth consideration as the primary proposal:
We had a design meeting with the language team yesterday (Feb 5th) which led to the changes described below, a rough summary:
I've started an implementation of this - I've more-or-less finished implementing the non-const parts of the proposal, and will proceed with the const parts. It won't land upstream until const traits implementors are okay with it. These are links to the compare with previous versions of the document (switch to the "Files changed" tab and then the rich diff):
These are the major changes since the last summary:
These are the minor changes since the last summary: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the summary comments they're incredibly useful. I've re-read this and looked through the updates and this proposal feels closer, however I still have a few comments...
Positive trait bounds are growing on me though, I like that they stop us having to worry about what "the empty set of traits" means, however I'm not convinced that's possible, because of supertrait bounds (as described in a comment).
|
||
This property trivially falls out of the previous proposals in this RFC: | ||
runtime-sized types are `Sized` (but not `const Sized`) and therefore can | ||
implement `Clone` and `Copy`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this RFC is implicitly dependent on unsized locals here, the only way I can see the Clone or Copy traits supporting Sized
types is with it. Consider <svint8_t as Clone>::clone
that returns an svint8_t but that doesn't have a stack size known at compile time, so you need a bunch of the unsized locals machinery to support it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just chatted at Rust Nation about this. My understanding of the answer to this is: in some sense yes, because locals/arguments/return types are no longer const Sized, but it doesn't depend on the full power of unsized locals (as they're Sized) and it's mostly a case of punting the problem to the backend and LLVM already handles this just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify further - these types don't need unsized_locals
machinery - not the skipped Sized
constraints in the front of the compiler, nor the MIR/codegen changes in the back of the compiler - they just emit different LLVM types and LLVM handles the rest.
``` | ||
|
||
Once this interaction with constness was realised, this RFC's dependency on const | ||
traits was introduced. Due to const traits (at the time of writing this RFC) being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like surprisingly weak motivation for all the const stuff that's making this hierarchy complicated. I wonder if we can replace Clone
with:
trait Clone {
fn clone(&self) -> Self;
fn clone_from(&mut self, other: &Self) where Self: Sized { ... }
}
and replace Clone bounds with Clone + Sized
, admittedly this is more edition shenanigans that feels a bit dirty (and I haven't completely thought it through).
I do agree that having const trait stuff would give you the ability to do some stuff: size_of
on non-const Sized types, and have arrays&slices of non-const Sized types (although this would require some complier work). I'm just not clear how useful those are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I agree the const stuff is making the hierarchy complicated, just the nature of these types. Const traits are a natural fit for this and if they were already an accepted and stable part of the language then I don't think there would be any concerns about using them for this proposal.
At a first glance, changing Clone
like you've suggested seems plausible but I don't think I'll add it as an alternative because of the additional advantages that using const traits give us that changing Clone
wouldn't, I think those are worth it - these types can't be used in const contexts and we need to be able to prevent that from happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's reasonable, I am worried that all of these const traits add too many variations here but you're right that they are correctly representing the types. I was mostly just trying to keep alive the notion that we don't have to perfectly represent all possible variations and if that makes the proposal significantly simpler then it might be a good idea.
which is now equivalent to a `MetaSized` bound in non-`const fn`s and | ||
`~const MetaSized` in `const fn`s and will be deprecated in the next edition. | ||
|
||
Traits now have an implicit default bound on `Self` of `const MetaSized`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still very nervous about this, I believe this is done solely for backwards compatibility reasons but I want to make sure it's clear this is sacrificing forwards compatibility.
If traits gain (debatably keep) an implicit const MetaSized
bound then:
- Everything keeps working in a nice way
- People writing libraries will just write
trait Foo {}
rather thantrait Foo: Pointee {}
unless they're specifically thinking about unsized types for some reason making them a second class citizen in the ecosystem (and relaxing the bound when asked is a breaking change) - If we ever add another layer below
Pointee
(eg externref) then no existing traits will work with them and it would be breaking to relax them. I think this means no std traits could ever support them.
If traits have no implicit bound (also debatably keep, depending on how you imagine this) then:
- You'd have to do this over an edition, and add in
: const MetaSized
everywhere so that default method implementations keep working. (removing that bound is safe for std, and dubious for crates). - You'd need to write
: Pointee
etc if you need to write default method impls, but I think that's a good thing. - This does force us to work out what you can do with "the empty set of traits", because that what these types have in default trait impls (with no supertraits) (this makes me less favourable towards positive bounds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd have to do this over an edition, and add in
: const MetaSized
everywhere so that default method implementations keep working. (removing that bound is safe for std, and dubious for crates).
I'm open to this as an alternative to be considered.
| `Pointee` | `T: Pointee` | `T: ?(const Sized)` | | ||
|
||
In other words, `?(const Sized)` fully opts out of all default bounds and then one has to | ||
explicitly opt back in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was straw-manned slightly in the design meeting, I disagree with the phrasing:
'I don't want to think of the "next most precise thing"'
and
"one level below" in the hierarchy
Neither proposal makes you think like that (ignoring the ?MetaSized
below), the ?Sized
variant requires you to write the positive bound always (and I'd argue that the T: Pointee
case should be written T: ?(const Sized) + Pointee
) so the only negative thinking you have to do is "do I need to disable the default const Sized
bound?".
However, I agree this discussion can be punted until stabilisation given it has essentially no impact on the semantics of what's going on.
P.S The idea of negative trait aliases (trait Foo = ?Sized;
) terrifies me, it feels like it'd be incredibly confusing at the use-site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither proposal makes you think like that (ignoring the ?MetaSized below), the ?Sized variant requires you to write the positive bound always (and I'd argue that the T: Pointee case should be written T: ?(const Sized) + Pointee) so the only negative thinking you have to do is "do I need to disable the default const Sized bound?".
I don't think there's a realistic chance of the "Keeping only ?Sized
" variant being accepted, it's very verbose and I don't think it is being seriously considered.
"Adding ?MetaSized
" is feasible but it does have the "opt-out of the next strictest thing" behaviour. This approach also has confusing behaviour with respect to stability, as you don't name the trait that you want and refer to it by another trait that has different stability. ?Trait
bounds are also just seen as confusing in general.
I obviously can't speak for the language team, but I'm not aware that anyone on the team currently prefers a variant with negative bounds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pushback @Skepfyr. It's true that we didn't explicitly discuss the idea of writing ?Sized
to "reset" and then a positive bound in combination with it. It's also true that this proposal has many of the advantages of positive bounds — but it's also true that it's very verbose and (in my judgment) not the right way forward for that reason. There is definitely a judgment call to be made here, my judgment is that having a small set of "well known" traits that, instead of declared your intent to invoke methods wind up declaring your intent to use it in a more limited way will "wear better" in the long run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my worry is that "small set of "well known" traits" won't be true, with trait aliases and potentially supertrait bounds relaxing the default bound. I agree that T: ?(const Sized) + Pointee
is horrifically verbose, but if it's just syntax then maybe we could just fix that?
There's nothing stopping us from doing something like one of these:
fn foo<T: ? + Pointee>() {} // Just drop the (const Sized)
fn foo<T: {} + Pointee>() {} // {} to imply the empty set
fn foo<T: unsized + Pointee>() {} // unsized is a keyword! (probably my favourite)
fn foo<unsized T: Pointee>() {} // Might be nicer here
a `const MetaSized` bound. As the `?Trait` syntax is currently accepted for any trait | ||
but ignored for every trait except `Sized`, `?MetaSized` and `?Pointee` bounds would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being altered by:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I'll update the wording here.
Traits which are a supertrait of any of the proposed traits will not | ||
automatically imply the proposed trait in any bounds where the trait is | ||
used, e.g. | ||
|
||
```rust | ||
trait NewTrait: MetaSized {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traits which are a supertrait of any of the proposed traits will not | |
automatically imply the proposed trait in any bounds where the trait is | |
used, e.g. | |
```rust | |
trait NewTrait: MetaSized {} | |
Traits which are a subtrait of any of the proposed traits will not | |
automatically imply the proposed trait in any bounds where the trait is | |
used, e.g. | |
```rust | |
trait NewTrait: MetaSized {} |
Traits which are a supertrait of any of the proposed traits will not | ||
automatically imply the proposed trait in any bounds where the trait is | ||
used, e.g. | ||
|
||
```rust | ||
trait NewTrait: MetaSized {} | ||
|
||
struct NewRc<T: NewTrait> {} // equiv to `T: NewTrait + Sized` as today | ||
``` | ||
|
||
If the user wanted `T: MetaSized` then it would need to be written explicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true in just the subtractive direction or in the additive direction as well? I.e., this works today:
trait Tr: Sized {}
fn f<T: ?Sized + Tr>() {
_ = const { size_of::<T>() }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only apply in the subtractive direction (at least w/out the changes you're proposing in Zulip and referencing here). I can clarify this.
This is forward compatible with trait bounds which have sizedness supertraits | ||
implying the removal of the default `const Sized` bound. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it? I mean, we accept this today:
trait Tr /* : ?Sized */ {}
trait Id {
type Ty: Tr;
}
fn f<T: Id>() {
_ = const { size_of::<T::Ty>() }
}
If we were to use the supertrait's ?Sized
bound to imply the removal of the default Sized
bound on the associated type, then this would break. It would seem the same could be said of the new things added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could still be done over an edition, adding + Sized
to T: Id
when we make this change so existing code doesn't break. It's certainly nicer to do it all at once and avoid that, I agree, but I do think it's technically forward compatible. I'm not opposed to making a change like this one as a pre-requisite to this RFC's proposed changes being stabilised, I just think it should be a separate proposal to this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a matter of terminology, we generally don't consider a change that requires an edition to be a backward compatible change. We just say, e.g., that it is a "change we could make over an edition". Probably the language should be updated here to reflect this.
Specifically, I think the outcome we arrived at is still that we only get one shot, when we stabilize these new bounds, to do this in a way that would work consistently across all editions. While we could later make things work differently in a new edition, and do a migration dance, that's something we'd generally avoid doing if we already knew we wanted a different behavior.
In terms of the dependencies here, I'm increasingly not a fan of us accepting RFCs with big open questions like this. Doing so ends up causing us to later stall the stabilization or the work toward it, causing pain. It's better if we can really nail down the important details, especially those big ones that meaningfully affect how the user would experience the feature. That way there's a clear path from RFC acceptance to stabilization.
So, it's OK if here we don't want to include this in this RFC, but I'd prefer to see us decide this question ahead of accepting this RFC, and if we decide that we'd like a behavior that isn't fully specified in this RFC, that we then accept another RFC for that ahead of accepting this one, so this one can be updated accordingly and reference it.
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- What names should be used for the traits? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a note for my fellow @rust-lang/lang members, I'd prefer if we did not leave this question (and perhaps some of the other bikeshed questions in this list) unresolved.
This is in the context of our ongoing discussions about streamlining our feature development processes.
It's always tempting for us to defer these sort of things at RFC time to let work go ahead, but this turns into a false economy, because we then end up getting overloaded and blocking it later, at a worse time. I'd propose that we just commit to the bikeshed now.
We can always later change our minds, of course, with good cause. But we should settle on a presumptive set of choices as part of accepting this RFC so that it has an unencumbered path to stabilization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light of the strong preference towards positive bounds, I'd like to leave one last comment that I believe it would be very helpful to have a visual indicator for bounds that relax requirements rather than adding them.
Moving from <T>
to <T: Pointee>
(or where T: {many bounds}
to T: {many bounds} + Pointee
) appears to be a breaking change. IMO this will cause significant confusion for library owners, especially as there are several different traits that have this new behavior. This issue could be resolved by introducing a keyword like <T: keyword Pointee>
, e.g. only Pointee
, just Pointee
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue could be resolved by introducing a keyword like
<T: keyword Pointee>
, e.g.only Pointee
,just Pointee
.
an existing keyword that might work: <T: become Pointee>
as
or =
or maybe @
could also work
All of Rust's types are either sized, which implement the
Sized
trait and have a statically known size during compilation, or unsized, which do not implement theSized
trait and are assumed to have a size which can be computed at runtime. However, this dichotomy misses two categories of type - types whose size is unknown during compilation but is a runtime constant, and types whose size can never be known. Supporting the former is a prerequisite to stable scalable vector types and supporting the latter is a prerequisite to unblocking extern types. This RFC proposes a hierarchy ofSized
traits in order to be able to support these use cases.This RFC relies on experimental, yet-to-be-RFC'd const traits, so this is blocked on that. I haven't squashed any of the previous revisions but can do so if/when this is approved. Already discussed in the 2024-11-13 t-lang design meeting with feedback incorporated.
See this comment for the most recent summary of changes to this RFC since it was opened.
Rendered