add some comments

Author: lcnr

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 `<T as Trait>::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<'_>,

compiler/rustc_middle/src/ty/trait_def.rs

@@ -146,6 +146,11 @@ impl<'tcx> TyCtxt<'tcx> {
         self_ty: Ty<'tcx>,
         mut f: F,
     ) -> Option<T> {
+        // 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<T:Copy> Copy for Option<T>`, we replace the type variable
-        // in `Option<T>` 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<T>`, `Vec<T>`, `ConcreteType` and so on.
         if let Some(simp) =
             fast_reject::simplify_type(self, self_ty, SimplifyParams::Yes, StripReferences::No)
         {

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<ty::TraitRef<'tcx>> {
+        // 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(),