fix: Impl search no longer selects an impl if multiple are applicable (#4662)

jfecher · TomAFrench · commit d251309dae57 · 2024-04-03T23:56:20.000+01:00
# Description ## Problem\* Resolves #4653 ## Summary\* If the object type is an unbound type variable, impl search currently just chooses the first available matching impl instead of erroring that type annotations are needed or similar. This can lead to confusing situations where the type chosen is chosen just because it was the first impl in the stdlib to be defined. This PR prevents that. ## Additional Context This PR depends on #4648 and should be merged after. ~~The `hashmap` test is currently failing because the `H: Hasher` constraint on its `Eq` implementation ~~is actually unsolvable since `H` isn't mentioned in the hashmap type at all~~. It is solvable since it is meantioned in `B`, although solving it so far has lead to errors during monomorphization. Previously it was fine since H was unbound and the first/only impl was just always chosen. Now I'll need to change or remove H to fix it.~~ This PR is now ready to review. I've been debugging a bug with monomorphizing some HashMap code for a little while but it turns out the bug is also present in master. So I'm considering it a separate issue: #4663 ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs
@@ -126,7 +126,7 @@ impl<'interner> TypeChecker<'interner> {
                     }
                 }
                 HirLiteral::Bool(_) => Type::Bool,
-                HirLiteral::Integer(_, _) => Type::polymorphic_integer_or_field(self.interner),
+                HirLiteral::Integer(_, _) => self.polymorphic_integer_or_field(),
                 HirLiteral::Str(string) => {
                     let len = Type::Constant(string.len() as u64);
                     Type::String(Box::new(len))
@@ -418,23 +418,28 @@ impl<'interner> TypeChecker<'interner> {
                 self.interner.select_impl_for_expression(function_ident_id, impl_kind);
             }
             Err(erroring_constraints) => {
-                // Don't show any errors where try_get_trait returns None.
-                // This can happen if a trait is used that was never declared.
-                let constraints = erroring_constraints
-                    .into_iter()
-                    .map(|constraint| {
-                        let r#trait = self.interner.try_get_trait(constraint.trait_id)?;
-                        let mut name = r#trait.name.to_string();
-                        if !constraint.trait_generics.is_empty() {
-                            let generics = vecmap(&constraint.trait_generics, ToString::to_string);
-                            name += &format!("<{}>", generics.join(", "));
-                        }
-                        Some((constraint.typ, name))
-                    })
-                    .collect::<Option<Vec<_>>>();
+                if erroring_constraints.is_empty() {
+                    self.errors.push(TypeCheckError::TypeAnnotationsNeeded { span });
+                } else {
+                    // Don't show any errors where try_get_trait returns None.
+                    // This can happen if a trait is used that was never declared.
+                    let constraints = erroring_constraints
+                        .into_iter()
+                        .map(|constraint| {
+                            let r#trait = self.interner.try_get_trait(constraint.trait_id)?;
+                            let mut name = r#trait.name.to_string();
+                            if !constraint.trait_generics.is_empty() {
+                                let generics =
+                                    vecmap(&constraint.trait_generics, ToString::to_string);
+                                name += &format!("<{}>", generics.join(", "));
+                            }
+                            Some((constraint.typ, name))
+                        })
+                        .collect::<Option<Vec<_>>>();
 
-                if let Some(constraints) = constraints {
-                    self.errors.push(TypeCheckError::NoMatchingImplFound { constraints, span });
+                    if let Some(constraints) = constraints {
+                        self.errors.push(TypeCheckError::NoMatchingImplFound { constraints, span });
+                    }
                 }
             }
         }
@@ -560,15 +565,13 @@ impl<'interner> TypeChecker<'interner> {
         let index_type = self.check_expression(&index_expr.index);
         let span = self.interner.expr_span(&index_expr.index);
 
-        index_type.unify(
-            &Type::polymorphic_integer_or_field(self.interner),
-            &mut self.errors,
-            || TypeCheckError::TypeMismatch {
+        index_type.unify(&self.polymorphic_integer_or_field(), &mut self.errors, || {
+            TypeCheckError::TypeMismatch {
                 expected_typ: "an integer".to_owned(),
                 expr_typ: index_type.to_string(),
                 expr_span: span,
-            },
-        );
+            }
+        });
 
         // When writing `a[i]`, if `a : &mut ...` then automatically dereference `a` as many
         // times as needed to get the underlying array.
@@ -1218,7 +1221,7 @@ impl<'interner> TypeChecker<'interner> {
                     self.errors
                         .push(TypeCheckError::InvalidUnaryOp { kind: rhs_type.to_string(), span });
                 }
-                let expected = Type::polymorphic_integer_or_field(self.interner);
+                let expected = self.polymorphic_integer_or_field();
                 rhs_type.unify(&expected, &mut self.errors, || TypeCheckError::InvalidUnaryOp {
                     kind: rhs_type.to_string(),
                     span,
diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs
@@ -37,6 +37,11 @@ pub struct TypeChecker<'interner> {
     /// on each variable, but it is only until function calls when the types
     /// needed for the trait constraint may become known.
     trait_constraints: Vec<(TraitConstraint, ExprId)>,
+
+    /// All type variables created in the current function.
+    /// This map is used to default any integer type variables at the end of
+    /// a function (before checking trait constraints) if a type wasn't already chosen.
+    type_variables: Vec<Type>,
 }
 
 /// Type checks a function and assigns the
@@ -127,6 +132,16 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
         }
     }
 
+    // Default any type variables that still need defaulting.
+    // This is done before trait impl search since leaving them bindable can lead to errors
+    // when multiple impls are available. Instead we default first to choose the Field or u64 impl.
+    for typ in &type_checker.type_variables {
+        if let Type::TypeVariable(variable, kind) = typ.follow_bindings() {
+            let msg = "TypeChecker should only track defaultable type vars";
+            variable.bind(kind.default_type().expect(msg));
+        }
+    }
+
     // Verify any remaining trait constraints arising from the function body
     for (constraint, expr_id) in std::mem::take(&mut type_checker.trait_constraints) {
         let span = type_checker.interner.expr_span(&expr_id);
@@ -333,7 +348,13 @@ fn check_function_type_matches_expected_type(
 
 impl<'interner> TypeChecker<'interner> {
     fn new(interner: &'interner mut NodeInterner) -> Self {
-        Self { interner, errors: Vec::new(), trait_constraints: Vec::new(), current_function: None }
+        Self {
+            interner,
+            errors: Vec::new(),
+            trait_constraints: Vec::new(),
+            type_variables: Vec::new(),
+            current_function: None,
+        }
     }
 
     fn check_function_body(&mut self, body: &ExprId) -> Type {
@@ -348,6 +369,7 @@ impl<'interner> TypeChecker<'interner> {
             interner,
             errors: Vec::new(),
             trait_constraints: Vec::new(),
+            type_variables: Vec::new(),
             current_function: None,
         };
         let statement = this.interner.get_global(id).let_statement;
@@ -381,6 +403,22 @@ impl<'interner> TypeChecker<'interner> {
             make_error,
         );
     }
+
+    /// Return a fresh integer or field type variable and log it
+    /// in self.type_variables to default it later.
+    fn polymorphic_integer_or_field(&mut self) -> Type {
+        let typ = Type::polymorphic_integer_or_field(self.interner);
+        self.type_variables.push(typ.clone());
+        typ
+    }
+
+    /// Return a fresh integer type variable and log it
+    /// in self.type_variables to default it later.
+    fn polymorphic_integer(&mut self) -> Type {
+        let typ = Type::polymorphic_integer(self.interner);
+        self.type_variables.push(typ.clone());
+        typ
+    }
 }
 
 // XXX: These tests are all manual currently.
diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs
@@ -71,7 +71,7 @@ impl<'interner> TypeChecker<'interner> {
             expr_span: range_span,
         });
 
-        let expected_type = Type::polymorphic_integer(self.interner);
+        let expected_type = self.polymorphic_integer();
 
         self.unify(&start_range_type, &expected_type, || TypeCheckError::TypeCannotBeUsed {
             typ: start_range_type.clone(),
@@ -233,15 +233,13 @@ impl<'interner> TypeChecker<'interner> {
                 let expr_span = self.interner.expr_span(index);
                 let location = *location;
 
-                index_type.unify(
-                    &Type::polymorphic_integer_or_field(self.interner),
-                    &mut self.errors,
-                    || TypeCheckError::TypeMismatch {
+                index_type.unify(&self.polymorphic_integer_or_field(), &mut self.errors, || {
+                    TypeCheckError::TypeMismatch {
                         expected_typ: "an integer".to_owned(),
                         expr_typ: index_type.to_string(),
                         expr_span,
-                    },
-                );
+                    }
+                });
 
                 let (mut lvalue_type, mut lvalue, mut mutable) =
                     self.check_lvalue(array, assign_span);
diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs
@@ -1003,7 +1003,12 @@ impl<'interner> Monomorphizer<'interner> {
                     Err(constraints) => {
                         let failed_constraints = vecmap(constraints, |constraint| {
                             let id = constraint.trait_id;
-                            let name = self.interner.get_trait(id).name.to_string();
+                            let mut name = self.interner.get_trait(id).name.to_string();
+                            if !constraint.trait_generics.is_empty() {
+                                let types =
+                                    vecmap(&constraint.trait_generics, |t| format!("{t:?}"));
+                                name += &format!("<{}>", types.join(", "));
+                            }
                             format!("  {}: {name}", constraint.typ)
                         })
                         .join("\n");
diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs
@@ -1079,6 +1079,7 @@ impl NodeInterner {
     /// constraint, but when where clauses are involved, the failing constraint may be several
     /// constraints deep. In this case, all of the constraints are returned, starting with the
     /// failing one.
+    /// If this list of failing constraints is empty, this means type annotations are required.
     pub fn lookup_trait_implementation(
         &self,
         object_type: &Type,
@@ -1121,6 +1122,10 @@ impl NodeInterner {
     }
 
     /// Similar to `lookup_trait_implementation` but does not apply any type bindings on success.
+    /// On error returns either:
+    /// - 1+ failing trait constraints, including the original.
+    ///   Each constraint after the first represents a `where` clause that was followed.
+    /// - 0 trait constraints indicating type annotations are needed to choose an impl.
     pub fn try_lookup_trait_implementation(
         &self,
         object_type: &Type,
@@ -1138,6 +1143,11 @@ impl NodeInterner {
         Ok((impl_kind, bindings))
     }
 
+    /// Returns the trait implementation if found.
+    /// On error returns either:
+    /// - 1+ failing trait constraints, including the original.
+    ///   Each constraint after the first represents a `where` clause that was followed.
+    /// - 0 trait constraints indicating type annotations are needed to choose an impl.
     fn lookup_trait_implementation_helper(
         &self,
         object_type: &Type,
@@ -1156,15 +1166,22 @@ impl NodeInterner {
 
         let object_type = object_type.substitute(type_bindings);
 
+        // If the object type isn't known, just return an error saying type annotations are needed.
+        if object_type.is_bindable() {
+            return Err(Vec::new());
+        }
+
         let impls =
             self.trait_implementation_map.get(&trait_id).ok_or_else(|| vec![make_constraint()])?;
 
+        let mut matching_impls = Vec::new();
+
         for (existing_object_type2, impl_kind) in impls {
             // Bug: We're instantiating only the object type's generics here, not all of the trait's generics like we need to
             let (existing_object_type, instantiation_bindings) =
                 existing_object_type2.instantiate(self);
 
-            let mut fresh_bindings = TypeBindings::new();
+            let mut fresh_bindings = type_bindings.clone();
 
             let mut check_trait_generics = |impl_generics: &[Type]| {
                 trait_generics.iter().zip(impl_generics).all(|(trait_generic, impl_generic2)| {
@@ -1189,16 +1206,13 @@ impl NodeInterner {
             }
 
             if object_type.try_unify(&existing_object_type, &mut fresh_bindings).is_ok() {
-                // The unification was successful so we can append fresh_bindings to our bindings list
-                type_bindings.extend(fresh_bindings);
-
                 if let TraitImplKind::Normal(impl_id) = impl_kind {
                     let trait_impl = self.get_trait_implementation(*impl_id);
                     let trait_impl = trait_impl.borrow();
 
                     if let Err(mut errors) = self.validate_where_clause(
                         &trait_impl.where_clause,
-                        type_bindings,
+                        &mut fresh_bindings,
                         &instantiation_bindings,
                         recursion_limit,
                     ) {
@@ -1207,11 +1221,20 @@ impl NodeInterner {
                     }
                 }
 
-                return Ok(impl_kind.clone());
+                matching_impls.push((impl_kind.clone(), fresh_bindings));
             }
         }
 
-        Err(vec![make_constraint()])
+        if matching_impls.len() == 1 {
+            let (impl_, fresh_bindings) = matching_impls.pop().unwrap();
+            *type_bindings = fresh_bindings;
+            Ok(impl_)
+        } else if matching_impls.is_empty() {
+            Err(vec![make_constraint()])
+        } else {
+            // multiple matching impls, type annotations needed
+            Err(vec![])
+        }
     }
 
     /// Verifies that each constraint in the given where clause is valid.
@@ -1227,12 +1250,11 @@ impl NodeInterner {
             // Instantiation bindings are generally safe to force substitute into the same type.
             // This is needed here to undo any bindings done to trait methods by monomorphization.
             // Otherwise, an impl for (A, B) could get narrowed to only an impl for e.g. (u8, u16).
-            let constraint_type = constraint.typ.force_substitute(instantiation_bindings);
-            let constraint_type = constraint_type.substitute(type_bindings);
+            let constraint_type =
+                constraint.typ.force_substitute(instantiation_bindings).substitute(type_bindings);
 
             let trait_generics = vecmap(&constraint.trait_generics, |generic| {
-                let generic = generic.force_substitute(instantiation_bindings);
-                generic.substitute(type_bindings)
+                generic.force_substitute(instantiation_bindings).substitute(type_bindings)
             });
 
             self.lookup_trait_implementation_helper(
@@ -1241,10 +1263,11 @@ impl NodeInterner {
                 &trait_generics,
                 // Use a fresh set of type bindings here since the constraint_type originates from
                 // our impl list, which we don't want to bind to.
-                &mut TypeBindings::new(),
+                type_bindings,
                 recursion_limit - 1,
             )?;
         }
+
         Ok(())
     }
 
diff --git a/noir_stdlib/src/hash/poseidon2.nr b/noir_stdlib/src/hash/poseidon2.nr
@@ -1,7 +1,7 @@
 use crate::hash::Hasher;
 use crate::default::Default;
 
-global RATE = 3;
+global RATE: u32 = 3;
 
 struct Poseidon2 {
     cache: [Field;3],
