-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Make WHERE_CLAUSES_OBJECT_SAFETY
a regular object safety violation
#125380
Conversation
The Miri subtree was changed cc @rust-lang/miri Some changes occurred in src/librustdoc/clean/types.rs cc @camelid HIR ty lowering was modified cc @fmease Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle |
@rfcbot fcp merge |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry y'all lol @rfcbot cancel |
@compiler-errors proposal cancelled. |
@rfcbot fcp merge |
Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
☔ The latest upstream changes (presumably #125410) made this pull request unmergeable. Please resolve the merge conflicts. |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
I tried blessing the codegen tests and nothing happened @bors r=oli-obk rollup=never |
…-obk Make `WHERE_CLAUSES_OBJECT_SAFETY` a regular object safety violation #### The issue In rust-lang#50781, we have known about unsound `where` clauses in function arguments: ```rust trait Impossible {} trait Foo { fn impossible(&self) where Self: Impossible; } impl Foo for &() { fn impossible(&self) where Self: Impossible, {} } // `where` clause satisfied for the object, meaning that the function now *looks* callable. impl Impossible for dyn Foo {} fn main() { let x: &dyn Foo = &&(); x.impossible(); } ``` ... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :( #### What did u change This PR removes the `WHERE_CLAUSES_OBJECT_SAFETY` lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than a `deny` because of the time that has passed since this lint was authored, and the single (1) regression (see below). That means that it's OK to mention `where Self: Trait` where clauses in your trait, but making such a trait into a `dyn Trait` object will report an object safety violation just like `where Self: Sized`, etc. ```rust trait Impossible {} trait Foo { fn impossible(&self) where Self: Impossible; // <~ This definition is valid, just not object-safe. } impl Foo for &() { fn impossible(&self) where Self: Impossible, {} } fn main() { let x: &dyn Foo = &&(); // <~ THIS is where we emit an error. } ``` #### Regressions From a recent crater run, there's only one crate that relies on this behavior: rust-lang#124305 (comment). The crate looks unmaintained and there seems to be no dependents. #### Further We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP. For example, given: ``` trait Tr { fn f(&self) where Self: Blanket; } impl<T: ?Sized> Blanket for T {} ``` Proving that some placeholder `S` implements `S: Blanket` would be sufficient to prove that the same (blanket) impl applies for both `Concerete: Blanket` and `dyn Trait: Blanket`. Repeating here that I don't think we need to implement this behavior right now. ---- r? lcnr
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
Finished benchmarking commit (90d6255): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (secondary -5.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 673.132s -> 673.151s (0.00%) |
I think |
yes we should have added it to the removed lints |
The issue
In #50781, we have known about unsound
where
clauses in function arguments:... which currently segfaults at runtime because we try to call a method in the vtable that doesn't exist. :(
What did u change
This PR removes the
WHERE_CLAUSES_OBJECT_SAFETY
lint and instead makes it a regular object safety violation. I choose to make this into a hard error immediately rather than adeny
because of the time that has passed since this lint was authored, and the single (1) regression (see below).That means that it's OK to mention
where Self: Trait
where clauses in your trait, but making such a trait into adyn Trait
object will report an object safety violation just likewhere Self: Sized
, etc.Regressions
From a recent crater run, there's only one crate that relies on this behavior: #124305 (comment). The crate looks unmaintained and there seems to be no dependents.
Further
We may later choose to relax this (e.g. when the where clause is implied by the supertraits of the trait or something), but this is not something I propose to do in this FCP.
For example, given:
Proving that some placeholder
S
implementsS: Blanket
would be sufficient to prove that the same (blanket) impl applies for bothConcerete: Blanket
anddyn Trait: Blanket
.Repeating here that I don't think we need to implement this behavior right now.
r? lcnr