Require coherence of supertrait associated item bounds for dyn-compability

compiler/rustc_middle/src/traits/mod.rs

@@ -782,6 +782,24 @@ pub enum DynCompatibilityViolation {
 
     /// Generic associated type (GAT).
     GenericAssocTy(Symbol, Span),
+
+    /// We consider a trait dyn-incompatible if it has supertrait bounds that
+    /// include two associated type/const bounds on the same associated type/const
+    /// `DefId`, and have generics that could be instantiated into the same concrete
+    /// types, but the bounds may have unequal terms.
+    ///
+    /// Trait objects from such traits could otherwise be instantiated into
+    /// a concrete type with conflicting associated types, violating coherence,
+    /// which is unsound. See #154662.
+    ///
+    /// Checking this predicate is conceptually like checking for
+    /// the coherence of the builtin impls for `dyn`, to make sure that the
+    /// associated type/const don't conflict with each other between the impls.
+    //
+    // FIXME: Improve diagnostics for this.
+    // * Tell the user the exact projections involved that are in conflict
+    // * Point to where the projection bound was written
+    IncoherentSupertraitAssocs(Symbol, Span),
 }
 
 impl DynCompatibilityViolation {
@@ -851,6 +869,10 @@ impl DynCompatibilityViolation {
             Self::GenericAssocTy(name, _) => {
                 format!("it contains generic associated type `{name}`").into()
             }
+            Self::IncoherentSupertraitAssocs(name, _) => {
+                format!("it has conflicting associated item bounds for {name} in supertraits")
+                    .into()
+            }
         }
     }
 
