From bea4a8851bf1fd634894859d4fd55f49c249e203 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 3 Aug 2023 15:03:15 -0500 Subject: [PATCH 01/12] Remove comptime --- crates/noirc_frontend/src/ast/mod.rs | 26 +- .../src/hir/resolution/resolver.rs | 12 +- .../noirc_frontend/src/hir/type_check/expr.rs | 241 +++--- .../noirc_frontend/src/hir/type_check/mod.rs | 45 +- .../noirc_frontend/src/hir/type_check/stmt.rs | 27 +- crates/noirc_frontend/src/hir_def/types.rs | 699 +++--------------- crates/noirc_frontend/src/lexer/token.rs | 3 - .../src/monomorphization/mod.rs | 8 +- crates/noirc_frontend/src/node_interner.rs | 8 +- crates/noirc_frontend/src/parser/parser.rs | 46 +- 10 files changed, 236 insertions(+), 879 deletions(-) diff --git a/crates/noirc_frontend/src/ast/mod.rs b/crates/noirc_frontend/src/ast/mod.rs index 6aa373c66a9..1934c3f790c 100644 --- a/crates/noirc_frontend/src/ast/mod.rs +++ b/crates/noirc_frontend/src/ast/mod.rs @@ -23,7 +23,7 @@ pub use type_alias::*; use crate::{ parser::{ParserError, ParserErrorReason}, token::IntType, - BinaryTypeOperator, CompTime, + BinaryTypeOperator, }; use iter_extended::vecmap; @@ -32,10 +32,10 @@ use iter_extended::vecmap; /// for structs within, but are otherwise identical to Types. #[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum UnresolvedType { - FieldElement(CompTime), + FieldElement, Array(Option, Box), // [4]Witness = Array(4, Witness) - Integer(CompTime, Signedness, u32), // u32 = Integer(unsigned, 32) - Bool(CompTime), + Integer(Signedness, u32), // u32 = Integer(unsigned, 32) + Bool, Expression(UnresolvedTypeExpression), String(Option), FormatString(UnresolvedTypeExpression, Box), @@ -81,14 +81,14 @@ impl std::fmt::Display for UnresolvedType { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { use UnresolvedType::*; match self { - FieldElement(is_const) => write!(f, "{is_const}Field"), + FieldElement => write!(f, "Field"), Array(len, typ) => match len { None => write!(f, "[{typ}]"), Some(len) => write!(f, "[{typ}; {len}]"), }, - Integer(is_const, sign, num_bits) => match sign { - Signedness::Signed => write!(f, "{is_const}i{num_bits}"), - Signedness::Unsigned => write!(f, "{is_const}u{num_bits}"), + Integer(sign, num_bits) => match sign { + Signedness::Signed => write!(f, "i{num_bits}"), + Signedness::Unsigned => write!(f, "u{num_bits}"), }, Named(s, args) => { let args = vecmap(args, ToString::to_string); @@ -103,7 +103,7 @@ impl std::fmt::Display for UnresolvedType { write!(f, "({})", elements.join(", ")) } Expression(expression) => expression.fmt(f), - Bool(is_const) => write!(f, "{is_const}bool"), + Bool => write!(f, "bool"), String(len) => match len { None => write!(f, "str<_>"), Some(len) => write!(f, "str<{len}>"), @@ -134,11 +134,11 @@ impl std::fmt::Display for UnresolvedTypeExpression { } impl UnresolvedType { - pub fn from_int_token(token: (CompTime, IntType)) -> UnresolvedType { + pub fn from_int_token(token: IntType) -> UnresolvedType { use {IntType::*, UnresolvedType::Integer}; - match token.1 { - Signed(num_bits) => Integer(token.0, Signedness::Signed, num_bits), - Unsigned(num_bits) => Integer(token.0, Signedness::Unsigned, num_bits), + match token { + Signed(num_bits) => Integer(Signedness::Signed, num_bits), + Unsigned(num_bits) => Integer(Signedness::Unsigned, num_bits), } } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 681c853899f..3bfe30d915c 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -332,7 +332,7 @@ impl<'a> Resolver<'a> { /// freshly created TypeVariables created to new_variables. fn resolve_type_inner(&mut self, typ: UnresolvedType, new_variables: &mut Generics) -> Type { match typ { - UnresolvedType::FieldElement(comp_time) => Type::FieldElement(comp_time), + UnresolvedType::FieldElement => Type::FieldElement, UnresolvedType::Array(size, elem) => { let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); let size = if size.is_none() { @@ -343,8 +343,8 @@ impl<'a> Resolver<'a> { Type::Array(Box::new(size), elem) } UnresolvedType::Expression(expr) => self.convert_expression_type(expr), - UnresolvedType::Integer(comp_time, sign, bits) => Type::Integer(comp_time, sign, bits), - UnresolvedType::Bool(comp_time) => Type::Bool(comp_time), + UnresolvedType::Integer(sign, bits) => Type::Integer(sign, bits), + UnresolvedType::Bool => Type::Bool, UnresolvedType::String(size) => { let resolved_size = self.resolve_array_size(size, new_variables); Type::String(Box::new(resolved_size)) @@ -816,9 +816,9 @@ impl<'a> Resolver<'a> { fn find_numeric_generics_in_type(typ: &Type, found: &mut HashMap>) { match typ { - Type::FieldElement(_) - | Type::Integer(_, _, _) - | Type::Bool(_) + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool | Type::Unit | Type::Error | Type::TypeVariable(_, _) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 6c111a1d6a0..bb99ecd1ace 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -12,7 +12,7 @@ use crate::{ }, node_interner::{DefinitionKind, ExprId, FuncId}, token::Attribute::Deprecated, - CompTime, Shared, Signedness, TypeBinding, TypeVariableKind, UnaryOp, + Shared, Signedness, TypeBinding, TypeVariableKind, UnaryOp, }; use super::{errors::TypeCheckError, TypeChecker}; @@ -75,22 +75,17 @@ impl<'interner> TypeChecker<'interner> { for (index, elem_type) in elem_types.iter().enumerate().skip(1) { let location = self.interner.expr_location(&arr[index]); - elem_type.unify( - &first_elem_type, - location.span, - &mut self.errors, - || { - TypeCheckError::NonHomogeneousArray { - first_span: self.interner.expr_location(&arr[0]).span, - first_type: first_elem_type.to_string(), - first_index: index, - second_span: location.span, - second_type: elem_type.to_string(), - second_index: index + 1, - } - .add_context("elements in an array must have the same type") - }, - ); + elem_type.unify(&first_elem_type, &mut self.errors, || { + TypeCheckError::NonHomogeneousArray { + first_span: self.interner.expr_location(&arr[0]).span, + first_type: first_elem_type.to_string(), + first_index: index, + second_span: location.span, + second_type: elem_type.to_string(), + second_index: index + 1, + } + .add_context("elements in an array must have the same type") + }); } arr_type @@ -105,7 +100,7 @@ impl<'interner> TypeChecker<'interner> { }; Type::Array(Box::new(length), Box::new(elem_type)) } - HirLiteral::Bool(_) => Type::Bool(CompTime::new(self.interner)), + HirLiteral::Bool(_) => Type::Bool, HirLiteral::Integer(_) => Type::polymorphic_integer(self.interner), HirLiteral::Str(string) => { let len = Type::Constant(string.len() as u64); @@ -204,27 +199,18 @@ impl<'interner> TypeChecker<'interner> { // Check that start range and end range have the same types let range_span = start_span.merge(end_span); - self.unify(&start_range_type, &end_range_type, range_span, || { - TypeCheckError::TypeMismatch { - expected_typ: start_range_type.to_string(), - expr_typ: end_range_type.to_string(), - expr_span: range_span, - } + self.unify(&start_range_type, &end_range_type, || TypeCheckError::TypeMismatch { + expected_typ: start_range_type.to_string(), + expr_typ: end_range_type.to_string(), + expr_span: range_span, }); - let expected_comptime = if self.is_unconstrained() { - CompTime::new(self.interner) - } else { - CompTime::Yes(Some(range_span)) - }; let fresh_id = self.interner.next_type_variable_id(); let type_variable = Shared::new(TypeBinding::Unbound(fresh_id)); - let expected_type = Type::TypeVariable( - type_variable, - TypeVariableKind::IntegerOrField(expected_comptime), - ); + let expected_type = + Type::TypeVariable(type_variable, TypeVariableKind::IntegerOrField); - self.unify(&start_range_type, &expected_type, range_span, || { + self.unify(&start_range_type, &expected_type, || { TypeCheckError::TypeCannotBeUsed { typ: start_range_type.clone(), place: "for loop", @@ -252,12 +238,10 @@ impl<'interner> TypeChecker<'interner> { }; let span = self.interner.expr_span(&id); - self.unify(&expr_type, &Type::Unit, span, || { - TypeCheckError::TypeMismatch { - expected_typ: Type::Unit.to_string(), - expr_typ: expr_type.to_string(), - expr_span: span, - } + self.unify(&expr_type, &Type::Unit, || TypeCheckError::TypeMismatch { + expected_typ: Type::Unit.to_string(), + expr_typ: expr_type.to_string(), + expr_span: span, }); } else { block_type = expr_type; @@ -293,12 +277,10 @@ impl<'interner> TypeChecker<'interner> { let actual_return = self.check_expression(&lambda.body); let span = self.interner.expr_span(&lambda.body); - actual_return.make_subtype_of(&lambda.return_type, span, &mut self.errors, || { - TypeCheckError::TypeMismatch { - expected_typ: lambda.return_type.to_string(), - expr_typ: actual_return.to_string(), - expr_span: span, - } + self.unify(&actual_return, &lambda.return_type, || TypeCheckError::TypeMismatch { + expected_typ: lambda.return_type.to_string(), + expr_typ: actual_return.to_string(), + expr_span: span, }); Type::Function(params, Box::new(lambda.return_type), Box::new(env_type)) @@ -399,7 +381,7 @@ 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), span, &mut self.errors, || { + index_type.unify(&Type::polymorphic_integer(self.interner), &mut self.errors, || { TypeCheckError::TypeMismatch { expected_typ: "an integer".to_owned(), expr_typ: index_type.to_string(), @@ -426,49 +408,27 @@ impl<'interner> TypeChecker<'interner> { } fn check_cast(&mut self, from: Type, to: Type, span: Span) -> Type { - let is_comp_time = match from.follow_bindings() { - Type::Integer(is_comp_time, ..) => is_comp_time, - Type::FieldElement(is_comp_time) => is_comp_time, - Type::TypeVariable(_, TypeVariableKind::IntegerOrField(is_comp_time)) => is_comp_time, + match from.follow_bindings() { + Type::Integer(..) + | Type::FieldElement + | Type::TypeVariable(_, TypeVariableKind::IntegerOrField) + | Type::Bool => (), + Type::TypeVariable(_, _) => { self.errors.push(TypeCheckError::TypeAnnotationsNeeded { span }); return Type::Error; } - Type::Bool(is_comp_time) => is_comp_time, Type::Error => return Type::Error, from => { self.errors.push(TypeCheckError::InvalidCast { from, span }); return Type::Error; } - }; + } match to { - Type::Integer(dest_comp_time, sign, bits) => { - if dest_comp_time.is_comp_time() - && is_comp_time.unify(&dest_comp_time, span).is_err() - { - self.errors.push(TypeCheckError::CannotCastToComptimeType { span }); - } - - Type::Integer(is_comp_time, sign, bits) - } - Type::FieldElement(dest_comp_time) => { - if dest_comp_time.is_comp_time() - && is_comp_time.unify(&dest_comp_time, span).is_err() - { - self.errors.push(TypeCheckError::CannotCastToComptimeType { span }); - } - - Type::FieldElement(is_comp_time) - } - Type::Bool(dest_comp_time) => { - if dest_comp_time.is_comp_time() - && is_comp_time.unify(&dest_comp_time, span).is_err() - { - self.errors.push(TypeCheckError::CannotCastToComptimeType { span }); - } - Type::Bool(dest_comp_time) - } + Type::Integer(sign, bits) => Type::Integer(sign, bits), + Type::FieldElement => Type::FieldElement, + Type::Bool => Type::Bool, Type::Error => Type::Error, _ => { self.errors.push(TypeCheckError::UnsupportedCast { span }); @@ -518,9 +478,8 @@ impl<'interner> TypeChecker<'interner> { let expr_span = self.interner.expr_span(&if_expr.condition); - let bool_type = Type::Bool(CompTime::new(self.interner)); - self.unify(&cond_type, &bool_type, expr_span, || TypeCheckError::TypeMismatch { - expected_typ: Type::Bool(CompTime::No(None)).to_string(), + self.unify(&cond_type, &Type::Bool, || TypeCheckError::TypeMismatch { + expected_typ: Type::Bool.to_string(), expr_typ: cond_type.to_string(), expr_span, }); @@ -531,7 +490,7 @@ impl<'interner> TypeChecker<'interner> { let else_type = self.check_expression(&alternative); let expr_span = self.interner.expr_span(expr_id); - self.unify(&then_type, &else_type, expr_span, || { + self.unify(&then_type, &else_type, || { let err = TypeCheckError::TypeMismatch { expected_typ: then_type.to_string(), expr_typ: else_type.to_string(), @@ -580,7 +539,7 @@ impl<'interner> TypeChecker<'interner> { let arg_type = self.check_expression(&arg); let span = self.interner.expr_span(expr_id); - self.make_subtype_of(&arg_type, ¶m_type, arg, || { + self.unify_with_coercions(&arg_type, ¶m_type, arg, || { TypeCheckError::TypeMismatch { expected_typ: param_type.to_string(), expr_typ: arg_type.to_string(), @@ -700,11 +659,11 @@ impl<'interner> TypeChecker<'interner> { match (lhs_type, rhs_type) { // Avoid reporting errors multiple times - (Error, _) | (_, Error) => Ok(Bool(CompTime::Yes(None))), + (Error, _) | (_, Error) => Ok(Bool), // Matches on TypeVariable must be first to follow any type // bindings. - (var @ TypeVariable(int, _), other) | (other, var @ TypeVariable(int, _)) => { + (TypeVariable(int, _), other) | (other, TypeVariable(int, _)) => { if let TypeBinding::Bound(binding) = &*int.borrow() { return self.comparator_operand_type_rules(other, binding, op, span); } @@ -721,11 +680,8 @@ impl<'interner> TypeChecker<'interner> { })); } - let comptime = var.try_get_comptime(); - if other.try_bind_to_polymorphic_int(int, &comptime, true, op.location.span).is_ok() - || other == &Type::Error - { - Ok(Bool(comptime.into_owned())) + if other.try_bind_to_polymorphic_int(int).is_ok() || other == &Type::Error { + Ok(Bool) } else { Err(TypeCheckError::TypeMismatchWithSource { rhs: lhs_type.clone(), @@ -735,10 +691,7 @@ impl<'interner> TypeChecker<'interner> { }) } } - ( - Integer(comptime_x, sign_x, bit_width_x), - Integer(comptime_y, sign_y, bit_width_y), - ) => { + (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { return Err(TypeCheckError::IntegerSignedness { sign_x: *sign_x, @@ -753,58 +706,48 @@ impl<'interner> TypeChecker<'interner> { span, }); } - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(Bool(comptime)) + Ok(Bool) } - (Integer(..), FieldElement(..)) | (FieldElement(..), Integer(..)) => { + (Integer(..), FieldElement) | (FieldElement, Integer(..)) => { Err(TypeCheckError::IntegerAndFieldBinaryOperation { span }) } (Integer(..), typ) | (typ, Integer(..)) => { Err(TypeCheckError::IntegerTypeMismatch { typ: typ.clone(), span }) } - (FieldElement(comptime_x), FieldElement(comptime_y)) => { + (FieldElement, FieldElement) => { if op.kind.is_valid_for_field_type() { - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(Bool(comptime)) + Ok(Bool) } else { Err(TypeCheckError::FieldComparison { span }) } } // <= and friends are technically valid for booleans, just not very useful - (Bool(comptime_x), Bool(comptime_y)) => { - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(Bool(comptime)) - } + (Bool, Bool) => Ok(Bool), // Special-case == and != for arrays (Array(x_size, x_type), Array(y_size, y_type)) if matches!(op.kind, Equal | NotEqual) => { - x_type.unify(y_type, op.location.span, &mut self.errors, || { - TypeCheckError::TypeMismatchWithSource { - rhs: lhs_type.clone(), - lhs: rhs_type.clone(), - source: Source::ArrayElements, - span: op.location.span, - } + self.unify(x_type, y_type, || TypeCheckError::TypeMismatchWithSource { + rhs: lhs_type.clone(), + lhs: rhs_type.clone(), + source: Source::ArrayElements, + span: op.location.span, }); - self.unify(x_size, y_size, op.location.span, || { - TypeCheckError::TypeMismatchWithSource { - rhs: lhs_type.clone(), - lhs: rhs_type.clone(), - source: Source::ArrayLen, - span: op.location.span, - } + self.unify(x_size, y_size, || TypeCheckError::TypeMismatchWithSource { + rhs: lhs_type.clone(), + lhs: rhs_type.clone(), + source: Source::ArrayLen, + span: op.location.span, }); - // We could check if all elements of all arrays are comptime but I am lazy - Ok(Bool(CompTime::No(Some(op.location.span)))) + Ok(Bool) } (lhs @ NamedGeneric(binding_a, _), rhs @ NamedGeneric(binding_b, _)) => { if binding_a == binding_b { - return Ok(Bool(CompTime::No(Some(op.location.span)))); + return Ok(Bool); } Err(TypeCheckError::TypeMismatchWithSource { rhs: lhs.clone(), @@ -814,16 +757,14 @@ impl<'interner> TypeChecker<'interner> { }) } (String(x_size), String(y_size)) => { - x_size.unify(y_size, op.location.span, &mut self.errors, || { - TypeCheckError::TypeMismatchWithSource { - rhs: *x_size.clone(), - lhs: *y_size.clone(), - span: op.location.span, - source: Source::StringLen, - } + self.unify(x_size, y_size, || TypeCheckError::TypeMismatchWithSource { + rhs: *x_size.clone(), + lhs: *y_size.clone(), + span: op.location.span, + source: Source::StringLen, }); - Ok(Bool(CompTime::No(Some(op.location.span)))) + Ok(Bool) } (lhs, rhs) => Err(TypeCheckError::TypeMismatchWithSource { rhs: lhs.clone(), @@ -894,12 +835,10 @@ impl<'interner> TypeChecker<'interner> { } for (param, (arg, _, arg_span)) in fn_params.iter().zip(callsite_args) { - arg.make_subtype_of(param, *arg_span, &mut self.errors, || { - TypeCheckError::TypeMismatch { - expected_typ: param.to_string(), - expr_typ: arg.to_string(), - expr_span: *arg_span, - } + self.unify(arg, param, || TypeCheckError::TypeMismatch { + expected_typ: param.to_string(), + expr_typ: arg.to_string(), + expr_span: *arg_span, }); } @@ -962,7 +901,7 @@ impl<'interner> TypeChecker<'interner> { // Matches on TypeVariable must be first so that we follow any type // bindings. - (var @ TypeVariable(int, _), other) | (other, var @ TypeVariable(int, _)) => { + (TypeVariable(int, _), other) | (other, TypeVariable(int, _)) => { if let TypeBinding::Bound(binding) = &*int.borrow() { return self.infix_operand_type_rules(binding, op, other, span); } @@ -990,10 +929,7 @@ impl<'interner> TypeChecker<'interner> { })); } - let comptime = var.try_get_comptime(); - if other.try_bind_to_polymorphic_int(int, &comptime, true, op.location.span).is_ok() - || other == &Type::Error - { + if other.try_bind_to_polymorphic_int(int).is_ok() || other == &Type::Error { Ok(other.clone()) } else { Err(TypeCheckError::TypeMismatchWithSource { @@ -1004,10 +940,7 @@ impl<'interner> TypeChecker<'interner> { }) } } - ( - Integer(comptime_x, sign_x, bit_width_x), - Integer(comptime_y, sign_y, bit_width_y), - ) => { + (Integer(sign_x, bit_width_x), Integer(sign_y, bit_width_y)) => { if sign_x != sign_y { return Err(TypeCheckError::IntegerSignedness { sign_x: *sign_x, @@ -1027,11 +960,10 @@ impl<'interner> TypeChecker<'interner> { { Err(TypeCheckError::InvalidInfixOp { kind: "Signed integer", span }) } else { - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(Integer(comptime, *sign_x, *bit_width_x)) + Ok(Integer(*sign_x, *bit_width_x)) } } - (Integer(..), FieldElement(..)) | (FieldElement(..), Integer(..)) => { + (Integer(..), FieldElement) | (FieldElement, Integer(..)) => { Err(TypeCheckError::IntegerAndFieldBinaryOperation { span }) } (Integer(..), typ) | (typ, Integer(..)) => { @@ -1051,17 +983,14 @@ impl<'interner> TypeChecker<'interner> { (Unit, _) | (_, Unit) => Ok(Unit), // The result of two Fields is always a witness - (FieldElement(comptime_x), FieldElement(comptime_y)) => { + (FieldElement, FieldElement) => { if op.is_bitwise() { return Err(TypeCheckError::InvalidBitwiseOperationOnField { span }); } - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(FieldElement(comptime)) + Ok(FieldElement) } - (Bool(comptime_x), Bool(comptime_y)) => { - Ok(Bool(comptime_x.and(comptime_y, op.location.span))) - } + (Bool, Bool) => Ok(Bool), (lhs, rhs) => Err(TypeCheckError::TypeMismatchWithSource { rhs: lhs.clone(), @@ -1079,7 +1008,7 @@ impl<'interner> TypeChecker<'interner> { span: Span, ) -> Type { let mut unify = |expected| { - rhs_type.unify(&expected, span, &mut self.errors, || TypeCheckError::TypeMismatch { + rhs_type.unify(&expected, &mut self.errors, || TypeCheckError::TypeMismatch { expr_typ: rhs_type.to_string(), expected_typ: expected.to_string(), expr_span: span, @@ -1097,7 +1026,7 @@ impl<'interner> TypeChecker<'interner> { return rhs_type; } - unify(Type::Bool(CompTime::new(self.interner))) + unify(Type::Bool) } crate::UnaryOp::MutableReference => { Type::MutableReference(Box::new(rhs_type.follow_bindings())) diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 1883c0abf62..76e59ef5eab 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -15,7 +15,6 @@ mod expr; mod stmt; pub use errors::TypeCheckError; -use noirc_errors::Span; use crate::{ node_interner::{ExprId, FuncId, NodeInterner, StmtId}, @@ -26,7 +25,6 @@ type TypeCheckFn = Box Result<(), TypeCheckError>>; pub struct TypeChecker<'interner> { delayed_type_checks: Vec, - current_function: Option, interner: &'interner mut NodeInterner, errors: Vec, } @@ -41,7 +39,7 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec Vec Vec TypeChecker<'interner> { - fn new(current_function: FuncId, interner: &'interner mut NodeInterner) -> Self { - Self { - delayed_type_checks: Vec::new(), - current_function: Some(current_function), - interner, - errors: vec![], - } + fn new(interner: &'interner mut NodeInterner) -> Self { + Self { delayed_type_checks: Vec::new(), interner, errors: vec![] } } pub fn push_delayed_type_check(&mut self, f: TypeCheckFn) { @@ -102,42 +95,30 @@ impl<'interner> TypeChecker<'interner> { } pub fn check_global(id: &StmtId, interner: &'interner mut NodeInterner) -> Vec { - let mut this = Self { - delayed_type_checks: Vec::new(), - current_function: None, - interner, - errors: vec![], - }; + let mut this = Self { delayed_type_checks: Vec::new(), interner, errors: vec![] }; this.check_statement(id); this.errors } - fn is_unconstrained(&self) -> bool { - self.current_function.map_or(false, |current_function| { - self.interner.function_meta(¤t_function).is_unconstrained - }) - } - /// Wrapper of Type::unify using self.errors fn unify( &mut self, actual: &Type, expected: &Type, - span: Span, make_error: impl FnOnce() -> TypeCheckError, ) { - actual.unify(expected, span, &mut self.errors, make_error); + actual.unify(expected, &mut self.errors, make_error); } - /// Wrapper of Type::make_subtype_of using self.errors - fn make_subtype_of( + /// Wrapper of Type::unify_with_coercions using self.errors + fn unify_with_coercions( &mut self, actual: &Type, expected: &Type, expression: ExprId, make_error: impl FnOnce() -> TypeCheckError, ) { - actual.make_subtype_with_coercions( + actual.unify_with_coercions( expected, expression, self.interner, @@ -220,7 +201,7 @@ mod test { // Create let statement let let_stmt = HirLetStatement { pattern: Identifier(z), - r#type: Type::FieldElement(crate::CompTime::No(None)), + r#type: Type::FieldElement, expression: expr_id, }; let stmt_id = interner.push_stmt(HirStatement::Let(let_stmt)); @@ -247,13 +228,13 @@ mod test { is_internal: None, is_unconstrained: false, typ: Type::Function( - vec![Type::field(None), Type::field(None)], + vec![Type::FieldElement, Type::FieldElement], Box::new(Type::Unit), Box::new(Type::Unit), ), parameters: vec![ - Param(Identifier(x), Type::field(None), noirc_abi::AbiVisibility::Private), - Param(Identifier(y), Type::field(None), noirc_abi::AbiVisibility::Private), + Param(Identifier(x), Type::FieldElement, noirc_abi::AbiVisibility::Private), + Param(Identifier(y), Type::FieldElement, noirc_abi::AbiVisibility::Private), ] .into(), return_visibility: noirc_abi::AbiVisibility::Private, diff --git a/crates/noirc_frontend/src/hir/type_check/stmt.rs b/crates/noirc_frontend/src/hir/type_check/stmt.rs index 3130a8616de..b3a672cb05b 100644 --- a/crates/noirc_frontend/src/hir/type_check/stmt.rs +++ b/crates/noirc_frontend/src/hir/type_check/stmt.rs @@ -6,7 +6,6 @@ use crate::hir_def::stmt::{ }; use crate::hir_def::types::Type; use crate::node_interner::{DefinitionId, ExprId, StmtId}; -use crate::CompTime; use super::errors::{Source, TypeCheckError}; use super::TypeChecker; @@ -75,7 +74,7 @@ impl<'interner> TypeChecker<'interner> { } }, HirPattern::Struct(struct_type, fields, span) => { - self.unify(struct_type, &typ, *span, || TypeCheckError::TypeMismatch { + self.unify(struct_type, &typ, || TypeCheckError::TypeMismatch { expected_typ: typ.to_string(), expr_typ: struct_type.to_string(), expr_span: *span, @@ -108,7 +107,7 @@ impl<'interner> TypeChecker<'interner> { }); let span = self.interner.expr_span(&assign_stmt.expression); - self.make_subtype_of(&expr_type, &lvalue_type, assign_stmt.expression, || { + self.unify_with_coercions(&expr_type, &lvalue_type, assign_stmt.expression, || { TypeCheckError::TypeMismatchWithSource { rhs: expr_type.clone(), lhs: lvalue_type.clone(), @@ -178,7 +177,6 @@ impl<'interner> TypeChecker<'interner> { index_type.unify( &Type::polymorphic_integer(self.interner), - expr_span, &mut self.errors, || TypeCheckError::TypeMismatch { expected_typ: "an integer".to_owned(), @@ -212,12 +210,10 @@ impl<'interner> TypeChecker<'interner> { let element_type = Type::type_variable(self.interner.next_type_variable_id()); let expected_type = Type::MutableReference(Box::new(element_type.clone())); - reference_type.unify(&expected_type, assign_span, &mut self.errors, || { - TypeCheckError::TypeMismatch { - expected_typ: expected_type.to_string(), - expr_typ: reference_type.to_string(), - expr_span: assign_span, - } + self.unify(&reference_type, &expected_type, || TypeCheckError::TypeMismatch { + expected_typ: expected_type.to_string(), + expr_typ: reference_type.to_string(), + expr_span: assign_span, }); (element_type.clone(), HirLValue::Dereference { lvalue, element_type }) @@ -226,9 +222,7 @@ impl<'interner> TypeChecker<'interner> { } fn check_let_stmt(&mut self, let_stmt: HirLetStatement) { - let mut resolved_type = self.check_declaration(let_stmt.expression, let_stmt.r#type); - - resolved_type.set_comp_time_span(self.interner.expr_span(&let_stmt.expression)); + let resolved_type = self.check_declaration(let_stmt.expression, let_stmt.r#type); // Set the type of the pattern to be equal to the annotated type self.bind_pattern(&let_stmt.pattern, resolved_type); @@ -238,10 +232,9 @@ impl<'interner> TypeChecker<'interner> { let expr_type = self.check_expression(&stmt.0); let expr_span = self.interner.expr_span(&stmt.0); - let bool_type = Type::Bool(CompTime::new(self.interner)); - self.unify(&expr_type, &bool_type, expr_span, || TypeCheckError::TypeMismatch { + self.unify(&expr_type, &Type::Bool, || TypeCheckError::TypeMismatch { expr_typ: expr_type.to_string(), - expected_typ: Type::Bool(CompTime::No(None)).to_string(), + expected_typ: Type::Bool.to_string(), expr_span, }); } @@ -259,7 +252,7 @@ impl<'interner> TypeChecker<'interner> { // Now check if LHS is the same type as the RHS // Importantly, we do not coerce any types implicitly let expr_span = self.interner.expr_span(&rhs_expr); - self.make_subtype_of(&expr_type, &annotated_type, rhs_expr, || { + self.unify_with_coercions(&expr_type, &annotated_type, rhs_expr, || { TypeCheckError::TypeMismatch { expected_typ: annotated_type.to_string(), expr_typ: expr_type.to_string(), diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index d77b8033ba1..27b6c4b4adb 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -1,5 +1,4 @@ use std::{ - borrow::Cow, cell::RefCell, collections::{BTreeSet, HashMap}, rc::Rc, @@ -19,21 +18,19 @@ use super::expr::{HirCallExpression, HirExpression, HirIdent}; #[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum Type { - /// A primitive Field type, and whether or not it is known at compile-time. - FieldElement(CompTime), + /// A primitive Field type + FieldElement, /// Array(N, E) is an array of N elements of type E. It is expected that N /// is either a type variable of some kind or a Type::Constant. Array(Box, Box), - /// A primitive integer type with the given sign, bit count, and whether it is known at compile-time. - /// E.g. `u32` would be `Integer(CompTime::No(None), Unsigned, 32)` - Integer(CompTime, Signedness, u32), + /// A primitive integer type with the given sign and bit count. + /// E.g. `u32` would be `Integer(Unsigned, 32)` + Integer(Signedness, u32), - /// The primitive `bool` type. Like other primitive types, whether booleans are known at CompTime - /// is also tracked. Unlike the other primitives however, it isn't as useful since it is - /// primarily only used when converting between a bool and an integer type for indexing arrays. - Bool(CompTime), + /// The primitive `bool` type. + Bool, /// String(N) is an array of characters of length N. It is expected that N /// is either a type variable of some kind or a Type::Constant. @@ -361,7 +358,7 @@ pub enum TypeVariableKind { /// This is the type of undecorated integer literals like `46`. Typing them in this way /// allows them to be polymorphic over the actual integer/field type used without requiring /// type annotations on each integer literal. - IntegerOrField(CompTime), + IntegerOrField, /// A potentially constant array size. This will only bind to itself, Type::NotConstant, or /// Type::Constant(n) with a matching size. This defaults to Type::Constant(n) if still unbound @@ -405,225 +402,9 @@ impl TypeBinding { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct TypeVariableId(pub usize); -/// Noir's type system keeps track of whether or not every primitive type's value -/// is known at compile-time. This is exposed through users through the `comptime` -/// keyword in noir which can be prefixed before primitive types. A usage like -/// `t: comptime Field` would correspond to a Field type with a CompTime::Yes(None) -/// variant of this enum -/// -/// Note that whether or not a variable is comptime can also be inferred based on its use. -/// A value passed to a function that expects a `comptime Field` must be CompTime::Yes, -/// likewise a parameter of the current function that is just a `Field` can only be CompTime::No. -/// There is also the case of integer literals which are typed as CompTime::Maybe. These are -/// polymorphically comptime because they can be used in both contexts. -#[derive(Debug, Clone, Eq)] -pub enum CompTime { - // Yes and No variants have optional spans representing the location in the source code - // which caused them to be compile time. - Yes(Option), - No(Option), - - /// Maybe has an id and shared inner reference that can be rebound later to - /// another specific CompTime variant. - Maybe(TypeVariableId, Shared>), -} - -impl std::hash::Hash for CompTime { - fn hash(&self, state: &mut H) { - core::mem::discriminant(self).hash(state); - - if let CompTime::Maybe(id, binding) = self { - if let Some(is_comp_time) = &*binding.borrow() { - is_comp_time.hash(state); - } else { - id.hash(state); - } - } - } -} - -impl PartialEq for CompTime { - fn eq(&self, other: &Self) -> bool { - match (self, other) { - (CompTime::Maybe(id1, binding1), CompTime::Maybe(id2, binding2)) => { - if let Some(new_self) = &*binding1.borrow() { - return new_self == other; - } - if let Some(new_other) = &*binding2.borrow() { - return self == new_other; - } - id1 == id2 - } - (CompTime::Yes(_), CompTime::Yes(_)) | (CompTime::No(_), CompTime::No(_)) => true, - _ => false, - } - } -} - -/// Internal enum for `unify` to remember the type context of each span -/// to provide better error messages -#[derive(Debug)] -pub enum SpanKind { - CompTime(Span), - NotCompTime(Span), - None, -} - -impl CompTime { - pub fn new(interner: &mut NodeInterner) -> Self { - let id = interner.next_type_variable_id(); - Self::Maybe(id, Shared::new(None)) - } - - /// Set the Span on this CompTime (if it has one) to keep track of - /// when it was last changed to give better error messages. - fn set_span(&mut self, new_span: Span) { - match self { - CompTime::Yes(span) | CompTime::No(span) => *span = Some(new_span), - CompTime::Maybe(_, binding) => { - if let Some(binding) = &mut *binding.borrow_mut() { - binding.set_span(new_span); - } - } - } - } - - /// Try to unify these two CompTime constraints. - pub fn unify(&self, other: &Self, span: Span) -> Result<(), SpanKind> { - match (self, other) { - (CompTime::Yes(_), CompTime::Yes(_)) | (CompTime::No(_), CompTime::No(_)) => Ok(()), - - (CompTime::Yes(y), CompTime::No(n)) | (CompTime::No(n), CompTime::Yes(y)) => { - Err(match (y, n) { - (_, Some(span)) => SpanKind::NotCompTime(*span), - (Some(span), _) => SpanKind::CompTime(*span), - _ => SpanKind::None, - }) - } - - (CompTime::Maybe(_, binding), other) | (other, CompTime::Maybe(_, binding)) - if binding.borrow().is_some() => - { - let binding = &*binding.borrow(); - binding.as_ref().unwrap().unify(other, span) - } - - (CompTime::Maybe(id1, _), CompTime::Maybe(id2, _)) if id1 == id2 => Ok(()), - - // Both are unbound and do not refer to each other, arbitrarily set one equal to the other - (CompTime::Maybe(_, binding), other) | (other, CompTime::Maybe(_, binding)) => { - let mut clone = other.clone(); - clone.set_span(span); - *binding.borrow_mut() = Some(clone); - Ok(()) - } - } - } - - /// Try to unify these two CompTime constraints. - pub fn is_subtype_of(&self, other: &Self, span: Span) -> Result<(), SpanKind> { - match (self, other) { - (CompTime::Yes(_), CompTime::Yes(_)) - | (CompTime::No(_), CompTime::No(_)) - - // This is one of the only 2 differing cases between this and CompTime::unify - | (CompTime::Yes(_), CompTime::No(_)) => Ok(()), - - (CompTime::No(n), CompTime::Yes(y)) => { - Err(match (y, n) { - (_, Some(span)) => SpanKind::NotCompTime(*span), - (Some(span), _) => SpanKind::CompTime(*span), - _ => SpanKind::None, - }) - } - - (CompTime::Maybe(_, binding), other) if binding.borrow().is_some() => { - let binding = &*binding.borrow(); - binding.as_ref().unwrap().is_subtype_of(other, span) - } - - (other, CompTime::Maybe(_, binding)) if binding.borrow().is_some() => { - let binding = &*binding.borrow(); - other.is_subtype_of(binding.as_ref().unwrap(), span) - } - - (CompTime::Maybe(id1, _), CompTime::Maybe(id2, _)) if id1 == id2 => Ok(()), - - // This is the other differing case between this and CompTime::unify. - // If this is polymorphically comptime, don't force it to be non-comptime because it is - // passed as an argument to a function expecting a non-comptime parameter. - (CompTime::Maybe(_, binding), CompTime::No(_)) if binding.borrow().is_none() => Ok(()), - - (CompTime::Maybe(_, binding), other) => { - let mut clone = other.clone(); - clone.set_span(span); - *binding.borrow_mut() = Some(clone); - Ok(()) - } - (other, CompTime::Maybe(_, binding)) => { - let mut clone = other.clone(); - clone.set_span(span); - *binding.borrow_mut() = Some(clone); - Ok(()) - } - } - } - - /// Combine these two CompTimes together, returning - /// - CompTime::Yes if both are Yes, - /// - CompTime::No if either are No, - /// - or if both are Maybe, unify them both and return the lhs. - pub fn and(&self, other: &Self, span: Span) -> Self { - match (self, other) { - (CompTime::Yes(_), CompTime::Yes(_)) => CompTime::Yes(Some(span)), - - (CompTime::No(_), CompTime::No(_)) - | (CompTime::Yes(_), CompTime::No(_)) - | (CompTime::No(_), CompTime::Yes(_)) => CompTime::No(Some(span)), - - (CompTime::Maybe(_, binding), other) | (other, CompTime::Maybe(_, binding)) - if binding.borrow().is_some() => - { - let binding = &*binding.borrow(); - binding.as_ref().unwrap().and(other, span) - } - - (CompTime::Maybe(id1, _), CompTime::Maybe(id2, _)) if id1 == id2 => self.clone(), - - (CompTime::Maybe(_, binding), other) | (other, CompTime::Maybe(_, binding)) => { - let mut clone = other.clone(); - clone.set_span(span); - *binding.borrow_mut() = Some(clone); - other.clone() - } - } - } - - pub fn is_comp_time(&self) -> bool { - match self { - CompTime::Yes(_) => true, - CompTime::No(_) => false, - CompTime::Maybe(_, binding) => { - if let Some(binding) = &*binding.borrow() { - return binding.is_comp_time(); - } - true - } - } - } -} - impl Type { - pub fn field(span: Option) -> Type { - Type::FieldElement(CompTime::No(span)) - } - - pub fn comp_time(span: Option) -> Type { - Type::FieldElement(CompTime::Yes(span)) - } - - pub fn default_int_type(span: Option) -> Type { - Type::field(span) + pub fn default_int_type() -> Type { + Type::FieldElement } pub fn type_variable(id: TypeVariableId) -> Type { @@ -640,7 +421,7 @@ impl Type { pub fn polymorphic_integer(interner: &mut NodeInterner) -> Type { let id = interner.next_type_variable_id(); - let kind = TypeVariableKind::IntegerOrField(CompTime::new(interner)); + let kind = TypeVariableKind::IntegerOrField; Type::TypeVariable(Shared::new(TypeBinding::Unbound(id)), kind) } @@ -659,11 +440,11 @@ impl Type { } pub fn is_field(&self) -> bool { - matches!(self.follow_bindings(), Type::FieldElement(_)) + matches!(self.follow_bindings(), Type::FieldElement) } pub fn is_signed(&self) -> bool { - matches!(self.follow_bindings(), Type::Integer(_, Signedness::Signed, _)) + matches!(self.follow_bindings(), Type::Integer(Signedness::Signed, _)) } fn contains_numeric_typevar(&self, target_id: TypeVariableId) -> bool { @@ -682,9 +463,9 @@ impl Type { }; match self { - Type::FieldElement(_) - | Type::Integer(_, _, _) - | Type::Bool(_) + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool | Type::Unit | Type::Error | Type::TypeVariable(_, _) @@ -722,25 +503,13 @@ impl Type { } } } - - pub(crate) fn try_get_comptime(&self) -> Cow { - match self { - Type::FieldElement(comptime) - | Type::Integer(comptime, _, _) - | Type::Bool(comptime) - | Type::TypeVariable(_, TypeVariableKind::IntegerOrField(comptime)) => { - Cow::Borrowed(comptime) - } - _ => Cow::Owned(CompTime::No(None)), - } - } } impl std::fmt::Display for Type { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Type::FieldElement(comp_time) => { - write!(f, "{comp_time}Field") + Type::FieldElement => { + write!(f, "Field") } Type::Array(len, typ) => { if matches!(len.follow_bindings(), Type::NotConstant) { @@ -749,12 +518,12 @@ impl std::fmt::Display for Type { write!(f, "[{typ}; {len}]") } } - Type::Integer(comp_time, sign, num_bits) => match sign { - Signedness::Signed => write!(f, "{comp_time}i{num_bits}"), - Signedness::Unsigned => write!(f, "{comp_time}u{num_bits}"), + Type::Integer(sign, num_bits) => match sign { + Signedness::Signed => write!(f, "i{num_bits}"), + Signedness::Unsigned => write!(f, "u{num_bits}"), }, Type::TypeVariable(id, TypeVariableKind::Normal) => write!(f, "{}", id.borrow()), - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField(_)) => { + Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { if let TypeBinding::Unbound(_) = &*binding.borrow() { // Show a Field by default if this TypeVariableKind::IntegerOrField is unbound, since that is // what they bind to by default anyway. It is less confusing than displaying it @@ -784,7 +553,7 @@ impl std::fmt::Display for Type { let elements = vecmap(elements, ToString::to_string); write!(f, "({})", elements.join(", ")) } - Type::Bool(comp_time) => write!(f, "{comp_time}bool"), + Type::Bool => write!(f, "bool"), Type::String(len) => write!(f, "str<{len}>"), Type::FmtString(len, elements) => { write!(f, "fmtstr<{len}, {elements}>") @@ -846,60 +615,16 @@ impl std::fmt::Display for TypeBinding { } } -impl std::fmt::Display for CompTime { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - CompTime::Yes(_) => write!(f, "comptime "), - CompTime::No(_) => Ok(()), - CompTime::Maybe(_, binding) => match &*binding.borrow() { - Some(binding) => binding.fmt(f), - None => write!(f, "comptime "), - }, - } - } -} +pub struct UnificationError; impl Type { - /// Mutate the span for the `CompTime` enum to track where a type is required to be `comptime` - /// for error messages that show both the erroring call site and the call site before - /// which required the variable to be comptime or non-comptime. - pub fn set_comp_time_span(&mut self, new_span: Span) { - match self { - Type::FieldElement(comptime) | Type::Integer(comptime, _, _) => { - comptime.set_span(new_span); - } - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField(span)) => { - if let TypeBinding::Bound(binding) = &mut *binding.borrow_mut() { - return binding.set_comp_time_span(new_span); - } - span.set_span(new_span); - } - _ => (), - } - } - - pub fn set_comp_time(&mut self, new_comptime: CompTime) { - match self { - Type::FieldElement(comptime) | Type::Integer(comptime, _, _) => { - *comptime = new_comptime; - } - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField(comptime)) => { - if let TypeBinding::Bound(binding) = &mut *binding.borrow_mut() { - return binding.set_comp_time(new_comptime); - } - *comptime = new_comptime; - } - _ => (), - } - } - /// Try to bind a MaybeConstant variable to self, succeeding if self is a Constant, /// MaybeConstant, or type variable. pub fn try_bind_to_maybe_constant( &self, var: &TypeVariable, target_length: u64, - ) -> Result<(), SpanKind> { + ) -> Result<(), UnificationError> { let target_id = match &*var.borrow() { TypeBinding::Bound(_) => unreachable!(), TypeBinding::Unbound(id) => *id, @@ -939,91 +664,65 @@ impl Type { *binding.borrow_mut() = TypeBinding::Bound(clone); Ok(()) } - TypeVariableKind::Constant(_) | TypeVariableKind::IntegerOrField(_) => { - Err(SpanKind::None) + TypeVariableKind::Constant(_) | TypeVariableKind::IntegerOrField => { + Err(UnificationError) } }, } } - _ => Err(SpanKind::None), + _ => Err(UnificationError), } } /// Try to bind a PolymorphicInt variable to self, succeeding if self is an integer, field, /// other PolymorphicInt type, or type variable. If use_subtype is true, the CompTime fields /// of each will be checked via sub-typing rather than unification. - pub fn try_bind_to_polymorphic_int( - &self, - var: &TypeVariable, - var_comp_time: &CompTime, - use_subtype: bool, - span: Span, - ) -> Result<(), SpanKind> { + pub fn try_bind_to_polymorphic_int(&self, var: &TypeVariable) -> Result<(), UnificationError> { let target_id = match &*var.borrow() { TypeBinding::Bound(_) => unreachable!(), TypeBinding::Unbound(id) => *id, }; - let bind = |int_comp_time: &CompTime| { - let mut clone = self.clone(); - let mut new_comp_time = var_comp_time.clone(); - new_comp_time.set_span(span); - clone.set_comp_time(new_comp_time); - - *var.borrow_mut() = TypeBinding::Bound(clone); - - if use_subtype { - var_comp_time.is_subtype_of(int_comp_time, span) - } else { - var_comp_time.unify(int_comp_time, span) - } - }; - match self { - Type::FieldElement(int_comp_time, ..) | Type::Integer(int_comp_time, ..) => { - bind(int_comp_time) + Type::FieldElement | Type::Integer(..) => { + *var.borrow_mut() = TypeBinding::Bound(self.clone()); + Ok(()) } - Type::TypeVariable(self_var, TypeVariableKind::IntegerOrField(int_comp_time)) => { + Type::TypeVariable(self_var, TypeVariableKind::IntegerOrField) => { let borrow = self_var.borrow(); match &*borrow { - TypeBinding::Bound(typ) => { - typ.try_bind_to_polymorphic_int(var, var_comp_time, use_subtype, span) - } + TypeBinding::Bound(typ) => typ.try_bind_to_polymorphic_int(var), // Avoid infinitely recursive bindings TypeBinding::Unbound(id) if *id == target_id => Ok(()), TypeBinding::Unbound(_) => { drop(borrow); - bind(int_comp_time) + *var.borrow_mut() = TypeBinding::Bound(self.clone()); + Ok(()) } } } Type::TypeVariable(binding, TypeVariableKind::Normal) => { let borrow = binding.borrow(); match &*borrow { - TypeBinding::Bound(typ) => { - typ.try_bind_to_polymorphic_int(var, var_comp_time, use_subtype, span) - } + TypeBinding::Bound(typ) => typ.try_bind_to_polymorphic_int(var), // Avoid infinitely recursive bindings TypeBinding::Unbound(id) if *id == target_id => Ok(()), TypeBinding::Unbound(_) => { drop(borrow); // PolymorphicInt is more specific than TypeVariable so we bind the type // variable to PolymorphicInt instead. - let mut clone = Type::TypeVariable( - var.clone(), - TypeVariableKind::IntegerOrField(var_comp_time.clone()), - ); - clone.set_comp_time_span(span); + let clone = + Type::TypeVariable(var.clone(), TypeVariableKind::IntegerOrField); *binding.borrow_mut() = TypeBinding::Bound(clone); Ok(()) } } } - _ => Err(SpanKind::None), + _ => Err(UnificationError), } } - pub fn try_bind_to(&self, var: &TypeVariable) -> Result<(), SpanKind> { + pub fn try_bind_to(&self, var: &TypeVariable) -> Result<(), UnificationError> { let target_id = match &*var.borrow() { TypeBinding::Bound(_) => unreachable!(), TypeBinding::Unbound(id) => *id, @@ -1041,7 +740,7 @@ impl Type { // Check if the target id occurs within self before binding. Otherwise this could // cause infinitely recursive types if self.occurs(target_id) { - Err(SpanKind::None) + Err(UnificationError) } else { *var.borrow_mut() = TypeBinding::Bound(self.clone()); Ok(()) @@ -1055,20 +754,6 @@ impl Type { } } - fn is_comp_time(&self) -> bool { - match self { - Type::FieldElement(comptime) => comptime.is_comp_time(), - Type::Integer(comptime, ..) => comptime.is_comp_time(), - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField(comptime)) => { - if let TypeBinding::Bound(binding) = &*binding.borrow() { - return binding.is_comp_time(); - } - comptime.is_comp_time() - } - _ => false, - } - } - /// Try to unify this type with another, setting any type variables found /// equal to the other type in the process. Unification is more strict /// than sub-typing but less strict than Eq. Returns true if the unification @@ -1077,58 +762,38 @@ impl Type { pub fn unify( &self, expected: &Type, - span: Span, - errors: &mut Vec, - make_error: impl FnOnce() -> TypeCheckError, - ) { - if let Err(err_span) = self.try_unify(expected, span) { - Self::issue_errors(expected, err_span, errors, make_error); - } - } - - fn issue_errors( - expected: &Type, - err_span: SpanKind, errors: &mut Vec, make_error: impl FnOnce() -> TypeCheckError, ) { - errors.push(make_error()); - - match (expected.is_comp_time(), err_span) { - (true, SpanKind::NotCompTime(span)) => { - errors.push(TypeCheckError::NotCompTime { span }); - } - (false, SpanKind::CompTime(span)) => { - errors.push(TypeCheckError::CompTime { span }); - } - _ => (), + if let Err(UnificationError) = self.try_unify(expected) { + errors.push(make_error()); } } /// `try_unify` is a bit of a misnomer since although errors are not committed, /// any unified bindings are on success. - fn try_unify(&self, other: &Type, span: Span) -> Result<(), SpanKind> { + fn try_unify(&self, other: &Type) -> Result<(), UnificationError> { use Type::*; use TypeVariableKind as Kind; match (self, other) { (Error, _) | (_, Error) => Ok(()), - (TypeVariable(binding, Kind::IntegerOrField(comptime)), other) - | (other, TypeVariable(binding, Kind::IntegerOrField(comptime))) => { + (TypeVariable(binding, Kind::IntegerOrField), other) + | (other, TypeVariable(binding, Kind::IntegerOrField)) => { // If it is already bound, unify against what it is bound to if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.try_unify(other, span); + return link.try_unify(other); } // Otherwise, check it is unified against an integer and bind it - other.try_bind_to_polymorphic_int(binding, comptime, false, span) + other.try_bind_to_polymorphic_int(binding) } (TypeVariable(binding, Kind::Normal), other) | (other, TypeVariable(binding, Kind::Normal)) => { if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.try_unify(other, span); + return link.try_unify(other); } other.try_bind_to(binding) @@ -1137,30 +802,30 @@ impl Type { (TypeVariable(binding, Kind::Constant(length)), other) | (other, TypeVariable(binding, Kind::Constant(length))) => { if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.try_unify(other, span); + return link.try_unify(other); } other.try_bind_to_maybe_constant(binding, *length) } (Array(len_a, elem_a), Array(len_b, elem_b)) => { - len_a.try_unify(len_b, span)?; - elem_a.try_unify(elem_b, span) + len_a.try_unify(len_b)?; + elem_a.try_unify(elem_b) } - (String(len_a), String(len_b)) => len_a.try_unify(len_b, span), + (String(len_a), String(len_b)) => len_a.try_unify(len_b), (FmtString(len_a, elements_a), FmtString(len_b, elements_b)) => { - len_a.try_unify(len_b, span)?; - elements_a.try_unify(elements_b, span) + len_a.try_unify(len_b)?; + elements_a.try_unify(elements_b) } (Tuple(elements_a), Tuple(elements_b)) => { if elements_a.len() != elements_b.len() { - Err(SpanKind::None) + Err(UnificationError) } else { for (a, b) in elements_a.iter().zip(elements_b) { - a.try_unify(b, span)?; + a.try_unify(b)?; } Ok(()) } @@ -1172,28 +837,14 @@ impl Type { (Struct(fields_a, args_a), Struct(fields_b, args_b)) => { if fields_a == fields_b { for (a, b) in args_a.iter().zip(args_b) { - a.try_unify(b, span)?; + a.try_unify(b)?; } Ok(()) } else { - Err(SpanKind::None) - } - } - - (FieldElement(comptime_a), FieldElement(comptime_b)) => { - comptime_a.unify(comptime_b, span) - } - - (Integer(comptime_a, signed_a, bits_a), Integer(comptime_b, signed_b, bits_b)) => { - if signed_a == signed_b && bits_a == bits_b { - comptime_a.unify(comptime_b, span) - } else { - Err(SpanKind::None) + Err(UnificationError) } } - (Bool(comptime_a), Bool(comptime_b)) => comptime_a.unify(comptime_b, span), - (NamedGeneric(binding_a, name_a), NamedGeneric(binding_b, name_b)) => { // Ensure NamedGenerics are never bound during type checking assert!(binding_a.borrow().is_unbound()); @@ -1202,54 +853,40 @@ impl Type { if name_a == name_b { Ok(()) } else { - Err(SpanKind::None) + Err(UnificationError) } } (Function(params_a, ret_a, _env_a), Function(params_b, ret_b, _env_b)) => { if params_a.len() == params_b.len() { for (a, b) in params_a.iter().zip(params_b.iter()) { - a.try_unify(b, span)?; + a.try_unify(b)?; } - ret_b.try_unify(ret_a, span) + ret_b.try_unify(ret_a) } else { - Err(SpanKind::None) + Err(UnificationError) } } - (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), + (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b), (other_a, other_b) => { if other_a == other_b { Ok(()) } else { - Err(SpanKind::None) + Err(UnificationError) } } } } - /// The `subtype` term here is somewhat loose, the only sub-typing relations remaining - /// have to do with CompTime tracking. - pub fn make_subtype_of( - &self, - expected: &Type, - span: Span, - errors: &mut Vec, - make_error: impl FnOnce() -> TypeCheckError, - ) { - if let Err(err_span) = self.is_subtype_of(expected, span) { - Self::issue_errors(expected, err_span, errors, make_error); - } - } - /// Similar to `make_subtype_of` but if the check fails this will attempt to coerce the /// argument to the target type. When this happens, the given expression is wrapped in /// a new expression to convert its type. E.g. `array` -> `array.as_slice()` /// /// Currently the only type coercion in Noir is `[T; N]` into `[T]` via `.as_slice()`. - pub fn make_subtype_with_coercions( + pub fn unify_with_coercions( &self, expected: &Type, expression: ExprId, @@ -1257,10 +894,9 @@ impl Type { errors: &mut Vec, make_error: impl FnOnce() -> TypeCheckError, ) { - let span = interner.expr_span(&expression); - if let Err(err_span) = self.is_subtype_of(expected, span) { - if !self.try_array_to_slice_coercion(expected, expression, span, interner) { - Self::issue_errors(expected, err_span, errors, make_error); + if let Err(UnificationError) = self.try_unify(expected) { + if !self.try_array_to_slice_coercion(expected, expression, interner) { + errors.push(make_error()); } } } @@ -1271,7 +907,6 @@ impl Type { &self, target: &Type, expression: ExprId, - span: Span, interner: &mut NodeInterner, ) -> bool { let this = self.follow_bindings(); @@ -1285,7 +920,7 @@ impl Type { if matches!(size1, Type::Constant(_)) && matches!(size2, Type::NotConstant) { // Still have to ensure the element types match. // Don't need to issue an error here if not, it will be done in make_subtype_of_with_coercions - if element1.is_subtype_of(element2, span).is_ok() { + if element1.try_unify(element2).is_ok() { convert_array_expression_to_slice(expression, this, target, interner); return true; } @@ -1294,164 +929,6 @@ impl Type { false } - /// Checks if self is a subtype of `other`. Returns Ok(()) if it is and Err(_) if not. - /// Note that this function may permanently bind type variables regardless of whether it - /// returned Ok or Err. - pub fn is_subtype_of(&self, other: &Type, span: Span) -> Result<(), SpanKind> { - use Type::*; - match (self, other) { - (Error, _) | (_, Error) => Ok(()), - - (TypeVariable(binding, TypeVariableKind::IntegerOrField(comptime)), other) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.is_subtype_of(other, span); - } - - // Otherwise, check it is unified against an integer and bind it - other.try_bind_to_polymorphic_int(binding, comptime, true, span) - } - // These needs to be a separate case to keep the argument order of is_subtype_of - (other, TypeVariable(binding, TypeVariableKind::IntegerOrField(comptime))) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return other.is_subtype_of(link, span); - } - - // use_subtype is false here since we have other <: PolymorphicInt - // while the flag expects PolymorphicInt <: other - other.try_bind_to_polymorphic_int(binding, comptime, false, span) - } - - (TypeVariable(binding, TypeVariableKind::Normal), other) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.is_subtype_of(other, span); - } - - other.try_bind_to(binding) - } - (other, TypeVariable(binding, TypeVariableKind::Normal)) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return other.is_subtype_of(link, span); - } - - other.try_bind_to(binding) - } - - (TypeVariable(binding, TypeVariableKind::Constant(length)), other) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return link.is_subtype_of(other, span); - } - - other.try_bind_to_maybe_constant(binding, *length) - } - (other, TypeVariable(binding, TypeVariableKind::Constant(length))) => { - if let TypeBinding::Bound(link) = &*binding.borrow() { - return other.is_subtype_of(link, span); - } - - other.try_bind_to_maybe_constant(binding, *length) - } - - (Array(len_a, elem_a), Array(len_b, elem_b)) => { - len_a.is_subtype_of(len_b, span)?; - elem_a.is_subtype_of(elem_b, span) - } - - (String(len_a), String(len_b)) => len_a.is_subtype_of(len_b, span), - - (FmtString(len_a, elements_a), FmtString(len_b, elements_b)) => { - len_a.is_subtype_of(len_b, span)?; - elements_a.is_subtype_of(elements_b, span) - } - - (Tuple(elements_a), Tuple(elements_b)) => { - if elements_a.len() != elements_b.len() { - Err(SpanKind::None) - } else { - for (a, b) in elements_a.iter().zip(elements_b) { - a.is_subtype_of(b, span)?; - } - Ok(()) - } - } - - // No recursive try_unify call needed for struct fields, we just - // check the struct ids match. - (Struct(struct_a, args_a), Struct(struct_b, args_b)) => { - if struct_a == struct_b && args_a.len() == args_b.len() { - for (a, b) in args_a.iter().zip(args_b) { - a.is_subtype_of(b, span)?; - } - Ok(()) - } else { - Err(SpanKind::None) - } - } - - (FieldElement(comptime_a), FieldElement(comptime_b)) => { - comptime_a.is_subtype_of(comptime_b, span) - } - - (Integer(comptime_a, signed_a, bits_a), Integer(comptime_b, signed_b, bits_b)) => { - if signed_a == signed_b && bits_a == bits_b { - comptime_a.is_subtype_of(comptime_b, span) - } else { - Err(SpanKind::None) - } - } - - (Bool(comptime_a), Bool(comptime_b)) => comptime_a.is_subtype_of(comptime_b, span), - - (NamedGeneric(binding_a, name_a), NamedGeneric(binding_b, name_b)) => { - // Ensure NamedGenerics are never bound during type checking - assert!(binding_a.borrow().is_unbound()); - assert!(binding_b.borrow().is_unbound()); - - if name_a == name_b { - Ok(()) - } else { - Err(SpanKind::None) - } - } - - (Function(params_a, ret_a, _env_a), Function(params_b, ret_b, _env_b)) => { - if params_a.len() == params_b.len() { - for (a, b) in params_a.iter().zip(params_b) { - a.is_subtype_of(b, span)?; - } - - // return types are contravariant, so this must be ret_b <: ret_a instead of the reverse - ret_b.is_subtype_of(ret_a, span) - } else { - Err(SpanKind::None) - } - } - - // `T <: U => &mut T <: &mut U` would be unsound(*), so mutable - // references are never subtypes of each other. - // - // (*) Consider: - // ``` - // // Assume Dog <: Animal and Cat <: Animal - // let x: &mut Dog = ...; - // - // fn set_to_cat(y: &mut Animal) { - // *y = Cat; - // } - // - // set_to_cat(x); // uh-oh: x: Dog, yet it now holds a Cat - // ``` - (MutableReference(elem_a), MutableReference(elem_b)) => elem_a.try_unify(elem_b, span), - - (other_a, other_b) => { - if other_a == other_b { - Ok(()) - } else { - Err(SpanKind::None) - } - } - } - } - /// If this type is a Type::Constant (used in array lengths), or is bound /// to a Type::Constant, return the constant as a u64. pub fn evaluate_to_u64(&self) -> Option { @@ -1473,14 +950,14 @@ impl Type { // in this method, you most likely want to distinguish between public and private pub fn as_abi_type(&self) -> AbiType { match self { - Type::FieldElement(_) => AbiType::Field, + Type::FieldElement => AbiType::Field, Type::Array(size, typ) => { let length = size .evaluate_to_u64() .expect("Cannot have variable sized arrays as a parameter to main"); AbiType::Array { length, typ: Box::new(typ.as_abi_type()) } } - Type::Integer(_, sign, bit_width) => { + Type::Integer(sign, bit_width) => { let sign = match sign { Signedness::Unsigned => noirc_abi::Sign::Unsigned, Signedness::Signed => noirc_abi::Sign::Signed, @@ -1488,13 +965,13 @@ impl Type { AbiType::Integer { sign, width: *bit_width } } - Type::TypeVariable(binding, TypeVariableKind::IntegerOrField(_)) => { + Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => { match &*binding.borrow() { TypeBinding::Bound(typ) => typ.as_abi_type(), - TypeBinding::Unbound(_) => Type::default_int_type(None).as_abi_type(), + TypeBinding::Unbound(_) => Type::default_int_type().as_abi_type(), } } - Type::Bool(_) => AbiType::Boolean, + Type::Bool => AbiType::Boolean, Type::String(size) => { let size = size .evaluate_to_u64() @@ -1640,9 +1117,9 @@ impl Type { Type::MutableReference(Box::new(element.substitute(type_bindings))) } - Type::FieldElement(_) - | Type::Integer(_, _, _) - | Type::Bool(_) + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool | Type::Constant(_) | Type::Error | Type::NotConstant @@ -1678,9 +1155,9 @@ impl Type { } Type::MutableReference(element) => element.occurs(target_id), - Type::FieldElement(_) - | Type::Integer(_, _, _) - | Type::Bool(_) + Type::FieldElement + | Type::Integer(_, _) + | Type::Bool | Type::Constant(_) | Type::Error | Type::NotConstant @@ -1731,13 +1208,9 @@ impl Type { // Expect that this function should only be called on instantiated types Forall(..) => unreachable!(), - FieldElement(_) - | Integer(_, _, _) - | Bool(_) - | Constant(_) - | Unit - | Error - | NotConstant => self.clone(), + FieldElement | Integer(_, _) | Bool | Constant(_) | Unit | Error | NotConstant => { + self.clone() + } } } } @@ -1790,7 +1263,7 @@ impl TypeVariableKind { /// during monomorphization. pub(crate) fn default_type(&self) -> Type { match self { - TypeVariableKind::IntegerOrField(_) | TypeVariableKind::Normal => Type::field(None), + TypeVariableKind::IntegerOrField | TypeVariableKind::Normal => Type::default_int_type(), TypeVariableKind::Constant(length) => Type::Constant(*length), } } diff --git a/crates/noirc_frontend/src/lexer/token.rs b/crates/noirc_frontend/src/lexer/token.rs index 3ef1d2a5dde..e9b67433ab4 100644 --- a/crates/noirc_frontend/src/lexer/token.rs +++ b/crates/noirc_frontend/src/lexer/token.rs @@ -443,7 +443,6 @@ pub enum Keyword { Assert, Bool, Char, - CompTime, Constrain, Contract, Crate, @@ -482,7 +481,6 @@ impl fmt::Display for Keyword { Keyword::Assert => write!(f, "assert"), Keyword::Bool => write!(f, "bool"), Keyword::Char => write!(f, "char"), - Keyword::CompTime => write!(f, "comptime"), Keyword::Constrain => write!(f, "constrain"), Keyword::Contract => write!(f, "contract"), Keyword::Crate => write!(f, "crate"), @@ -524,7 +522,6 @@ impl Keyword { "assert" => Keyword::Assert, "bool" => Keyword::Bool, "char" => Keyword::Char, - "comptime" => Keyword::CompTime, "constrain" => Keyword::Constrain, "contract" => Keyword::Contract, "crate" => Keyword::Crate, diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index c8167baf6bb..28849167d56 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -620,9 +620,9 @@ impl<'interner> Monomorphizer<'interner> { /// Convert a non-tuple/struct type to a monomorphized type fn convert_type(typ: &HirType) -> ast::Type { match typ { - HirType::FieldElement(_) => ast::Type::Field, - HirType::Integer(_, sign, bits) => ast::Type::Integer(*sign, *bits), - HirType::Bool(_) => ast::Type::Bool, + HirType::FieldElement => ast::Type::Field, + HirType::Integer(sign, bits) => ast::Type::Integer(*sign, *bits), + HirType::Bool => ast::Type::Bool, HirType::String(size) => ast::Type::String(size.evaluate_to_u64().unwrap_or(0)), HirType::FmtString(size, fields) => { let size = size.evaluate_to_u64().unwrap_or(0); @@ -653,7 +653,7 @@ impl<'interner> Monomorphizer<'interner> { // like automatic solving of traits. It should be fine since it is strictly // after type checking, but care should be taken that it doesn't change which // impls are chosen. - *binding.borrow_mut() = TypeBinding::Bound(HirType::field(None)); + *binding.borrow_mut() = TypeBinding::Bound(HirType::default_int_type()); ast::Type::Field } diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 6b3d2757c14..ad1638c3f0b 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -664,11 +664,11 @@ fn get_type_method_key(typ: &Type) -> Option { use TypeMethodKey::*; let typ = typ.follow_bindings(); match &typ { - Type::FieldElement(_) => Some(FieldOrInt), + Type::FieldElement => Some(FieldOrInt), Type::Array(_, _) => Some(Array), - Type::Integer(_, _, _) => Some(FieldOrInt), - Type::TypeVariable(_, TypeVariableKind::IntegerOrField(_)) => Some(FieldOrInt), - Type::Bool(_) => Some(Bool), + Type::Integer(_, _) => Some(FieldOrInt), + Type::TypeVariable(_, TypeVariableKind::IntegerOrField) => Some(FieldOrInt), + Type::Bool => Some(Bool), Type::String(_) => Some(String), Type::Unit => Some(Unit), Type::Tuple(_) => Some(Tuple), diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 6445205eae6..dd84c2e73cc 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -34,10 +34,10 @@ use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ - BinaryOp, BinaryOpKind, BlockExpression, CompTime, ConstrainStatement, FunctionDefinition, - Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, - NoirTrait, NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, - TraitImplItem, TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, + BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, FunctionDefinition, Ident, + IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, NoirTrait, + NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, TraitImplItem, + TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, }; use chumsky::prelude::*; @@ -548,17 +548,9 @@ fn check_statements_require_semicolon( }) } -/// Parse an optional ': type' and implicitly add a 'comptime' to the type +/// Parse an optional ': type' fn global_type_annotation() -> impl NoirParser { ignore_then_commit(just(Token::Colon), parse_type()) - .map(|r#type| match r#type { - UnresolvedType::FieldElement(_) => UnresolvedType::FieldElement(CompTime::Yes(None)), - UnresolvedType::Bool(_) => UnresolvedType::Bool(CompTime::Yes(None)), - UnresolvedType::Integer(_, sign, size) => { - UnresolvedType::Integer(CompTime::Yes(None), sign, size) - } - other => other, - }) .or_not() .map(|opt| opt.unwrap_or(UnresolvedType::Unspecified)) } @@ -834,19 +826,12 @@ fn optional_distinctness() -> impl NoirParser { }) } -fn maybe_comp_time() -> impl NoirParser { - keyword(Keyword::CompTime).or_not().map(|opt| match opt { - Some(_) => CompTime::Yes(None), - None => CompTime::No(None), - }) -} - fn field_type() -> impl NoirParser { - maybe_comp_time().then_ignore(keyword(Keyword::Field)).map(UnresolvedType::FieldElement) + keyword(Keyword::Field).map(|_| UnresolvedType::FieldElement) } fn bool_type() -> impl NoirParser { - maybe_comp_time().then_ignore(keyword(Keyword::Bool)).map(UnresolvedType::Bool) + keyword(Keyword::Bool).map(|_| UnresolvedType::Bool) } fn string_type() -> impl NoirParser { @@ -871,14 +856,13 @@ fn format_string_type( } fn int_type() -> impl NoirParser { - maybe_comp_time() - .then(filter_map(|span, token: Token| match token { - Token::IntType(int_type) => Ok(int_type), - unexpected => { - Err(ParserError::expected_label(ParsingRuleLabel::IntegerType, unexpected, span)) - } - })) - .map(UnresolvedType::from_int_token) + filter_map(|span, token: Token| match token { + Token::IntType(int_type) => Ok(int_type), + unexpected => { + Err(ParserError::expected_label(ParsingRuleLabel::IntegerType, unexpected, span)) + } + }) + .map(UnresolvedType::from_int_token) } fn named_type(type_parser: impl NoirParser) -> impl NoirParser { @@ -1747,7 +1731,7 @@ mod test { vec![ "fn func_name() {}", "fn f(foo: pub u8, y : pub Field) -> u8 { x + a }", - "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", + "fn f(f: pub Field, y : Field, z : Field) -> u8 { x + a }", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}", "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5]) {}", "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }", From 9fdd5a3bd53de6fbe0b3f61b8fe93d11a6d9844b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 3 Aug 2023 15:08:12 -0500 Subject: [PATCH 02/12] Remove comptime from stdlib and tests --- .../tests/test_data/array_len/src/main.nr | 2 +- .../test_data/bit_shifts_comptime/src/main.nr | 4 ++-- .../test_data/bit_shifts_runtime/src/main.nr | 4 ++-- .../comptime_array_access/Nargo.toml | 6 ------ .../comptime_array_access/Prover.toml | 1 - .../comptime_array_access/src/main.nr | 17 --------------- .../target/comptime_array_access.json | 1 - .../comptime_array_access/target/witness.tr | Bin 65 -> 0 bytes .../tests/test_data/global_consts/src/main.nr | 6 +++--- .../test_data/numeric_generics/src/main.nr | 2 +- .../tests/test_data/regression/src/main.nr | 4 ++-- noir_stdlib/src/array.nr | 2 +- noir_stdlib/src/field.nr | 2 +- noir_stdlib/src/hash.nr | 2 +- noir_stdlib/src/hash/poseidon.nr | 20 +++++++++--------- noir_stdlib/src/hash/poseidon/bn254.nr | 4 ++-- noir_stdlib/src/hash/poseidon/bn254/consts.nr | 2 +- 17 files changed, 27 insertions(+), 52 deletions(-) delete mode 100644 crates/nargo_cli/tests/test_data/comptime_array_access/Nargo.toml delete mode 100644 crates/nargo_cli/tests/test_data/comptime_array_access/Prover.toml delete mode 100644 crates/nargo_cli/tests/test_data/comptime_array_access/src/main.nr delete mode 100644 crates/nargo_cli/tests/test_data/comptime_array_access/target/comptime_array_access.json delete mode 100644 crates/nargo_cli/tests/test_data/comptime_array_access/target/witness.tr diff --git a/crates/nargo_cli/tests/test_data/array_len/src/main.nr b/crates/nargo_cli/tests/test_data/array_len/src/main.nr index 65c2295cefb..ac9811e021e 100644 --- a/crates/nargo_cli/tests/test_data/array_len/src/main.nr +++ b/crates/nargo_cli/tests/test_data/array_len/src/main.nr @@ -18,7 +18,7 @@ fn main(x: Field, len3: [u8; 3], len4: [Field; 4]) { assert(add_lens(len3, len4) == 7); assert(nested_call(len4) == 5); - // std::array::len returns a comptime value + // std::array::len returns a compile-time known value assert(len4[len3.len()] == 4); // Regression for #1023, ensure .len still works after calling to_le_bytes on a witness. diff --git a/crates/nargo_cli/tests/test_data/bit_shifts_comptime/src/main.nr b/crates/nargo_cli/tests/test_data/bit_shifts_comptime/src/main.nr index c1c6890febb..87e8479300e 100644 --- a/crates/nargo_cli/tests/test_data/bit_shifts_comptime/src/main.nr +++ b/crates/nargo_cli/tests/test_data/bit_shifts_comptime/src/main.nr @@ -2,12 +2,12 @@ fn main(x: u64) { let two: u64 = 2; let three: u64 = 3; - // comptime shifts on comptime values + // shifts on constant values assert(two << 2 == 8); assert((two << 3) / 8 == two); assert((three >> 1) == 1); - // comptime shifts on runtime values + // shifts on runtime values assert(x << 1 == 128); assert(x >> 2 == 16); } diff --git a/crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr index 271a1ecb880..f4d9c3916a6 100644 --- a/crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr +++ b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr @@ -1,9 +1,9 @@ fn main(x: u64, y: u64) { - // runtime shifts on comptime values + // runtime shifts on compile-time known values assert(64 << y == 128); assert(64 >> y == 32); // runtime shifts on runtime values assert(x << y == 128); assert(x >> y == 32); -} \ No newline at end of file +} diff --git a/crates/nargo_cli/tests/test_data/comptime_array_access/Nargo.toml b/crates/nargo_cli/tests/test_data/comptime_array_access/Nargo.toml deleted file mode 100644 index c0f183dd74e..00000000000 --- a/crates/nargo_cli/tests/test_data/comptime_array_access/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "comptime_array_access" -authors = [""] -compiler_version = "0.1" - -[dependencies] diff --git a/crates/nargo_cli/tests/test_data/comptime_array_access/Prover.toml b/crates/nargo_cli/tests/test_data/comptime_array_access/Prover.toml deleted file mode 100644 index ec8d8e34856..00000000000 --- a/crates/nargo_cli/tests/test_data/comptime_array_access/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -a = [1, 2, 3] diff --git a/crates/nargo_cli/tests/test_data/comptime_array_access/src/main.nr b/crates/nargo_cli/tests/test_data/comptime_array_access/src/main.nr deleted file mode 100644 index 04f08bb70c5..00000000000 --- a/crates/nargo_cli/tests/test_data/comptime_array_access/src/main.nr +++ /dev/null @@ -1,17 +0,0 @@ -fn main(a: [Field; 3]) { - let i = 1; - - // Using a comptime variable as a parameter should not make it non-comptime - let ii = foo(i); - let elem1 = a[i]; - - // Nor should using it in an expression with a non-comptime variable. - let two = i + ii; - assert(i == ii); - - let elem2 = a[i]; - assert(elem1 == elem2); - assert(two == 2); -} - -fn foo(x: Field) -> Field { x } diff --git a/crates/nargo_cli/tests/test_data/comptime_array_access/target/comptime_array_access.json b/crates/nargo_cli/tests/test_data/comptime_array_access/target/comptime_array_access.json deleted file mode 100644 index 5e17d6d504e..00000000000 --- a/crates/nargo_cli/tests/test_data/comptime_array_access/target/comptime_array_access.json +++ /dev/null @@ -1 +0,0 @@ -{"backend":"acvm-backend-barretenberg","abi":{"parameters":[{"name":"a","type":{"kind":"array","length":3,"type":{"kind":"field"}},"visibility":"private"}],"param_witnesses":{"a":[1,2,3]},"return_type":null,"return_witnesses":[]},"bytecode":"H4sIAAAAAAAA/81UUQrDIAzN2ur+dpbEaI1/u8pk9v5H2EodhDK2jyr0gTzNR/KSJ7kCgIUNpvJY+fI+g3qDiq+4V2acvS/RFWJ6oEtZAvqQZyGhIOHphLmIl5hyipjIc6ElJF5ww6Ry4UGMSt8vzXgM1FKz1mvUfao8qNjHC9uhJ9jV2c/x9iXWtHgPk0yHvBbaff5efdv2HqFKeeqZ/ltgK15vGI6d+QQAAA==","proving_key":null,"verification_key":null} \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data/comptime_array_access/target/witness.tr b/crates/nargo_cli/tests/test_data/comptime_array_access/target/witness.tr deleted file mode 100644 index 5e9a938686fd7ca0a535b48abd021d26fdda5f20..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 65 zcmb2|=3oE;rvGa%+~#C3;BdZZH~089?vnp2MM{|S6(+3zESNd*-QS&Crx{zk_B&VA Uy}6`$TEl(mJdVQ73ycg507;n}m;e9( diff --git a/crates/nargo_cli/tests/test_data/global_consts/src/main.nr b/crates/nargo_cli/tests/test_data/global_consts/src/main.nr index 2ed6e4593dd..13b9159b480 100644 --- a/crates/nargo_cli/tests/test_data/global_consts/src/main.nr +++ b/crates/nargo_cli/tests/test_data/global_consts/src/main.nr @@ -35,7 +35,7 @@ fn main(a: [Field; M + N - N], b: [Field; 30 + N / 2], c : pub [Field; foo::MAGI let mut y = 5; let mut x = M; for i in 0..N*N { - let M: comptime Field = 10; + let M: Field = 10; x = M; y = i; @@ -87,8 +87,8 @@ mod mysubmodule { assert(x | y == 1); } - fn my_helper() -> comptime Field { - let N: comptime Field = 15; // Like in Rust, local variables override globals + fn my_helper() -> Field { + let N: Field = 15; // Like in Rust, local variables override globals let x = N; x } diff --git a/crates/nargo_cli/tests/test_data/numeric_generics/src/main.nr b/crates/nargo_cli/tests/test_data/numeric_generics/src/main.nr index f1efafc19fd..1e03a382fed 100644 --- a/crates/nargo_cli/tests/test_data/numeric_generics/src/main.nr +++ b/crates/nargo_cli/tests/test_data/numeric_generics/src/main.nr @@ -23,7 +23,7 @@ struct MyStruct { } impl MyStruct { - fn insert(mut self: Self, index: comptime Field, elem: Field) -> Self { + fn insert(mut self: Self, index: Field, elem: Field) -> Self { // Regression test for numeric generics on impls assert(index as u64 < S as u64); diff --git a/crates/nargo_cli/tests/test_data/regression/src/main.nr b/crates/nargo_cli/tests/test_data/regression/src/main.nr index e01888dea37..54769c39709 100644 --- a/crates/nargo_cli/tests/test_data/regression/src/main.nr +++ b/crates/nargo_cli/tests/test_data/regression/src/main.nr @@ -1,4 +1,4 @@ -global NIBBLE_LENGTH: comptime Field = 16; +global NIBBLE_LENGTH: Field = 16; fn compact_decode(input: [u8; N], length: Field) -> ([u4; NIBBLE_LENGTH], Field) { @@ -87,4 +87,4 @@ fn main(x: [u8; 5], z: Field) assert(enc_val1.0 == [0x94,0xb8,0x8f,0x61,0xe6,0xfb,0xda,0x83,0xfb,0xff,0xfa,0xbe,0x36,0x41,0x12,0x13,0x74,0x80,0x39,0x80,0x18,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00]); assert(enc_val1.1 == 21); -} \ No newline at end of file +} diff --git a/noir_stdlib/src/array.nr b/noir_stdlib/src/array.nr index db349317f91..9082e161e91 100644 --- a/noir_stdlib/src/array.nr +++ b/noir_stdlib/src/array.nr @@ -3,7 +3,7 @@ // by the methods in the `slice` module impl [T; N] { #[builtin(array_len)] - fn len(_self: Self) -> comptime Field {} + fn len(_self: Self) -> Field {} #[builtin(arraysort)] fn sort(_self: Self) -> Self {} diff --git a/noir_stdlib/src/field.nr b/noir_stdlib/src/field.nr index b7773182d66..5d3581689f5 100644 --- a/noir_stdlib/src/field.nr +++ b/noir_stdlib/src/field.nr @@ -40,7 +40,7 @@ impl Field { } #[builtin(modulus_num_bits)] -fn modulus_num_bits() -> comptime Field {} +fn modulus_num_bits() -> Field {} #[builtin(modulus_be_bits)] fn modulus_be_bits() -> [u1] {} diff --git a/noir_stdlib/src/hash.nr b/noir_stdlib/src/hash.nr index e5dbdadaf7f..5b0db2f2941 100644 --- a/noir_stdlib/src/hash.nr +++ b/noir_stdlib/src/hash.nr @@ -11,7 +11,7 @@ fn pedersen(input : [Field; N]) -> [Field; 2] { } #[foreign(pedersen)] -fn pedersen_with_separator(_input : [Field; N], _separator : comptime u32) -> [Field; 2] {} +fn pedersen_with_separator(_input : [Field; N], _separator : u32) -> [Field; 2] {} #[foreign(hash_to_field_128_security)] fn hash_to_field(_input : [Field; N]) -> Field {} diff --git a/noir_stdlib/src/hash/poseidon.nr b/noir_stdlib/src/hash/poseidon.nr index cb1e34927b4..cb97716d987 100644 --- a/noir_stdlib/src/hash/poseidon.nr +++ b/noir_stdlib/src/hash/poseidon.nr @@ -3,19 +3,19 @@ mod bn254; // Instantiations of Poseidon for prime field of the same order as BN use crate::field::modulus_num_bits; struct PoseidonConfig { - t: comptime Field, // Width, i.e. state size - rf: comptime u8, // Number of full rounds; should be even - rp: comptime u8, // Number of partial rounds - alpha: comptime Field, // S-box power; depends on the underlying field + t: Field, // Width, i.e. state size + rf: u8, // Number of full rounds; should be even + rp: u8, // Number of partial rounds + alpha: Field, // S-box power; depends on the underlying field ark: [Field; M], // Additive round keys mds: [Field; N] // MDS Matrix in row-major order } fn config( - t: comptime Field, - rf: comptime u8, - rp: comptime u8, - alpha: comptime Field, + t: Field, + rf: u8, + rp: u8, + alpha: Field, ark: [Field; M], mds: [Field; N]) -> PoseidonConfig { @@ -64,8 +64,8 @@ fn permute( fn absorb( pos_conf: PoseidonConfig, mut state: [Field; O], // Initial state; usually [0; O] - rate: comptime Field, // Rate - capacity: comptime Field, // Capacity; usually 1 + rate: Field, // Rate + capacity: Field, // Capacity; usually 1 msg: [Field; P]) // Arbitrary length message -> [Field; O] { assert(pos_conf.t == rate + capacity); diff --git a/noir_stdlib/src/hash/poseidon/bn254.nr b/noir_stdlib/src/hash/poseidon/bn254.nr index 9ba26dbd878..0b4e5b5bf41 100644 --- a/noir_stdlib/src/hash/poseidon/bn254.nr +++ b/noir_stdlib/src/hash/poseidon/bn254.nr @@ -68,8 +68,8 @@ fn permute( fn absorb( pos_conf: PoseidonConfig, mut state: [Field; O], // Initial state; usually [0; O] - rate: comptime Field, // Rate - capacity: comptime Field, // Capacity; usually 1 + rate: Field, // Rate + capacity: Field, // Capacity; usually 1 msg: [Field; P] // Arbitrary length message ) -> [Field; O] { diff --git a/noir_stdlib/src/hash/poseidon/bn254/consts.nr b/noir_stdlib/src/hash/poseidon/bn254/consts.nr index daa5260e59d..83923eff6ef 100644 --- a/noir_stdlib/src/hash/poseidon/bn254/consts.nr +++ b/noir_stdlib/src/hash/poseidon/bn254/consts.nr @@ -12,7 +12,7 @@ fn rp() -> [u8; 16] { } // S-box power -fn alpha() -> comptime Field { +fn alpha() -> Field { 5 } From 72c448cc6d15236ca3e8dbaaeeacda1eb066e6c6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 3 Aug 2023 15:14:44 -0500 Subject: [PATCH 03/12] Deprecate comptime keyword --- crates/noirc_frontend/src/lexer/token.rs | 3 ++ crates/noirc_frontend/src/parser/errors.rs | 7 ++++ crates/noirc_frontend/src/parser/parser.rs | 37 +++++++++++++--------- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/crates/noirc_frontend/src/lexer/token.rs b/crates/noirc_frontend/src/lexer/token.rs index e9b67433ab4..3ef1d2a5dde 100644 --- a/crates/noirc_frontend/src/lexer/token.rs +++ b/crates/noirc_frontend/src/lexer/token.rs @@ -443,6 +443,7 @@ pub enum Keyword { Assert, Bool, Char, + CompTime, Constrain, Contract, Crate, @@ -481,6 +482,7 @@ impl fmt::Display for Keyword { Keyword::Assert => write!(f, "assert"), Keyword::Bool => write!(f, "bool"), Keyword::Char => write!(f, "char"), + Keyword::CompTime => write!(f, "comptime"), Keyword::Constrain => write!(f, "constrain"), Keyword::Contract => write!(f, "contract"), Keyword::Crate => write!(f, "crate"), @@ -522,6 +524,7 @@ impl Keyword { "assert" => Keyword::Assert, "bool" => Keyword::Bool, "char" => Keyword::Char, + "comptime" => Keyword::CompTime, "constrain" => Keyword::Constrain, "contract" => Keyword::Contract, "crate" => Keyword::Crate, diff --git a/crates/noirc_frontend/src/parser/errors.rs b/crates/noirc_frontend/src/parser/errors.rs index 9b072d83b09..d20a3db6202 100644 --- a/crates/noirc_frontend/src/parser/errors.rs +++ b/crates/noirc_frontend/src/parser/errors.rs @@ -27,6 +27,8 @@ pub enum ParserErrorReason { PatternInTraitFunctionParameter, #[error("Traits are an experimental feature that are not yet in a working state")] TraitsAreExperimental, + #[error("comptime keyword is deprecated")] + ComptimeDeprecated, } /// Represents a parsing error, or a parsing error in the making. @@ -117,6 +119,11 @@ impl From for Diagnostic { "The 'constrain' keyword has been deprecated. Please use the 'assert' function instead.".into(), error.span, ), + ParserErrorReason::ComptimeDeprecated => Diagnostic::simple_warning( + "Use of deprecated keyword 'comptime'".into(), + "The 'comptime' keyword has been deprecated. It can be removed without affecting your program".into(), + error.span, + ), reason @ ParserErrorReason::ExpectedPatternButFoundType(ty) => { Diagnostic::simple_error(reason.to_string(), format!("{ty} is a type and cannot be used as a variable name"), error.span) } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index dd84c2e73cc..9d6091b84bb 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -34,10 +34,10 @@ use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ - BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, FunctionDefinition, Ident, - IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, NoirTrait, - NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, TraitImplItem, - TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, + BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, FunctionDefinition, + Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, + NoirTrait, NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, + TraitImplItem, TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, }; use chumsky::prelude::*; @@ -548,7 +548,7 @@ fn check_statements_require_semicolon( }) } -/// Parse an optional ': type' +/// Parse an optional ': type' and implicitly add a 'comptime' to the type fn global_type_annotation() -> impl NoirParser { ignore_then_commit(just(Token::Colon), parse_type()) .or_not() @@ -826,12 +826,18 @@ fn optional_distinctness() -> impl NoirParser { }) } +fn maybe_comp_time() -> impl NoirParser<()> { + keyword(Keyword::CompTime).or_not().validate(|opt, span, emit| if opt.is_some() { + emit(ParserError::with_reason(ParserErrorReason::ComptimeDeprecated, span)); + }) +} + fn field_type() -> impl NoirParser { - keyword(Keyword::Field).map(|_| UnresolvedType::FieldElement) + maybe_comp_time().then_ignore(keyword(Keyword::Field)).map(|_| UnresolvedType::FieldElement) } fn bool_type() -> impl NoirParser { - keyword(Keyword::Bool).map(|_| UnresolvedType::Bool) + maybe_comp_time().then_ignore(keyword(Keyword::Bool)).map(|_| UnresolvedType::Bool) } fn string_type() -> impl NoirParser { @@ -856,13 +862,14 @@ fn format_string_type( } fn int_type() -> impl NoirParser { - filter_map(|span, token: Token| match token { - Token::IntType(int_type) => Ok(int_type), - unexpected => { - Err(ParserError::expected_label(ParsingRuleLabel::IntegerType, unexpected, span)) - } - }) - .map(UnresolvedType::from_int_token) + maybe_comp_time() + .then(filter_map(|span, token: Token| match token { + Token::IntType(int_type) => Ok(int_type), + unexpected => { + Err(ParserError::expected_label(ParsingRuleLabel::IntegerType, unexpected, span)) + } + })) + .map(|(_, token)| UnresolvedType::from_int_token(token)) } fn named_type(type_parser: impl NoirParser) -> impl NoirParser { @@ -1731,7 +1738,7 @@ mod test { vec![ "fn func_name() {}", "fn f(foo: pub u8, y : pub Field) -> u8 { x + a }", - "fn f(f: pub Field, y : Field, z : Field) -> u8 { x + a }", + "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}", "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5]) {}", "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }", From 3a4cad972148f1db77c9289fb7110794679b50a6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 3 Aug 2023 15:58:10 -0500 Subject: [PATCH 04/12] Add unknown loop bound error --- crates/noirc_evaluator/src/errors.rs | 43 ++++++----- crates/noirc_evaluator/src/ssa.rs | 2 +- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 77 ++++++++++++------- crates/noirc_frontend/src/parser/parser.rs | 14 ++-- 4 files changed, 80 insertions(+), 56 deletions(-) diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index 27a87ccce36..00dd9876c45 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -30,6 +30,10 @@ pub enum RuntimeError { UnInitialized { name: String, location: Option }, #[error("Integer sized {num_bits:?} is over the max supported size of {max_num_bits:?}")] UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option }, + #[error( + "Could not determine loop bound at compile-time. Loop bounds must be compile-time known" + )] + UnknownLoopBound { location: Option }, } #[derive(Debug, PartialEq, Eq, Clone, Error)] @@ -50,34 +54,36 @@ pub enum InternalError { UnExpected { expected: String, found: String, location: Option }, } -impl From for FileDiagnostic { - fn from(error: RuntimeError) -> Self { - match error { - RuntimeError::InternalError(ref ice_error) => match ice_error { +impl RuntimeError { + fn location(&self) -> Option { + match self { + RuntimeError::InternalError( InternalError::DegreeNotReduced { location } | InternalError::EmptyArray { location } | InternalError::General { location, .. } | InternalError::MissingArg { location, .. } | InternalError::NotAConstant { location, .. } | InternalError::UndeclaredAcirVar { location } - | InternalError::UnExpected { location, .. } => { - let file_id = location.map(|loc| loc.file).unwrap(); - FileDiagnostic { file_id, diagnostic: error.into() } - } - }, - RuntimeError::FailedConstraint { location, .. } + | InternalError::UnExpected { location, .. }, + ) + | RuntimeError::FailedConstraint { location, .. } | RuntimeError::IndexOutOfBounds { location, .. } | RuntimeError::InvalidRangeConstraint { location, .. } | RuntimeError::TypeConversion { location, .. } | RuntimeError::UnInitialized { location, .. } - | RuntimeError::UnsupportedIntegerSize { location, .. } => { - let file_id = location.map(|loc| loc.file).unwrap(); - FileDiagnostic { file_id, diagnostic: error.into() } - } + | RuntimeError::UnknownLoopBound { location, .. } + | RuntimeError::UnsupportedIntegerSize { location, .. } => *location, } } } +impl From for FileDiagnostic { + fn from(error: RuntimeError) -> Self { + let file_id = error.location().map(|loc| loc.file).unwrap(); + FileDiagnostic { file_id, diagnostic: error.into() } + } +} + impl From for Diagnostic { fn from(error: RuntimeError) -> Diagnostic { match error { @@ -87,13 +93,8 @@ impl From for Diagnostic { "".to_string(), noirc_errors::Span::new(0..0) ), - RuntimeError::FailedConstraint { location, .. } - | RuntimeError::IndexOutOfBounds { location, .. } - | RuntimeError::InvalidRangeConstraint { location, .. } - | RuntimeError::TypeConversion { location, .. } - | RuntimeError::UnInitialized { location, .. } - | RuntimeError::UnsupportedIntegerSize { location, .. } => { - let span = if let Some(loc) = location { loc.span } else { noirc_errors::Span::new(0..0) }; + _ => { + let span = if let Some(loc) = error.location() { loc.span } else { noirc_errors::Span::new(0..0) }; Diagnostic::simple_error("".to_owned(), error.to_string(), span) } } diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index c57bb330b09..785f9184eb9 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -49,7 +49,7 @@ pub(crate) fn optimize_into_acir( ssa = ssa .inline_functions() .print(print_ssa_passes, "After Inlining:") - .unroll_loops() + .unroll_loops()? .print(print_ssa_passes, "After Unrolling:") .simplify_cfg() .print(print_ssa_passes, "After Simplifying:") diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index f6d7c952277..4cabfa4d46b 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -7,30 +7,35 @@ //! b. If we have previously modified any of the blocks in the loop, //! restart from step 1 to refresh the context. //! c. If not, try to unroll the loop. If successful, remember the modified -//! blocks. If not, remember that the loop failed to unroll and leave it -//! unmodified. +//! blocks. If unsuccessfuly either error if the abort_on_error flag is set, +//! or otherwise remember that the loop failed to unroll and leave it unmodified. //! //! Note that this pass also often creates superfluous jmp instructions in the //! program that will need to be removed by a later simplify cfg pass. use std::collections::{HashMap, HashSet}; -use crate::ssa::{ - ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, dfg::DataFlowGraph, dom::DominatorTree, - function::Function, function_inserter::FunctionInserter, - instruction::TerminatorInstruction, post_order::PostOrder, value::ValueId, +use noirc_errors::Location; + +use crate::{ + errors::RuntimeError, + ssa::{ + ir::{ + basic_block::BasicBlockId, cfg::ControlFlowGraph, dfg::DataFlowGraph, + dom::DominatorTree, function::Function, function_inserter::FunctionInserter, + instruction::TerminatorInstruction, post_order::PostOrder, value::ValueId, + }, + ssa_gen::Ssa, }, - ssa_gen::Ssa, }; impl Ssa { /// Unroll all loops in each SSA function. /// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state. - pub(crate) fn unroll_loops(mut self) -> Ssa { + pub(crate) fn unroll_loops(mut self) -> Result { for function in self.functions.values_mut() { - find_all_loops(function).unroll_each_loop(function); + find_all_loops(function).unroll_each_loop(function, true)?; } - self + Ok(self) } } @@ -96,25 +101,34 @@ fn find_all_loops(function: &Function) -> Loops { impl Loops { /// Unroll all loops within a given function. /// Any loops which fail to be unrolled (due to using non-constant indices) will be unmodified. - fn unroll_each_loop(mut self, function: &mut Function) { + fn unroll_each_loop( + mut self, + function: &mut Function, + abort_on_error: bool, + ) -> Result<(), RuntimeError> { while let Some(next_loop) = self.yet_to_unroll.pop() { // If we've previously modified a block in this loop we need to refresh the context. // This happens any time we have nested loops. if next_loop.blocks.iter().any(|block| self.modified_blocks.contains(block)) { let mut new_context = find_all_loops(function); new_context.failed_to_unroll = self.failed_to_unroll; - return new_context.unroll_each_loop(function); + return new_context.unroll_each_loop(function, abort_on_error); } // Don't try to unroll the loop again if it is known to fail if !self.failed_to_unroll.contains(&next_loop.header) { - if unroll_loop(function, &self.cfg, &next_loop).is_ok() { - self.modified_blocks.extend(next_loop.blocks); - } else { - self.failed_to_unroll.insert(next_loop.header); + match unroll_loop(function, &self.cfg, &next_loop) { + Ok(_) => self.modified_blocks.extend(next_loop.blocks), + Err(location) if abort_on_error => { + return Err(RuntimeError::UnknownLoopBound { location }); + } + Err(_) => { + self.failed_to_unroll.insert(next_loop.header); + } } } } + Ok(()) } } @@ -151,7 +165,11 @@ fn find_blocks_in_loop( /// Unroll a single loop in the function. /// Returns Err(()) if it failed to unroll and Ok(()) otherwise. -fn unroll_loop(function: &mut Function, cfg: &ControlFlowGraph, loop_: &Loop) -> Result<(), ()> { +fn unroll_loop( + function: &mut Function, + cfg: &ControlFlowGraph, + loop_: &Loop, +) -> Result<(), Option> { let mut unroll_into = get_pre_header(cfg, loop_); let mut jump_value = get_induction_variable(function, unroll_into)?; @@ -181,7 +199,10 @@ fn get_pre_header(cfg: &ControlFlowGraph, loop_: &Loop) -> BasicBlockId { /// /// Expects the current block to terminate in `jmp h(N)` where h is the loop header and N is /// a Field value. -fn get_induction_variable(function: &Function, block: BasicBlockId) -> Result { +fn get_induction_variable( + function: &Function, + block: BasicBlockId, +) -> Result> { match function.dfg[block].terminator() { Some(TerminatorInstruction::Jmp { arguments, .. }) => { // This assumption will no longer be valid if e.g. mutable variables are represented as @@ -193,10 +214,10 @@ fn get_induction_variable(function: &Function, block: BasicBlockId) -> Result Err(()), + _ => Err(None), } } @@ -208,7 +229,7 @@ fn unroll_loop_header<'a>( loop_: &'a Loop, unroll_into: BasicBlockId, induction_value: ValueId, -) -> Result>, ()> { +) -> Result>, Option> { // We insert into a fresh block first and move instructions into the unroll_into block later // only once we verify the jmpif instruction has a constant condition. If it does not, we can // just discard this fresh block and leave the loop unmodified. @@ -225,7 +246,8 @@ fn unroll_loop_header<'a>( match context.dfg()[fresh_block].unwrap_terminator() { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { - let next_blocks = context.handle_jmpif(*condition, *then_destination, *else_destination); + let condition = *condition; + let next_blocks = context.handle_jmpif(condition, *then_destination, *else_destination); // If there is only 1 next block the jmpif evaluated to a single known block. // This is the expected case and lets us know if we should loop again or not. @@ -240,7 +262,7 @@ fn unroll_loop_header<'a>( } else { // If this case is reached the loop either uses non-constant indices or we need // another pass, such as mem2reg to resolve them to constants. - Err(()) + Err(context.inserter.function.dfg.get_value_location(&condition)) } } other => unreachable!("Expected loop header to terminate in a JmpIf to the loop body, but found {other:?} instead"), @@ -547,7 +569,7 @@ mod tests { // } // The final block count is not 1 because unrolling creates some unnecessary jmps. // If a simplify cfg pass is ran afterward, the expected block count will be 1. - let ssa = ssa.unroll_loops(); + let ssa = ssa.unroll_loops().unwrap(); assert_eq!(ssa.main().reachable_blocks().len(), 5); } @@ -595,8 +617,7 @@ mod tests { let ssa = builder.finish(); assert_eq!(ssa.main().reachable_blocks().len(), 4); - // Expected ssa is unchanged - let ssa = ssa.unroll_loops(); - assert_eq!(ssa.main().reachable_blocks().len(), 4); + // Expected that we failed to unroll the loop + assert!(ssa.unroll_loops().is_err()); } } diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 9d6091b84bb..acf00410f55 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -34,10 +34,10 @@ use crate::lexer::Lexer; use crate::parser::{force, ignore_then_commit, statement_recovery}; use crate::token::{Attribute, Keyword, Token, TokenKind}; use crate::{ - BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, FunctionDefinition, - Ident, IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, - NoirTrait, NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, - TraitImplItem, TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, + BinaryOp, BinaryOpKind, BlockExpression, ConstrainStatement, FunctionDefinition, Ident, + IfExpression, InfixExpression, LValue, Lambda, Literal, NoirFunction, NoirStruct, NoirTrait, + NoirTypeAlias, Path, PathKind, Pattern, Recoverable, TraitConstraint, TraitImpl, TraitImplItem, + TraitItem, TypeImpl, UnaryOp, UnresolvedTypeExpression, UseTree, UseTreeKind, }; use chumsky::prelude::*; @@ -827,8 +827,10 @@ fn optional_distinctness() -> impl NoirParser { } fn maybe_comp_time() -> impl NoirParser<()> { - keyword(Keyword::CompTime).or_not().validate(|opt, span, emit| if opt.is_some() { - emit(ParserError::with_reason(ParserErrorReason::ComptimeDeprecated, span)); + keyword(Keyword::CompTime).or_not().validate(|opt, span, emit| { + if opt.is_some() { + emit(ParserError::with_reason(ParserErrorReason::ComptimeDeprecated, span)); + } }) } From 5cb3701ae0566211eef0fb0e5addae276ab20671 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 11:14:50 -0500 Subject: [PATCH 05/12] Fix end location for for loops --- crates/noirc_evaluator/src/ssa/ir/dfg.rs | 4 ++-- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 3 +++ crates/noirc_frontend/src/monomorphization/ast.rs | 3 +++ crates/noirc_frontend/src/monomorphization/mod.rs | 2 ++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/dfg.rs b/crates/noirc_evaluator/src/ssa/ir/dfg.rs index 1dd54499632..f8b5146b2ed 100644 --- a/crates/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/dfg.rs @@ -408,11 +408,11 @@ impl DataFlowGraph { } pub(crate) fn get_location(&self, id: &InstructionId) -> Option { - self.locations.get(id).cloned() + self.locations.get(id).copied() } pub(crate) fn get_value_location(&self, id: &ValueId) -> Option { - match &self.values[*id] { + match &self.values[self.resolve(*id)] { Value::Instruction { instruction, .. } => self.get_location(instruction), _ => None, } diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 0c0dd35211b..8a93e35ce83 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -297,7 +297,10 @@ impl<'a> FunctionContext<'a> { let index_type = Self::convert_non_tuple_type(&for_expr.index_type); let loop_index = self.builder.add_block_parameter(loop_entry, index_type); + self.builder.set_location(for_expr.start_range_location); let start_index = self.codegen_non_tuple_expression(&for_expr.start_range); + + self.builder.set_location(for_expr.end_range_location); let end_index = self.codegen_non_tuple_expression(&for_expr.end_range); self.builder.terminate_with_jmp(loop_entry, vec![start_index]); diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index 33c3bbebff4..35fe59e49f1 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -74,6 +74,9 @@ pub struct For { pub start_range: Box, pub end_range: Box, pub block: Box, + + pub start_range_location: Location, + pub end_range_location: Location, } #[derive(Debug, Clone)] diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 28849167d56..95210f4b2d1 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -335,6 +335,8 @@ impl<'interner> Monomorphizer<'interner> { index_type: Self::convert_type(&self.interner.id_type(for_expr.start_range)), start_range: Box::new(start), end_range: Box::new(end), + start_range_location: self.interner.expr_location(&for_expr.start_range), + end_range_location: self.interner.expr_location(&for_expr.end_range), block, }) } From ab67882dba4dc071bb694a8b7969a03472ebbbaa Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 11:38:27 -0500 Subject: [PATCH 06/12] Add Location to Jmp --- .../src/brillig/brillig_gen/brillig_block.rs | 2 +- crates/noirc_evaluator/src/ssa/ir/instruction.rs | 15 ++++++++++----- crates/noirc_evaluator/src/ssa/ir/printer.rs | 2 +- crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- crates/noirc_evaluator/src/ssa/opt/inlining.rs | 8 ++++++-- .../noirc_evaluator/src/ssa/opt/simplify_cfg.rs | 3 ++- crates/noirc_evaluator/src/ssa/opt/unrolling.rs | 9 +++++---- crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs | 14 +++++++++++++- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 10 +++++++++- 9 files changed, 48 insertions(+), 17 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index ded6be71bd5..1183534fb93 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -116,7 +116,7 @@ impl<'block> BrilligBlock<'block> { self.create_block_label_for_current_function(*else_destination), ); } - TerminatorInstruction::Jmp { destination, arguments } => { + TerminatorInstruction::Jmp { destination, arguments, location: _ } => { let target = &dfg[*destination]; for (src, dest) in arguments.iter().zip(target.parameters()) { // Destination variable might have already been created by another block that jumps to this target diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 680715fb0ec..20cafb074fb 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -1,5 +1,6 @@ use acvm::{acir::BlackBoxFunc, FieldElement}; use iter_extended::vecmap; +use noirc_errors::Location; use num_bigint::BigUint; use super::{ @@ -419,8 +420,10 @@ pub(crate) enum TerminatorInstruction { /// Unconditional Jump /// - /// Jumps to specified `destination` with `arguments` - Jmp { destination: BasicBlockId, arguments: Vec }, + /// Jumps to specified `destination` with `arguments`. + /// The optional Location here is expected to be used to issue an error when the start range of + /// a for loop cannot be deduced at compile-time. + Jmp { destination: BasicBlockId, arguments: Vec, location: Option }, /// Return from the current function with the given return values. /// @@ -445,9 +448,11 @@ impl TerminatorInstruction { then_destination: *then_destination, else_destination: *else_destination, }, - Jmp { destination, arguments } => { - Jmp { destination: *destination, arguments: vecmap(arguments, |value| f(*value)) } - } + Jmp { destination, arguments, location } => Jmp { + destination: *destination, + arguments: vecmap(arguments, |value| f(*value)), + location: *location, + }, Return { return_values } => { Return { return_values: vecmap(return_values, |value| f(*value)) } } diff --git a/crates/noirc_evaluator/src/ssa/ir/printer.rs b/crates/noirc_evaluator/src/ssa/ir/printer.rs index f2fb90b3464..e6faf4570c5 100644 --- a/crates/noirc_evaluator/src/ssa/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa/ir/printer.rs @@ -100,7 +100,7 @@ pub(crate) fn display_terminator( f: &mut Formatter, ) -> Result { match terminator { - Some(TerminatorInstruction::Jmp { destination, arguments }) => { + Some(TerminatorInstruction::Jmp { destination, arguments, location: _ }) => { writeln!(f, " jmp {}({})", destination, value_list(function, arguments)) } Some(TerminatorInstruction::JmpIf { condition, then_destination, else_destination }) => { diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 1bcdf433d79..8481f5610bb 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -285,7 +285,7 @@ impl<'f> Context<'f> { let end = self.branch_ends[&block]; self.inline_branch_end(end, then_branch, else_branch) } - TerminatorInstruction::Jmp { destination, arguments } => { + TerminatorInstruction::Jmp { destination, arguments, location: _ } => { if let Some((end_block, _)) = self.conditions.last() { if destination == end_block { return block; diff --git a/crates/noirc_evaluator/src/ssa/opt/inlining.rs b/crates/noirc_evaluator/src/ssa/opt/inlining.rs index d4c118fd3f4..36398bd98fe 100644 --- a/crates/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/crates/noirc_evaluator/src/ssa/opt/inlining.rs @@ -433,10 +433,14 @@ impl<'function> PerFunctionContext<'function> { block_queue: &mut Vec, ) -> Option<(BasicBlockId, Vec)> { match self.source_function.dfg[block_id].unwrap_terminator() { - TerminatorInstruction::Jmp { destination, arguments } => { + TerminatorInstruction::Jmp { destination, arguments, location } => { let destination = self.translate_block(*destination, block_queue); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); - self.context.builder.terminate_with_jmp(destination, arguments); + self.context.builder.terminate_with_jmp_with_location( + destination, + arguments, + *location, + ); None } TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { diff --git a/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 58259cec90c..26c7b161550 100644 --- a/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -86,7 +86,8 @@ fn check_for_constant_jmpif( let destination = if constant.is_zero() { *else_destination } else { *then_destination }; - let jmp = TerminatorInstruction::Jmp { destination, arguments: Vec::new() }; + let arguments = Vec::new(); + let jmp = TerminatorInstruction::Jmp { destination, arguments, location: None }; function.dfg[block].set_terminator(jmp); cfg.recompute_block(function, block); } diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index 4cabfa4d46b..e969aff1829 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -204,7 +204,7 @@ fn get_induction_variable( block: BasicBlockId, ) -> Result> { match function.dfg[block].terminator() { - Some(TerminatorInstruction::Jmp { arguments, .. }) => { + Some(TerminatorInstruction::Jmp { arguments, location, .. }) => { // This assumption will no longer be valid if e.g. mutable variables are represented as // block parameters. If that becomes the case we'll need to figure out which variable // is generally constant and increasing to guess which parameter is the induction @@ -214,7 +214,7 @@ fn get_induction_variable( if function.dfg.get_numeric_constant(value).is_some() { Ok(value) } else { - Err(function.dfg.get_value_location(&value)) + Err(*location) } } _ => Err(None), @@ -354,7 +354,7 @@ impl<'f> LoopIteration<'f> { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { self.handle_jmpif(*condition, *then_destination, *else_destination) } - TerminatorInstruction::Jmp { destination, arguments } => { + TerminatorInstruction::Jmp { destination, arguments, location: _ } => { if self.get_original_block(*destination) == self.loop_.header { assert_eq!(arguments.len(), 1); self.induction_value = Some((self.insert_block, arguments[0])); @@ -383,7 +383,8 @@ impl<'f> LoopIteration<'f> { self.source_block = self.get_original_block(destination); - let jmp = TerminatorInstruction::Jmp { destination, arguments: Vec::new() }; + let arguments = Vec::new(); + let jmp = TerminatorInstruction::Jmp { destination, arguments, location: None }; self.inserter.function.dfg.set_block_terminator(self.insert_block, jmp); vec![destination] } diff --git a/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs index 066b5b51199..61daa06e37b 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_builder/mod.rs @@ -281,7 +281,19 @@ impl FunctionBuilder { destination: BasicBlockId, arguments: Vec, ) { - self.terminate_block_with(TerminatorInstruction::Jmp { destination, arguments }); + self.terminate_with_jmp_with_location(destination, arguments, None); + } + + /// Terminate the current block with a jmp instruction to jmp to the given + /// block with the given arguments. This version also remembers the Location of the jmp + /// for issuing error messages. + pub(crate) fn terminate_with_jmp_with_location( + &mut self, + destination: BasicBlockId, + arguments: Vec, + location: Option, + ) { + self.terminate_block_with(TerminatorInstruction::Jmp { destination, arguments, location }); } /// Terminate the current block with a jmpif instruction to jmp with the given arguments diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 8a93e35ce83..1bc854654d0 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -303,10 +303,18 @@ impl<'a> FunctionContext<'a> { self.builder.set_location(for_expr.end_range_location); let end_index = self.codegen_non_tuple_expression(&for_expr.end_range); - self.builder.terminate_with_jmp(loop_entry, vec![start_index]); + // Set the location of the initial jmp instruction to the start range. This is the location + // used to issue an error if the start range cannot be determined at compile-time. + let jmp_location = Some(for_expr.start_range_location); + self.builder.terminate_with_jmp_with_location(loop_entry, vec![start_index], jmp_location); // Compile the loop entry block self.builder.switch_to_block(loop_entry); + + // Set the location of the ending Lt instruction and the jmpif back-edge of the loop to the + // end range. These are the instructions used to issue an error if the end of the range + // cannot be determined at compile-time. + self.builder.set_location(for_expr.end_range_location); let jump_condition = self.builder.insert_binary(loop_index, BinaryOp::Lt, end_index); self.builder.terminate_with_jmpif(jump_condition, loop_body, loop_end); From 6c9481ae515d0c5120f0c0d36545fae6728498d6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 11:45:02 -0500 Subject: [PATCH 07/12] Shorten error message --- crates/noirc_evaluator/src/errors.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index 00dd9876c45..3506e39c1fc 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -30,9 +30,7 @@ pub enum RuntimeError { UnInitialized { name: String, location: Option }, #[error("Integer sized {num_bits:?} is over the max supported size of {max_num_bits:?}")] UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option }, - #[error( - "Could not determine loop bound at compile-time. Loop bounds must be compile-time known" - )] + #[error("Could not determine loop bound at compile-time")] UnknownLoopBound { location: Option }, } From 1e373448bb4ea8ffd5abc984729fdc17f9bf22e8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 12:11:26 -0500 Subject: [PATCH 08/12] Fix test --- crates/noirc_evaluator/src/ssa/ir/cfg.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/noirc_evaluator/src/ssa/ir/cfg.rs b/crates/noirc_evaluator/src/ssa/ir/cfg.rs index a91123438fa..8faa0fa8565 100644 --- a/crates/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/crates/noirc_evaluator/src/ssa/ir/cfg.rs @@ -220,6 +220,7 @@ mod tests { func.dfg[block2_id].set_terminator(TerminatorInstruction::Jmp { destination: ret_block_id, arguments: vec![], + location: None, }); func.dfg[block0_id].set_terminator(TerminatorInstruction::JmpIf { condition: cond, From 32c1b45ec58d5753db3cbd93f5dbe21439637668 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 12:29:07 -0500 Subject: [PATCH 09/12] Only abort_on_error for acir functions --- crates/noirc_evaluator/src/ssa/opt/unrolling.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index e969aff1829..b0c405c0d76 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -20,9 +20,15 @@ use crate::{ errors::RuntimeError, ssa::{ ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, dfg::DataFlowGraph, - dom::DominatorTree, function::Function, function_inserter::FunctionInserter, - instruction::TerminatorInstruction, post_order::PostOrder, value::ValueId, + basic_block::BasicBlockId, + cfg::ControlFlowGraph, + dfg::DataFlowGraph, + dom::DominatorTree, + function::{Function, RuntimeType}, + function_inserter::FunctionInserter, + instruction::TerminatorInstruction, + post_order::PostOrder, + value::ValueId, }, ssa_gen::Ssa, }, @@ -33,7 +39,8 @@ impl Ssa { /// If any loop cannot be unrolled, it is left as-is or in a partially unrolled state. pub(crate) fn unroll_loops(mut self) -> Result { for function in self.functions.values_mut() { - find_all_loops(function).unroll_each_loop(function, true)?; + let abort_on_error = function.runtime() == RuntimeType::Acir; + find_all_loops(function).unroll_each_loop(function, abort_on_error)?; } Ok(self) } From d9523e83159263c2427c8b58e08f6474d7f6f1f2 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 4 Aug 2023 13:07:59 -0500 Subject: [PATCH 10/12] Remove comptime from frontend test --- crates/noirc_frontend/src/parser/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index acf00410f55..e4a4e5912f3 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -1740,7 +1740,7 @@ mod test { vec![ "fn func_name() {}", "fn f(foo: pub u8, y : pub Field) -> u8 { x + a }", - "fn f(f: pub Field, y : Field, z : comptime Field) -> u8 { x + a }", + "fn f(f: pub Field, y : Field, z : Field) -> u8 { x + a }", "fn func_name(f: Field, y : pub Field, z : pub [u8;5],) {}", "fn func_name(x: [Field], y : [Field;2],y : pub [Field;2], z : pub [u8;5]) {}", "fn main(x: pub u8, y: pub u8) -> distinct pub [u8; 2] { [x, y] }", From b2012ce66a14e27f98409f8c15e47295c0b3ac08 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 8 Aug 2023 12:46:28 -0500 Subject: [PATCH 11/12] Remove extra parameter --- crates/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +- crates/noirc_frontend/src/hir/type_check/expr.rs | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index aca809a85fa..7409a199641 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1121,7 +1121,7 @@ impl Context { #[cfg(test)] mod tests { - use std::{rc::Rc, collections::HashMap}; + use std::{collections::HashMap, rc::Rc}; use acvm::{ acir::{ diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 16146d25135..f0c1adf761b 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -1019,8 +1019,9 @@ impl<'interner> TypeChecker<'interner> { match op { crate::UnaryOp::Minus => { let expected = Type::polymorphic_integer(self.interner); - rhs_type.unify(&expected, span, &mut self.errors, || { - TypeCheckError::InvalidUnaryOp { kind: rhs_type.to_string(), span } + rhs_type.unify(&expected, &mut self.errors, || TypeCheckError::InvalidUnaryOp { + kind: rhs_type.to_string(), + span, }); expected } From 1a6e6916db94687d6012fd3c4465f9728cf1dabb Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 8 Aug 2023 15:08:55 -0500 Subject: [PATCH 12/12] Remove comptime more thoroughly --- crates/noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- crates/noirc_frontend/src/hir/resolution/errors.rs | 7 ------- crates/noirc_frontend/src/hir/type_check/errors.rs | 11 ----------- crates/noirc_frontend/src/hir/type_check/expr.rs | 1 - crates/noirc_frontend/src/hir/type_check/mod.rs | 7 ++----- crates/noirc_frontend/src/hir_def/types.rs | 3 +-- crates/noirc_frontend/src/monomorphization/ast.rs | 1 - crates/noirc_frontend/src/monomorphization/mod.rs | 9 ++------- crates/noirc_frontend/src/parser/parser.rs | 10 ++-------- 9 files changed, 8 insertions(+), 43 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index b0c405c0d76..ac38f102712 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -577,7 +577,7 @@ mod tests { // } // The final block count is not 1 because unrolling creates some unnecessary jmps. // If a simplify cfg pass is ran afterward, the expected block count will be 1. - let ssa = ssa.unroll_loops().unwrap(); + let ssa = ssa.unroll_loops().expect("All loops should be unrolled"); assert_eq!(ssa.main().reachable_blocks().len(), 5); } diff --git a/crates/noirc_frontend/src/hir/resolution/errors.rs b/crates/noirc_frontend/src/hir/resolution/errors.rs index e9cf8f31393..1215d897eda 100644 --- a/crates/noirc_frontend/src/hir/resolution/errors.rs +++ b/crates/noirc_frontend/src/hir/resolution/errors.rs @@ -42,8 +42,6 @@ pub enum ResolverError { NecessaryPub { ident: Ident }, #[error("'distinct' keyword can only be used with main method")] DistinctNotAllowed { ident: Ident }, - #[error("Expected const value where non-constant value was used")] - ExpectedComptimeVariable { name: String, span: Span }, #[error("Missing expression for declared constant")] MissingRhsExpr { name: String, span: Span }, #[error("Expression invalid in an array length context")] @@ -206,11 +204,6 @@ impl From for Diagnostic { diag.add_note("The `distinct` keyword is only valid when used on the main function of a program, as its only purpose is to ensure that all witness indices that occur in the abi are unique".to_owned()); diag } - ResolverError::ExpectedComptimeVariable { name, span } => Diagnostic::simple_error( - format!("expected constant variable where non-constant variable {name} was used"), - "expected const variable".to_string(), - span, - ), ResolverError::MissingRhsExpr { name, span } => Diagnostic::simple_error( format!( "no expression specifying the value stored by the constant variable {name}" diff --git a/crates/noirc_frontend/src/hir/type_check/errors.rs b/crates/noirc_frontend/src/hir/type_check/errors.rs index 8b3623db3c3..84bf511d314 100644 --- a/crates/noirc_frontend/src/hir/type_check/errors.rs +++ b/crates/noirc_frontend/src/hir/type_check/errors.rs @@ -47,14 +47,6 @@ pub enum TypeCheckError { AccessUnknownMember { lhs_type: Type, field_name: String, span: Span }, #[error("Function expects {expected} parameters but {found} given")] ParameterCountMismatch { expected: usize, found: usize, span: Span }, - #[error("The value is non-comptime because of this expression, which uses another non-comptime value")] - NotCompTime { span: Span }, - #[error( - "The value is comptime because of this expression, which forces the value to be comptime" - )] - CompTime { span: Span }, - #[error("Cannot cast to a comptime type, argument to cast is not known at compile-time")] - CannotCastToComptimeType { span: Span }, #[error("Only integer and Field types may be casted to")] UnsupportedCast { span: Span }, #[error("Index {index} is out of bounds for this tuple {lhs_type} of length {length}")] @@ -165,9 +157,6 @@ impl From for Diagnostic { TypeCheckError::InvalidCast { span, .. } | TypeCheckError::ExpectedFunction { span, .. } | TypeCheckError::AccessUnknownMember { span, .. } - | TypeCheckError::CompTime { span } - | TypeCheckError::NotCompTime { span } - | TypeCheckError::CannotCastToComptimeType { span } | TypeCheckError::UnsupportedCast { span } | TypeCheckError::TupleIndexOutOfBounds { span, .. } | TypeCheckError::VariableMustBeMutable { span, .. } diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index f0c1adf761b..99b312adb0d 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -882,7 +882,6 @@ impl<'interner> TypeChecker<'interner> { } // Given a binary operator and another type. This method will produce the output type - // XXX: Review these rules. In particular, the interaction between integers, comptime and private/public variables fn infix_operand_type_rules( &mut self, lhs_type: &Type, diff --git a/crates/noirc_frontend/src/hir/type_check/mod.rs b/crates/noirc_frontend/src/hir/type_check/mod.rs index 76e59ef5eab..8768120af2f 100644 --- a/crates/noirc_frontend/src/hir/type_check/mod.rs +++ b/crates/noirc_frontend/src/hir/type_check/mod.rs @@ -4,11 +4,8 @@ //! the HIR of each function and outputs the inferred type of each HIR node into the NodeInterner, //! keyed by the ID of the node. //! -//! The algorithm for checking and inferring types itself is somewhat ad-hoc. It includes both -//! unification and subtyping, with the only difference between the two being how CompTime -//! is handled (See note on CompTime and make_subtype_of for details). Additionally, although -//! this algorithm features inference via TypeVariables, there is no generalization step as -//! all functions are required to give their full signatures. Closures are inferred but are +//! Although this algorithm features inference via TypeVariables, there is no generalization step +//! as all functions are required to give their full signatures. Closures are inferred but are //! never generalized and thus cannot be used polymorphically. mod errors; mod expr; diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 545310d3f44..d938421cb0e 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -675,8 +675,7 @@ impl Type { } /// Try to bind a PolymorphicInt variable to self, succeeding if self is an integer, field, - /// other PolymorphicInt type, or type variable. If use_subtype is true, the CompTime fields - /// of each will be checked via sub-typing rather than unification. + /// other PolymorphicInt type, or type variable. pub fn try_bind_to_polymorphic_int(&self, var: &TypeVariable) -> Result<(), UnificationError> { let target_id = match &*var.borrow() { TypeBinding::Bound(_) => unreachable!(), diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index 7c276657769..b26db6e1afb 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -208,7 +208,6 @@ pub struct Function { /// - All type variables and generics removed /// - Concrete lengths for each array and string /// - Several other variants removed (such as Type::Constant) -/// - No CompTime /// - All structs replaced with tuples #[derive(Debug, PartialEq, Eq, Clone)] pub enum Type { diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 2c92c1fc849..b93c95efe07 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -959,14 +959,9 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Assign(ast::Assign { expression, lvalue }) } - fn lvalue( - &mut self, - lvalue: HirLValue, - ) -> ast::LValue { + fn lvalue(&mut self, lvalue: HirLValue) -> ast::LValue { match lvalue { - HirLValue::Ident(ident, _) => { - ast::LValue::Ident(self.local_ident(&ident).unwrap()) - } + HirLValue::Ident(ident, _) => ast::LValue::Ident(self.local_ident(&ident).unwrap()), HirLValue::MemberAccess { object, field_index, .. } => { let field_index = field_index.unwrap(); let object = Box::new(self.lvalue(*object)); diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index f83e4cad3c0..232aff618b0 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -125,7 +125,7 @@ fn global_declaration() -> impl NoirParser { keyword(Keyword::Global).labelled(ParsingRuleLabel::Global), ident().map(Pattern::Identifier), ); - let p = then_commit(p, global_type_annotation()); + let p = then_commit(p, optional_type_annotation()); let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, literal_or_collection(expression()).map_with_span(Expression::new)); p.map(LetStatement::new_let).map(TopLevelStatement::Global) @@ -548,13 +548,7 @@ fn check_statements_require_semicolon( }) } -/// Parse an optional ': type' and implicitly add a 'comptime' to the type -fn global_type_annotation() -> impl NoirParser { - ignore_then_commit(just(Token::Colon), parse_type()) - .or_not() - .map(|opt| opt.unwrap_or(UnresolvedType::Unspecified)) -} - +/// Parse an optional ': type' fn optional_type_annotation<'a>() -> impl NoirParser + 'a { ignore_then_commit(just(Token::Colon), parse_type()) .or_not()