Skip to content
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

Merged
merged 8 commits into from
Feb 7, 2025

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Dec 16, 2024

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

@WaffleLapkin WaffleLapkin added T-lang Relevant to the language team, which will review and decide on the PR/issue. F-trait_upcasting `#![feature(trait_upcasting)]` labels Dec 16, 2024
@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Dec 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

@WaffleLapkin WaffleLapkin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Dec 16, 2024
@compiler-errors
Copy link
Member

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.

@compiler-errors compiler-errors added I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 16, 2024
@compiler-errors
Copy link
Member

And maybe link to the last stabilization too

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Dec 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

The Miri subtree was changed

cc @rust-lang/miri

@WaffleLapkin WaffleLapkin added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Dec 16, 2024
@RalfJung RalfJung added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 16, 2024
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Dec 16, 2024

Stabilization report

The core of the feature hasn't changed since the last attempt to stabilize it.

But as a reminder this feature allows "upcasting" dyn Sub to dyn Super, such as in this example:

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 *const dyn Trait must always come with a vtable that is valid for Trait. This was also discussed during previous stabilization attempts, but still, something to keep in mind.

I believe that the feature is well tested and is ready for stabilization.

Previous stabilization attempt problems

After the last attempt to stabilize this feature @steffahn found two unsound interactions between trait upcasting and pointer casting (one of which also required feature(arbitrary_self_types)): #120222 and #120217. This caused a revert of the stabilization PR.

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.

No new issues were found since.

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.

@WaffleLapkin WaffleLapkin removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 16, 2024
@WaffleLapkin
Copy link
Member Author

(@RalfJung I'm pretty sure I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination implies nomination and you don't need I-lang-nominated Nominated for discussion during a lang team meeting. )

@RalfJung
Copy link
Member

RalfJung commented Dec 16, 2024

@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.

@RalfJung
Copy link
Member

RalfJung commented Dec 16, 2024

The only possible downside

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 *const dyn Trait must always come with a vtable that is valid for Trait.

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.

@WaffleLapkin WaffleLapkin added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 16, 2024
@WaffleLapkin
Copy link
Member Author

@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.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 6, 2025
@rust-log-analyzer

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...)
@WaffleLapkin WaffleLapkin force-pushed the trait_upcasting_as_a_treat branch from 1aac2f1 to 4915995 Compare February 6, 2025 23:38
@rustbot rustbot added PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 6, 2025
@WaffleLapkin WaffleLapkin removed PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 6, 2025
@WaffleLapkin
Copy link
Member Author

With that, I think we can finally

bors r plus

@bors
Copy link
Contributor

bors commented Feb 6, 2025

📌 Commit 4915995 has been approved by compiler-errors

It is now in the queue for this repository.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2025
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
…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
@bors bors merged commit cbd44d7 into rust-lang:master Feb 7, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 7, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
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
@WaffleLapkin WaffleLapkin deleted the trait_upcasting_as_a_treat branch February 7, 2025 22:21
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 8, 2025
…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
hannobraun added a commit to hannobraun/fornjot that referenced this pull request Feb 10, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-trait_upcasting `#![feature(trait_upcasting)]` finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet