fix: Prevent mutating immutable bindings to mutable types (#3075)

compiler/noirc_frontend/src/hir/type_check/stmt.rs

@@ -140,7 +140,12 @@ impl<'interner> TypeChecker<'interner> {
     fn check_assign_stmt(&mut self, assign_stmt: HirAssignStatement, stmt_id: &StmtId) {
         let expr_type = self.check_expression(&assign_stmt.expression);
         let span = self.interner.expr_span(&assign_stmt.expression);
-        let (lvalue_type, new_lvalue) = self.check_lvalue(assign_stmt.lvalue, span);
+        let (lvalue_type, new_lvalue, mutable) = self.check_lvalue(&assign_stmt.lvalue, span);
+
+        if !mutable {
+            let (name, span) = self.get_lvalue_name_and_span(&assign_stmt.lvalue);
+            self.errors.push(TypeCheckError::VariableMustBeMutable { name, span });
+        }
 
         // Must push new lvalue to the interner, we've resolved any field indices
         self.interner.update_statement(stmt_id, |stmt| match stmt {
@@ -159,39 +164,52 @@ impl<'interner> TypeChecker<'interner> {
         });
     }
 
+    fn get_lvalue_name_and_span(&self, lvalue: &HirLValue) -> (String, Span) {
+        match lvalue {
+            HirLValue::Ident(name, _) => {
+                let span = name.location.span;
+
+                if let Some(definition) = self.interner.try_definition(name.id) {
+                    (definition.name.clone(), span)
+                } else {
+                    ("(undeclared variable)".into(), span)
+                }
+            }
+            HirLValue::MemberAccess { object, .. } => self.get_lvalue_name_and_span(object),
+            HirLValue::Index { array, .. } => self.get_lvalue_name_and_span(array),
+            HirLValue::Dereference { lvalue, .. } => self.get_lvalue_name_and_span(lvalue),
+        }
+    }
+
     /// Type check an lvalue - the left hand side of an assignment statement.
-    fn check_lvalue(&mut self, lvalue: HirLValue, assign_span: Span) -> (Type, HirLValue) {
+    fn check_lvalue(&mut self, lvalue: &HirLValue, assign_span: Span) -> (Type, HirLValue, bool) {
         match lvalue {
             HirLValue::Ident(ident, _) => {
+                let mut mutable = true;
+
                 let typ = if ident.id == DefinitionId::dummy_id() {
                     Type::Error
                 } else {
-                    // Do we need to store TypeBindings here?
-                    let typ = self.interner.id_type(ident.id).instantiate(self.interner).0;
-                    let typ = typ.follow_bindings();
-
                     if let Some(definition) = self.interner.try_definition(ident.id) {
-                        if !definition.mutable && !matches!(typ, Type::MutableReference(_)) {
-                            self.errors.push(TypeCheckError::VariableMustBeMutable {
-                                name: definition.name.clone(),
-                                span: ident.location.span,
-                            });
-                        }
+                        mutable = definition.mutable;
                     }
 
-                    typ
+                    let typ = self.interner.id_type(ident.id).instantiate(self.interner).0;
+                    typ.follow_bindings()
                 };
 
-                (typ.clone(), HirLValue::Ident(ident, typ))
+                (typ.clone(), HirLValue::Ident(*ident, typ), mutable)
             }
             HirLValue::MemberAccess { object, field_name, .. } => {
-                let (lhs_type, object) = self.check_lvalue(*object, assign_span);
+                let (lhs_type, object, mut mutable) = self.check_lvalue(object, assign_span);
                 let mut object = Box::new(object);
                 let span = field_name.span();
+                let field_name = field_name.clone();
 
                 let object_ref = &mut object;
+                let mutable_ref = &mut mutable;
 
-                let (typ, field_index) = self
+                let (object_type, field_index) = self
                     .check_field_access(
                         &lhs_type,
                         &field_name.0.contents,
@@ -206,16 +224,19 @@ impl<'interner> TypeChecker<'interner> {
 
                             let lvalue = std::mem::replace(object_ref, Box::new(tmp_value));
                             *object_ref = Box::new(HirLValue::Dereference { lvalue, element_type });
+                            *mutable_ref = true;
                         },
                     )
                     .unwrap_or((Type::Error, 0));
 
                 let field_index = Some(field_index);
-                (typ.clone(), HirLValue::MemberAccess { object, field_name, field_index, typ })
+                let typ = object_type.clone();
+                let lvalue = HirLValue::MemberAccess { object, field_name, field_index, typ };
+                (object_type, lvalue, mutable)
             }
             HirLValue::Index { array, index, .. } => {
-                let index_type = self.check_expression(&index);
-                let expr_span = self.interner.expr_span(&index);
+                let index_type = self.check_expression(index);
+                let expr_span = self.interner.expr_span(index);
 
                 index_type.unify(
                     &Type::polymorphic_integer(self.interner),
@@ -227,7 +248,7 @@ impl<'interner> TypeChecker<'interner> {
                     },
                 );
 
-                let (array_type, array) = self.check_lvalue(*array, assign_span);
+                let (array_type, array, mutable) = self.check_lvalue(array, assign_span);
                 let array = Box::new(array);
 
                 let typ = match array_type.follow_bindings() {
@@ -244,10 +265,10 @@ impl<'interner> TypeChecker<'interner> {
                     }
                 };
 
-                (typ.clone(), HirLValue::Index { array, index, typ })
+                (typ.clone(), HirLValue::Index { array, index: *index, typ }, mutable)
             }
             HirLValue::Dereference { lvalue, element_type: _ } => {
-                let (reference_type, lvalue) = self.check_lvalue(*lvalue, assign_span);
+                let (reference_type, lvalue, _) = self.check_lvalue(lvalue, assign_span);
                 let lvalue = Box::new(lvalue);
 
                 let element_type = Type::type_variable(self.interner.next_type_variable_id());
@@ -259,7 +280,8 @@ impl<'interner> TypeChecker<'interner> {
                     expr_span: assign_span,
                 });
 
-                (element_type.clone(), HirLValue::Dereference { lvalue, element_type })
+                // Dereferences are always mutable since we already type checked against a &mut T
+                (element_type.clone(), HirLValue::Dereference { lvalue, element_type }, true)
             }
         }
     }

tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "mutability_regression_2911"
+type = "bin"
+authors = [""]
+compiler_version = "0.9.0"
+
+[dependencies]

tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr

@@ -0,0 +1,5 @@
+// Expect 'Variable must be mutable to be assigned to' error
+fn main() {
+    let slice : &mut [Field] = &mut [];
+    slice = &mut (*slice).push_back(1);
+}