Auto merge of #114941 - compiler-errors:inline-shadowed-by-dyn, r=lcnr

Don't resolve generic impls that may be shadowed by dyn built-in impls

**NOTE:** This is a hack. This is not trying to be a general fix for the issue that we've allowed overlapping built-in trait object impls and user-written impls for quite a long time, and traits like `Any` rely on this (#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (#114928) that interacts with this pre-existing issue.

Builtin `dyn Trait` impls may overlap with user-provided blanket impls (`impl<T: ?Sized> Trait for T`) in generic contexts. This leads to bugs when instances are resolved in polymorphic contexts, since we typically prefer object candidates over impl candidates.

This PR implements a (hacky) heuristic to `resolve_associated_item` to account for that unfortunate hole in the type system -- we now bail with ambiguity if we try to resolve a non-rigid instance whose self type is not known to be sized. This makes sure we can still inline instances like `impl<T: Sized> Trait for T`, which can never overlap with `dyn Trait`'s built-in impl, but we avoid inlining an impl that may be shadowed by a `dyn Trait`.

Fixes #114928

compiler/rustc_middle/src/ty/sty.rs

@@ -2945,6 +2945,33 @@ impl<'tcx> Ty<'tcx> {
             _ => false,
         }
     }
+
+    pub fn is_known_rigid(self) -> bool {
+        match self.kind() {
+            Bool
+            | Char
+            | Int(_)
+            | Uint(_)
+            | Float(_)
+            | Adt(_, _)
+            | Foreign(_)
+            | Str
+            | Array(_, _)
+            | Slice(_)
+            | RawPtr(_)
+            | Ref(_, _, _)
+            | FnDef(_, _)
+            | FnPtr(_)
+            | Dynamic(_, _, _)
+            | Closure(_, _)
+            | Generator(_, _, _)
+            | GeneratorWitness(_)
+            | GeneratorWitnessMIR(_, _)
+            | Never
+            | Tuple(_) => true,
+            Error(_) | Infer(_) | Alias(_, _) | Param(_) | Bound(_, _) | Placeholder(_) => false,
+        }
+    }
 }
 
 /// Extra information about why we ended up with a particular variance.

compiler/rustc_ty_utils/src/instance.rs

@@ -141,11 +141,34 @@ fn resolve_associated_item<'tcx>(
                     false
                 }
             };
-
             if !eligible {
                 return Ok(None);
             }
 
+            // HACK: We may have overlapping `dyn Trait` built-in impls and
+            // user-provided blanket impls. Detect that case here, and return
+            // ambiguity.
+            //
+            // This should not affect totally monomorphized contexts, only
+            // resolve calls that happen polymorphically, such as the mir-inliner
+            // and const-prop (and also some lints).
+            let self_ty = rcvr_args.type_at(0);
+            if !self_ty.is_known_rigid() {
+                let predicates = tcx
+                    .predicates_of(impl_data.impl_def_id)
+                    .instantiate(tcx, impl_data.args)
+                    .predicates;
+                let sized_def_id = tcx.lang_items().sized_trait();
+                // If we find a `Self: Sized` bound on the item, then we know
+                // that `dyn Trait` can certainly never apply here.
+                if !predicates.into_iter().filter_map(ty::Clause::as_trait_clause).any(|clause| {
+                    Some(clause.def_id()) == sized_def_id
+                        && clause.skip_binder().self_ty() == self_ty
+                }) {
+                    return Ok(None);
+                }
+            }
+
             // Any final impl is required to define all associated items.
             if !leaf_def.item.defaultness(tcx).has_value() {
                 let guard = tcx.sess.delay_span_bug(

tests/mir-opt/dont_inline_type_id.call.Inline.diff

@@ -0,0 +1,20 @@
+- // MIR for `call` before Inline
++ // MIR for `call` after Inline
+
+  fn call(_1: &T) -> TypeId {
+      debug s => _1;
+      let mut _0: std::any::TypeId;
+      let mut _2: &T;
+
+      bb0: {
+          StorageLive(_2);
+          _2 = &(*_1);
+          _0 = <T as Any>::type_id(move _2) -> [return: bb1, unwind unreachable];
+      }
+
+      bb1: {
+          StorageDead(_2);
+          return;
+      }
+  }
+

tests/mir-opt/dont_inline_type_id.rs

@@ -0,0 +1,15 @@
+// unit-test: Inline
+// compile-flags: --crate-type=lib -C panic=abort
+
+use std::any::Any;
+use std::any::TypeId;
+
+struct A<T: ?Sized + 'static> {
+    a: i32,
+    b: T,
+}
+
+// EMIT_MIR dont_inline_type_id.call.Inline.diff
+pub fn call<T: ?Sized + 'static>(s: &T) -> TypeId {
+    s.type_id()
+}

