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

Remove feature(dyn_compatible_for_dispatch) from the compiler #136522

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Feb 4, 2025

This PR proposes the removal of feature(dyn_compatible_for_dispatch) from the compiler.

  • As far as I can tell from the tracking issue, there's very little demand for this feature. I think that if this feature becomes useful in the future, then a fresh implementation from a fresh set of eyes, with renewed understanding of how this feature fits into the picture of Rust as it exists today would be great to have; however, in the absence of this demand, I don't see a particularly good reason to keep this implementation around.

  • The RFC didn't receive very much discussion outside of the lang team, and while the discussion it received seemed to suggest that this feature was aiming to simplify the language and improve expressibility, I don't think this feature has really demonstrated either of those goals in practice. Furthermore, nobody seems to have owned this feature for quite some time or express desire to push for its stabilization.

  • Relatedly, I find some of the RFC discussion like "when we make things impossible it's often presumptuous"1 and "I tend to want to take a 'we are all adults here' attitude toward unsafe code"2 to be particularly uncompelling. Of course this is no criticism to the authors of those comments since they're pretty old comments now, but type soundness is (IMO) the primary goal of the types team. This feature doesn't really do much other than further complicating the story of where we must validate object safety for soundness, along making dyn-incompatible trait object types almost seem useful, but very much remain UB to create and misleading to users who don't know better.

  • Dyn compatibility's story has gotten more complicated since the feature was proposed in 2017, and now it needs to interact with things like associated consts, GATs, RPITITs, trait upcasting, dyn*, etc. While some of this is exercised in the codebase today, I'm not confident all of the corners of this feature have been hammered out. Reducing the "surface area" for what can go wrong in the compiler, especially around a side of the language (dyn Trait) that has been known to be particularly unsound in the past, seems good enough motivation to get rid of this for now.

cc @rust-lang/types @rust-lang/lang

Tracking:

r? types

Footnotes

  1. https://github.com/rust-lang/rfcs/pull/2027#issuecomment-307592857

  2. https://github.com/rust-lang/rfcs/pull/2027#issuecomment-307645838

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2025

changes to the core type system

cc @compiler-errors, @lcnr

@compiler-errors
Copy link
Member Author

I'm nominating this for T-types discussion just to see what people think. I've CC'd lang as well, though I think simply removing an implementation from the compiler falls under the responsibility of T-compiler and doesn't need lang team sign off.

@compiler-errors compiler-errors added I-types-nominated Nominated for discussion during a types team meeting. F-dyn_compatible_for_dispatch `#![feature(dyn_compatible_for_dispatch)]`; formerly `object_safe_for_dispatch` T-types Relevant to the types team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-tidy Area: The tidy tool labels Feb 4, 2025
@@ -639,10 +639,10 @@ fn object_ty_for_trait<'tcx>(
/// a pointer.
///
/// In practice, we cannot use `dyn Trait` explicitly in the obligation because it would result in
/// a new check that `Trait` is dyn-compatible, creating a cycle (until dyn_compatible_for_dispatch
/// is stabilized, see tracking issue <https://github.com/rust-lang/rust/issues/43561>).
Copy link
Member Author

@compiler-errors compiler-errors Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, I don't think that this is even true. Even if dyn_compatible_for_dispatch was stabilized, if we used dyn Trait rather than a "placeholder" parameter here, we'd still have a query cycle if the DispatchFromDyn goal we register below needs to prove that dyn Trait: Trait.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stray stderr file (there's a few of them)

@traviscross traviscross added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 5, 2025
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 5, 2025

I'm happy to remove this logic. My recollection of the motivation for the RFC was that the desire was more to use traits as something module-like for ergonomics -- i.e., being able to say Display::some_helper_fn_related_to_display_where_the_self_type_is_not_important. I think that decision has not aged well given that today it would be be expanding to <dyn Display as HelperTrait>::some_helper... it seems odd. The RFC itself did not contain any kind of compelling example (only Foo/Bar) so I can't really remember the specific case in question (and I didn't see anything in the thread as I skimmed through).

I do want us to revisit our dyn-safety rules, I tend to disagree with "past me" and no longer believe that we should guarantee that Trait: Trait always, etc etc but that's a much broader discussion and goes beyond the scope of the RFC.

(I do agree with the sentiment expressed on the RFC thread that we should enable unsafe code, I think Rust's top goal is reliability, but the goal of "no limits" is also a core design axiom-- but I don't really think that's terribly relevant here.)

That said, I do feel a bit confused how RFC #2027 is related to this attribute exactly.

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 5, 2025
@traviscross
Copy link
Contributor

As a minor procedural matter, we should probably leave the tracking issue itself open until and unless we de-RFC the underlying RFC 2027 or at least we lang FCP close the tracking issue. I've edited the PR description here so it won't auto-close upon merging this.

@compiler-errors compiler-errors removed the I-types-nominated Nominated for discussion during a types team meeting. label Feb 6, 2025
@oli-obk oli-obk assigned oli-obk and unassigned spastorino Feb 6, 2025
@oli-obk
Copy link
Contributor

oli-obk commented Feb 6, 2025

r=me with stray stderr files removed

@jackh726
Copy link
Member

jackh726 commented Feb 6, 2025

FWIW procedurally, if the RFC was opened today, there would be no way that I could see the types team not needing approve it before it would be accepted. So, for all intents and purposes, I think a types team FCP (here or on the tracking issue) should be enough to effectively unaccept the original RFC.

That being said, this PR could land without an FCP, since it's an unstable feature. If that were to be the case, I agree the tracking issue should stay open until one or both of lang and types does an FCP to unaccept the RFC.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 6, 2025

Yea, we ripped out impls without FCPs before.

@lcnr
Copy link
Contributor

lcnr commented Feb 12, 2025

seems like there's general consensus

r=me with stray stderr files removed

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 12, 2025
@lcnr lcnr removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Feb 12, 2025
@compiler-errors compiler-errors force-pushed the dyn_compatible_for_dispatch branch from b6ef2b1 to f3d31f7 Compare February 24, 2025 18:48
@rustbot rustbot added A-tidy Area: The tidy tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2025

changes to the core type system

cc @compiler-errors, @lcnr

@compiler-errors
Copy link
Member Author

@bors r=oli-obk rollup

@bors
Copy link
Collaborator

bors commented Feb 24, 2025

📌 Commit f3d31f7 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2025
…mpiler-errors

Rollup of 11 pull requests

Successful merges:

 - rust-lang#136522 (Remove `feature(dyn_compatible_for_dispatch)` from the compiler)
 - rust-lang#137289 (Consolidate and improve error messaging for `CoerceUnsized` and `DispatchFromDyn`)
 - rust-lang#137321 (Correct doc about `temp_dir()` behavior on Android)
 - rust-lang#137417 (rustc_target: Add more RISC-V atomic-related features)
 - rust-lang#137489 (remove `#[rustc_intrinsic_must_be_overridde]`)
 - rust-lang#137530 (DWARF mixed versions with LTO on MIPS)
 - rust-lang#137543 (std: Fix another new symlink test on Windows)
 - rust-lang#137548 (Pass correct `TypingEnv` to `InlineAsmCtxt`)
 - rust-lang#137550 (Don't immediately panic if dropck fails without returning errors)
 - rust-lang#137552 (Update books)
 - rust-lang#137556 (rename simd_shuffle_generic → simd_shuffle_const_generic)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 748af6b into rust-lang:master Feb 25, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 25, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2025
Rollup merge of rust-lang#136522 - compiler-errors:dyn_compatible_for_dispatch, r=oli-obk

Remove `feature(dyn_compatible_for_dispatch)` from the compiler

This PR proposes the removal of `feature(dyn_compatible_for_dispatch)` from the compiler.

* As far as I can tell from the tracking issue, there's very little demand for this feature. I think that if this feature becomes useful in the future, then a fresh implementation from a fresh set of eyes, with renewed understanding of how this feature fits into the picture of Rust as it exists **today** would be great to have; however, in the absence of this demand, I don't see a particularly good reason to keep this implementation around.

* The RFC didn't receive very much discussion outside of the lang team, and while the discussion it received seemed to suggest that this feature was aiming to simplify the language and improve expressibility, I don't think this feature has really demonstrated either of those goals in practice. Furthermore, nobody seems to have owned this feature for quite some time or express desire to push for its stabilization.

* Relatedly, I find some of the RFC discussion like "when we make things impossible it's often presumptuous"[^1] and "I tend to want to take a 'we are all adults here' attitude toward unsafe code"[^2] to be particularly uncompelling. Of course this is no criticism to the authors of those comments since they're pretty old comments now, but type soundness is (IMO) the primary goal of the types team. This feature doesn't really do much other than further complicating the story of where we must validate object safety for soundness, along making dyn-incompatible trait object types *almost* seem useful, but very much remain UB to create and misleading to users who don't know better.

* Dyn compatibility's story has gotten more complicated since the feature was proposed in 2017, and now it needs to interact with things like associated consts, GATs, RPITITs, trait upcasting, `dyn*`, etc. While some of this is exercised in the codebase today, I'm not confident all of the corners of this feature have been hammered out. Reducing the "surface area" for what can go wrong in the compiler, especially around a side of the language (`dyn Trait`) that has been known to be particularly unsound in the past, seems good enough motivation to get rid of this for now.

[^1]: rust-lang/rfcs#2027 (comment)
[^2]: rust-lang/rfcs#2027 (comment)

cc `@rust-lang/types` `@rust-lang/lang`

Tracking:

- rust-lang#43561

r? types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tidy Area: The tidy tool F-dyn_compatible_for_dispatch `#![feature(dyn_compatible_for_dispatch)]`; formerly `object_safe_for_dispatch` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants