diff --git a/compiler/rustc_middle/src/ty/fast_reject.rs b/compiler/rustc_middle/src/ty/fast_reject.rs index 243bc14754031..c4043d9698cc1 100644 --- a/compiler/rustc_middle/src/ty/fast_reject.rs +++ b/compiler/rustc_middle/src/ty/fast_reject.rs @@ -60,15 +60,29 @@ pub enum StripReferences { No, } -/// Tries to simplify a type by dropping type parameters, deref'ing away any reference types, etc. -/// The idea is to get something simple that we can use to quickly decide if two types could unify -/// during method lookup. +/// Tries to simplify a type by only returning the outermost injective¹ layer, if one exists. /// -/// If `can_simplify_params` is false, then we will fail to simplify type parameters entirely. This -/// is useful when those type parameters would be instantiated with fresh type variables, since -/// then we can't say much about whether two types would unify. Put another way, -/// `can_simplify_params` should be true if type parameters appear free in `ty` and `false` if they -/// are to be considered bound. +/// The idea is to get something simple that we can use to quickly decide if two types could unify, +/// for example during method lookup. +/// +/// A special case here are parameters and projections. Projections can be normalized to +/// a different type, meaning that `::Assoc` and `u8` can be unified, even though +/// their outermost layer is different while parameters like `T` of impls are later replaced +/// with an inference variable, which then also allows unification with other types. +/// +/// When using `SimplifyParams::Yes`, we still return a simplified type for params and projections², +/// the reasoning for this can be seen at the places doing this. +/// +/// For diagnostics we strip references with `StripReferences::Yes`. This is currently the best +/// way to skip some unhelpful suggestions. +/// +/// ¹ meaning that if two outermost layers are different, then the whole types are also different. +/// ² FIXME(@lcnr): this seems like it can actually end up being unsound with the way it's used during +/// candidate selection. We do not consider non blanket impls for `<_ as Trait>::Assoc` even +/// though `_` can be inferred to a concrete type later at which point a concrete impl +/// could actually apply. After experimenting for about an hour I wasn't able to cause any issues +/// this way so I am not going to change this until we actually find an issue as I am really +/// interesting in getting an actual test for this. pub fn simplify_type( tcx: TyCtxt<'_>, ty: Ty<'_>, diff --git a/compiler/rustc_middle/src/ty/trait_def.rs b/compiler/rustc_middle/src/ty/trait_def.rs index 4a415b7a22843..cbb88def7e27d 100644 --- a/compiler/rustc_middle/src/ty/trait_def.rs +++ b/compiler/rustc_middle/src/ty/trait_def.rs @@ -146,6 +146,11 @@ impl<'tcx> TyCtxt<'tcx> { self_ty: Ty<'tcx>, mut f: F, ) -> Option { + // FIXME: This depends on the set of all impls for the trait. That is + // unfortunate wrt. incremental compilation. + // + // If we want to be faster, we could have separate queries for + // blanket and non-blanket impls, and compare them separately. let impls = self.trait_impls_of(def_id); for &impl_def_id in impls.blanket_impls.iter() { @@ -154,31 +159,13 @@ impl<'tcx> TyCtxt<'tcx> { } } - // simplify_type(.., false) basically replaces type parameters and - // projections with infer-variables. This is, of course, done on - // the impl trait-ref when it is instantiated, but not on the - // predicate trait-ref which is passed here. - // - // for example, if we match `S: Copy` against an impl like - // `impl Copy for Option`, we replace the type variable - // in `Option` with an infer variable, to `Option<_>` (this - // doesn't actually change fast_reject output), but we don't - // replace `S` with anything - this impl of course can't be - // selected, and as there are hundreds of similar impls, - // considering them would significantly harm performance. - - // This depends on the set of all impls for the trait. That is - // unfortunate. When we get red-green recompilation, we would like - // to have a way of knowing whether the set of relevant impls - // changed. The most naive - // way would be to compute the Vec of relevant impls and see whether - // it differs between compilations. That shouldn't be too slow by - // itself - we do quite a bit of work for each relevant impl anyway. - // - // If we want to be faster, we could have separate queries for - // blanket and non-blanket impls, and compare them separately. + // Note that we're using `SimplifyParams::Yes` to query `non_blanket_impls` while using + // `SimplifyParams::No` while actually adding them. // - // I think we'll cross that bridge when we get to it. + // This way, when searching for some impl for `T: Trait`, we do not look at any impls + // whose outer level is not a parameter or projection. Especially for things like + // `T: Clone` this is incredibly useful as we would otherwise look at all the impls + // of `Clone` for `Option`, `Vec`, `ConcreteType` and so on. if let Some(simp) = fast_reject::simplify_type(self, self_ty, SimplifyParams::Yes, StripReferences::No) { diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 00b469c686ad1..5893dca8dbf67 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -1440,6 +1440,14 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> { &self, trait_ref: ty::PolyTraitRef<'tcx>, ) -> Vec> { + // We simplify params and strip references here. + // + // This both removes a lot of unhelpful suggestions, e.g. + // when searching for `&Foo: Trait` it doesn't suggestion `impl Trait for &Bar`, + // while also suggesting impls for `&Foo` when we're looking for `Foo: Trait`. + // + // The second thing isn't necessarily always a good thing, but + // any other simple setup results in a far worse output, so 🤷 let simp = fast_reject::simplify_type( self.tcx, trait_ref.skip_binder().self_ty(),