-
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]>
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.
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.
I don't call this backwards compatible in the RFC, only "forward compatible", to mean that making a change like this now or later is still possible with this RFC - which is true, we can still make a change like this (w/out an edition now or w/ an edition later) if this RFC is accepted.
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.
Right, that's still the nit though. If starting from some state X, the change Y would not be a backward compatible change, then X is not forward compatible with Y. If we stabilize these new bounds, then we can't backward-compatibly change the meaning, so stabilizing these bounds is not forward compatible with changing the meaning.
It's of course OK to mention that and how this could be changed over an edition.
# 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
Pushed small updates following above comments and updated previous summary comment. |
`rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we coming up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
`rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we come up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
`rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we come up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
…=lcnr codegen_llvm: avoid `Deref` impls w/ extern type `rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we come up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
…, r=compiler-errors trait_sel: resolve vars in host effects In the standard library, the `Extend` impl for `Iterator` (specialised with `TrustedLen`) has a parameter which is constrained by a projection predicate. This projection predicate provides a value for an inference variable but - if the default bound is `const Sized` instead of `Sized` - host effect evaluation wasn't resolving variables first. Added a test that doesn't depend on a rust-lang/rfcs#3729 implementation. Adding the extra resolve can the number of errors in some tests when they gain host effect predicates, but this is not unexpected as calls to `resolve_vars_if_possible` can cause more error tainting to happen.
…r=compiler-errors hir_analysis: skip self type of host effect preds in variances_of Discovered as part of an implementation of rust-lang/rfcs#3729 - w/out this then when introducing const trait bounds: many more interesting tests change with different output, missing errors, new errors, etc related to this but they all depend on feature flags and are much more complex than this test. r? `@oli-obk`
…=lcnr codegen_llvm: avoid `Deref` impls w/ extern type `rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we come up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
…, r=compiler-errors trait_sel: resolve vars in host effects In the standard library, the `Extend` impl for `Iterator` (specialised with `TrustedLen`) has a parameter which is constrained by a projection predicate. This projection predicate provides a value for an inference variable but - if the default bound is `const Sized` instead of `Sized` - host effect evaluation wasn't resolving variables first. Added a test that doesn't depend on a rust-lang/rfcs#3729 implementation. Adding the extra resolve can the number of errors in some tests when they gain host effect predicates, but this is not unexpected as calls to `resolve_vars_if_possible` can cause more error tainting to happen.
…r=compiler-errors hir_analysis: skip self type of host effect preds in variances_of Discovered as part of an implementation of rust-lang/rfcs#3729 - w/out this then when introducing const trait bounds: many more interesting tests change with different output, missing errors, new errors, etc related to this but they all depend on feature flags and are much more complex than this test. r? ``@oli-obk``
Rollup merge of rust-lang#137613 - davidtwco:const-traits-variances, r=compiler-errors hir_analysis: skip self type of host effect preds in variances_of Discovered as part of an implementation of rust-lang/rfcs#3729 - w/out this then when introducing const trait bounds: many more interesting tests change with different output, missing errors, new errors, etc related to this but they all depend on feature flags and are much more complex than this test. r? ``@oli-obk``
Rollup merge of rust-lang#137604 - davidtwco:host-effect-resolve-vars, r=compiler-errors trait_sel: resolve vars in host effects In the standard library, the `Extend` impl for `Iterator` (specialised with `TrustedLen`) has a parameter which is constrained by a projection predicate. This projection predicate provides a value for an inference variable but - if the default bound is `const Sized` instead of `Sized` - host effect evaluation wasn't resolving variables first. Added a test that doesn't depend on a rust-lang/rfcs#3729 implementation. Adding the extra resolve can the number of errors in some tests when they gain host effect predicates, but this is not unexpected as calls to `resolve_vars_if_possible` can cause more error tainting to happen.
Rollup merge of rust-lang#137603 - davidtwco:extern-types-no-deref, r=lcnr codegen_llvm: avoid `Deref` impls w/ extern type `rustc_codegen_llvm` relied on `Deref` impls where `Deref::Target` was or contained an extern type - in my experimental implementation of rust-lang/rfcs#3729, this isn't possible as the `Target` associated type's `?Sized` bound cannot be relaxed backwards compatibly (unless we come up with some way of doing this). In later pull requests with the rust-lang/rfcs#3729 implementation, breakage like this could only occur for nightly users relying on the `extern_types` feature. Upstreaming this to avoid needing to keep carrying this patch locally, and I think it'll necessarily need to change eventually.
An initial implementation of this is available at rust-lang/rust#137944 |
- Allow `std::ptr::Pointee` to be implemented manually on user types, which would | ||
replace the compiler's implementation. |
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.
normal auto traits (such as Send and Sync) can be explicitly implemented, the intent of this RFC seems to be that these new traits cannot be (similar to Sized
). This should be explicitly noted.
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