From b657a387f59f951934251f88ce8cbe17728a72f2 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 5 Oct 2023 14:57:59 -0700 Subject: [PATCH 1/3] Fix mut type check --- .../noirc_frontend/src/hir/type_check/stmt.rs | 68 ++++++++++++------- 1 file changed, 45 insertions(+), 23 deletions(-) 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) } } } From d437e96efc894949d6a8613ce7d270e11ecfd2d9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 9 Oct 2023 14:19:57 -0500 Subject: [PATCH 2/3] Add regression test --- .../compile_failure/mutability_regression_3008/Nargo.toml | 7 +++++++ .../compile_failure/mutability_regression_3008/src/main.nr | 5 +++++ 2 files changed, 12 insertions(+) create mode 100644 tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml create mode 100644 tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/src/main.nr diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml new file mode 100644 index 00000000000..c53235b4dcd --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "mutability_regression_3008" +type = "bin" +authors = [""] +compiler_version = "0.9.0" + +[dependencies] diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/src/main.nr b/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/src/main.nr new file mode 100644 index 00000000000..a0d53706f97 --- /dev/null +++ b/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/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); +} From 820a1d6740f26d6eb26ca840d0642a41f7b2ab14 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 10 Oct 2023 08:40:33 -0500 Subject: [PATCH 3/3] Rename test to issue 2911 --- .../Nargo.toml | 2 +- .../src/main.nr | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename tooling/nargo_cli/tests/compile_failure/{mutability_regression_3008 => mutability_regression_2911}/Nargo.toml (69%) rename tooling/nargo_cli/tests/compile_failure/{mutability_regression_3008 => mutability_regression_2911}/src/main.nr (100%) diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml b/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml similarity index 69% rename from tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml rename to tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml index c53235b4dcd..5136fad35ce 100644 --- a/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/Nargo.toml +++ b/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "mutability_regression_3008" +name = "mutability_regression_2911" type = "bin" authors = [""] compiler_version = "0.9.0" diff --git a/tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/src/main.nr b/tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr similarity index 100% rename from tooling/nargo_cli/tests/compile_failure/mutability_regression_3008/src/main.nr rename to tooling/nargo_cli/tests/compile_failure/mutability_regression_2911/src/main.nr