fix: Impl search no longer selects an impl if multiple are applicable

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,

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.

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);

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");

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(())
     }
 

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],