fix: Fix methods not mutating fields (#2087)

* Fix methods not mutating fields

* Update doc comment

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;
+    }
+}

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)
+            }
         }
     }
 

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, "*"),
         }
     }
 }

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),
                 }))
             }

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 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) => {
+                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

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.