Merge #8133

8133: Ignore type bindings in generic_predicates_for_param (fix panic on ena and crates depending on it) r=flodiebold a=flodiebold

This allows us to handle more cases without a query cycle, which includes certain cases that rustc accepted. That in turn means we avoid triggering salsa-rs/salsa#257 on valid code (it will still happen if the user writes an actual cycle).

We actually accept more definitions than rustc now; that's because rustc only ignores bindings when looking up super traits, whereas we now also ignore them when looking for predicates to disambiguate associated type shorthand. We could introduce a separate query for super traits if necessary, but for now I think this should be fine.

Co-authored-by: Florian Diebold <flodiebold@gmail.com>

crates/hir/src/lib.rs

@@ -2068,7 +2068,10 @@ impl Type {
                 match pred {
                     WhereClause::Implemented(trait_ref) => {
                         cb(type_.clone());
-                        walk_substs(db, type_, &trait_ref.substitution, cb);
+                        // skip the self type. it's likely the type we just got the bounds from
+                        for ty in trait_ref.substitution.iter().skip(1) {
+                            walk_type(db, &type_.derived(ty.clone()), cb);
+                        }
                     }
                     _ => (),
                 }

crates/hir_ty/src/display.rs

@@ -571,13 +571,22 @@ impl HirDisplay for Ty {
                         write!(f, "{}", param_data.name.clone().unwrap_or_else(Name::missing))?
                     }
                     TypeParamProvenance::ArgumentImplTrait => {
-                        let bounds = f.db.generic_predicates_for_param(id);
                         let substs = Substitution::type_params_for_generics(f.db, &generics);
-                        write_bounds_like_dyn_trait_with_prefix(
-                            "impl",
-                            &bounds.iter().map(|b| b.clone().subst(&substs)).collect::<Vec<_>>(),
-                            f,
-                        )?;
+                        let bounds = f
+                            .db
+                            .generic_predicates(id.parent)
+                            .into_iter()
+                            .map(|pred| pred.clone().subst(&substs))
+                            .filter(|wc| match &wc {
+                                WhereClause::Implemented(tr) => tr.self_type_parameter() == self,
+                                WhereClause::AliasEq(AliasEq {
+                                    alias: AliasTy::Projection(proj),
+                                    ty: _,
+                                }) => proj.self_type_parameter() == self,
+                                _ => false,
+                            })
+                            .collect::<Vec<_>>();
+                        write_bounds_like_dyn_trait_with_prefix("impl", &bounds, f)?;
                     }
                 }
             }

crates/hir_ty/src/lib.rs

@@ -106,6 +106,10 @@ impl ProjectionTy {
         }
     }
 
