Address some review comments

 - bring back tests I commented out
 - add a few comments
 - add an is_var helper

Author: flodiebold

chalk-ir/src/lib.rs

@@ -229,6 +229,14 @@ impl<I: Interner> Ty<I> {
         }
     }
 
+    /// Returns true if this is a `BoundVar` or `InferenceVar`.
+    pub fn is_var(&self, interner: &I) -> bool {
+        match self.data(interner) {
+            TyData::BoundVar(_) | TyData::InferenceVar(_) => true,
+            _ => false,
+        }
+    }
+
     pub fn is_alias(&self, interner: &I) -> bool {
         match self.data(interner) {
             TyData::Alias(..) => true,

chalk-solve/src/clauses.rs

@@ -233,6 +233,11 @@ fn program_clauses_that_could_match<I: Interner>(
                 // and `bounded_ty` is the `exists<T> { .. }`
                 // clauses shown above.
 
+                // Turn free BoundVars in the type into new existentials. E.g.
+                // we might get some `dyn Foo<?X>`, and we don't want to return
+                // a clause with a free variable. We can instead return a
+                // slightly more general clause by basically turning this into
+                // `exists<A> dyn Foo<A>`.
                 let generalized_dyn_ty = generalize::Generalize::apply(db.interner(), dyn_ty);
 
                 builder.push_binders(&generalized_dyn_ty, |builder, dyn_ty| {
@@ -278,7 +283,7 @@ fn program_clauses_that_could_match<I: Interner>(
             match_ty(builder, environment, ty)?
         }
         DomainGoal::FromEnv(_) => (), // Computed in the environment
-        DomainGoal::Normalize(Normalize { alias, ty }) => {
+        DomainGoal::Normalize(Normalize { alias, ty: _ }) => {
             // Normalize goals derive from `AssociatedTyValue` datums,
             // which are found in impls. That is, if we are
             // normalizing (e.g.) `<T as Iterator>::Item>`, then
@@ -294,30 +299,12 @@ fn program_clauses_that_could_match<I: Interner>(
             let trait_id = associated_ty_datum.trait_id;
             let trait_parameters = db.trait_parameters_from_projection(alias);
 
-            if (alias
-                .self_type_parameter(interner)
-                .bound(interner)
-                .is_some()
-                || alias
-                    .self_type_parameter(interner)
-                    .inference_var(interner)
-                    .is_some())
-                && (ty.bound(interner).is_some() || ty.inference_var(interner).is_some())
-            {
-                return Err(Floundered);
-            }
-
             let trait_datum = db.trait_datum(trait_id);
 
-            // FIXME
-            if (alias
-                .self_type_parameter(interner)
-                .bound(interner)
-                .is_some()
-                || alias
-                    .self_type_parameter(interner)
-                    .inference_var(interner)
-                    .is_some())
+            // Flounder if the self-type is unknown and the trait is non-enumerable.
+            //
+            // e.g., Normalize(<?X as Iterator>::Item = u32)
+            if (alias.self_type_parameter(interner).is_var(interner))
                 && trait_datum.is_non_enumerable_trait()
             {
                 return Err(Floundered);

chalk-solve/src/clauses/generalize.rs

@@ -2,7 +2,9 @@
 //! ones. We use this when building clauses that contain types passed to
 //! `program_clauses`; these may contain variables, and just copying those
 //! variables verbatim leads to problems. Instead, we return a slightly more
-//! general program clause, with new variables in those places.
+//! general program clause, with new variables in those places. This can only
+//! happen with `dyn Trait` currently; that's the only case where we use the
+//! types passed to `program_clauses` in the clauses we generate.
 
 use chalk_engine::fallible::Fallible;
 use chalk_ir::{

tests/test/projection.rs

@@ -76,7 +76,6 @@ fn normalize_basic() {
             "Unique"
         }
 
-        /* TODO: this doesn't seem to be correct, actually
         goal {
             forall<T> {
                 if (T: Iterator) {
@@ -86,10 +85,9 @@ fn normalize_basic() {
                 }
             }
         } yields {
-            // True for `U = T`, of course, but also true for `U = Vec<T>`.
+            // True for `U = T`, of course, but also true for `U = Vec<<T as Iterator>::Item>`.
             "Ambiguous"
         }
-        */
     }
 }
 
@@ -142,6 +140,9 @@ fn projection_equality() {
             exists<U> {
                 S: Trait2<U>
             }
+        } yields[SolverChoice::slg_default()] {
+            // this is wrong, chalk#234
+            "Ambiguous"
         } yields[SolverChoice::recursive()] {
             "Unique; substitution [?0 := u32]"
         }
@@ -167,6 +168,9 @@ fn projection_equality_from_env() {
                     }
                 }
             }
+        } yields[SolverChoice::slg_default()] {
+            // this is wrong, chalk#234
+            "Ambiguous"
         } yields[SolverChoice::recursive()] {
             "Unique; substitution [?0 := u32]"
         }
@@ -194,7 +198,10 @@ fn projection_equality_nested() {
                     }
                 }
             }
-        } yields[SolverChoice::recursive()] {
+        } yields[SolverChoice::slg_default()] {
+            // this is wrong, chalk#234
+            "Ambiguous"
+        }  yields[SolverChoice::recursive()] {
             "Unique; substitution [?0 := u32]"
         }
     }
@@ -235,6 +242,9 @@ fn iterator_flatten() {
                     }
                 }
             }
+        } yields[SolverChoice::slg_default()] {
+            // this is wrong, chalk#234
+            "Ambiguous"
         } yields[SolverChoice::recursive()] {
             "Unique; substitution [?0 := u32]"
         }
@@ -536,17 +546,18 @@ fn normalize_under_binder() {
             }
         }
 
-        /* TODO: this doesn't seem to be correct, actually
         goal {
             exists<U> {
                 forall<'a> {
                     Ref<'a, I32>: Deref<'a, Item = U>
                 }
             }
-        } yields {
+        } yields[SolverChoice::slg_default()] {
+            // chalk#234, I think
             "Ambiguous"
+        } yields[SolverChoice::recursive()] {
+            "Unique; substitution [?0 := I32], lifetime constraints []"
         }
-        */
 
         goal {
             exists<U> {
@@ -558,17 +569,18 @@ fn normalize_under_binder() {
             "Unique; substitution [?0 := I32], lifetime constraints []"
         }
 
-        /* TODO: this doesn't seem to be correct, actually
         goal {
             forall<'a> {
                 exists<U> {
                     Ref<'a, I32>: Id<'a, Item = U>
                 }
             }
-        } yields {
+        } yields[SolverChoice::slg_default()] {
+            // chalk#234, I think
             "Ambiguous"
+        } yields[SolverChoice::recursive()] {
+            "Unique; substitution [?0 := Ref<'!1_0, I32>], lifetime constraints []"
         }
-        */
 
         goal {
             forall<'a> {