From d5ee20ea43ccf1130f7d34231562f13e98ea636b Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 10 Oct 2023 09:12:01 -0500 Subject: [PATCH] fix: Prevent mutating immutable bindings to mutable types (#3075) --- .../noirc_frontend/src/hir/type_check/stmt.rs | 68 ++++++++++++------- .../mutability_regression_2911/Nargo.toml | 7 ++ .../mutability_regression_2911/src/main.nr | 5 ++ 3 files changed, 57 insertions(+), 23 deletions(-) create mode 100644 tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/type_check/stmt.rs b/compiler/noirc_frontend/src/hir/type_check/stmt.rs index a3cb49c6d2e..0e843a51dc8 100644 --- a/compiler/noirc_frontend/src/hir/type_check/stmt.rs +++ b/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) } } } diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml new file mode 100644 index 00000000000..5136fad35ce --- /dev/null +++ b/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] diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr b/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr new file mode 100644 index 00000000000..a0d53706f97 --- /dev/null +++ b/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); +}