Auto merge of #86986 - lcnr:simplify_type, r=nikomatsakis,oli-obk

extend `simplify_type`

might cause a slight perf inprovement and imo more accurately represents what types there are.

considering that I was going to use this in #85048 it seems like we might need this in the future anyways 🤷

compiler/rustc_metadata/src/rmeta/encoder.rs

@@ -26,6 +26,7 @@ use rustc_middle::mir::interpret;
 use rustc_middle::thir;
 use rustc_middle::traits::specialization_graph;
 use rustc_middle::ty::codec::TyEncoder;
+use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences};
 use rustc_middle::ty::{self, SymbolName, Ty, TyCtxt};
 use rustc_serialize::{opaque, Encodable, Encoder};
 use rustc_session::config::CrateType;
@@ -2033,15 +2034,19 @@ impl EncodeContext<'a, 'tcx> {
 
 struct ImplVisitor<'tcx> {
     tcx: TyCtxt<'tcx>,
-    impls: FxHashMap<DefId, Vec<(DefIndex, Option<ty::fast_reject::SimplifiedType>)>>,
+    impls: FxHashMap<DefId, Vec<(DefIndex, Option<fast_reject::SimplifiedType>)>>,
 }
 
 impl<'tcx, 'v> ItemLikeVisitor<'v> for ImplVisitor<'tcx> {
     fn visit_item(&mut self, item: &hir::Item<'_>) {
         if let hir::ItemKind::Impl { .. } = item.kind {
             if let Some(trait_ref) = self.tcx.impl_trait_ref(item.def_id.to_def_id()) {
-                let simplified_self_ty =
-                    ty::fast_reject::simplify_type(self.tcx, trait_ref.self_ty(), false);
+                let simplified_self_ty = fast_reject::simplify_type(
+                    self.tcx,
+                    trait_ref.self_ty(),
+                    SimplifyParams::No,
+                    StripReferences::No,
+                );
 
                 self.impls
                     .entry(trait_ref.def_id)

compiler/rustc_middle/src/ty/fast_reject.rs

@@ -1,3 +1,4 @@
+use crate::mir::Mutability;
 use crate::ty::{self, Ty, TyCtxt};
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_hir::def_id::DefId;
@@ -27,9 +28,12 @@ where
     UintSimplifiedType(ty::UintTy),
     FloatSimplifiedType(ty::FloatTy),
     AdtSimplifiedType(D),
+    ForeignSimplifiedType(D),
     StrSimplifiedType,
     ArraySimplifiedType,
-    PtrSimplifiedType,
+    SliceSimplifiedType,
+    RefSimplifiedType(Mutability),
+    PtrSimplifiedType(Mutability),
     NeverSimplifiedType,
     TupleSimplifiedType(usize),
     /// A trait object, all of whose components are markers
@@ -42,22 +46,48 @@ where
     OpaqueSimplifiedType(D),
     FunctionSimplifiedType(usize),
     ParameterSimplifiedType,
-    ForeignSimplifiedType(DefId),
 }
 
-/// 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.
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+pub enum SimplifyParams {
+    Yes,
+    No,
+}
+
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+pub enum StripReferences {
+    Yes,
+    No,
+}
+
+/// Tries to simplify a type by only returning the outermost injective¹ layer, if one exists.
+///
+/// The idea is to get something simple that we can use to quickly decide if two types could unify,
+/// for example during method lookup.
 ///
-/// 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.
+/// 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<'_>,
-    can_simplify_params: bool,
+    can_simplify_params: SimplifyParams,
+    strip_references: StripReferences,
 ) -> Option<SimplifiedType> {
     match *ty.kind() {
         ty::Bool => Some(BoolSimplifiedType),
@@ -67,19 +97,24 @@ pub fn simplify_type(
         ty::Float(float_type) => Some(FloatSimplifiedType(float_type)),
         ty::Adt(def, _) => Some(AdtSimplifiedType(def.did)),
         ty::Str => Some(StrSimplifiedType),
-        ty::Array(..) | ty::Slice(_) => Some(ArraySimplifiedType),
-        ty::RawPtr(_) => Some(PtrSimplifiedType),
+        ty::Array(..) => Some(ArraySimplifiedType),
+        ty::Slice(..) => Some(SliceSimplifiedType),
+        ty::RawPtr(ptr) => Some(PtrSimplifiedType(ptr.mutbl)),
         ty::Dynamic(ref trait_info, ..) => match trait_info.principal_def_id() {
             Some(principal_def_id) if !tcx.trait_is_auto(principal_def_id) => {
                 Some(TraitSimplifiedType(principal_def_id))
             }
             _ => Some(MarkerTraitObjectSimplifiedType),
         },
-        ty::Ref(_, ty, _) => {
-            // since we introduce auto-refs during method lookup, we
-            // just treat &T and T as equivalent from the point of
-            // view of possibly unifying
-            simplify_type(tcx, ty, can_simplify_params)
+        ty::Ref(_, ty, mutbl) => {
+            if strip_references == StripReferences::Yes {
+                // For diagnostics, when recommending similar impls we want to
+                // recommend impls even when there is a reference mismatch,
+                // so we treat &T and T equivalently in that case.
+                simplify_type(tcx, ty, can_simplify_params, strip_references)
+            } else {
+                Some(RefSimplifiedType(mutbl))
+            }
         }
         ty::FnDef(def_id, _) | ty::Closure(def_id, _) => Some(ClosureSimplifiedType(def_id)),
         ty::Generator(def_id, _, _) => Some(GeneratorSimplifiedType(def_id)),
@@ -90,7 +125,7 @@ pub fn simplify_type(
         ty::Tuple(ref tys) => Some(TupleSimplifiedType(tys.len())),
         ty::FnPtr(ref f) => Some(FunctionSimplifiedType(f.skip_binder().inputs().len())),
         ty::Projection(_) | ty::Param(_) => {
-            if can_simplify_params {
+            if can_simplify_params == SimplifyParams::Yes {
                 // In normalized types, projections don't unify with
                 // anything. when lazy normalization happens, this
                 // will change. It would still be nice to have a way
@@ -120,9 +155,12 @@ impl<D: Copy + Debug + Ord + Eq> SimplifiedTypeGen<D> {
             UintSimplifiedType(t) => UintSimplifiedType(t),
             FloatSimplifiedType(t) => FloatSimplifiedType(t),
             AdtSimplifiedType(d) => AdtSimplifiedType(map(d)),
+            ForeignSimplifiedType(d) => ForeignSimplifiedType(map(d)),
             StrSimplifiedType => StrSimplifiedType,
             ArraySimplifiedType => ArraySimplifiedType,
-            PtrSimplifiedType => PtrSimplifiedType,
+            SliceSimplifiedType => SliceSimplifiedType,
+            RefSimplifiedType(m) => RefSimplifiedType(m),
+            PtrSimplifiedType(m) => PtrSimplifiedType(m),
             NeverSimplifiedType => NeverSimplifiedType,
             MarkerTraitObjectSimplifiedType => MarkerTraitObjectSimplifiedType,
             TupleSimplifiedType(n) => TupleSimplifiedType(n),
@@ -133,7 +171,6 @@ impl<D: Copy + Debug + Ord + Eq> SimplifiedTypeGen<D> {
             OpaqueSimplifiedType(d) => OpaqueSimplifiedType(map(d)),
             FunctionSimplifiedType(n) => FunctionSimplifiedType(n),
             ParameterSimplifiedType => ParameterSimplifiedType,
-            ForeignSimplifiedType(d) => ForeignSimplifiedType(d),
         }
     }
 }
@@ -149,12 +186,13 @@ where
             | CharSimplifiedType
             | StrSimplifiedType
             | ArraySimplifiedType
-            | PtrSimplifiedType
+            | SliceSimplifiedType
             | NeverSimplifiedType
             | ParameterSimplifiedType
             | MarkerTraitObjectSimplifiedType => {
                 // nothing to do
             }
+            RefSimplifiedType(m) | PtrSimplifiedType(m) => m.hash_stable(hcx, hasher),
             IntSimplifiedType(t) => t.hash_stable(hcx, hasher),
             UintSimplifiedType(t) => t.hash_stable(hcx, hasher),
             FloatSimplifiedType(t) => t.hash_stable(hcx, hasher),

compiler/rustc_middle/src/ty/trait_def.rs

@@ -1,5 +1,5 @@
 use crate::traits::specialization_graph;
-use crate::ty::fast_reject;
+use crate::ty::fast_reject::{self, SimplifyParams, StripReferences};
 use crate::ty::fold::TypeFoldable;
 use crate::ty::{Ty, TyCtxt};
 use rustc_hir as hir;
@@ -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,32 +159,16 @@ 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.
-        if let Some(simp) = fast_reject::simplify_type(self, self_ty, true) {
+        // 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)
+        {
             if let Some(impls) = impls.non_blanket_impls.get(&simp) {
                 for &impl_def_id in impls {
                     if let result @ Some(_) = f(impl_def_id) {
@@ -238,7 +227,9 @@ pub(super) fn trait_impls_of_provider(tcx: TyCtxt<'_>, trait_id: DefId) -> Trait
             continue;
         }
 
-        if let Some(simplified_self_ty) = fast_reject::simplify_type(tcx, impl_self_ty, false) {
+        if let Some(simplified_self_ty) =
+            fast_reject::simplify_type(tcx, impl_self_ty, SimplifyParams::No, StripReferences::No)
+        {
             impls.non_blanket_impls.entry(simplified_self_ty).or_default().push(impl_def_id);
         } else {
             impls.blanket_impls.push(impl_def_id);

compiler/rustc_trait_selection/src/traits/coherence.rs

@@ -12,9 +12,10 @@ use crate::traits::{
     self, Normalized, Obligation, ObligationCause, PredicateObligation, SelectionContext,
 };
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
+use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences};
 use rustc_middle::ty::fold::TypeFoldable;
 use rustc_middle::ty::subst::Subst;
-use rustc_middle::ty::{self, fast_reject, Ty, TyCtxt};
+use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_span::symbol::sym;
 use rustc_span::DUMMY_SP;
 use std::iter;
@@ -82,12 +83,11 @@ where
         impl2_ref.iter().flat_map(|tref| tref.substs.types()),
     )
     .any(|(ty1, ty2)| {
-        let t1 = fast_reject::simplify_type(tcx, ty1, false);
-        let t2 = fast_reject::simplify_type(tcx, ty2, false);
+        let t1 = fast_reject::simplify_type(tcx, ty1, SimplifyParams::No, StripReferences::No);
+        let t2 = fast_reject::simplify_type(tcx, ty2, SimplifyParams::No, StripReferences::No);
         if let (Some(t1), Some(t2)) = (t1, t2) {
             // Simplified successfully
-            // Types cannot unify if they differ in their reference mutability or simplify to different types
-            t1 != t2 || ty1.ref_mutability() != ty2.ref_mutability()
+            t1 != t2
         } else {
             // Types might unify
             false

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -21,10 +21,10 @@ use rustc_hir::Item;
 use rustc_hir::Node;
 use rustc_middle::thir::abstract_const::NotConstEvaluatable;
 use rustc_middle::ty::error::ExpectedFound;
+use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences};
 use rustc_middle::ty::fold::TypeFolder;
 use rustc_middle::ty::{
-    self, fast_reject, AdtKind, SubtypePredicate, ToPolyTraitRef, ToPredicate, Ty, TyCtxt,
-    TypeFoldable,
+    self, AdtKind, SubtypePredicate, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, TypeFoldable,
 };
 use rustc_session::DiagnosticMessageId;
 use rustc_span::symbol::{kw, sym};
@@ -1440,14 +1440,32 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
         &self,
         trait_ref: ty::PolyTraitRef<'tcx>,
     ) -> Vec<ty::TraitRef<'tcx>> {
-        let simp = fast_reject::simplify_type(self.tcx, trait_ref.skip_binder().self_ty(), true);
+        // 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(),
+            SimplifyParams::Yes,
+            StripReferences::Yes,
+        );
         let all_impls = self.tcx.all_impls(trait_ref.def_id());
 
         match simp {
             Some(simp) => all_impls
                 .filter_map(|def_id| {
                     let imp = self.tcx.impl_trait_ref(def_id).unwrap();
-                    let imp_simp = fast_reject::simplify_type(self.tcx, imp.self_ty(), true);
+                    let imp_simp = fast_reject::simplify_type(
+                        self.tcx,
+                        imp.self_ty(),
+                        SimplifyParams::Yes,
+                        StripReferences::Yes,
+                    );
                     if let Some(imp_simp) = imp_simp {
                         if simp != imp_simp {
                             return None;

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -35,7 +35,7 @@ use rustc_infer::infer::LateBoundRegionConversionTime;
 use rustc_middle::dep_graph::{DepKind, DepNodeIndex};
 use rustc_middle::mir::interpret::ErrorHandled;
 use rustc_middle::thir::abstract_const::NotConstEvaluatable;
-use rustc_middle::ty::fast_reject;
+use rustc_middle::ty::fast_reject::{self, SimplifyParams, StripReferences};
 use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::relate::TypeRelation;
 use rustc_middle::ty::subst::{GenericArgKind, Subst, SubstsRef};
@@ -2089,10 +2089,21 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
             |(obligation_arg, impl_arg)| {
                 match (obligation_arg.unpack(), impl_arg.unpack()) {
                     (GenericArgKind::Type(obligation_ty), GenericArgKind::Type(impl_ty)) => {
-                        let simplified_obligation_ty =
-                            fast_reject::simplify_type(self.tcx(), obligation_ty, true);
-                        let simplified_impl_ty =
-                            fast_reject::simplify_type(self.tcx(), impl_ty, false);
+                        // Note, we simplify parameters for the obligation but not the
+                        // impl so that we do not reject a blanket impl but do reject
+                        // more concrete impls if we're searching for `T: Trait`.
+                        let simplified_obligation_ty = fast_reject::simplify_type(
+                            self.tcx(),
+                            obligation_ty,
+                            SimplifyParams::Yes,
+                            StripReferences::No,
+                        );
+                        let simplified_impl_ty = fast_reject::simplify_type(
+                            self.tcx(),
+                            impl_ty,
+                            SimplifyParams::No,
+                            StripReferences::No,
+                        );
 
                         simplified_obligation_ty.is_some()
                             && simplified_impl_ty.is_some()