Auto merge of #145807 - zachs18:only-consider-auto-traits-empty, r=compiler-errors

When determining if a trait has no entries for the purposes of omitting vptrs from subtrait vtables, consider its transitive supertraits' entries, instead of just its own entries.

When determining if a non-first supertrait vptr can be omitted from a subtrait vtable, check if the supertrait or any of its (transitive) supertraits have methods, instead of only checking if the supertrait itself has methods.

This fixes the soundness issue where a vptr would be omitted for a supertrait with no methods but that itself had a supertrait with methods, while still optimizing the case where the supertrait is "truly" empty (it has no own vtable entries, and none of its (transitive) supertraits have any own vtable entries).

Fixes <#145752>

-----

Old description:

~~Treat all non-auto traits as non-empty (possibly having methods) for purposes of determining if we need to emit a vptr for a non-direct supertrait (and for new "sibling" entries after a direct or non-direct supertrait).~~

This fixes (I believe) the soundness issue, ~~but regresses vtable sizes and possibly upcasting perf in some cases when using trait hierarchies with empty non-auto traits (see `tests/ui/traits/vtable/multiple-markers.stderr`) since we use vptrs in some cases where we could re-use the vtable.~~

Fixes <#145752>

Re-opens (not anymore) <#114942>

Should not affect <#131813> (i.e. the soundness issue is still fixed, ~~though the relevant vtables in the `trait Evil` example will be larger now~~)

cc implementation history <#131864> <#113856>

-----

~~It should be possible to check if a trait has any methods from itself *or* supertraits (instead of just from itself), but to fix the immediate soundness issue, just assume any non-auto trait could have methods. A more optimistic check can be implemented later (or if someone does it soon it could just supercede this PR 😄).~~ Done in latest push

`@rustbot` label A-dyn-trait F-trait_upcasting

Author: bors

compiler/rustc_trait_selection/src/traits/vtable.rs

