From 779923a82ed3c6d108198446a6e1e0f70ff1c94e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 28 Jul 2023 15:31:13 -0500 Subject: [PATCH 1/2] Fix methods not mutating fields --- .../tests/test_data/references/src/main.nr | 19 ++++++ .../src/ssa_refactor/ssa_gen/mod.rs | 4 +- crates/noirc_frontend/src/ast/expression.rs | 11 +++- crates/noirc_frontend/src/ast/statement.rs | 2 +- .../noirc_frontend/src/hir/type_check/expr.rs | 61 ++++++++++++++++--- crates/noirc_frontend/src/parser/parser.rs | 2 +- 6 files changed, 85 insertions(+), 14 deletions(-) diff --git a/crates/nargo_cli/tests/test_data/references/src/main.nr b/crates/nargo_cli/tests/test_data/references/src/main.nr index d2c0b7f1244..b112875b9ff 100644 --- a/crates/nargo_cli/tests/test_data/references/src/main.nr +++ b/crates/nargo_cli/tests/test_data/references/src/main.nr @@ -30,6 +30,8 @@ fn main(mut x: Field) { }; *c.bar.array = [3, 4]; assert(*c.bar.array == [3, 4]); + + regression_1887(); } fn add1(x: &mut Field) { @@ -58,3 +60,20 @@ impl S { fn mutate_copy(mut a: Field) { a = 7; } + +// Previously the `foo.bar` in `foo.bar.mutate()` would insert an automatic dereference +// of `foo` which caused the method to wrongly be mutating a copy of bar rather than the original. +fn regression_1887() { + let foo = &mut Foo { bar: Bar { x: 0 } }; + foo.bar.mutate(); + assert(foo.bar.x == 32); +} + +struct Foo { bar: Bar } +struct Bar { x: Field } + +impl Bar { + fn mutate(&mut self) { + self.x = 32; + } +} diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 2b6db4e7586..710450eb1e6 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -185,7 +185,9 @@ impl<'a> FunctionContext<'a> { } }) } - noirc_frontend::UnaryOp::Dereference => self.dereference(&rhs, &unary.result_type), + noirc_frontend::UnaryOp::Dereference { .. } => { + self.dereference(&rhs, &unary.result_type) + } } } diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index e36f5b5d260..1f1d226310f 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -271,7 +271,14 @@ pub enum UnaryOp { Minus, Not, MutableReference, - Dereference, + + /// If implicitly_added is true, this operation was implicitly added by the compiler for a + /// field dereference. The compiler may undo some of these implicitly added dereferences if + /// the reference later turns out to be needed (e.g. passing a field by reference to a function + /// requiring an &mut parameter). + Dereference { + implicitly_added: bool, + }, } impl UnaryOp { @@ -496,7 +503,7 @@ impl Display for UnaryOp { UnaryOp::Minus => write!(f, "-"), UnaryOp::Not => write!(f, "!"), UnaryOp::MutableReference => write!(f, "&mut"), - UnaryOp::Dereference => write!(f, "*"), + UnaryOp::Dereference { .. } => write!(f, "*"), } } } diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 7292d227c3e..e35394e0729 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -456,7 +456,7 @@ impl LValue { })), LValue::Dereference(lvalue) => { ExpressionKind::Prefix(Box::new(crate::PrefixExpression { - operator: crate::UnaryOp::Dereference, + operator: crate::UnaryOp::Dereference { implicitly_added: false }, rhs: lvalue.as_expression(span), })) } diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 2c6578944be..12dc9941f84 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -280,6 +280,12 @@ impl<'interner> TypeChecker<'interner> { /// if the given object type is already a mutable reference. If not, add one. /// This is used to automatically transform a method call: `foo.bar()` into a function /// call: `bar(&mut foo)`. + /// + /// A notable corner case of this function is where it interacts with auto-deref of `.`. + /// If a field is being mutated e.g. `foo.bar.mutate_bar()` where `foo: &mut Foo`, the compiler + /// will insert a dereference before bar `(*foo).bar.mutate_bar()` which would cause us to + /// mutate a copy of bar rather than a reference to it. We must check for this corner case here + /// and remove the implicitly added dereference operator if we find one. fn try_add_mutable_reference_to_object( &mut self, method_call: &mut HirMethodCallExpression, @@ -306,19 +312,56 @@ impl<'interner> TypeChecker<'interner> { } let new_type = Type::MutableReference(Box::new(actual_type)); - argument_types[0].0 = new_type.clone(); - 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); + + // First try to remove a dereference operator that may have been implicitly + // 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` + 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); + } } } } } + /// Given a method object: `(*foo).bar` of a method call `(*foo).bar.baz()`, remove the + /// implicitly added dereference operator if one is found. + /// + /// Returns true if a dereference was removed. + fn try_remove_implicit_dereference(&mut self, object: ExprId) -> Option<()> { + match self.interner.expression(&object) { + HirExpression::MemberAccess(access) => { + self.try_remove_implicit_dereference(access.lhs)?; + + // Since we removed a dereference, instead of returning the field directly, + // we expect to be returning a reference to the field, so update the type accordingly. + let current_type = self.interner.id_type(object); + let reference_type = Type::MutableReference(Box::new(current_type)); + self.interner.push_expr_type(&object, reference_type); + Some(()) + } + HirExpression::Prefix(prefix) => match prefix.operator { + UnaryOp::Dereference { implicitly_added: true } => { + // Found a dereference we can remove. Now just replace it with its rhs to remove it. + let rhs = self.interner.expression(&prefix.rhs); + self.interner.replace_expr(&object, rhs); + + let rhs_type = self.interner.id_type(prefix.rhs); + self.interner.push_expr_type(&object, rhs_type); + Some(()) + } + _ => None, + }, + _ => None, + } + } + fn check_index_expression(&mut self, index_expr: expr::HirIndexExpression) -> Type { let index_type = self.check_expression(&index_expr.index); let span = self.interner.expr_span(&index_expr.index); @@ -525,7 +568,7 @@ impl<'interner> TypeChecker<'interner> { let dereference_lhs = |this: &mut Self, lhs_type, element| { let old_lhs = *access_lhs; *access_lhs = this.interner.push_expr(HirExpression::Prefix(HirPrefixExpression { - operator: crate::UnaryOp::Dereference, + operator: crate::UnaryOp::Dereference { implicitly_added: true }, rhs: old_lhs, })); this.interner.push_expr_type(&old_lhs, lhs_type); @@ -1006,7 +1049,7 @@ impl<'interner> TypeChecker<'interner> { crate::UnaryOp::MutableReference => { Type::MutableReference(Box::new(rhs_type.follow_bindings())) } - crate::UnaryOp::Dereference => { + crate::UnaryOp::Dereference { implicitly_added: _ } => { let element_type = self.interner.next_type_variable(); unify(Type::MutableReference(Box::new(element_type.clone()))); element_type diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index c8142ffa947..c6d84416975 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -1267,7 +1267,7 @@ where { just(Token::Star) .ignore_then(term_parser) - .map(|rhs| ExpressionKind::prefix(UnaryOp::Dereference, rhs)) + .map(|rhs| ExpressionKind::prefix(UnaryOp::Dereference { implicitly_added: false }, rhs)) } /// Atoms are parameterized on whether constructor expressions are allowed or not. From 07cc85ecab8ee040bf1a5a65a52d2469a6d090b1 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 28 Jul 2023 15:36:16 -0500 Subject: [PATCH 2/2] Update doc comment --- crates/noirc_frontend/src/hir/type_check/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 12dc9941f84..8c396ea6814 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -333,7 +333,7 @@ impl<'interner> TypeChecker<'interner> { /// Given a method object: `(*foo).bar` of a method call `(*foo).bar.baz()`, remove the /// implicitly added dereference operator if one is found. /// - /// Returns true if a dereference was removed. + /// Returns Some(()) if a dereference was removed and None otherwise. fn try_remove_implicit_dereference(&mut self, object: ExprId) -> Option<()> { match self.interner.expression(&object) { HirExpression::MemberAccess(access) => {