@@ -860,7 +882,8 @@ impl DynCompatibilityViolation {
             | Self::SizedSelf(_)
             | Self::SupertraitSelf(_)
             | Self::SupertraitNonLifetimeBinder(..)
-            | Self::SupertraitConst(_) => DynCompatibilityViolationSolution::None,
+            | Self::SupertraitConst(_)
+            | Self::IncoherentSupertraitAssocs(..) => DynCompatibilityViolationSolution::None,
             Self::Method(
                 name,
                 MethodViolation::StaticMethod(Some((add_self_sugg, make_sized_sugg))),
@@ -890,7 +913,8 @@ impl DynCompatibilityViolation {
             | Self::SupertraitConst(spans) => spans.clone(),
             Self::Method(_, _, span)
             | Self::AssocConst(_, _, span)
-            | Self::GenericAssocTy(_, span) => {
+            | Self::GenericAssocTy(_, span)
+            | Self::IncoherentSupertraitAssocs(_, span) => {
                 if *span != DUMMY_SP {
                     smallvec![*span]
                 } else {

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

@@ -6,14 +6,17 @@
 
 use std::ops::ControlFlow;
 
+use itertools::Itertools;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::FatalError;
 use rustc_hir::def_id::DefId;
 use rustc_hir::{self as hir, LangItem};
+use rustc_infer::infer::{BoundRegionConversionTime, DefineOpaqueTypes};
 use rustc_middle::query::Providers;
 use rustc_middle::ty::{
-    self, EarlyBinder, GenericArgs, Ty, TyCtxt, TypeFoldable, TypeFolder, TypeSuperFoldable,
-    TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor, TypingMode, Unnormalized,
-    Upcast, elaborate,
+    self, Clause, EarlyBinder, GenericArgs, PolyProjectionPredicate, Ty, TyCtxt, TypeFoldable,
+    TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable, TypeVisitableExt,
+    TypeVisitor, TypingMode, Unnormalized, Upcast, elaborate,
 };
 use rustc_span::{DUMMY_SP, Span};
 use smallvec::SmallVec;
@@ -54,7 +57,8 @@ fn dyn_compatibility_violations(
     debug!("dyn_compatibility_violations: {:?}", trait_def_id);
     tcx.arena.alloc_from_iter(
         elaborate::supertrait_def_ids(tcx, trait_def_id)
-            .flat_map(|def_id| dyn_compatibility_violations_for_trait(tcx, def_id)),
+            .flat_map(|def_id| dyn_compatibility_violations_for_trait(tcx, def_id))
+            .chain(incoherent_supertrait_assocs(tcx, trait_def_id)),
     )
 }
 
@@ -978,6 +982,91 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for IllegalRpititVisitor<'tcx> {
     }
 }
 
+/// Computes [`DynCompatibilityViolation::IncoherentSupertraitAssocs`]
+#[instrument(level = "debug", skip(tcx))]
+fn incoherent_supertrait_assocs(
+    tcx: TyCtxt<'_>,
+    trait_def_id: DefId,
+) -> impl Iterator<Item = DynCompatibilityViolation> {
+    let predicates = tcx
+        .predicates_of(trait_def_id)
+        .instantiate_identity(tcx)
+        .predicates
+        .into_iter()
+        .map(Unnormalized::skip_normalization);
+    // Map from associated items to projection predicates that apply to them.
+    let mut preds_for_assoc = FxHashMap::<DefId, Vec<PolyProjectionPredicate<'_>>>::default();
+    elaborate(tcx, predicates).filter_map(Clause::as_projection_clause).flat_map(move |proj| {
+        let prev_projs = preds_for_assoc.entry(proj.item_def_id()).or_default();
+        let violations: Vec<_> = prev_projs
+            .iter()
+            .copied()
+            .filter(move |&prev_proj| {
+                !does_pair_have_coherent_supertrait_assocs(tcx, trait_def_id, prev_proj, proj)
+            })
+            .map(move |_| {
+                DynCompatibilityViolation::IncoherentSupertraitAssocs(
+                    tcx.item_name(proj.item_def_id()),
+                    tcx.def_ident_span(proj.item_def_id())
+                        .expect("Associated items should have a def_ident_span"),
+                )
+            })
+            .collect();
+        prev_projs.push(proj);
+        violations
+    })
+}
+
+#[instrument(level = "debug", skip(tcx), ret)]
+fn does_pair_have_coherent_supertrait_assocs<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    trait_def_id: DefId,
+    proj_1: PolyProjectionPredicate<'tcx>,
+    proj_2: PolyProjectionPredicate<'tcx>,
+) -> bool {
+    // We syntactically compare the two terms. If they're equal, then
+    // they do not conflict with each other.
+    // FIXME: This is an overly strict definition of equality. Could this be done better?
+    if proj_1.term() == proj_2.term() {
+        return true;
+    }
+
+    let infcx = tcx
+        .infer_ctxt()
+        .with_next_trait_solver(tcx.next_trait_solver_in_coherence())
+        .build(TypingMode::Coherence);
+    // We instantiate type parameters in the two projections with the same
+    // fresh inference variables.
+    let trait_args = infcx.fresh_args_for_item(DUMMY_SP, trait_def_id);
+    let process_proj = |proj: PolyProjectionPredicate<'tcx>| {
+        let instantiated_proj = EarlyBinder::bind(tcx, proj).instantiate(tcx, trait_args);
+        infcx.instantiate_binder_with_fresh_vars(
+            DUMMY_SP,
+            BoundRegionConversionTime::AssocTypeProjection(proj.item_def_id()),
+            // FIXME: Normalizing here could maybe make more code compile?
+            instantiated_proj.skip_normalization(),
+        )
+    };
+    let proj_1 = process_proj(proj_1);
+    let proj_2 = process_proj(proj_2);
+    assert_eq!(
+        proj_1.projection_term.kind, proj_2.projection_term.kind,
+        "should compare the same projection kind"
+    );
+    proj_1.projection_term.args.iter().zip_eq(proj_2.projection_term.args).any(|(arg_1, arg_2)| {
+        // Note that we discard any obligations we get here.
+        // We don't care about proving them.
+        //
+        // If this call returns an Err, then the two sets of generic args
+        // can't possibly be instantiated with the same concrete types.
+        // So, we return true from the function
+        infcx
+            .at(&ObligationCause::dummy(), tcx.param_env(trait_def_id))
+            .eq(DefineOpaqueTypes::Yes, arg_1, arg_2)
+            .is_err()
+    })
+}
+
 pub(crate) fn provide(providers: &mut Providers) {
     *providers = Providers {
         dyn_compatibility_violations,

tests/ui/associated-type-bounds/conflicting-bounds-different-generics-legitimate.rs

@@ -0,0 +1,31 @@
+//@ run-pass
+
+// This is a legitimate use case, where the `dyn Sub` trait object ends up
+// having two different "values" for `Assoc`. This is allowed because we know
+// that the two values are for `Super<i32>` and `Super<i64>`, which can't
+// possibly be the same trait.
+
+trait Super<T> {
+    type Assoc;
+}
+
+trait Sub: Super<i32, Assoc = u32> + Super<i64, Assoc = u64> {
+    fn method(&self) {}
+}
+
+fn foo(x: &dyn Sub) {
+    x.method();
+}
+
+struct Thing;
+impl Super<i32> for Thing {
+    type Assoc = u32;
+}
+impl Super<i64> for Thing {
+    type Assoc = u64;
+}
+impl Sub for Thing {}
+
+fn main() {
+    foo(&Thing);
+}

tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.rs

@@ -0,0 +1,28 @@
+// We currently accept conflicting associated type bounds with different generics,
+// which results in an ICE, since those generics can be instantiated with the
+// same concrete type.
+// See https://github.com/rust-lang/rust/issues/154662
+
+trait Super<T> {
+    type Assoc;
+}
+
+trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+    fn method(&self) {}
+}
+
+fn foo<T, U>(x: Option<&dyn Sub<T, U>>) {
+    //~^ ERROR the trait `Sub` is not dyn compatible
+    if false {
+        x.unwrap().method();
+        //~^ ERROR the trait `Sub` is not dyn compatible
+    }
+}
+
+fn main() {
+    // This ends up proving that `dyn Sub<i16, i16>` implements `Super<i16>`.
+    // However, `dyn Sub<i16, i16>` has bounds for both `Assoc = u32` and `Assoc = u64`,
+    // which is nonsense.
+    foo::<i16, i16>(None);
+    //~^ ERROR the trait `Sub` is not dyn compatible
+}

tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.stderr

@@ -0,0 +1,51 @@
+error[E0038]: the trait `Sub` is not dyn compatible
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:14:25
+   |
+LL | fn foo<T, U>(x: Option<&dyn Sub<T, U>>) {
+   |                         ^^^^^^^^^^^^^ `Sub` is not dyn compatible
+   |
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility>
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:7:10
+   |
+LL |     type Assoc;
+   |          ^^^^^ ...because it has conflicting associated item bounds for Assoc in supertraits
+...
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       --- this trait is not dyn compatible...
+
+error[E0038]: the trait `Sub` is not dyn compatible
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:17:9
+   |
+LL |         x.unwrap().method();
+   |         ^^^^^^^^^^ `Sub` is not dyn compatible
+   |
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility>
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:7:10
+   |
+LL |     type Assoc;
+   |          ^^^^^ ...because it has conflicting associated item bounds for Assoc in supertraits
+...
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       --- this trait is not dyn compatible...
+
+error[E0038]: the trait `Sub` is not dyn compatible
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:26:21
+   |
+LL |     foo::<i16, i16>(None);
+   |                     ^^^^ `Sub` is not dyn compatible
+   |
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility>
+  --> $DIR/conflicting-bounds-different-generics-simple.rs:7:10
+   |
+LL |     type Assoc;
+   |          ^^^^^ ...because it has conflicting associated item bounds for Assoc in supertraits
+...
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       --- this trait is not dyn compatible...
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0038`.

tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.rs

@@ -0,0 +1,55 @@
+// We currently accept conflicting associated type bounds with different generics,
+// which results in unsoundness, since those generics can be instantiated with the
+// same concrete type.
+// See https://github.com/rust-lang/rust/issues/154662
+
+type Payload = Box<i32>;
+type Src<'a> = &'a Payload;
+type Dst = &'static Payload;
+
+trait Super<T> {
+    type Assoc;
+}
+
+trait Sub<'a, A1, A2>: Super<A1, Assoc = Src<'a>> + Super<A2, Assoc = Dst> {}
+
+trait Callback<A1, A2> {
+    fn callback<U: Super<A1> + Super<A2> + ?Sized>(
+        payload: <U as Super<A1>>::Assoc,
+    ) -> <U as Super<A2>>::Assoc;
+}
+struct CallbackStruct;
+impl Callback<i16, i16> for CallbackStruct {
+    fn callback<U: Super<i16> + ?Sized>(payload: U::Assoc) -> U::Assoc {
+        payload
+    }
+}
+
+fn require_trait<
+    'a,
+    A1,
+    A2,
+    U: Super<A1, Assoc = Src<'a>> + Super<A2, Assoc = Dst> + ?Sized,
+    C: Callback<A1, A2>,
+>(
+    payload: Src<'a>,
+) -> Dst {
+    C::callback::<U>(payload)
+}
+
+fn use_dyn<'a, A1, A2, C: Callback<A1, A2>>(payload: Src<'a>) -> Dst {
+    require_trait::<'a, A1, A2, dyn Sub<'a, A1, A2>, C>(payload)
+    //~^ ERROR the trait `Sub` is not dyn compatible
+}
+
+fn extend<'a>(payload: Src<'a>) -> Dst {
+    // `dyn Sub<'a, i16, i16>` has both an `Assoc = Src<'a>` bound and an `Assoc = Dst` bound.
+    use_dyn::<i16, i16, CallbackStruct>(payload)
+}
+
+fn main() {
+    let payload: Box<Payload> = Box::new(Box::new(1));
+    let wrong: &'static Payload = extend(&*payload);
+    drop(payload);
+    println!("{wrong}");
+}

tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.stderr

@@ -0,0 +1,19 @@
+error[E0038]: the trait `Sub` is not dyn compatible
+  --> $DIR/conflicting-bounds-different-generics-unsound.rs:41:37
+   |
+LL |     require_trait::<'a, A1, A2, dyn Sub<'a, A1, A2>, C>(payload)
+   |                                     ^^^^^^^^^^^^^^^ `Sub` is not dyn compatible
+   |
+note: for a trait to be dyn compatible it needs to allow building a vtable
+      for more information, visit <https://doc.rust-lang.org/reference/items/traits.html#dyn-compatibility>
+  --> $DIR/conflicting-bounds-different-generics-unsound.rs:11:10
+   |
+LL |     type Assoc;
+   |          ^^^^^ ...because it has conflicting associated item bounds for Assoc in supertraits
+...
+LL | trait Sub<'a, A1, A2>: Super<A1, Assoc = Src<'a>> + Super<A2, Assoc = Dst> {}
+   |       --- this trait is not dyn compatible...
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0038`.

tests/ui/dyn-compatibility/multiple-supers-should-work.rs

@@ -1,5 +1,3 @@
-//@ check-pass
-
 // We previously incorrectly deduplicated the list of projection bounds
 // of trait objects, causing us to incorrectly reject this code, cc #136458.
 
@@ -17,5 +15,7 @@ impl<T, U> Trait<T, U> for () {}
 
 fn main() {
     let x: &dyn Trait<(), _> = &();
+    //~^ ERROR the trait `Trait` is not dyn compatible
     let y: &dyn Trait<_, ()> = x;
+    //~^ ERROR the trait `Trait` is not dyn compatible
 }