tests/mir-opt/inline_generically_if_sized.call.Inline.diff

@@ -0,0 +1,24 @@
+- // MIR for `call` before Inline
++ // MIR for `call` after Inline
+
+  fn call(_1: &T) -> i32 {
+      debug s => _1;
+      let mut _0: i32;
+      let mut _2: &T;
++     scope 1 (inlined <T as Foo>::bar) {
++         debug self => _2;
++     }
+
+      bb0: {
+          StorageLive(_2);
+          _2 = &(*_1);
+-         _0 = <T as Foo>::bar(move _2) -> [return: bb1, unwind unreachable];
+-     }
+- 
+-     bb1: {
++         _0 = const 0_i32;
+          StorageDead(_2);
+          return;
+      }
+  }
+

tests/mir-opt/inline_generically_if_sized.rs

@@ -0,0 +1,17 @@
+// unit-test: Inline
+// compile-flags: --crate-type=lib -C panic=abort
+
+trait Foo {
+    fn bar(&self) -> i32;
+}
+
+impl<T> Foo for T {
+    fn bar(&self) -> i32 {
+        0
+    }
+}
+
+// EMIT_MIR inline_generically_if_sized.call.Inline.diff
+pub fn call<T>(s: &T) -> i32 {
+    s.bar()
+}

tests/ui/const_prop/dont-propagate-generic-instance-2.rs

@@ -0,0 +1,26 @@
+// run-pass
+
+#![feature(inline_const)]
+
+// Makes sure we don't propagate generic instances of `Self: ?Sized` blanket impls.
+// This is relevant when we have an overlapping impl and builtin dyn instance.
+// See <https://github.com/rust-lang/rust/pull/114941> for more context.
+
+trait Trait {
+    fn foo(&self) -> &'static str;
+}
+
+impl<T: ?Sized> Trait for T {
+    fn foo(&self) -> &'static str {
+        std::any::type_name::<T>()
+    }
+}
+
+fn bar<T: ?Sized>() -> fn(&T) -> &'static str {
+    const { Trait::foo as fn(&T) -> &'static str }
+    // If const prop were to propagate the instance
+}
+
+fn main() {
+    assert_eq!("i32", bar::<dyn Trait>()(&1i32));
+}

tests/ui/const_prop/dont-propagate-generic-instance.rs

@@ -0,0 +1,24 @@
+// run-pass
+
+// Makes sure we don't propagate generic instances of `Self: ?Sized` blanket impls.
+// This is relevant when we have an overlapping impl and builtin dyn instance.
+// See <https://github.com/rust-lang/rust/pull/114941> for more context.
+
+trait Trait {
+    fn foo(&self) -> &'static str;
+}
+
+impl<T: ?Sized> Trait for T {
+    fn foo(&self) -> &'static str {
+        std::any::type_name::<T>()
+    }
+}
+
+const fn bar<T: ?Sized>() -> fn(&T) -> &'static str {
+    Trait::foo
+    // If const prop were to propagate the instance
+}
+
+fn main() {
+    assert_eq!("i32", bar::<dyn Trait>()(&1i32));
+}