From 65a05755b1660c7e950d284a4c04cc5f528ed7fe Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 15 Jul 2024 11:19:07 -0500 Subject: [PATCH 1/4] Fix mutability issue --- .../src/hir/comptime/interpreter.rs | 36 +++++++++++++++---- .../noirc_frontend/src/hir/comptime/value.rs | 10 +++--- .../src/parser/parser/structs.rs | 13 +++++-- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 02714f77605..0bd03d4abea 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -241,6 +241,8 @@ impl<'a> Interpreter<'a> { Ok(()) } HirPattern::Mutable(pattern, _) => { + // Create a mutable reference to store to + let argument = Value::Pointer(Shared::new(argument), true); self.define_pattern(pattern, typ, argument, location) } HirPattern::Tuple(pattern_fields, _) => match (argument, typ) { @@ -334,8 +336,19 @@ impl<'a> Interpreter<'a> { } } - /// Evaluate an expression and return the result + /// Evaluate an expression and return the result. + /// This will automatically dereference a mutable variable if used. pub fn evaluate(&mut self, id: ExprId) -> IResult { + match self.evaluate_no_dereference(id)? { + Value::Pointer(elem, true) => Ok(elem.borrow().clone()), + other => Ok(other), + } + } + + /// Evaluating a mutable variable will dereference it automatically. + /// This function should be used when that is not desired - e.g. when + /// compiling a `&mut var` expression to grab the original reference. + fn evaluate_no_dereference(&mut self, id: ExprId) -> IResult { match self.interner.expression(&id) { HirExpression::Ident(ident, _) => self.evaluate_ident(ident, id), HirExpression::Literal(literal) => self.evaluate_literal(literal, id), @@ -592,7 +605,10 @@ impl<'a> Interpreter<'a> { } fn evaluate_prefix(&mut self, prefix: HirPrefixExpression, id: ExprId) -> IResult { - let rhs = self.evaluate(prefix.rhs)?; + let rhs = match prefix.operator { + UnaryOp::MutableReference => self.evaluate_no_dereference(prefix.rhs)?, + _ => self.evaluate(prefix.rhs)?, + }; self.evaluate_prefix_with_value(rhs, prefix.operator, id) } @@ -634,9 +650,17 @@ impl<'a> Interpreter<'a> { Err(InterpreterError::InvalidValueForUnary { value, location, operator: "not" }) } }, - UnaryOp::MutableReference => Ok(Value::Pointer(Shared::new(rhs))), + UnaryOp::MutableReference => { + // If this is a mutable variable (auto_deref = true), turn this into an explicit + // mutable reference just by switching the value of `auto_deref`. Otherwise, wrap + // the value in a fresh reference. + match rhs { + Value::Pointer(elem, true) => Ok(Value::Pointer(elem, false)), + other => Ok(Value::Pointer(Shared::new(other), false)), + } + } UnaryOp::Dereference { implicitly_added: _ } => match rhs { - Value::Pointer(element) => Ok(element.borrow().clone()), + Value::Pointer(element, _) => Ok(element.borrow().clone()), value => { let location = self.interner.expr_location(&id); Err(InterpreterError::NonPointerDereferenced { value, location }) @@ -1303,7 +1327,7 @@ impl<'a> Interpreter<'a> { HirLValue::Ident(ident, typ) => self.mutate(ident.id, rhs, ident.location), HirLValue::Dereference { lvalue, element_type: _, location } => { match self.evaluate_lvalue(&lvalue)? { - Value::Pointer(value) => { + Value::Pointer(value, _) => { *value.borrow_mut() = rhs; Ok(()) } @@ -1356,7 +1380,7 @@ impl<'a> Interpreter<'a> { HirLValue::Ident(ident, _) => self.lookup(ident), HirLValue::Dereference { lvalue, element_type: _, location } => { match self.evaluate_lvalue(lvalue)? { - Value::Pointer(value) => Ok(value.borrow().clone()), + Value::Pointer(value, _) => Ok(value.borrow().clone()), value => { Err(InterpreterError::NonPointerDereferenced { value, location: *location }) } diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index 46d1fc23973..6f83f9bf23e 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -41,7 +41,7 @@ pub enum Value { Closure(HirLambda, Vec, Type), Tuple(Vec), Struct(HashMap, Value>, Type), - Pointer(Shared), + Pointer(Shared, /* auto_deref */ bool), Array(Vector, Type), Slice(Vector, Type), Code(Rc), @@ -79,7 +79,7 @@ impl Value { Value::Slice(_, typ) => return Cow::Borrowed(typ), Value::Code(_) => Type::Quoted(QuotedType::Quoted), Value::StructDefinition(_) => Type::Quoted(QuotedType::StructDefinition), - Value::Pointer(element) => { + Value::Pointer(element, _) => { let element = element.borrow().get_type().into_owned(); Type::MutableReference(Box::new(element)) } @@ -199,7 +199,7 @@ impl Value { } }; } - Value::Pointer(_) + Value::Pointer(..) | Value::StructDefinition(_) | Value::TraitDefinition(_) | Value::FunctionDefinition(_) @@ -309,7 +309,7 @@ impl Value { HirExpression::Literal(HirLiteral::Slice(HirArrayLiteral::Standard(elements))) } Value::Code(block) => HirExpression::Unquote(unwrap_rc(block)), - Value::Pointer(_) + Value::Pointer(..) | Value::StructDefinition(_) | Value::TraitDefinition(_) | Value::FunctionDefinition(_) @@ -400,7 +400,7 @@ impl Display for Value { let fields = vecmap(fields, |(name, value)| format!("{}: {}", name, value)); write!(f, "{typename} {{ {} }}", fields.join(", ")) } - Value::Pointer(value) => write!(f, "&mut {}", value.borrow()), + Value::Pointer(value, _) => write!(f, "&mut {}", value.borrow()), Value::Array(values, _) => { let values = vecmap(values, ToString::to_string); write!(f, "[{}]", values.join(", ")) diff --git a/compiler/noirc_frontend/src/parser/parser/structs.rs b/compiler/noirc_frontend/src/parser/parser/structs.rs index 7da956bdfea..d45c2e64687 100644 --- a/compiler/noirc_frontend/src/parser/parser/structs.rs +++ b/compiler/noirc_frontend/src/parser/parser/structs.rs @@ -32,9 +32,16 @@ pub(super) fn struct_definition() -> impl NoirParser { .then(ident()) .then(function::generics()) .then(fields) - .validate(|(((raw_attributes, name), generics), fields), span, emit| { - let attributes = validate_secondary_attributes(raw_attributes, span, emit); - TopLevelStatement::Struct(NoirStruct { name, attributes, generics, fields, span }) + .validate(|((((attributes, is_comptime), name), generics), fields), span, emit| { + let attributes = validate_secondary_attributes(attributes, span, emit); + TopLevelStatement::Struct(NoirStruct { + name, + attributes, + generics, + fields, + span, + is_comptime, + }) }) } From e03b94f178c573f70f437144de97512dcaa09590 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 15 Jul 2024 11:33:47 -0500 Subject: [PATCH 2/4] Add test --- compiler/noirc_frontend/src/hir/comptime/tests.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/compiler/noirc_frontend/src/hir/comptime/tests.rs b/compiler/noirc_frontend/src/hir/comptime/tests.rs index e8e05506c94..6fdd956caf6 100644 --- a/compiler/noirc_frontend/src/hir/comptime/tests.rs +++ b/compiler/noirc_frontend/src/hir/comptime/tests.rs @@ -77,6 +77,18 @@ fn mutating_mutable_references() { assert_eq!(result, Value::I64(4)); } +#[test] +fn mutation_leaks() { + let program = "comptime fn main() -> pub i8 { + let mut x = 3; + let y = &mut x; + *y = 5; + x + }"; + let result = interpret(program, vec!["main".into()]); + assert_eq!(result, Value::I8(5)); +} + #[test] fn mutating_arrays() { let program = "comptime fn main() -> pub u8 { From 9e6fa42e1af208ed625cd54cc6007b4ed2ed3a62 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 15 Jul 2024 12:06:47 -0500 Subject: [PATCH 3/4] Fix lvalues --- compiler/noirc_frontend/src/hir/comptime/interpreter.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 0bd03d4abea..995ab5bd86e 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1377,7 +1377,10 @@ impl<'a> Interpreter<'a> { fn evaluate_lvalue(&mut self, lvalue: &HirLValue) -> IResult { match lvalue { - HirLValue::Ident(ident, _) => self.lookup(ident), + HirLValue::Ident(ident, _) => match self.lookup(ident)? { + Value::Pointer(elem, true) => Ok(elem.borrow().clone()), + other => Ok(other), + }, HirLValue::Dereference { lvalue, element_type: _, location } => { match self.evaluate_lvalue(lvalue)? { Value::Pointer(value, _) => Ok(value.borrow().clone()), From 6f302601b37ac66e0f7aaff913d2ae5977aec7ee Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 15 Jul 2024 12:37:41 -0500 Subject: [PATCH 4/4] Undo parser change --- .../noirc_frontend/src/parser/parser/structs.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/parser/parser/structs.rs b/compiler/noirc_frontend/src/parser/parser/structs.rs index d45c2e64687..7da956bdfea 100644 --- a/compiler/noirc_frontend/src/parser/parser/structs.rs +++ b/compiler/noirc_frontend/src/parser/parser/structs.rs @@ -32,16 +32,9 @@ pub(super) fn struct_definition() -> impl NoirParser { .then(ident()) .then(function::generics()) .then(fields) - .validate(|((((attributes, is_comptime), name), generics), fields), span, emit| { - let attributes = validate_secondary_attributes(attributes, span, emit); - TopLevelStatement::Struct(NoirStruct { - name, - attributes, - generics, - fields, - span, - is_comptime, - }) + .validate(|(((raw_attributes, name), generics), fields), span, emit| { + let attributes = validate_secondary_attributes(raw_attributes, span, emit); + TopLevelStatement::Struct(NoirStruct { name, attributes, generics, fields, span }) }) }