+    pub fn self_type_parameter(&self) -> &Ty {
+        &self.substitution[0]
+    }
+
     fn trait_(&self, db: &dyn HirDatabase) -> TraitId {
         match from_assoc_type_id(self.associated_ty_id).lookup(db.upcast()).container {
             AssocContainerId::TraitId(it) => it,
@@ -936,10 +940,19 @@ impl Ty {
                 let param_data = &generic_params.types[id.local_id];
                 match param_data.provenance {
                     hir_def::generics::TypeParamProvenance::ArgumentImplTrait => {
+                        let substs = Substitution::type_params(db, id.parent);
                         let predicates = db
-                            .generic_predicates_for_param(id)
+                            .generic_predicates(id.parent)
                             .into_iter()
-                            .map(|pred| pred.value.clone())
+                            .map(|pred| pred.clone().subst(&substs))
+                            .filter(|wc| match &wc {
+                                WhereClause::Implemented(tr) => tr.self_type_parameter() == self,
+                                WhereClause::AliasEq(AliasEq {
+                                    alias: AliasTy::Projection(proj),
+                                    ty: _,
+                                }) => proj.self_type_parameter() == self,
+                                _ => false,
+                            })
                             .collect_vec();
 
                         Some(predicates)

crates/hir_ty/src/lower.rs

@@ -189,7 +189,10 @@ impl<'a> TyLoweringContext<'a> {
                 let self_ty =
                     TyKind::BoundVar(BoundVar::new(DebruijnIndex::INNERMOST, 0)).intern(&Interner);
                 let predicates = self.with_shifted_in(DebruijnIndex::ONE, |ctx| {
-                    bounds.iter().flat_map(|b| ctx.lower_type_bound(b, self_ty.clone())).collect()
+                    bounds
+                        .iter()
+                        .flat_map(|b| ctx.lower_type_bound(b, self_ty.clone(), false))
+                        .collect()
                 });
                 TyKind::Dyn(predicates).intern(&Interner)
             }
@@ -666,6 +669,7 @@ impl<'a> TyLoweringContext<'a> {
     pub(crate) fn lower_where_predicate(
         &'a self,
         where_predicate: &'a WherePredicate,
+        ignore_bindings: bool,
     ) -> impl Iterator<Item = WhereClause> + 'a {
         match where_predicate {
             WherePredicate::ForLifetime { target, bound, .. }
@@ -688,7 +692,9 @@ impl<'a> TyLoweringContext<'a> {
                         .intern(&Interner)
                     }
                 };
-                self.lower_type_bound(bound, self_ty).collect::<Vec<_>>().into_iter()
+                self.lower_type_bound(bound, self_ty, ignore_bindings)
+                    .collect::<Vec<_>>()
+                    .into_iter()
             }
             WherePredicate::Lifetime { .. } => vec![].into_iter(),
         }
@@ -698,6 +704,7 @@ impl<'a> TyLoweringContext<'a> {
         &'a self,
         bound: &'a TypeBound,
         self_ty: Ty,
+        ignore_bindings: bool,
     ) -> impl Iterator<Item = WhereClause> + 'a {
         let mut bindings = None;
         let trait_ref = match bound {
@@ -711,6 +718,7 @@ impl<'a> TyLoweringContext<'a> {
         trait_ref.into_iter().chain(
             bindings
                 .into_iter()
+                .filter(move |_| !ignore_bindings)
                 .flat_map(move |tr| self.assoc_type_bindings_from_type_bound(bound, tr)),
         )
     }
@@ -755,6 +763,7 @@ impl<'a> TyLoweringContext<'a> {
                     preds.extend(self.lower_type_bound(
                         bound,
                         TyKind::Alias(AliasTy::Projection(projection_ty.clone())).intern(&Interner),
+                        false,
                     ));
                 }
                 preds
@@ -766,7 +775,7 @@ impl<'a> TyLoweringContext<'a> {
         let self_ty =
             TyKind::BoundVar(BoundVar::new(DebruijnIndex::INNERMOST, 0)).intern(&Interner);
         let predicates = self.with_shifted_in(DebruijnIndex::ONE, |ctx| {
-            bounds.iter().flat_map(|b| ctx.lower_type_bound(b, self_ty.clone())).collect()
+            bounds.iter().flat_map(|b| ctx.lower_type_bound(b, self_ty.clone(), false)).collect()
         });
         ReturnTypeImplTrait { bounds: Binders::new(1, predicates) }
     }
@@ -896,7 +905,9 @@ pub(crate) fn generic_predicates_for_param_query(
             },
             WherePredicate::Lifetime { .. } => false,
         })
-        .flat_map(|pred| ctx.lower_where_predicate(pred).map(|p| Binders::new(generics.len(), p)))
+        .flat_map(|pred| {
+            ctx.lower_where_predicate(pred, true).map(|p| Binders::new(generics.len(), p))
+        })
         .collect()
 }
 
@@ -918,7 +929,7 @@ pub(crate) fn trait_environment_query(
     let mut traits_in_scope = Vec::new();
     let mut clauses = Vec::new();
     for pred in resolver.where_predicates_in_scope() {
-        for pred in ctx.lower_where_predicate(pred) {
+        for pred in ctx.lower_where_predicate(pred, false) {
             if let WhereClause::Implemented(tr) = &pred {
                 traits_in_scope.push((tr.self_type_parameter().clone(), tr.hir_trait_id()));
             }
@@ -966,7 +977,9 @@ pub(crate) fn generic_predicates_query(
     let generics = generics(db.upcast(), def);
     resolver
         .where_predicates_in_scope()
-        .flat_map(|pred| ctx.lower_where_predicate(pred).map(|p| Binders::new(generics.len(), p)))
+        .flat_map(|pred| {
+            ctx.lower_where_predicate(pred, false).map(|p| Binders::new(generics.len(), p))
+        })
         .collect()
 }
 

crates/hir_ty/src/tests.rs

@@ -369,3 +369,72 @@ fn check_infer_with_mismatches(ra_fixture: &str, expect: Expect) {
     actual.push('\n');
     expect.assert_eq(&actual);
 }
+
+#[test]
+fn salsa_bug() {
+    let (mut db, pos) = TestDB::with_position(
+        "
+        //- /lib.rs
+        trait Index {
+            type Output;
+        }
+
+        type Key<S: UnificationStoreBase> = <S as UnificationStoreBase>::Key;
+
+        pub trait UnificationStoreBase: Index<Output = Key<Self>> {
+            type Key;
+
+            fn len(&self) -> usize;
+        }
+
+        pub trait UnificationStoreMut: UnificationStoreBase {
+            fn push(&mut self, value: Self::Key);
+        }
+
+        fn main() {
+            let x = 1;
+            x.push(1);$0
+        }
+    ",
+    );
+
+    let module = db.module_for_file(pos.file_id);
+    let crate_def_map = module.def_map(&db);
+    visit_module(&db, &crate_def_map, module.local_id, &mut |def| {
+        db.infer(def);
+    });
+
+    let new_text = "
+        //- /lib.rs
+        trait Index {
+            type Output;
+        }
+
+        type Key<S: UnificationStoreBase> = <S as UnificationStoreBase>::Key;
+
+        pub trait UnificationStoreBase: Index<Output = Key<Self>> {
+            type Key;
+
+            fn len(&self) -> usize;
+        }
+
+        pub trait UnificationStoreMut: UnificationStoreBase {
+            fn push(&mut self, value: Self::Key);
+        }
+
+        fn main() {
+
+            let x = 1;
+            x.push(1);
+        }
+    "
+    .to_string();
+
+    db.set_file_text(pos.file_id, Arc::new(new_text));
+
+    let module = db.module_for_file(pos.file_id);
+    let crate_def_map = module.def_map(&db);
+    visit_module(&db, &crate_def_map, module.local_id, &mut |def| {
+        db.infer(def);
+    });
+}

crates/hir_ty/src/tests/traits.rs

@@ -2271,6 +2271,56 @@ fn test<T, U>() where T: Trait<U::Item>, U: Trait<T::Item> {
     );
 }
 
+#[test]
+fn unselected_projection_in_trait_env_cycle_3() {
+    // this is a cycle for rustc; we currently accept it
+    check_types(
+        r#"
+//- /main.rs
+trait Trait {
+    type Item;
+    type OtherItem;
+}
+
+fn test<T>() where T: Trait<OtherItem = T::Item> {
+    let x: T::Item = no_matter;
+}                   //^ Trait::Item<T>
+"#,
+    );
+}
+
+#[test]
+fn unselected_projection_in_trait_env_no_cycle() {
+    // this is not a cycle
+    check_types(
+        r#"
+//- /main.rs
+trait Index {
+    type Output;
+}
+
+type Key<S: UnificationStoreBase> = <S as UnificationStoreBase>::Key;
+
+pub trait UnificationStoreBase: Index<Output = Key<Self>> {
+    type Key;
+
+    fn len(&self) -> usize;
+}
+
+pub trait UnificationStoreMut: UnificationStoreBase {
+    fn push(&mut self, value: Self::Key);
+}
+
+fn test<T>(t: T) where T: UnificationStoreMut {
+    let x;
+    t.push(x);
+    let y: Key<T>;
+    (x, y);
+}      //^ (UnificationStoreBase::Key<T>, UnificationStoreBase::Key<T>)
+"#,
+    );
+}
+
 #[test]
 fn inline_assoc_type_bounds_1() {
     check_types(

crates/hir_ty/src/traits/chalk.rs

@@ -395,7 +395,7 @@ pub(crate) fn associated_ty_data_query(
     let bounds = type_alias_data
         .bounds
         .iter()
-        .flat_map(|bound| ctx.lower_type_bound(bound, self_ty.clone()))
+        .flat_map(|bound| ctx.lower_type_bound(bound, self_ty.clone(), false))
         .filter_map(|pred| generic_predicate_to_inline_bound(db, &pred, &self_ty))
         .map(|bound| make_binders(bound.shifted_in(&Interner), 0))
         .collect();