fix!: Ban Fields in for loop indices and bitwise ops

compiler/noirc_frontend/src/ast/expression.rs

@@ -236,7 +236,15 @@ impl BinaryOpKind {
     }
 
     pub fn is_valid_for_field_type(self) -> bool {
-        matches!(self, BinaryOpKind::Equal | BinaryOpKind::NotEqual)
+        matches!(
+            self,
+            BinaryOpKind::Add
+                | BinaryOpKind::Subtract
+                | BinaryOpKind::Multiply
+                | BinaryOpKind::Divide
+                | BinaryOpKind::Equal
+                | BinaryOpKind::NotEqual
+        )
     }
 
     pub fn as_string(self) -> &'static str {
@@ -280,14 +288,6 @@ impl BinaryOpKind {
             BinaryOpKind::Modulo => Token::Percent,
         }
     }
-
-    pub fn is_bit_shift(&self) -> bool {
-        matches!(self, BinaryOpKind::ShiftRight | BinaryOpKind::ShiftLeft)
-    }
-
-    pub fn is_modulo(&self) -> bool {
-        matches!(self, BinaryOpKind::Modulo)
-    }
 }
 
 #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Debug, Copy, Clone)]

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -1464,7 +1464,7 @@ impl<'a> Resolver<'a> {
                                 // checker does not check definition kinds and otherwise expects
                                 // parameters to already be typed.
                                 if self.interner.id_type(hir_ident.id) == Type::Error {
-                                    let typ = Type::polymorphic_integer(self.interner);
+                                    let typ = Type::polymorphic_integer_or_field(self.interner);
                                     self.interner.push_definition_type(hir_ident.id, typ);
                                 }
                             }

compiler/noirc_frontend/src/hir/type_check/expr.rs

@@ -104,7 +104,7 @@ impl<'interner> TypeChecker<'interner> {
                         Type::Array(Box::new(length), Box::new(elem_type))
                     }
                     HirLiteral::Bool(_) => Type::Bool,
