Require coherence of supertrait associated item bounds for dyn-compability

compiler/rustc_middle/src/traits/mod.rs

@@ -782,6 +782,23 @@ 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.
+    ///
+    /// The two spans point to where the associated type bound is written.
+    /// The two strings are the pre-rendered user-facing text for the bounds, with generics.
+    IncoherentSupertraitAssocs(Symbol, [Span; 2], [String; 2]),
 }
 
 impl DynCompatibilityViolation {
@@ -851,6 +868,10 @@ impl DynCompatibilityViolation {
             Self::GenericAssocTy(name, _) => {
                 format!("it contains generic associated type `{name}`").into()
             }
+            Self::IncoherentSupertraitAssocs(name, _, [bound_1, bound_2]) => {
+                format!("it has conflicting associated item bounds for `{name}` in supertraits: `{bound_1}` and `{bound_2}`")
+                    .into()
+            }
         }
     }
 
@@ -860,7 +881,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))),
@@ -897,6 +919,9 @@ impl DynCompatibilityViolation {
                     smallvec![]
                 }
             }
+            Self::IncoherentSupertraitAssocs(_, spans, _) => {
+                spans.iter().copied().filter(|span| *span != DUMMY_SP).collect()
+            }
         }
     }
 }

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,19 @@
 
 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_infer::traits::util::ClauseWithSupertraitSpan;
 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, EarlyBinder, GenericArgs, PolyProjectionClause, ProjectionClause, Ty, TyCtxt,
+    TypeFoldable, TypeFolder, TypeSuperFoldable, TypeSuperVisitable, TypeVisitable,
+    TypeVisitableExt, TypeVisitor, TypingMode, Unnormalized, Upcast, elaborate,
 };
 use rustc_span::{DUMMY_SP, Span};
 use smallvec::SmallVec;
@@ -23,9 +27,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 +60,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 +985,109 @@ 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)
+        .into_iter()
+        .map(|(clause, span)| ClauseWithSupertraitSpan::new(clause.skip_norm_wip(), span));
+    // Map from associated items to projection clauses that apply to them.
+    let mut projs_for_assoc = FxHashMap::<DefId, Vec<(PolyProjectionClause<'_>, Span)>>::default();
+    elaborate(tcx, clauses)
+        .filter_map(|x| Some((x.clause.as_projection_clause()?, x.supertrait_span)))
+        .flat_map(move |(proj, span)| {
+            let prev_projs = projs_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 |(prev_proj, prev_span)| {
+                    // Putting proj before prev_proj seems to match the source code order,
+                    // at least in the tests.
+                    DynCompatibilityViolation::IncoherentSupertraitAssocs(
+                        tcx.item_name(proj.item_def_id()),
+                        [span, prev_span],
+                        ty::print::with_no_trimmed_paths!([
+                            proj.to_string(),
+                            prev_proj.to_string(),
+                        ]),
+                    )
+                })
+                .collect();
+            prev_projs.push((proj, span));
+            violations
+        })
+}
+
+#[instrument(level = "debug", skip(tcx), ret)]
+fn does_pair_have_coherent_supertrait_assocs<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    trait_def_id: DefId,
+    proj_1: PolyProjectionClause<'tcx>,
+    proj_2: PolyProjectionClause<'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: PolyProjectionClause<'tcx>| -> ProjectionClause<'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,48 @@
+//@ run-pass
+
+// Legitimate use case #1:
+// The `dyn Sub1` 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 Sub1: Super<i32, Assoc = u32> + Super<i64, Assoc = u64> {
+    fn method1(&self) {}
+}
+
+fn foo(x: &dyn Sub1) {
+    x.method1();
+}
+
+// Legitimate use case #2:
+// The `dyn Sub2<T, U>` trait object has "values" for `Assoc` specified twice,
+// on different generics that might be actually equal types.
+// This is allowed because the two values are the same, so there's no conflict.
+
+trait Sub2<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u32> {
+    fn method2(&self) {}
+}
+
+fn bar<T, U>(x: &dyn Sub2<T, U>) {
+    x.method2();
+}
+
+// Actually call the two functions.
+
+struct Thing;
+impl Super<i32> for Thing {
+    type Assoc = u32;
+}
+impl Super<i64> for Thing {
+    type Assoc = u64;
+}
+impl Sub1 for Thing {}
+impl Sub2<i32, i32> for Thing {}
+
+fn main() {
+    foo(&Thing);
+    bar(&Thing);
+}

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

@@ -0,0 +1,28 @@
+// We previously accepted conflicting associated type bounds with different generics,
+// which resulted 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 would end 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:10:27
+   |
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       ---                 ^^^^^^^^^^^             ^^^^^^^^^^^ ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as Super<U>>::Assoc == u64`
+   |       |                   |
+   |       |                   ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as 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:10:27
+   |
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       ---                 ^^^^^^^^^^^             ^^^^^^^^^^^ ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as Super<U>>::Assoc == u64`
+   |       |                   |
+   |       |                   ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as 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:10:27
+   |
+LL | trait Sub<T, U>: Super<T, Assoc = u32> + Super<U, Assoc = u64> {
+   |       ---                 ^^^^^^^^^^^             ^^^^^^^^^^^ ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as Super<U>>::Assoc == u64`
+   |       |                   |
+   |       |                   ...because it has conflicting associated item bounds for `Assoc` in supertraits: `<Self as Super<T>>::Assoc == u32` and `<Self as 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-transitive.rs

@@ -0,0 +1,28 @@
+// Make sure that the dyn-compatibility check for coherent associated types
+// work correctly when the associated type bounds come from the supertrait.
+
+// Case #1:
+// Both the conflicting bounds come from a supertrait.
+
+trait OneSuper<T> {
+    type Assoc;
+}
+trait OneSub<T, U>: OneSuper<T, Assoc = u32> + OneSuper<U, Assoc = u64> {}
+trait OneSubSub<T, U>: OneSub<T, U> {}
+
+fn one_check<T, U>(_: &dyn OneSubSub<T, U>) {}
+//~^ ERROR the trait `OneSubSub` is not dyn compatible
+
+// Case #2:
+// One of the conflicting bounds come from a supertrait, and one is direct.
+
+trait TwoSuper<T> {
+    type Assoc;
+}
+trait TwoSub<T>: TwoSuper<T, Assoc = u32> {}
+trait TwoSubSub<T, U>: TwoSub<T> + TwoSuper<U, Assoc = u64> {}
+
+fn two_check<T, U>(_: &dyn TwoSubSub<T, U>) {}
+//~^ ERROR the trait `TwoSubSub` is not dyn compatible
+
+fn main() {}