fix!: Ban Fields in for loop indices and bitwise ops (#4376)

# Description ## Problem\* Resolves #3639 Resolves #4193 ## Summary\* Uses the new TypeVariableKind::Integer in for loops and bitwise operations to prevent `Field` types from being used there. Removes the old `delayed_type_checks` hack. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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. --------- Co-authored-by: kevaundray <[email protected]> Co-authored-by: TomAFrench <[email protected]>
noir-lang · Feb 22, 2024 · 601fd9a · 601fd9a
1 parent ceb8001
commit 601fd9a
Show file tree

Hide file tree

Showing 32 changed files with 184 additions and 238 deletions.
diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs
@@ -115,6 +115,11 @@ impl FunctionBuilder {
         self.numeric_constant(value.into(), Type::field())
     }
 
+    /// Insert a numeric constant into the current function of type Type::length_type()
+    pub(crate) fn length_constant(&mut self, value: impl Into<FieldElement>) -> ValueId {
+        self.numeric_constant(value.into(), Type::length_type())
+    }
+
     /// Insert an array constant into the current function with the given element values.
     pub(crate) fn array_constant(&mut self, elements: im::Vector<ValueId>, typ: Type) -> ValueId {
         self.current_function.dfg.make_array(elements, typ)

diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
@@ -77,7 +77,7 @@ pub(super) fn simplify_call(
         Intrinsic::ArrayLen => {
             if let Some(length) = dfg.try_get_array_length(arguments[0]) {
                 let length = FieldElement::from(length as u128);
-                SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::field()))
+                SimplifyResult::SimplifiedTo(dfg.make_constant(length, Type::length_type()))
             } else if matches!(dfg.type_of_value(arguments[1]), Type::Slice(_)) {
                 SimplifyResult::SimplifiedTo(arguments[0])
             } else {
@@ -283,7 +283,7 @@ fn update_slice_length(
     operator: BinaryOp,
     block: BasicBlockId,
 ) -> ValueId {
-    let one = dfg.make_constant(FieldElement::one(), Type::field());
+    let one = dfg.make_constant(FieldElement::one(), Type::length_type());
     let instruction = Instruction::Binary(Binary { lhs: slice_len, operator, rhs: one });
     let call_stack = dfg.get_value_call_stack(slice_len);
     dfg.insert_instruction_and_results(instruction, block, None, call_stack).first()
@@ -296,8 +296,8 @@ fn simplify_slice_push_back(
     dfg: &mut DataFlowGraph,
     block: BasicBlockId,
 ) -> SimplifyResult {
-    // The capacity must be an integer so that we can compare it against the slice length which is represented as a field
-    let capacity = dfg.make_constant((slice.len() as u128).into(), Type::unsigned(64));
+    // The capacity must be an integer so that we can compare it against the slice length
+    let capacity = dfg.make_constant((slice.len() as u128).into(), Type::length_type());
     let len_equals_capacity_instr =
         Instruction::Binary(Binary { lhs: arguments[0], operator: BinaryOp::Eq, rhs: capacity });
     let call_stack = dfg.get_value_call_stack(arguments[0]);
@@ -362,7 +362,7 @@ fn simplify_slice_pop_back(
 
     let new_slice_length = update_slice_length(arguments[0], dfg, BinaryOp::Sub, block);
 
-    let element_size = dfg.make_constant((element_count as u128).into(), Type::field());
+    let element_size = dfg.make_constant((element_count as u128).into(), Type::length_type());
     let flattened_len_instr = Instruction::binary(BinaryOp::Mul, arguments[0], element_size);
     let mut flattened_len = dfg
         .insert_instruction_and_results(flattened_len_instr, block, None, CallStack::new())
@@ -478,7 +478,7 @@ fn make_constant_slice(
 
     let typ = Type::Slice(Rc::new(vec![typ]));
     let length = FieldElement::from(result_constants.len() as u128);
-    (dfg.make_constant(length, Type::field()), dfg.make_array(result_constants.into(), typ))
+    (dfg.make_constant(length, Type::length_type()), dfg.make_array(result_constants.into(), typ))
 }
 
 /// Returns a slice (represented by a tuple (len, slice)) of constants corresponding to the limbs of the radix decomposition.

diff --git a/compiler/noirc_evaluator/src/ssa/ir/types.rs b/compiler/noirc_evaluator/src/ssa/ir/types.rs
@@ -90,6 +90,11 @@ impl Type {
         Type::Numeric(NumericType::NativeField)
     }
 
+    /// Creates the type of an array's length.
+    pub(crate) fn length_type() -> Type {
+        Type::unsigned(64)
+    }
+
     /// Returns the bit size of the provided numeric type.
     ///
     /// # Panics

diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
@@ -192,7 +192,7 @@ impl<'a> FunctionContext<'a> {
             ast::Type::Slice(elements) => {
                 let element_types = Self::convert_type(elements).flatten();
                 Tree::Branch(vec![
-                    Tree::Leaf(f(Type::field())),
+                    Tree::Leaf(f(Type::length_type())),
                     Tree::Leaf(f(Type::Slice(Rc::new(element_types)))),
                 ])
             }
@@ -640,13 +640,13 @@ impl<'a> FunctionContext<'a> {
         let result_alloc = self.builder.set_location(location).insert_allocate(Type::bool());
         let true_value = self.builder.numeric_constant(1u128, Type::bool());
         self.builder.insert_store(result_alloc, true_value);
-        let zero = self.builder.field_constant(0u128);
+        let zero = self.builder.length_constant(0u128);
         self.builder.terminate_with_jmp(loop_start, vec![zero]);
 
         // loop_start
         self.builder.switch_to_block(loop_start);
-        let i = self.builder.add_block_parameter(loop_start, Type::field());
-        let array_length = self.builder.field_constant(array_length as u128);
+        let i = self.builder.add_block_parameter(loop_start, Type::length_type());
+        let array_length = self.builder.length_constant(array_length as u128);
         let v0 = self.builder.insert_binary(i, BinaryOp::Lt, array_length);
         self.builder.terminate_with_jmpif(v0, loop_body, loop_end);
 
@@ -658,7 +658,7 @@ impl<'a> FunctionContext<'a> {
         let v4 = self.builder.insert_load(result_alloc, Type::bool());
         let v5 = self.builder.insert_binary(v4, BinaryOp::And, v3);
         self.builder.insert_store(result_alloc, v5);
-        let one = self.builder.field_constant(1u128);
+        let one = self.builder.length_constant(1u128);
         let v6 = self.builder.insert_binary(i, BinaryOp::Add, one);
         self.builder.terminate_with_jmp(loop_start, vec![v6]);
 

diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs
@@ -128,6 +128,7 @@ impl<'a> FunctionContext<'a> {
     }
 
     fn codegen_expression(&mut self, expr: &Expression) -> Result<Values, RuntimeError> {
+        eprintln!("Codegen {expr}");
         match expr {
             Expression::Ident(ident) => Ok(self.codegen_ident(ident)),
             Expression::Literal(literal) => self.codegen_literal(literal),
@@ -196,7 +197,7 @@ impl<'a> FunctionContext<'a> {
                     }
                     ast::Type::Slice(_) => {
                         let slice_length =
-                            self.builder.field_constant(array.contents.len() as u128);
+                            self.builder.length_constant(array.contents.len() as u128);
                         let slice_contents =
                             self.codegen_array_checked(elements, typ[1].clone())?;
                         Tree::Branch(vec![slice_length.into(), slice_contents])
@@ -221,7 +222,7 @@ impl<'a> FunctionContext<'a> {
                 // A caller needs multiple pieces of information to make use of a format string
                 // The message string, the number of fields to be formatted, and the fields themselves
                 let string = self.codegen_string(string);
-                let field_count = self.builder.field_constant(*number_of_fields as u128);
+                let field_count = self.builder.length_constant(*number_of_fields as u128);
                 let fields = self.codegen_expression(fields)?;
 
                 Ok(Tree::Branch(vec![string, field_count.into(), fields]))
@@ -347,8 +348,10 @@ impl<'a> FunctionContext<'a> {
     }
 
     fn codegen_binary(&mut self, binary: &ast::Binary) -> Result<Values, RuntimeError> {
+        eprintln!("Start binary");
         let lhs = self.codegen_non_tuple_expression(&binary.lhs)?;
         let rhs = self.codegen_non_tuple_expression(&binary.rhs)?;
+        eprintln!("Insert binary");
         Ok(self.insert_binary(lhs, binary.operator, rhs, binary.location))
     }
 
@@ -615,7 +618,7 @@ impl<'a> FunctionContext<'a> {
         {
             match intrinsic {
                 Intrinsic::SliceInsert => {
-                    let one = self.builder.field_constant(1u128);
+                    let one = self.builder.length_constant(1u128);
 
                     // We add one here in the case of a slice insert as a slice insert at the length of the slice
                     // can be converted to a slice push back

diff --git a/compiler/noirc_frontend/src/ast/expression.rs b/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)]

diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/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.definition_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);
                                 }
                             }

diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/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))
@@ -529,13 +529,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.
@@ -807,43 +809,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);
@@ -1071,6 +1043,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
@@ -1093,58 +1097,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);
@@ -1169,11 +1130,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))
             }
@@ -1213,7 +1175,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,