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

Allow dropping dyn trait object's principal #114679

Closed

Conversation

compiler-errors
Copy link
Member

Allows us to cast from dyn Trait + Send to dyn Send, dropping the trait object's principal trait. This is distinct from trait upcasting, since it's trivial even if we were to totally rip out upcasting support -- it uses the same codepath as dropping auto traits, like dyn Trait + Send to dyn Trait, which is currently allowed without feature gates on stable.

This seems like something that would be nice to have, if anything, just for consistency. Nominating for T-lang discussion and FCP for that reason. I don't personally think this needs a feature gate, but that is also I guess up for debate!

This was originally noticed by lcnr in #113393 (comment).

r? types on the impl

@compiler-errors compiler-errors added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-lang-nominated Nominated for discussion during a lang team meeting. labels Aug 10, 2023
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Aug 10, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 10, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@lcnr
Copy link
Contributor

lcnr commented Aug 10, 2023

r=me after you answer my question and add some (run-pass) tests 😁

It might make sense to still stabilize this with trait upcasting because unlike dyn Trait + Send to dyn Trait, it does impact the vtable?

Does the current impl simply transmute the trait vtable to the empty auto trait vtable?

@lcnr lcnr assigned lcnr and unassigned jackh726 Aug 10, 2023
@compiler-errors
Copy link
Member Author

Does the current impl simply transmute the trait vtable to the empty auto trait vtable?

Yes, it does -- it just reinterprets the vtable as being its common prefix. There's no new vtable being produced.

@lcnr
Copy link
Contributor

lcnr commented Aug 11, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 11, 2023
@compiler-errors compiler-errors added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 15, 2023
@@ -154,7 +154,9 @@ pub fn unsized_info<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
) if src_dyn_kind == target_dyn_kind => {
let old_info =
old_info.expect("unsized_info: missing old info for trait upcasting coercion");
if data_a.principal_def_id() == data_b.principal_def_id() {
if data_a.principal_def_id() == data_b.principal_def_id()
|| data_b.principal().is_none()
Copy link
Member Author

Choose a reason for hiding this comment

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

@bjorn3: if this PR ends up being accepted, the change will need to be made to cg-clif as well. should I do that in this PR, or in a separate one?

Copy link
Member

Choose a reason for hiding this comment

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

Doing it in the same PR would be nice I think. You can change

if data_a.principal_def_id() == data_b.principal_def_id() {
// A NOP cast that doesn't actually change anything, should be allowed even with invalid vtables.
return old_info;
}
the same way as you did here.

@pnkfelix
Copy link
Member

pnkfelix commented Aug 22, 2023

Discussed in T-lang triage meeting; our conclusion was that if immediate stabilization is desired, then @rust-lang/types is the right team to own authoring a stabilization report (or, if the effort for a stabilization report is unwarranted, then the PR author can instead add a feature gate), so this can remain S-waiting-on-team, but the relevant team is now T-types.

@rustbot label: +T-types +I-types-nominated

@rustbot rustbot added I-types-nominated Nominated for discussion during a types team meeting. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Aug 22, 2023
@oli-obk oli-obk removed T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. labels Sep 6, 2023
@lcnr
Copy link
Contributor

lcnr commented Sep 11, 2023

discussed in the t-types meeting: waiting on stabilization report + FCP by @compiler-errors

@lcnr lcnr removed the I-types-nominated Nominated for discussion during a types team meeting. label Sep 11, 2023
@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 21, 2023
@compiler-errors
Copy link
Member Author

I actually don't see any case where someone would want this. It's cute from the type system perspective, but I don't have the time to convince T-lang that this has a real world use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants