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/coherence.rs

@@ -560,7 +560,7 @@ fn impl_intersection_has_negative_obligation(
     .any(|(clause, _)| try_prove_negated_where_clause(infcx, clause, param_env))
 }
 
-fn plug_infer_with_placeholders<'tcx>(
+pub(super) fn plug_infer_with_placeholders<'tcx>(
     infcx: &InferCtxt<'tcx>,
     universe: ty::UniverseIndex,
     value: impl TypeVisitable<TyCtxt<'tcx>>,

compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs

@@ -6,15 +6,18 @@
 
 use std::ops::ControlFlow;
 
+use itertools::Itertools;
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::FatalError;
 use rustc_hir as hir;
 use rustc_hir::attrs::lang_items::LangItem;
-use rustc_hir::def_id::DefId;
+use rustc_hir::def_id::{CRATE_DEF_ID, DefId};
+use rustc_infer::infer::BoundRegionConversionTime;
 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, ProjectionPredicate, Ty,
+    TyCtxt, TypeFoldable, TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable,
+    TypeVisitableExt, TypeVisitor, TypingMode, Unnormalized, Upcast, elaborate,
 };
 use rustc_span::{DUMMY_SP, Span};
 use smallvec::SmallVec;
@@ -23,9 +26,10 @@ use tracing::{debug, instrument};
 use super::elaborate;
 use crate::infer::TyCtxtInferExt;
 pub use crate::traits::DynCompatibilityViolation;
+use crate::traits::coherence::plug_infer_with_placeholders;
 use crate::traits::query::evaluate_obligation::InferCtxtExt;
 use crate::traits::{
-    AssocConstViolation, MethodViolation, Obligation, ObligationCause,
+    AssocConstViolation, MethodViolation, Obligation, ObligationCause, ObligationCtxt,
     normalize_param_env_or_error, util,
 };
 
@@ -55,7 +59,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)),
     )
 }
 
@@ -979,6 +984,103 @@ 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 clauses = tcx
+        .clauses_of(trait_def_id)
+        .instantiate_identity(tcx)
+        .clauses
+        .into_iter()
+        .map(Unnormalized::skip_norm_wip);
+    // Map from associated items to projection predicates that apply to them.
+    let mut preds_for_assoc = FxHashMap::<DefId, Vec<PolyProjectionPredicate<'_>>>::default();
+    elaborate(tcx, clauses).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 {
+    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>| -> ProjectionPredicate<'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()),
+            instantiated_proj.skip_norm_wip(),
+        )
+    };
+    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"
+    );
+
+    let ocx = ObligationCtxt::new(&infcx);
+    let param_env = tcx.param_env(trait_def_id);
+    // Constrain the two projections to be on the same trait, including generics.
+    // If this fails, then the two projections do not conflict with each other,
+    // as they're projecting different things.
+    let can_equate_generics =
+        proj_1.projection_term.args.iter().zip_eq(proj_2.projection_term.args).all(
+            |(arg_1, arg_2)| ocx.eq(&ObligationCause::dummy(), param_env, arg_1, arg_2).is_ok(),
+        );
+    // FIXME: Is it sound to return true here if we call .resolve_regions() here,
+    // and that produces an error?
+    if !can_equate_generics || ocx.try_evaluate_obligations().has_errors() {
+        return true;
+    }
+    // Discard any ambiguous obligations. In doing so, we're conservatively assuming that
+    // the two projections might apply to the same trait-with-generics. This is sound, since
+    // it can only cause this function to return false when it could have returned true,
+    // which at worst can only cause a trait to be marked as dyn-incompatible.
+    // FIXME: Is this strictly necessary? Can retaining the ambiguous obligations
+    // to the following check cause any problems?
+    drop(ocx);
+
+    // Now that we've constrained the two projections to be on the same thing,
+    // we check whether the two terms are necessarily equal to each other.
+    // If they are, then the two projections are coherent.
+    plug_infer_with_placeholders(&infcx, ty::UniverseIndex::ROOT, (proj_1, proj_2));
+    let ocx = ObligationCtxt::new(&infcx);
+    ocx.eq(&ObligationCause::dummy(), param_env, proj_1.term, proj_2.term).is_ok()
+        && ocx.evaluate_obligations_error_on_ambiguity().no_errors()
+        && ocx.resolve_regions(CRATE_DEF_ID, param_env, []).is_empty()
+}
+
 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}");
+}