-
Notifications
You must be signed in to change notification settings - Fork 14k
Replace #[rustc_do_not_implement_via_object] with #[rustc_dyn_incompatible_trait]
#148637
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
base: main
Are you sure you want to change the base?
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I know it says "DO NOT MERGE YET". Is it ready for review? If not, can you convert it to a draft PR? Also, you might as well squash the tidy commits, there's no need for them to be separate. |
5344d24 to
95686b6
Compare
This comment has been minimized.
This comment has been minimized.
#[rustc_do_not_implement_via_object] with #[rustc_dyn_incompatible_trait]#[rustc_do_not_implement_via_object] with #[rustc_dyn_incompatible_trait]
Yes, it is ready for review. I added that because I have questions in the PR description that should be addressed (and the commits amended based on the answers) before merging. |
|
Making |
|
☔ The latest upstream changes (presumably #148753) made this pull request unmergeable. Please resolve the merge conflicts. |
…tible_trait], which makes the marked trait dyn-incompatible. Removes the attribute from `MetaSized` and `PointeeSized`. `dyn MetaSized` is a perfectly cromulent type, and seems to only have had #[rustc_do_not_implement_via_object] so the builtin object candidate would not overlap with the builtin MetaSized impl that all `dyn` types get. Resolves this by checking `is_sizedness_trait` where the trait solvers previously checked `implement_via_object`. (is this necessary?) `dyn PointeeSized` alone is rejected for other reasons (since `dyn PointeeSized` is considered to have no trait because `PointeeSized` is removed at an earlier stage of the compiler), but `(dyn PointeeSized + Send)` is valid and equivalent to `dyn Send`.
95686b6 to
eab8bbb
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
This PR is really out of my wheelhouse. @compiler-errors reviewed a precursor (#134573) but I don't know if he's reviewing PRs right now, so... r? compiler |
|
r? types probably is a better assignment |
May fix #148615
This affects three groups of traits:
MetaSized#[rustc_do_not_implement_via_object]. Still dyn-compatible after this change. See question 1 belowPointeeSized#[rustc_do_not_implement_via_object]. It doesn't seem to have been doing anything anyway (playground), sincePointeeSizedis removed before trait solving(?).#[rustc_do_not_implement_via_object](includingPointee,Tuple,Destruct, etc)#[rustc_dyn_incompatible_trait]and are now dyn-incompatible.Questions before merge:
dyn MetaSized: MetaSizedhaving bothSizedCandidateandObjectCandidateMetaSized(which is the only trait that was previously#[rustc_do_not_implement_via_object]that is still dyn-compatible after this change). Is it fine to just remove them? Removing them (as I did in the second commit) doesn't change any UI test results.dyn MetaSizedcould get itsMetaSizedimplementation in two ways: the object candidate (the normaldyn Trait: Trait) that was supressed by#[rustc_do_not_implement_via_object], and theSizedCandidate(that alldyn Traitget fordyn Trait: MetaSized). Given thatMetaSizedhas no associated types or methods, is it fine that these both exist now? Or is it better to only have theSizedCandidateand leave these checks in (i.e. drop the second commit, and remove the FIXMEs)?trait Foo: Pointee {}, you now get a note that reads like Foo "opted out of dyn-compatbility", when reallyPointeedid that.compiler/rustc_hir/src/attrs/encode_cross_crate.rs: ShouldDynIncompatibleTraitattribute be encoded cross crate? I'm not sure what this actually does.diagnostic example