diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index b8cd0791a783e..567be657bd0b0 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/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 { diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index 78ccf04d456a3..66270a3d1f903 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/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>, diff --git a/compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs b/compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs index 6d86a2cce6400..7a18d39f74d3f 100644 --- a/compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs +++ b/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> for IllegalRpititVisitor<'tcx> { } } +/// Computes [`DynCompatibilityViolation::IncoherentSupertraitAssocs`] +#[instrument(level = "debug", skip(tcx))] +fn incoherent_supertrait_assocs( + tcx: TyCtxt<'_>, + trait_def_id: DefId, +) -> impl Iterator { + 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::>>::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, diff --git a/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-legitimate.rs b/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-legitimate.rs new file mode 100644 index 0000000000000..56dee9bd8d293 --- /dev/null +++ b/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` and `Super`, which can't +// possibly be the same trait. + +trait Super { + type Assoc; +} + +trait Sub: Super + Super { + fn method(&self) {} +} + +fn foo(x: &dyn Sub) { + x.method(); +} + +struct Thing; +impl Super for Thing { + type Assoc = u32; +} +impl Super for Thing { + type Assoc = u64; +} +impl Sub for Thing {} + +fn main() { + foo(&Thing); +} diff --git a/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.rs b/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.rs new file mode 100644 index 0000000000000..e37f0fe363b83 --- /dev/null +++ b/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 { + type Assoc; +} + +trait Sub: Super + Super { + fn method(&self) {} +} + +fn foo(x: Option<&dyn Sub>) { + //~^ 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` implements `Super`. + // However, `dyn Sub` has bounds for both `Assoc = u32` and `Assoc = u64`, + // which is nonsense. + foo::(None); + //~^ ERROR the trait `Sub` is not dyn compatible +} diff --git a/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.stderr b/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-simple.stderr new file mode 100644 index 0000000000000..4a9a1000ee338 --- /dev/null +++ b/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(x: Option<&dyn Sub>) { + | ^^^^^^^^^^^^^ `Sub` is not dyn compatible + | +note: for a trait to be dyn compatible it needs to allow building a vtable + for more information, visit + --> $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: Super + Super { + | --- 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 + --> $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: Super + Super { + | --- 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::(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 + --> $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: Super + Super { + | --- this trait is not dyn compatible... + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0038`. diff --git a/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.rs b/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.rs new file mode 100644 index 0000000000000..9b8e5addbf6a3 --- /dev/null +++ b/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; +type Src<'a> = &'a Payload; +type Dst = &'static Payload; + +trait Super { + type Assoc; +} + +trait Sub<'a, A1, A2>: Super> + Super {} + +trait Callback { + fn callback + Super + ?Sized>( + payload: >::Assoc, + ) -> >::Assoc; +} +struct CallbackStruct; +impl Callback for CallbackStruct { + fn callback + ?Sized>(payload: U::Assoc) -> U::Assoc { + payload + } +} + +fn require_trait< + 'a, + A1, + A2, + U: Super> + Super + ?Sized, + C: Callback, +>( + payload: Src<'a>, +) -> Dst { + C::callback::(payload) +} + +fn use_dyn<'a, A1, A2, C: Callback>(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::(payload) +} + +fn main() { + let payload: Box = Box::new(Box::new(1)); + let wrong: &'static Payload = extend(&*payload); + drop(payload); + println!("{wrong}"); +} diff --git a/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.stderr b/tests/ui/associated-type-bounds/conflicting-bounds-different-generics-unsound.stderr new file mode 100644 index 0000000000000..2e1a3fcd150f6 --- /dev/null +++ b/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 + --> $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> + Super {} + | --- this trait is not dyn compatible... + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0038`. diff --git a/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.rs b/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.rs index 2d8230973325d..5d7af696fccad 100644 --- a/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.rs +++ b/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.rs @@ -1,5 +1,3 @@ -//@ build-pass (FIXME(62277): could be check-pass?) - // FIXME(eddyb) shorten the name so windows doesn't choke on it. #![crate_name = "trait_test"] @@ -47,5 +45,7 @@ fn main() { // Make sure this works both with and without the associated type // being specified. let _x: Box> = Box::new(2u32); + //~^ ERROR the trait `NormalizingHelper` is not dyn compatible let _y: Box> = Box::new(2u32); + //~^ ERROR the trait `NormalizingHelper` is not dyn compatible } diff --git a/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.stderr b/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.stderr new file mode 100644 index 0000000000000..ea5799bc69a3d --- /dev/null +++ b/tests/ui/traits/object/with-self-in-projection-output-repeated-supertrait.stderr @@ -0,0 +1,37 @@ +error[E0038]: the trait `NormalizingHelper` is not dyn compatible + --> $DIR/with-self-in-projection-output-repeated-supertrait.rs:47:21 + | +LL | let _x: Box> = Box::new(2u32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `NormalizingHelper` is not dyn compatible + | +note: for a trait to be dyn compatible it needs to allow building a vtable + for more information, visit + --> $DIR/with-self-in-projection-output-repeated-supertrait.rs:27:10 + | +LL | type Output; + | ^^^^^^ ...because it has conflicting associated item bounds for `Output` in supertraits +... +LL | trait NormalizingHelper: Base::Out> + Base { + | ----------------- this trait is not dyn compatible... + = help: only type `u32` implements `NormalizingHelper`; consider using it directly instead. + +error[E0038]: the trait `NormalizingHelper` is not dyn compatible + --> $DIR/with-self-in-projection-output-repeated-supertrait.rs:49:21 + | +LL | let _y: Box> = Box::new(2u32); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `NormalizingHelper` is not dyn compatible + | +note: for a trait to be dyn compatible it needs to allow building a vtable + for more information, visit + --> $DIR/with-self-in-projection-output-repeated-supertrait.rs:27:10 + | +LL | type Output; + | ^^^^^^ ...because it has conflicting associated item bounds for `Output` in supertraits +... +LL | trait NormalizingHelper: Base::Out> + Base { + | ----------------- this trait is not dyn compatible... + = help: only type `u32` implements `NormalizingHelper`; consider using it directly instead. + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0038`.