-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Stabilize feature(trait_upcasting)
#134367
Stabilize feature(trait_upcasting)
#134367
Conversation
Some changes occurred in tests/ui/sanitizer cc @rust-lang/project-exploit-mitigations, @rcvalle |
Could you please provide some links that specify what has been done since the last stabilization? Ideally both the issues and the resulting fix PRs. Ty for doing this tho :) Gna nominate for T-lang but this should be an easy decision. |
And maybe link to the last stabilization too |
This comment has been minimized.
This comment has been minimized.
The Miri subtree was changed cc @rust-lang/miri |
Stabilization reportThe core of the feature hasn't changed since the last attempt to stabilize it. But as a reminder this feature allows "upcasting" trait Sub: Super {}
trait Super {}
fn upcast(x: &dyn Sub) -> &dyn Super {
x // implicit coercion
} This is a long wanted feature that people used workarounds for for a long while now. One possible downside is that this forces us into including more data in the vtables. However, our measurements show that the overhead is mostly negligible. Also note that we ate already including this overhead on stable for countless versions and no one ever complained. Another possible downside is that this feature this allows upcasting of raw trait pointers in safe code. That puts constraints on their library invariant (safety invariant) -- specifically, even I believe that the feature is well tested and is ready for stabilization. Previous stabilization attempt problemsAfter the last attempt to stabilize this feature @steffahn found two unsound interactions between trait upcasting and pointer casting (one of which also required Both issues were since fixed in #120248 by adding additional checks for casting pointers, to uphold the library invariant of pointers to trait objects which is needed for this feature.
After this comment @steffahn found another soundness issue: #135315, which was then promptly fixed by @compiler-errors in #135318. No new issues were found since. |
(@RalfJung I'm pretty sure
I-lang-easy-decision
|
@traviscross told me the opposite -- it still needs to be nominated with the regular label. The label description should probably clarify or else people will inevitably keep getting this wrong. |
That's a very strong statement.^^ I'd say there is at least one other downside, or point worth mentioning: this allows upcasting of raw trait pointers in safe code. That puts constraints on their library invariant (safety invariant) -- specifically, even I'm fine with that, and AFAIK @rust-lang/types agrees, but it is a choice we are making here that should be called out explicitly. @rust-lang/opsem is still discussing what to do with the language invariant for raw trait pointers; having that be different from the library invariant is likely surprising but OTOH we still might want a weaker invariant here. |
@RalfJung I did call this out, but maybe not explicitly enough ("the only" slipped from an earlier draft). I've updated the wording to more clearly highlight this. |
This comment was marked as outdated.
This comment was marked as outdated.
(so that it doesn't talk about trait upcasting stabilization in the future tense)
We can't remove `TraitUpcasting` variant, since we need to use the index in winnowing.
this commit makes `deref_into_dyn_supertrait` lint allow-by-default, removes future incompatibility (we finally live in a broken world), and changes the wording in the documentation. previously documentation erroneously said that it lints against *usage* of the deref impl, while it actually (since 104742) lints on the impl itself (oooops, my oversight, should have updated it 2+ years ago...)
1aac2f1
to
4915995
Compare
…treat, r=compiler-errors Stabilize `feature(trait_upcasting)` This feature was "done" for a while now, I think it's finally time to stabilize it! Stabilization report: rust-lang#134367 (comment). cc reference PR: rust-lang/reference#1622. Closes rust-lang#65991 (tracking issue), closes rust-lang#89460 (the lint is no longer future incompat). r? compiler-errors
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#134367 (Stabilize `feature(trait_upcasting)`) - rust-lang#135354 ([Debuginfo] Add MSVC Synthetic and Summary providers to LLDB) - rust-lang#135940 (Update toolstate maintainers) - rust-lang#135945 (Remove some unnecessary parens in `assert!` conditions) - rust-lang#136577 (Pattern Migration 2024: try to suggest eliding redundant binding modifiers) - rust-lang#136598 (Fix suggestion for `dependency_on_unit_never_type_fallback` involving closures + format args expansions) - rust-lang#136653 (Remove dead code from rustc_codegen_llvm and the LLVM wrapper) - rust-lang#136664 (replace one `.map_or(true, ...)` with `.is_none_or(...)`) Failed merges: - rust-lang#136193 (Implement pattern type ffi checks) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#134367 (Stabilize `feature(trait_upcasting)`) - rust-lang#135940 (Update toolstate maintainers) - rust-lang#135945 (Remove some unnecessary parens in `assert!` conditions) - rust-lang#136577 (Pattern Migration 2024: try to suggest eliding redundant binding modifiers) - rust-lang#136598 (Fix suggestion for `dependency_on_unit_never_type_fallback` involving closures + format args expansions) - rust-lang#136653 (Remove dead code from rustc_codegen_llvm and the LLVM wrapper) - rust-lang#136664 (replace one `.map_or(true, ...)` with `.is_none_or(...)`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#134367 - WaffleLapkin:trait_upcasting_as_a_treat, r=compiler-errors Stabilize `feature(trait_upcasting)` This feature was "done" for a while now, I think it's finally time to stabilize it! Stabilization report: rust-lang#134367 (comment). cc reference PR: rust-lang/reference#1622. Closes rust-lang#65991 (tracking issue), closes rust-lang#89460 (the lint is no longer future incompat). r? compiler-errors
…compiler-errors Stabilize `feature(trait_upcasting)` This feature was "done" for a while now, I think it's finally time to stabilize it! Stabilization report: rust-lang/rust#134367 (comment). cc reference PR: rust-lang/reference#1622. Closes #65991 (tracking issue), closes #89460 (the lint is no longer future incompat). r? compiler-errors
I would like to use trait upcasting: rust-lang/rust#65991 Which has already been stabilized: rust-lang/rust#134367 This happened only a few days ago, so the feature is not available on stable yet.
This feature was "done" for a while now, I think it's finally time to stabilize it! Stabilization report: #134367 (comment).
cc reference PR: rust-lang/reference#1622.
Closes #65991 (tracking issue), closes #89460 (the lint is no longer future incompat).
r? compiler-errors