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 :grin:

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

[rust-log-analyzer]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
---- [ui] tests/ui/impl-trait/unsized_coercion5.rs#next stdout ----

error in revision `next`: ui test compiled successfully!
status: exit status: 0
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/impl-trait/unsized_coercion5.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "next" "--check-cfg" "cfg(FALSE,next,old)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.next" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.next/auxiliary" "-Znext-solver"
stderr: none


---- [ui] tests/ui/impl-trait/unsized_coercion5.rs#old stdout ----
---- [ui] tests/ui/impl-trait/unsized_coercion5.rs#old stdout ----
diff of stderr:

- error[E0308]: mismatched types
-   --> $DIR/unsized_coercion5.rs:16:32
-    |
- LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
-    |                -------------   ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected trait `Send`, found trait `Trait + Send`
-    |                expected due to this
-    |
-    = note: expected struct `Box<dyn Send>`
-               found struct `Box<dyn Trait + Send>`
-               found struct `Box<dyn Trait + Send>`
- 
12 error: cannot check whether the hidden type of opaque type satisfies auto traits
13   --> $DIR/unsized_coercion5.rs:16:32
14    |

32    = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
33    = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
- error: aborting due to 3 previous errors
+ error: aborting due to 2 previous errors
36 
- Some errors have detailed explanations: E0277, E0308.
- Some errors have detailed explanations: E0277, E0308.
- For more information about an error, try `rustc --explain E0277`.
+ For more information about this error, try `rustc --explain E0277`.
39 


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.old/unsized_coercion5.old.stderr
To only update this specific test, also pass `--test-args impl-trait/unsized_coercion5.rs`


error in revision `old`: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/impl-trait/unsized_coercion5.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--cfg" "old" "--check-cfg" "cfg(FALSE,next,old)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.old" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/impl-trait/unsized_coercion5.old/auxiliary"
--- stderr -------------------------------
error: cannot check whether the hidden type of opaque type satisfies auto traits
##[error]  --> /checkout/tests/ui/impl-trait/unsized_coercion5.rs:16:32
   |
   |
LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
   |
   |
   = note: fetching the hidden types of an opaque inside of the defining scope is not supported. You can try moving the opaque type and the item that actually registers a hidden type into a new submodule
note: opaque type is declared here
   |
   |
LL | fn hello() -> Box<impl Trait + ?Sized> {
   |                   ^^^^^^^^^^^^^^^^^^^
   = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
error[E0277]: the size for values of type `impl Trait + ?Sized` cannot be known at compilation time
##[error]  --> /checkout/tests/ui/impl-trait/unsized_coercion5.rs:16:32
   |
   |
LL |         let y: Box<dyn Send> = x as Box<dyn Trait + Send>;
   |
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = help: the trait `Sized` is not implemented for `impl Trait + ?Sized`
   = note: required for the cast from `Box<impl Trait + ?Sized>` to `Box<dyn Trait + Send>`
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.
------------------------------------------

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

[RalfJung]

Note that from an opsem perspective, vtables don't have a size -- they are opaque handles that just store a type-trait pair.

Given that we haven't really spec'ed out validity for vtables yet in opsem (and there are indeed some disagreements here), personally I'd be fine with just taking this as a further constraint that we have to take into account.

[bors]

:umbrella: The latest upstream changes (presumably #128155) made this pull request unmergeable. Please resolve the merge conflicts.

[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:

• [x] @BoxyUwU
• [x] @compiler-errors
• [x] @jackh726
• [x] @joshtriplett
• [x] @lcnr
• [x] @nikomatsakis
• [x] @oli-obk
• [ ] @pnkfelix
• [x] @scottmcm
• [x] @spastorino
• [x] @tmandry
• [x] @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]

:umbrella: 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]

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

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

[RalfJung]

Superseded by https://github.com/rust-lang/rust/pull/131857 -- should this PR be closed?