Allow dropping dyn Trait principal

[Jules-Bertholet]

Redux of #114679, fixes #126313

This allows the following examples to compile:

trait Trait: Send {}
fn foo(x: &dyn Trait) -> &dyn Send { x }

trait Trait {}
fn foo(x: &(dyn Trait + Send)) -> &dyn Send { x }

This makes the language more consistent, as we already allow:

trait Trait {}
fn foo(x: &(dyn Trait + Send)) -> &dyn Trait { x }

The PR includes a test case, in tests/ui/traits/dyn-drop-principal.rs.

Documentation

dyn upcasting coercions, of any kind, are currently entirely undocumented in the reference. A future reference PR should address this.

@rustbot label T-lang T-types needs-fcp A-coercions A-trait-objects

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[fmease]

cc @compiler-errors
r? @lcnr

[compiler-errors]

I'll let @lcnr do the judgement call of whether this needs a lang FCP or if types suffices. Thanks for picking this back up!

[lcnr]

sorry for the delay!

nominating for lang, to let them decide whether they mind delegating this to t-types. We simply reuse the vtable of the parent trait when dropping the principal. This relies on all vtables to have the same header used for dropping and size_of_val/align_of_val. I don't know whether that's currently guaranteed anywhere, cc @rust-lang/opsem i guess?

Also, please extend the test to also check size_of_val and align_of_val 😁

[RalfJung]

Is there a Miri test that does this? This may run afoul of Miri's vtable validity check, which is supposed to ensure that the vtable has the same principal as the dyn trait type indicates.

This relies on all vtables to have the same header which is used for dropping and size_of_val/align_of_val. I don't know whether that's currently guaranteed anywhere, cc @rust-lang/opsem i guess?

vtables are entirely an implementation detail. In the op.sem they don't even exist in-memory, just as abstract opaque concepts (similar to function pointers). So we're free to do whatever here.

But it does mean we have to define the validity of wide pointers such that it's okay for the vtable to have a principal even if the type does not.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[Jules-Bertholet]

Also, please extend the test to also check size_of_val and align_of_val

Done

[RalfJung]

I don't understand how this new test can pass, it seems like it should fail this check...

[RalfJung]

Miri test looks good, thanks!

[lcnr]

This relies on all vtables to have the same header which is used for dropping and size_of_val/align_of_val. I don't know whether that's currently guaranteed anywhere, cc @rust-lang/opsem i guess?

vtables are entirely an implementation detail. In the op.sem they don't even exist in-memory, just as abstract opaque concepts (similar to function pointers). So we're free to do whatever here.

But it does mean we have to define the validity of wide pointers such that it's okay for the vtable to have a principal even if the type does not.

does that mean this also requires an opsem FCP? I would expect that this sort of type pruning is definitely sound. I would expect that shrinking the size of the pointee is totally safe and we can't assume that all vtables for the same type are equal.

[lcnr]

cc @rust-lang/lang the nomination is for

nominating for lang, to let them decide whether they mind delegating this to t-types. We simply reuse the vtable of the parent trait when dropping the principal. This relies on all vtables to have the same header used for dropping and size_of_val/align_of_val.

[RalfJung]

Also worth noting that keeping the same vtable is not a guarantee and in fact in Miri you will get a different vtable for the "None" principal. I assume this could be done at runtime as well. Keeping the same vtable means runtime will see pointer equalities that are impossible to obtain in Miri - but we have that elsewhere, too, e.g. with deduplication of identical consts.

[traviscross]

@rustbot labels -I-lang-nominated
@rfcbot fcp merge

We discussed this today in lang triage. This sounded right to us, in terms of making the language more consistent. We discussed whether we were closing any meaningful doors here and couldn't really find any. This seems in particular consistent with the direction that we set with dyn trait upcasting. We'll of course do this as a dual FCP with T-types.

[rfcbot]

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

• @BoxyUwU
• @compiler-errors
• @jackh726
• @joshtriplett
• @lcnr
• @nikomatsakis
• @oli-obk
• @pnkfelix
• @scottmcm
• @spastorino
• @tmandry
• @traviscross

No concerns currently listed.

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

This seems entirely reasonable to me -- it's basically like every trait has the supertrait ε and you can go from dyn Foo + ε to just dyn ε.

@rfcbot reviewed

[bors]

☔ The latest upstream changes (presumably #130473) made this pull request unmergeable. Please resolve the merge conflicts.

[joshtriplett]

Ping @nikomatsakis @pnkfelix @tmandry for remaining checkboxes.

[tmandry]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@rfcbot reviewed

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[lcnr]

we still use data_a.principal_def_id() == data_b.principal_def_id() in coerce_dyn_star, can you add a test for that

[RalfJung]

Superseded by #131857 -- should this PR be closed?