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

Stablize trait_upcasting feature #101718

Closed
wants to merge 1 commit into from

Conversation

crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Sep 12, 2022

Closes #65991.
Closes #89460.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 12, 2022
@rust-highfive

This comment was marked as outdated.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2022
@crlf0710 crlf0710 added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 12, 2022
@crlf0710
Copy link
Member Author

r? @nikomatsakis

@crlf0710 crlf0710 force-pushed the stablize_trait_upcasting branch from 1f0111b to 81b4025 Compare September 12, 2022 12:29
@fee1-dead
Copy link
Member

fee1-dead commented Sep 12, 2022

Note: FCP for unblocking upcasts at #101336.

@nikomatsakis
Copy link
Contributor

@crlf0710 we need a stabilization report to propose FCP -- I think we can adapt the notes from #101336, however.

@fee1-dead fee1-dead added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 12, 2022
@crlf0710
Copy link
Member Author

Stabilization report

This PR proposes the stablization of the dyn trait upcasting coercion feature, which is a long-wished-for feature since 2013, and tracked with the Dyn upcasting coercion initiative. The associated Reference PR in flight is
rust-lang/reference#1259.

We are stablizing a new kind of coercion, which is "upcasts" from a dyn Trait to its supertraits:

trait Foo {
    fn foo_method(&self);
}
trait Bar: Foo {
    fn bar_method(&self);
}

let x: &dyn Bar = /* ... */;
let y: &dyn Foo = x; // compiles

The key factor for these upcasts is that they require adjusting the vtable. The current implementation strategy is that the vtables for the Bar trait embed pointers to a Foo vtable within them:

+----------------------------+
| Bar vtable for some type T |
|----------------------------|
| Foo vtable                 | ----------> +------------------+
| foo_method                 | -----+      | Foo vtable for T |
| bar_method                 | --+  |      |------------------|
+----------------------------+   |  |      | foo_method       | ---+
                                 |  |      +------------------+    |
                                 |  |                              |
                                 |  +---> <T as Foo>::foo_method <-+
                                 v
                     <T as Bar>::bar_method
                 
(this diagram is only meant to convey the general idea of the vtable
 layout, and doesn't represent the exact offsets we would use etc;
 in fact, with the current implementation,
 the first supertrait is stored 'inline' and hence no load is required)

This way, given a &dyn Bar object, we convert its Bar vtable to the appropriate Foo vtable by loading the appropriate field.

For some examples of code patterns that this feature enables, see Integration with Any: You can give any trait object downcast ability by using Any as its supertrait and Aligning type-erased closure bounds: You can use &mut dyn Fn as if it's a &mut dyn FnMut.

@crlf0710 crlf0710 added F-trait_upcasting `#![feature(trait_upcasting)]` relnotes Marks issues that should be documented in the release notes of the next release. labels Sep 14, 2022
@nikomatsakis

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 19, 2022
@nikomatsakis

This comment was marked as outdated.

@nikomatsakis

This comment was marked as outdated.

@rfcbot

This comment was marked as outdated.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 19, 2022
@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Sep 19, 2022
@nikomatsakis nikomatsakis removed the T-types Relevant to the types team, which will review and decide on the PR/issue. label Sep 19, 2022
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Now that the FCP in #101336 has ended, I'm going to move to stabilize dyn upcasting. @crlf0710 has provided a stabilization report here.

@rfcbot
Copy link

rfcbot commented Sep 19, 2022

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 19, 2022
@nikomatsakis
Copy link
Contributor

@rfcbot concern doc-pr

I believe we need a PR against the reference describing when upcasts are allowed (it doesn't, I think, have to document the vtable layout or anything like that).

I'm not sure if the reference also talks at all about validity/unsafety invariants, though.

@nikomatsakis
Copy link
Contributor

@rustbot label +I-types-nominated

I'm nominating this for @rust-lang/types visibility, however, to make sure we feel comfortable with this being stabilized from a type system perspective. It is implemented by extending the existing ability to "upcast" a &dyn T to &dyn U (e.g., from Foo + Send to Foo).

@rustbot rustbot added the I-types-nominated Nominated for discussion during a types team meeting. label Sep 19, 2022
@crlf0710
Copy link
Member Author

The Reference PR is at rust-lang/reference#1259, though admittedly the descriptions are pretty brief, only covered the major points... Let me know if the text needs any expansion :)

@joshtriplett
Copy link
Member

@rfcbot concern rfc

This has been the subject of a great deal of discussion and iteration, and I really do think this should have an RFC before stabilization. I've been expecting that it would get an RFC.

@nikomatsakis
Copy link
Contributor

Per discussion in the meeting, we're going to author an RFC for this change, so I'm canceling the FCP and closing the PR for now. @crlf0710 let's talk!

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Sep 20, 2022

@nikomatsakis proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 20, 2022
@nikomatsakis
Copy link
Contributor

I think we should keep the reference PR open, but add a note.

@jackh726 jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label Sep 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 22, 2023
…affleLapkin

Stabilize RFC3324 dyn upcasting coercion

This PR stabilize the `trait_upcasting` feature, aka rust-lang/rfcs#3324.

The FCP was completed here: rust-lang#65991 (comment).

~~And also remove the `deref_into_dyn_supertrait` lint which is now handled by dyn upcasting coercion.~~

Heavily inspired by rust-lang#101718
Fixes rust-lang#65991
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 22, 2023
…affleLapkin

Stabilize RFC3324 dyn upcasting coercion

This PR stabilize the `trait_upcasting` feature, aka rust-lang/rfcs#3324.

The FCP was completed here: rust-lang#65991 (comment).

~~And also remove the `deref_into_dyn_supertrait` lint which is now handled by dyn upcasting coercion.~~

Heavily inspired by rust-lang#101718
Fixes rust-lang#65991
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 23, 2023
Stabilize RFC3324 dyn upcasting coercion

This PR stabilize the `trait_upcasting` feature, aka rust-lang/rfcs#3324.

The FCP was completed here: rust-lang/rust#65991 (comment).

~~And also remove the `deref_into_dyn_supertrait` lint which is now handled by dyn upcasting coercion.~~

Heavily inspired by rust-lang/rust#101718
Fixes rust-lang/rust#65991
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-trait_upcasting `#![feature(trait_upcasting)]` needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
9 participants