diff --git a/crates/nargo_cli/tests/execution_success/references/src/main.nr b/crates/nargo_cli/tests/execution_success/references/src/main.nr index aeed8e0b901..ae96693c3d2 100644 --- a/crates/nargo_cli/tests/execution_success/references/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/references/src/main.nr @@ -34,6 +34,7 @@ fn main(mut x: Field) { regression_1887(); regression_2054(); regression_2030(); + regression_2255(); } fn add1(x: &mut Field) { @@ -97,3 +98,16 @@ fn regression_2030() { let _ = *array[0]; *array[0] = 1; } + +// The `mut x: &mut ...` caught a bug handling lvalues where a double-dereference would occur internally +// in one step rather than being tracked by two separate steps. This lead to assigning the 1 value to the +// incorrect outer `mut` reference rather than the correct `&mut` reference. +fn regression_2255() { + let x = &mut 0; + regression_2255_helper(x); + assert(*x == 1); +} + +fn regression_2255_helper(mut x: &mut Field) { + *x = 1; +} diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs index 6de804a37b8..bcba7bf5992 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -537,7 +537,14 @@ impl<'a> FunctionContext<'a> { /// version, as it is only needed for recursion. pub(super) fn extract_current_value(&mut self, lvalue: &ast::LValue) -> LValue { match lvalue { - ast::LValue::Ident(ident) => LValue::Ident(self.ident_lvalue(ident)), + ast::LValue::Ident(ident) => { + let (reference, should_auto_deref) = self.ident_lvalue(ident); + if should_auto_deref { + LValue::Dereference { reference } + } else { + LValue::Ident + } + } ast::LValue::Index { array, index, .. } => self.index_lvalue(array, index).2, ast::LValue::MemberAccess { object, field_index } => { let (old_object, object_lvalue) = self.extract_current_value_recursive(object); @@ -551,18 +558,19 @@ impl<'a> FunctionContext<'a> { } } - pub(super) fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { + fn dereference_lvalue(&mut self, values: &Values, element_type: &ast::Type) -> Values { let element_types = Self::convert_type(element_type); values.map_both(element_types, |value, element_type| { - let reference = value.eval(self); + let reference = value.eval_reference(); self.builder.insert_load(reference, element_type).into() }) } - /// Compile the given identifier as a reference - ie. avoid calling .eval() - fn ident_lvalue(&self, ident: &ast::Ident) -> Values { + /// Compile the given identifier as a reference - ie. avoid calling .eval(). + /// Returns the variable's value and whether the variable is mutable. + fn ident_lvalue(&self, ident: &ast::Ident) -> (Values, bool) { match &ident.definition { - ast::Definition::Local(id) => self.lookup(*id), + ast::Definition::Local(id) => (self.lookup(*id), ident.mutable), other => panic!("Unexpected definition found for mutable value: {other}"), } } @@ -585,8 +593,13 @@ impl<'a> FunctionContext<'a> { fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) { match lvalue { ast::LValue::Ident(ident) => { - let variable = self.ident_lvalue(ident); - (variable.clone(), LValue::Ident(variable)) + let (variable, should_auto_deref) = self.ident_lvalue(ident); + if should_auto_deref { + let dereferenced = self.dereference_lvalue(&variable, &ident.typ); + (dereferenced, LValue::Dereference { reference: variable }) + } else { + (variable.clone(), LValue::Ident) + } } ast::LValue::Index { array, index, element_type, location } => { let (old_array, index, index_lvalue) = self.index_lvalue(array, index); @@ -601,7 +614,7 @@ impl<'a> FunctionContext<'a> { } ast::LValue::Dereference { reference, element_type } => { let (reference, _) = self.extract_current_value_recursive(reference); - let dereferenced = self.dereference(&reference, element_type); + let dereferenced = self.dereference_lvalue(&reference, element_type); (dereferenced, LValue::Dereference { reference }) } } @@ -614,7 +627,7 @@ impl<'a> FunctionContext<'a> { /// `extract_current_value` for more details. pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) { match lvalue { - LValue::Ident(references) => self.assign(references, new_value), + LValue::Ident => unreachable!("Cannot assign to a variable without a reference"), LValue::Index { old_array: mut array, index, array_lvalue } => { let element_size = self.builder.field_constant(self.element_size(array)); @@ -811,7 +824,7 @@ impl SharedContext { /// Used to remember the results of each step of extracting a value from an ast::LValue #[derive(Debug)] pub(super) enum LValue { - Ident(Values), + Ident, Index { old_array: ValueId, index: ValueId, array_lvalue: Box }, MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, Dereference { reference: Values }, diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 11c42d8b051..4cf9e83e21e 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -215,6 +215,14 @@ impl<'a> FunctionContext<'a> { } } + fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values { + let element_types = Self::convert_type(element_type); + values.map_both(element_types, |value, element_type| { + let reference = value.eval(self); + self.builder.insert_load(reference, element_type).into() + }) + } + fn codegen_reference(&mut self, expr: &Expression) -> Values { match expr { Expression::Ident(ident) => self.codegen_ident_reference(ident),