@@ -153,7 +153,12 @@ fn prepare_vtable_segments_inner<'tcx, T>(
 
         // emit innermost item, move to next sibling and stop there if possible, otherwise jump to outer level.
         while let Some((inner_most_trait_ref, emit_vptr, mut siblings)) = stack.pop() {
-            let has_entries = has_own_existential_vtable_entries(tcx, inner_most_trait_ref.def_id);
+            // We don't need to emit a vptr for "truly-empty" supertraits, but we *do* need to emit a
+            // vptr for supertraits that have no methods, but that themselves have supertraits
+            // with methods, so we check if any transitive supertrait has entries here (this includes
+            // the trait itself).
+            let has_entries = ty::elaborate::supertrait_def_ids(tcx, inner_most_trait_ref.def_id)
+                .any(|def_id| has_own_existential_vtable_entries(tcx, def_id));
 
             segment_visitor(VtblSegment::TraitOwnEntries {
                 trait_ref: inner_most_trait_ref,

tests/ui/traits/vtable/empty-supertrait-with-nonempty-supersupertrait.dump.stderr

@@ -0,0 +1,16 @@
+error: vtable entries: [
+           MetadataDropInPlace,
+           MetadataSize,
+           MetadataAlign,
+           Method(<dyn OneTwo as One>::one - shim(reify)),
+           Method(<dyn OneTwo as Two>::two - shim(reify)),
+           TraitVPtr(<dyn OneTwo as Two>),
+           TraitVPtr(<dyn OneTwo as TwoAgain>),
+       ]
+  --> $DIR/empty-supertrait-with-nonempty-supersupertrait.rs:40:1
+   |
+LL | type T = dyn OneTwo;
+   | ^^^^^^
+
+error: aborting due to 1 previous error
+

tests/ui/traits/vtable/empty-supertrait-with-nonempty-supersupertrait.rs

@@ -0,0 +1,49 @@
+//@ revisions: run dump
+//@[run] run-pass
+//@[dump] check-fail
+//! Regression test for #145752
+//! Ensure that `OneTwo` contains a vptr for `TwoAgain`
+#![allow(unused)]
+#![cfg_attr(dump, feature(rustc_attrs))]
+
+trait One {
+    fn one(&self) {
+        panic!("don't call this");
+    }
+}
+impl One for () {}
+
+trait Two {
+    fn two(&self) {
+        println!("good");
+    }
+}
+impl Two for () {}
+
+trait TwoAgain: Two {}
+impl<T: Two> TwoAgain for T {}
+
+trait OneTwo: One + TwoAgain {}
+impl<T: One + Two> OneTwo for T {}
+
+fn main() {
+    (&()).two();
+    (&() as &dyn OneTwo).two();
+    (&() as &dyn OneTwo as &dyn Two).two();
+
+    // these two used to panic because they called `one` due to #145752
+    (&() as &dyn OneTwo as &dyn TwoAgain).two();
+    (&() as &dyn OneTwo as &dyn TwoAgain as &dyn Two).two();
+}
+
+#[cfg_attr(dump, rustc_dump_vtable)]
+type T = dyn OneTwo;
+//[dump]~^ ERROR vtable entries: [
+//[dump]~| ERROR            MetadataDropInPlace,
+//[dump]~| ERROR            MetadataSize,
+//[dump]~| ERROR            MetadataAlign,
+//[dump]~| ERROR            Method(<dyn OneTwo as One>::one - shim(reify)),
+//[dump]~| ERROR            Method(<dyn OneTwo as Two>::two - shim(reify)),
+//[dump]~| ERROR            TraitVPtr(<dyn OneTwo as Two>),
+//[dump]~| ERROR            TraitVPtr(<dyn OneTwo as TwoAgain>),
+//[dump]~| ERROR        ]

tests/ui/traits/vtable/multiple-auto.rs

@@ -0,0 +1,50 @@
+// Related to <https://github.com/rust-lang/rust/issues/113840>
+//
+// This test makes sure that multiple auto traits can reuse the
+// same pointer for upcasting (e.g. `Send`/`Sync`)
+
+#![crate_type = "lib"]
+#![feature(rustc_attrs, auto_traits)]
+
+// Markers
+auto trait M0 {}
+auto trait M1 {}
+auto trait M2 {}
+
+// Just a trait with a method
+trait T {
+    fn method(&self) {}
+}
+
+trait A: M0 + M1 + M2 + T {}
+
+trait B: M0 + M1 + T + M2 {}
+
+trait C: M0 + T + M1 + M2 {}
+
+trait D: T + M0 + M1 + M2 {}
+
+struct S;
+
+impl M0 for S {}
+impl M1 for S {}
+impl M2 for S {}
+impl T for S {}
+
+#[rustc_dump_vtable]
+impl A for S {}
+//~^ ERROR vtable entries
+
+#[rustc_dump_vtable]
+impl B for S {}
+//~^ ERROR vtable entries
+
+#[rustc_dump_vtable]
+impl C for S {}
+//~^ ERROR vtable entries
+
+#[rustc_dump_vtable]
+impl D for S {}
+//~^ ERROR vtable entries
+
+fn main() {}

tests/ui/traits/vtable/multiple-auto.stderr

@@ -0,0 +1,46 @@
+error: vtable entries: [
+           MetadataDropInPlace,
+           MetadataSize,
+           MetadataAlign,
+           Method(<S as T>::method),
+       ]
+  --> $DIR/multiple-auto.rs:35:1
+   |
+LL | impl A for S {}
+   | ^^^^^^^^^^^^
+
+error: vtable entries: [
+           MetadataDropInPlace,
+           MetadataSize,
+           MetadataAlign,
+           Method(<S as T>::method),
+       ]
+  --> $DIR/multiple-auto.rs:39:1
+   |
+LL | impl B for S {}
+   | ^^^^^^^^^^^^
+
+error: vtable entries: [
+           MetadataDropInPlace,
+           MetadataSize,
+           MetadataAlign,
+           Method(<S as T>::method),
+       ]
+  --> $DIR/multiple-auto.rs:43:1
+   |
+LL | impl C for S {}
+   | ^^^^^^^^^^^^
+
+error: vtable entries: [
+           MetadataDropInPlace,
+           MetadataSize,
+           MetadataAlign,
+           Method(<S as T>::method),
+       ]
+  --> $DIR/multiple-auto.rs:47:1
+   |
+LL | impl D for S {}
+   | ^^^^^^^^^^^^
+
+error: aborting due to 4 previous errors
+