Handle cycles in impl types better

 - impl Trait<Self> for S is allowed
 - impl Trait for S<Self> is an invalid cycle, but we can add cycle recovery for
   it in Salsa now

Author: flodiebold

crates/ra_hir_ty/src/db.rs

@@ -11,7 +11,7 @@ use ra_db::{salsa, CrateId};
 use crate::{
     method_resolution::CrateImplBlocks,
     traits::{AssocTyValue, Impl},
-    CallableDef, FnSig, GenericPredicate, ImplTy, InferenceResult, Substs, Ty, TyDefId, TypeCtor,
+    CallableDef, FnSig, GenericPredicate, InferenceResult, Substs, TraitRef, Ty, TyDefId, TypeCtor,
     ValueTyDefId,
 };
 
@@ -27,8 +27,12 @@ pub trait HirDatabase: DefDatabase {
     #[salsa::invoke(crate::lower::value_ty_query)]
     fn value_ty(&self, def: ValueTyDefId) -> Ty;
 
-    #[salsa::invoke(crate::lower::impl_ty_query)]
-    fn impl_ty(&self, def: ImplId) -> ImplTy;
+    #[salsa::invoke(crate::lower::impl_self_ty_query)]
+    #[salsa::cycle(crate::lower::impl_self_ty_recover)]
+    fn impl_self_ty(&self, def: ImplId) -> Ty;
+
+    #[salsa::invoke(crate::lower::impl_trait_query)]
+    fn impl_trait(&self, def: ImplId) -> Option<TraitRef>;
 
     #[salsa::invoke(crate::lower::field_types_query)]
     fn field_types(&self, var: VariantId) -> Arc<ArenaMap<LocalStructFieldId, Ty>>;

crates/ra_hir_ty/src/infer/coerce.rs

@@ -8,7 +8,7 @@ use hir_def::{lang_item::LangItemTarget, resolver::Resolver, type_ref::Mutabilit
 use rustc_hash::FxHashMap;
 use test_utils::tested_by;
 
-use crate::{autoderef, db::HirDatabase, ImplTy, Substs, Ty, TypeCtor, TypeWalk};
+use crate::{autoderef, db::HirDatabase, Substs, Ty, TypeCtor, TypeWalk};
 
 use super::{InEnvironment, InferTy, InferenceContext, TypeVarValue};
 
@@ -54,10 +54,7 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
         impls
             .iter()
             .filter_map(|&impl_id| {
-                let trait_ref = match db.impl_ty(impl_id) {
-                    ImplTy::TraitRef(it) => it,
-                    ImplTy::Inherent(_) => return None,
-                };
+                let trait_ref = db.impl_trait(impl_id)?;
 
                 // `CoerseUnsized` has one generic parameter for the target type.
                 let cur_from_ty = trait_ref.substs.0.get(0)?;

crates/ra_hir_ty/src/infer/path.rs

@@ -244,7 +244,7 @@ impl<'a, D: HirDatabase> InferenceContext<'a, D> {
                 ContainerId::ImplId(it) => it,
                 _ => return None,
             };
-            let self_ty = self.db.impl_ty(impl_id).self_type().clone();
+            let self_ty = self.db.impl_self_ty(impl_id).clone();
             let self_ty_substs = self_ty.substs()?;
             let actual_substs = actual_def_ty.substs()?;
 

crates/ra_hir_ty/src/lib.rs

@@ -486,21 +486,6 @@ impl TypeWalk for TraitRef {
     }
 }
 
-#[derive(Clone, PartialEq, Eq, Debug)]
-pub enum ImplTy {
-    Inherent(Ty),
-    TraitRef(TraitRef),
-}
-
-impl ImplTy {
-    pub(crate) fn self_type(&self) -> &Ty {
-        match self {
-            ImplTy::Inherent(it) => it,
-            ImplTy::TraitRef(tr) => &tr.substs[0],
-        }
-    }
-}
-
 /// Like `generics::WherePredicate`, but with resolved types: A condition on the
 /// parameters of a generic item.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]

crates/ra_hir_ty/src/lower.rs

@@ -27,8 +27,8 @@ use crate::{
         all_super_traits, associated_type_by_name_including_super_traits, make_mut_slice,
         variant_data,
     },
-    FnSig, GenericPredicate, ImplTy, ProjectionPredicate, ProjectionTy, Substs, TraitEnvironment,
-    TraitRef, Ty, TypeCtor, TypeWalk,
+    FnSig, GenericPredicate, ProjectionPredicate, ProjectionTy, Substs, TraitEnvironment, TraitRef,
+    Ty, TypeCtor, TypeWalk,
 };
 
 impl Ty {
@@ -179,7 +179,7 @@ impl Ty {
                 let name = resolved_segment.name.clone();
                 Ty::Param { idx, name }
             }
-            TypeNs::SelfType(impl_id) => db.impl_ty(impl_id).self_type().clone(),
+            TypeNs::SelfType(impl_id) => db.impl_self_ty(impl_id).clone(),
             TypeNs::AdtSelfType(adt) => db.ty(adt.into()),
 
             TypeNs::AdtId(it) => Ty::from_hir_path_inner(db, resolver, resolved_segment, it.into()),
@@ -743,17 +743,24 @@ pub(crate) fn value_ty_query(db: &impl HirDatabase, def: ValueTyDefId) -> Ty {
     }
 }
 
-pub(crate) fn impl_ty_query(db: &impl HirDatabase, impl_id: ImplId) -> ImplTy {
+pub(crate) fn impl_self_ty_query(db: &impl HirDatabase, impl_id: ImplId) -> Ty {
     let impl_data = db.impl_data(impl_id);
     let resolver = impl_id.resolver(db);
-    let self_ty = Ty::from_hir(db, &resolver, &impl_data.target_type);
-    match impl_data.target_trait.as_ref() {
-        Some(trait_ref) => {
-            match TraitRef::from_hir(db, &resolver, trait_ref, Some(self_ty.clone())) {
-                Some(it) => ImplTy::TraitRef(it),
-                None => ImplTy::Inherent(self_ty),
-            }
-        }
-        None => ImplTy::Inherent(self_ty),
-    }
+    Ty::from_hir(db, &resolver, &impl_data.target_type)
+}
+
+pub(crate) fn impl_self_ty_recover(
+    _db: &impl HirDatabase,
+    _cycle: &[String],
+    _impl_id: &ImplId,
+) -> Ty {
+    Ty::Unknown
+}
+
+pub(crate) fn impl_trait_query(db: &impl HirDatabase, impl_id: ImplId) -> Option<TraitRef> {
+    let impl_data = db.impl_data(impl_id);
+    let resolver = impl_id.resolver(db);
+    let self_ty = db.impl_self_ty(impl_id);
+    let target_trait = impl_data.target_trait.as_ref()?;
+    TraitRef::from_hir(db, &resolver, target_trait, Some(self_ty.clone()))
 }

crates/ra_hir_ty/src/method_resolution.rs

@@ -19,7 +19,7 @@ use crate::{
     db::HirDatabase,
     primitive::{FloatBitness, Uncertain},
     utils::all_super_traits,
-    Canonical, ImplTy, InEnvironment, TraitEnvironment, TraitRef, Ty, TypeCtor,
+    Canonical, InEnvironment, TraitEnvironment, TraitRef, Ty, TypeCtor,
 };
 
 /// This is used as a key for indexing impls.
@@ -58,11 +58,12 @@ impl CrateImplBlocks {
         let crate_def_map = db.crate_def_map(krate);
         for (_module_id, module_data) in crate_def_map.modules.iter() {
             for &impl_id in module_data.impls.iter() {
-                match db.impl_ty(impl_id) {
-                    ImplTy::TraitRef(tr) => {
+                match db.impl_trait(impl_id) {
+                    Some(tr) => {
                         res.impls_by_trait.entry(tr.trait_).or_default().push(impl_id);
                     }
-                    ImplTy::Inherent(self_ty) => {
+                    None => {
+                        let self_ty = db.impl_self_ty(impl_id);
                         if let Some(self_ty_fp) = TyFingerprint::for_impl(&self_ty) {
                             res.impls.entry(self_ty_fp).or_default().push(impl_id);
                         }

crates/ra_hir_ty/src/tests.rs

@@ -4675,6 +4675,48 @@ fn test<T, U>() where T::Item: Trait2, T: Trait<U::Item>, U: Trait<()> {
     assert_eq!(t, "u32");
 }
 
+#[test]
+fn trait_impl_self_ty() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait<T> {
+   fn foo(&self);
+}
+
+struct S;
+
+impl Trait<Self> for S {}
+
+fn test() {
+    S.foo()<|>;
+}
+"#,
+    );
+    assert_eq!(t, "()");
+}
+
+#[test]
+fn trait_impl_self_ty_cycle() {
+    let t = type_at(
+        r#"
+//- /main.rs
+trait Trait {
+   fn foo(&self);
+}
+
+struct S<T>;
+
+impl Trait for S<Self> {}
+
+fn test() {
+    S.foo()<|>;
+}
+"#,
+    );
+    assert_eq!(t, "{unknown}");
+}
+
 #[test]
 // FIXME this is currently a Salsa panic; it would be nicer if it just returned
 // in Unknown, and we should be able to do that once Salsa allows us to handle

crates/ra_hir_ty/src/traits/chalk.rs

@@ -20,8 +20,8 @@ use ra_db::salsa::{InternId, InternKey};
 
 use super::{AssocTyValue, Canonical, ChalkContext, Impl, Obligation};
 use crate::{
-    db::HirDatabase, display::HirDisplay, ApplicationTy, GenericPredicate, ImplTy, ProjectionTy,
-    Substs, TraitRef, Ty, TypeCtor, TypeWalk,
+    db::HirDatabase, display::HirDisplay, ApplicationTy, GenericPredicate, ProjectionTy, Substs,
+    TraitRef, Ty, TypeCtor, TypeWalk,
 };
 
 /// This represents a trait whose name we could not resolve.
@@ -630,10 +630,7 @@ fn impl_block_datum(
     chalk_id: chalk_ir::ImplId,
     impl_id: ImplId,
 ) -> Option<Arc<ImplDatum<ChalkIr>>> {
-    let trait_ref = match db.impl_ty(impl_id) {
-        ImplTy::TraitRef(it) => it,
-        ImplTy::Inherent(_) => return None,
-    };
+    let trait_ref = db.impl_trait(impl_id)?;
     let impl_data = db.impl_data(impl_id);
 
     let generic_params = db.generic_params(impl_id.into());
@@ -787,11 +784,7 @@ fn type_alias_associated_ty_value(
         _ => panic!("assoc ty value should be in impl"),
     };
 
-    let trait_ref = match db.impl_ty(impl_id) {
-        ImplTy::TraitRef(it) => it,
-        // we don't return any assoc ty values if the impl'd trait can't be resolved
-        ImplTy::Inherent(_) => panic!("assoc ty value should not exist"),
-    };
+    let trait_ref = db.impl_trait(impl_id).expect("assoc ty value should not exist"); // we don't return any assoc ty values if the impl'd trait can't be resolved
 
     let assoc_ty = db
         .trait_data(trait_ref.trait_)