-                    HirLiteral::Integer(_, _) => Type::polymorphic_integer(self.interner),
+                    HirLiteral::Integer(_, _) => Type::polymorphic_integer_or_field(self.interner),
                     HirLiteral::Str(string) => {
                         let len = Type::Constant(string.len() as u64);
                         Type::String(Box::new(len))
@@ -528,13 +528,15 @@ 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(self.interner), &mut self.errors, || {
-            TypeCheckError::TypeMismatch {
+        index_type.unify(
+            &Type::polymorphic_integer_or_field(self.interner),
+            &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.
@@ -806,43 +808,13 @@ impl<'interner> TypeChecker<'interner> {
 
             // Matches on TypeVariable must be first to follow any type
             // bindings.
-            (TypeVariable(int, int_kind), other) | (other, TypeVariable(int, int_kind)) => {
-                if let TypeBinding::Bound(binding) = &*int.borrow() {
+            (TypeVariable(var, _), other) | (other, TypeVariable(var, _)) => {
+                if let TypeBinding::Bound(binding) = &*var.borrow() {
                     return self.comparator_operand_type_rules(other, binding, op, span);
                 }
 
-                if !op.kind.is_valid_for_field_type() && (other.is_bindable() || other.is_field()) {
-                    let other = other.follow_bindings();
-
-                    self.push_delayed_type_check(Box::new(move || {
-                        if other.is_field() || other.is_bindable() {
-                            Err(TypeCheckError::InvalidComparisonOnField { span })
-                        } else {
-                            Ok(())
-                        }
-                    }));
-                }
-
-                let mut bindings = TypeBindings::new();
-                if other
-                    .try_bind_to_polymorphic_int(
-                        int,
-                        &mut bindings,
-                        *int_kind == TypeVariableKind::Integer,
-                    )
-                    .is_ok()
-                    || other == &Type::Error
-                {
-                    Type::apply_type_bindings(bindings);
-                    Ok((Bool, false))
-                } else {
-                    Err(TypeCheckError::TypeMismatchWithSource {
-                        expected: lhs_type.clone(),
-                        actual: rhs_type.clone(),
-                        span,
-                        source: Source::Binary,
-                    })
-                }
+                self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span);
+                Ok((Bool, false))
             }
             (Alias(alias, args), other) | (other, Alias(alias, args)) => {
                 let alias = alias.borrow().get_type(args);
@@ -1070,6 +1042,38 @@ impl<'interner> TypeChecker<'interner> {
         }
     }
 
+    fn bind_type_variables_for_infix(
+        &mut self,
+        lhs_type: &Type,
+        op: &HirBinaryOp,
+        rhs_type: &Type,
+        span: Span,
+    ) {
+        self.unify(lhs_type, rhs_type, || TypeCheckError::TypeMismatchWithSource {
+            expected: lhs_type.clone(),
+            actual: rhs_type.clone(),
+            source: Source::Binary,
+            span,
+        });
+
+        // In addition to unifying both types, we also have to bind either
+        // the lhs or rhs to an integer type variable. This ensures if both lhs
+        // and rhs are type variables, that they will have the correct integer
+        // type variable kind instead of TypeVariableKind::Normal.
+        let target = if op.kind.is_valid_for_field_type() {
+            Type::polymorphic_integer_or_field(self.interner)
+        } else {
+            Type::polymorphic_integer(self.interner)
+        };
+
+        self.unify(lhs_type, &target, || TypeCheckError::TypeMismatchWithSource {
+            expected: lhs_type.clone(),
+            actual: rhs_type.clone(),
+            source: Source::Binary,
+            span,
+        });
+    }
+
     // Given a binary operator and another type. This method will produce the output type
     // and a boolean indicating whether to use the trait impl corresponding to the operator
     // or not. A value of false indicates the caller to use a primitive operation for this
@@ -1092,58 +1096,15 @@ impl<'interner> TypeChecker<'interner> {
 
             // Matches on TypeVariable must be first so that we follow any type
             // bindings.
-            (TypeVariable(int, int_kind), other) | (other, TypeVariable(int, int_kind)) => {
+            (TypeVariable(int, _), other) | (other, TypeVariable(int, _)) => {
                 if let TypeBinding::Bound(binding) = &*int.borrow() {
                     return self.infix_operand_type_rules(binding, op, other, span);
                 }
-                if (op.is_modulo() || op.is_bitwise()) && (other.is_bindable() || other.is_field())
-                {
-                    let other = other.follow_bindings();
-                    let kind = op.kind;
-                    // This will be an error if these types later resolve to a Field, or stay
-                    // polymorphic as the bit size will be unknown. Delay this error until the function
-                    // finishes resolving so we can still allow cases like `let x: u8 = 1 << 2;`.
-                    self.push_delayed_type_check(Box::new(move || {
-                        if other.is_field() {
-                            if kind == BinaryOpKind::Modulo {
-                                Err(TypeCheckError::FieldModulo { span })
-                            } else {
-                                Err(TypeCheckError::InvalidBitwiseOperationOnField { span })
-                            }
-                        } else if other.is_bindable() {
-                            Err(TypeCheckError::AmbiguousBitWidth { span })
-                        } else if kind.is_bit_shift() && other.is_signed() {
-                            Err(TypeCheckError::TypeCannotBeUsed {
-                                typ: other,
-                                place: "bit shift",
-                                span,
-                            })
-                        } else {
-                            Ok(())
-                        }
-                    }));
-                }
 
-                let mut bindings = TypeBindings::new();
-                if other
-                    .try_bind_to_polymorphic_int(
-                        int,
-                        &mut bindings,
-                        *int_kind == TypeVariableKind::Integer,
-                    )
-                    .is_ok()
-                    || other == &Type::Error
-                {
-                    Type::apply_type_bindings(bindings);
-                    Ok((other.clone(), false))
-                } else {
-                    Err(TypeCheckError::TypeMismatchWithSource {
-                        expected: lhs_type.clone(),
-                        actual: rhs_type.clone(),
-                        source: Source::Binary,
-                        span,
-                    })
-                }
+                self.bind_type_variables_for_infix(lhs_type, op, rhs_type, span);
+
+                // Both types are unified so the choice of which to return is arbitrary
+                Ok((other.clone(), false))
             }
             (Alias(alias, args), other) | (other, Alias(alias, args)) => {
                 let alias = alias.borrow().get_type(args);
@@ -1168,11 +1129,12 @@ impl<'interner> TypeChecker<'interner> {
             }
             // The result of two Fields is always a witness
             (FieldElement, FieldElement) => {
-                if op.is_bitwise() {
-                    return Err(TypeCheckError::InvalidBitwiseOperationOnField { span });
-                }
-                if op.is_modulo() {
-                    return Err(TypeCheckError::FieldModulo { span });
+                if !op.kind.is_valid_for_field_type() {
+                    if op.kind == BinaryOpKind::Modulo {
+                        return Err(TypeCheckError::FieldModulo { span });
+                    } else {
+                        return Err(TypeCheckError::InvalidBitwiseOperationOnField { span });
+                    }
                 }
                 Ok((FieldElement, false))
             }
@@ -1212,7 +1174,7 @@ impl<'interner> TypeChecker<'interner> {
                     self.errors
                         .push(TypeCheckError::InvalidUnaryOp { kind: rhs_type.to_string(), span });
                 }
-                let expected = Type::polymorphic_integer(self.interner);
+                let expected = Type::polymorphic_integer_or_field(self.interner);
                 rhs_type.unify(&expected, &mut self.errors, || TypeCheckError::InvalidUnaryOp {
                     kind: rhs_type.to_string(),
                     span,

compiler/noirc_frontend/src/hir/type_check/mod.rs

@@ -21,10 +21,7 @@ use crate::{
 
 use self::errors::Source;
 
-type TypeCheckFn = Box<dyn FnOnce() -> Result<(), TypeCheckError>>;
-
 pub struct TypeChecker<'interner> {
-    delayed_type_checks: Vec<TypeCheckFn>,
     interner: &'interner mut NodeInterner,
     errors: Vec<TypeCheckError>,
     current_function: Option<FuncId>,
@@ -80,15 +77,7 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
         type_checker.bind_pattern(&param.0, param.1);
     }
 
-    let (function_last_type, delayed_type_check_functions) =
-        type_checker.check_function_body(function_body_id);
-
-    // Go through any delayed type checking errors to see if they are resolved, or error otherwise.
-    for type_check_fn in delayed_type_check_functions {
-        if let Err(error) = type_check_fn() {
-            errors.push(error);
-        }
-    }
+    let function_last_type = type_checker.check_function_body(function_body_id);
 
     // Verify any remaining trait constraints arising from the function body
     for (constraint, expr_id) in std::mem::take(&mut type_checker.trait_constraints) {
@@ -175,30 +164,18 @@ fn function_info(interner: &NodeInterner, function_body_id: &ExprId) -> (noirc_e
 
 impl<'interner> TypeChecker<'interner> {
     fn new(interner: &'interner mut NodeInterner) -> Self {
-        Self {
-            delayed_type_checks: Vec::new(),
-            interner,
-            errors: Vec::new(),
-            trait_constraints: Vec::new(),
-            current_function: None,
-        }
-    }
-
-    pub fn push_delayed_type_check(&mut self, f: TypeCheckFn) {
-        self.delayed_type_checks.push(f);
+        Self { interner, errors: Vec::new(), trait_constraints: Vec::new(), current_function: None }
     }
 
-    fn check_function_body(&mut self, body: &ExprId) -> (Type, Vec<TypeCheckFn>) {
-        let body_type = self.check_expression(body);
-        (body_type, std::mem::take(&mut self.delayed_type_checks))
+    fn check_function_body(&mut self, body: &ExprId) -> Type {
+        self.check_expression(body)
     }
 
     pub fn check_global(
         id: GlobalId,
         interner: &'interner mut NodeInterner,
     ) -> Vec<TypeCheckError> {
         let mut this = Self {
-            delayed_type_checks: Vec::new(),
             interner,
             errors: Vec::new(),
             trait_constraints: Vec::new(),

compiler/noirc_frontend/src/hir/type_check/stmt.rs

@@ -73,13 +73,10 @@ impl<'interner> TypeChecker<'interner> {
 
         let expected_type = Type::polymorphic_integer(self.interner);
 
-        self.unify(&start_range_type, &expected_type, || {
-            TypeCheckError::TypeCannotBeUsed {
-                typ: start_range_type.clone(),
-                place: "for loop",
-                span: range_span,
-            }
-            .add_context("The range of a loop must be known at compile-time")
+        self.unify(&start_range_type, &expected_type, || TypeCheckError::TypeCannotBeUsed {
+            typ: start_range_type.clone(),
+            place: "for loop",
+            span: range_span,
         });
 
         self.interner.push_definition_type(for_loop.identifier.id, start_range_type);
@@ -235,7 +232,7 @@ impl<'interner> TypeChecker<'interner> {
                 let expr_span = self.interner.expr_span(index);
 
                 index_type.unify(
-                    &Type::polymorphic_integer(self.interner),
+                    &Type::polymorphic_integer_or_field(self.interner),
                     &mut self.errors,
                     || TypeCheckError::TypeMismatch {
                         expected_typ: "an integer".to_owned(),

compiler/noirc_frontend/src/hir_def/expr.rs

@@ -94,19 +94,6 @@ impl HirBinaryOp {
         let location = Location::new(op.span(), file);
         HirBinaryOp { location, kind }
     }
-
-    pub fn is_bitwise(&self) -> bool {
-        use BinaryOpKind::*;
-        matches!(self.kind, And | Or | Xor | ShiftRight | ShiftLeft)
-    }
-
-    pub fn is_bit_shift(&self) -> bool {
-        self.kind.is_bit_shift()
-    }
-
-    pub fn is_modulo(&self) -> bool {
-        self.kind.is_modulo()
-    }
 }
 
 #[derive(Debug, Clone)]