From a40ceb16a94293f2198a4127508b9c62586de7e8 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 12:19:22 +0300 Subject: [PATCH 1/4] fix(ssa): codegen missing check for unary minus --- crates/noirc_evaluator/src/ssa/ssa_gen/context.rs | 10 +++++++--- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index 0b3ee5b9acf..6d2ccce3421 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -291,7 +291,7 @@ impl<'a> FunctionContext<'a> { mut lhs: ValueId, operator: noirc_frontend::BinaryOpKind, mut rhs: ValueId, - location: Location, + location: Option, ) -> Values { let mut result = match operator { BinaryOpKind::ShiftLeft => self.insert_shift_left(lhs, rhs), @@ -299,14 +299,18 @@ impl<'a> FunctionContext<'a> { BinaryOpKind::Equal | BinaryOpKind::NotEqual if matches!(self.builder.type_of_value(lhs), Type::Array(..)) => { - return self.insert_array_equality(lhs, operator, rhs, location) + return self.insert_array_equality(lhs, operator, rhs, location.unwrap()) } _ => { let op = convert_operator(operator); if operator_requires_swapped_operands(operator) { std::mem::swap(&mut lhs, &mut rhs); } - self.builder.set_location(location).insert_binary(lhs, op, rhs) + + if let Some(location) = location { + self.builder.set_location(location); + } + self.builder.insert_binary(lhs, op, rhs) } }; diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 1367b1753af..4862216381f 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -207,7 +207,7 @@ impl<'a> FunctionContext<'a> { let rhs = rhs.into_leaf().eval(self); let typ = self.builder.type_of_value(rhs); let zero = self.builder.numeric_constant(0u128, typ); - self.builder.insert_binary(zero, BinaryOp::Sub, rhs).into() + self.insert_binary(zero, noirc_frontend::BinaryOpKind::Subtract, rhs, None).into() } noirc_frontend::UnaryOp::MutableReference => { self.codegen_reference(&unary.rhs).map(|rhs| { @@ -252,7 +252,7 @@ impl<'a> FunctionContext<'a> { fn codegen_binary(&mut self, binary: &ast::Binary) -> Values { let lhs = self.codegen_non_tuple_expression(&binary.lhs); let rhs = self.codegen_non_tuple_expression(&binary.rhs); - self.insert_binary(lhs, binary.operator, rhs, binary.location) + self.insert_binary(lhs, binary.operator, rhs, Some(binary.location)) } fn codegen_index(&mut self, index: &ast::Index) -> Values { From 287c05b85ce5cbf9729bc6f9859848da9973292f Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 12:26:07 +0300 Subject: [PATCH 2/4] test: add test for unary minus --- .../tests/execution_success/unary_operators/Nargo.toml | 7 +++++++ .../tests/execution_success/unary_operators/src/main.nr | 7 +++++++ 2 files changed, 14 insertions(+) create mode 100644 crates/nargo_cli/tests/execution_success/unary_operators/Nargo.toml create mode 100644 crates/nargo_cli/tests/execution_success/unary_operators/src/main.nr diff --git a/crates/nargo_cli/tests/execution_success/unary_operators/Nargo.toml b/crates/nargo_cli/tests/execution_success/unary_operators/Nargo.toml new file mode 100644 index 00000000000..65ad7c1c70c --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/unary_operators/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "unary_operators" +type = "bin" +authors = [""] +compiler_version = "0.10.3" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/execution_success/unary_operators/src/main.nr b/crates/nargo_cli/tests/execution_success/unary_operators/src/main.nr new file mode 100644 index 00000000000..1c9145fd81f --- /dev/null +++ b/crates/nargo_cli/tests/execution_success/unary_operators/src/main.nr @@ -0,0 +1,7 @@ +fn main() { + let x = -1; + assert(x == 1 - 2); + + let y: i32 = -1; + assert(x == 1 - 2); +} \ No newline at end of file From 274b06bc5bb7691cf63b69faaf2b7ebd1cfd7c08 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Wed, 23 Aug 2023 14:19:31 +0300 Subject: [PATCH 3/4] chore: cargo clippy suggestions --- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 4862216381f..ac3114a8e73 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -207,7 +207,7 @@ impl<'a> FunctionContext<'a> { let rhs = rhs.into_leaf().eval(self); let typ = self.builder.type_of_value(rhs); let zero = self.builder.numeric_constant(0u128, typ); - self.insert_binary(zero, noirc_frontend::BinaryOpKind::Subtract, rhs, None).into() + self.insert_binary(zero, noirc_frontend::BinaryOpKind::Subtract, rhs, None) } noirc_frontend::UnaryOp::MutableReference => { self.codegen_reference(&unary.rhs).map(|rhs| { From 1b47c859f32ab2fd0c515b2e63fc888ce08743f2 Mon Sep 17 00:00:00 2001 From: Alex Vitkov Date: Fri, 25 Aug 2023 08:57:39 +0300 Subject: [PATCH 4/4] fix: add location to monomorphizer's ast::Unary --- .../src/ssa/ssa_gen/context.rs | 9 ++--- crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs | 4 +- .../noirc_frontend/src/hir/type_check/expr.rs | 6 +++ .../src/monomorphization/ast.rs | 1 + .../src/monomorphization/mod.rs | 39 ++++++++++++------- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index 6d2ccce3421..5e29ed60d42 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -291,7 +291,7 @@ impl<'a> FunctionContext<'a> { mut lhs: ValueId, operator: noirc_frontend::BinaryOpKind, mut rhs: ValueId, - location: Option, + location: Location, ) -> Values { let mut result = match operator { BinaryOpKind::ShiftLeft => self.insert_shift_left(lhs, rhs), @@ -299,7 +299,7 @@ impl<'a> FunctionContext<'a> { BinaryOpKind::Equal | BinaryOpKind::NotEqual if matches!(self.builder.type_of_value(lhs), Type::Array(..)) => { - return self.insert_array_equality(lhs, operator, rhs, location.unwrap()) + return self.insert_array_equality(lhs, operator, rhs, location) } _ => { let op = convert_operator(operator); @@ -307,10 +307,7 @@ impl<'a> FunctionContext<'a> { std::mem::swap(&mut lhs, &mut rhs); } - if let Some(location) = location { - self.builder.set_location(location); - } - self.builder.insert_binary(lhs, op, rhs) + self.builder.set_location(location).insert_binary(lhs, op, rhs) } }; diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index ac3114a8e73..59bdf7ff0e6 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -207,7 +207,7 @@ impl<'a> FunctionContext<'a> { let rhs = rhs.into_leaf().eval(self); let typ = self.builder.type_of_value(rhs); let zero = self.builder.numeric_constant(0u128, typ); - self.insert_binary(zero, noirc_frontend::BinaryOpKind::Subtract, rhs, None) + self.insert_binary(zero, noirc_frontend::BinaryOpKind::Subtract, rhs, unary.location) } noirc_frontend::UnaryOp::MutableReference => { self.codegen_reference(&unary.rhs).map(|rhs| { @@ -252,7 +252,7 @@ impl<'a> FunctionContext<'a> { fn codegen_binary(&mut self, binary: &ast::Binary) -> Values { let lhs = self.codegen_non_tuple_expression(&binary.lhs); let rhs = self.codegen_non_tuple_expression(&binary.rhs); - self.insert_binary(lhs, binary.operator, rhs, Some(binary.location)) + self.insert_binary(lhs, binary.operator, rhs, binary.location) } fn codegen_index(&mut self, index: &ast::Index) -> Values { diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 9f00f4b61da..7a0c0eb2c7d 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -335,12 +335,15 @@ impl<'interner> TypeChecker<'interner> { // inserted by a field access expression `foo.bar` on a mutable reference `foo`. if self.try_remove_implicit_dereference(method_call.object).is_none() { // If that didn't work, then wrap the whole expression in an `&mut` + let location = self.interner.id_location(method_call.object); + method_call.object = self.interner.push_expr(HirExpression::Prefix(HirPrefixExpression { operator: UnaryOp::MutableReference, rhs: method_call.object, })); self.interner.push_expr_type(&method_call.object, new_type); + self.interner.push_expr_location(method_call.object, location.span, location.file); } } } @@ -567,6 +570,9 @@ impl<'interner> TypeChecker<'interner> { })); this.interner.push_expr_type(&old_lhs, lhs_type); this.interner.push_expr_type(access_lhs, element); + + let old_location = this.interner.id_location(old_lhs); + this.interner.push_expr_location(*access_lhs, span, old_location.file); }; match self.check_field_access(&lhs_type, &access.rhs.0.contents, span, dereference_lhs) { diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index d648e181865..2324624b1df 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -92,6 +92,7 @@ pub struct Unary { pub operator: crate::UnaryOp, pub rhs: Box, pub result_type: Type, + pub location: Location, } pub type BinaryOp = BinaryOpKind; diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 0d3297bf8a4..b2661789c01 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -291,11 +291,15 @@ impl<'interner> Monomorphizer<'interner> { HirExpression::Literal(HirLiteral::Unit) => ast::Expression::Block(vec![]), HirExpression::Block(block) => self.block(block.0), - HirExpression::Prefix(prefix) => ast::Expression::Unary(ast::Unary { - operator: prefix.operator, - rhs: Box::new(self.expr(prefix.rhs)), - result_type: Self::convert_type(&self.interner.id_type(expr)), - }), + HirExpression::Prefix(prefix) => { + let location = self.interner.expr_location(&expr); + ast::Expression::Unary(ast::Unary { + operator: prefix.operator, + rhs: Box::new(self.expr(prefix.rhs)), + result_type: Self::convert_type(&self.interner.id_type(expr)), + location, + }) + } HirExpression::Infix(infix) => { let lhs = Box::new(self.expr(infix.lhs)); @@ -816,7 +820,7 @@ impl<'interner> Monomorphizer<'interner> { }; let call = self - .try_evaluate_call(&func, &return_type) + .try_evaluate_call(&func, &id, &return_type) .unwrap_or(ast::Expression::Call(ast::Call { func, arguments, return_type, location })); if !block_expressions.is_empty() { @@ -897,6 +901,7 @@ impl<'interner> Monomorphizer<'interner> { fn try_evaluate_call( &mut self, func: &ast::Expression, + expr_id: &node_interner::ExprId, result_type: &ast::Type, ) -> Option { if let ast::Expression::Ident(ident) = func { @@ -907,7 +912,10 @@ impl<'interner> Monomorphizer<'interner> { (FieldElement::max_num_bits() as u128).into(), ast::Type::Field, ))), - "zeroed" => Some(self.zeroed_value_of_type(result_type)), + "zeroed" => { + let location = self.interner.expr_location(expr_id); + Some(self.zeroed_value_of_type(result_type, location)) + } "modulus_le_bits" => { let bits = FieldElement::modulus().to_radix_le(2); Some(self.modulus_array_literal(bits, 1)) @@ -1179,7 +1187,7 @@ impl<'interner> Monomorphizer<'interner> { /// Implements std::unsafe::zeroed by returning an appropriate zeroed /// ast literal or collection node for the given type. Note that for functions /// there is no obvious zeroed value so this should be considered unsafe to use. - fn zeroed_value_of_type(&mut self, typ: &ast::Type) -> ast::Expression { + fn zeroed_value_of_type(&mut self, typ: &ast::Type, location: noirc_errors::Location) -> ast::Expression { match typ { ast::Type::Field | ast::Type::Integer(..) => { ast::Expression::Literal(ast::Literal::Integer(0_u128.into(), typ.clone())) @@ -1189,7 +1197,7 @@ impl<'interner> Monomorphizer<'interner> { // anyway. ast::Type::Unit => ast::Expression::Literal(ast::Literal::Bool(false)), ast::Type::Array(length, element_type) => { - let element = self.zeroed_value_of_type(element_type.as_ref()); + let element = self.zeroed_value_of_type(element_type.as_ref(), location); ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { contents: vec![element; *length as usize], typ: ast::Type::Array(*length, element_type.clone()), @@ -1199,7 +1207,7 @@ impl<'interner> Monomorphizer<'interner> { ast::Expression::Literal(ast::Literal::Str("\0".repeat(*length as usize))) } ast::Type::FmtString(length, fields) => { - let zeroed_tuple = self.zeroed_value_of_type(fields); + let zeroed_tuple = self.zeroed_value_of_type(fields, location); let fields_len = match &zeroed_tuple { ast::Expression::Tuple(fields) => fields.len() as u64, _ => unreachable!("ICE: format string fields should be structured in a tuple, but got a {zeroed_tuple}"), @@ -1211,10 +1219,10 @@ impl<'interner> Monomorphizer<'interner> { )) } ast::Type::Tuple(fields) => { - ast::Expression::Tuple(vecmap(fields, |field| self.zeroed_value_of_type(field))) + ast::Expression::Tuple(vecmap(fields, |field| self.zeroed_value_of_type(field, location))) } ast::Type::Function(parameter_types, ret_type, env) => { - self.create_zeroed_function(parameter_types, ret_type, env) + self.create_zeroed_function(parameter_types, ret_type, env, location) } ast::Type::Slice(element_type) => { ast::Expression::Literal(ast::Literal::Array(ast::ArrayLiteral { @@ -1224,9 +1232,9 @@ impl<'interner> Monomorphizer<'interner> { } ast::Type::MutableReference(element) => { use crate::UnaryOp::MutableReference; - let rhs = Box::new(self.zeroed_value_of_type(element)); + let rhs = Box::new(self.zeroed_value_of_type(element, location)); let result_type = typ.clone(); - ast::Expression::Unary(ast::Unary { rhs, result_type, operator: MutableReference }) + ast::Expression::Unary(ast::Unary { rhs, result_type, operator: MutableReference, location }) } } } @@ -1242,6 +1250,7 @@ impl<'interner> Monomorphizer<'interner> { parameter_types: &[ast::Type], ret_type: &ast::Type, env_type: &ast::Type, + location: noirc_errors::Location, ) -> ast::Expression { let lambda_name = "zeroed_lambda"; @@ -1249,7 +1258,7 @@ impl<'interner> Monomorphizer<'interner> { (self.next_local_id(), false, "_".into(), parameter_type.clone()) }); - let body = self.zeroed_value_of_type(ret_type); + let body = self.zeroed_value_of_type(ret_type, location); let id = self.next_function_id(); let return_type = ret_type.clone();