Skip to content

fix: comptime code not mutating shared ref to struct field #9250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jfecher merged 9 commits into from
Jul 21, 2025
Merged

fix: comptime code not mutating shared ref to struct field #9250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
41a1fe8
Add a Shared wrapper for all struct and tuple fields
jfecher Jul 17, 2025
b14e1f0
Fix issue and find new issue
Jul 18, 2025
8bf8cd8
Format
Jul 18, 2025
d05d63b
Thought I already updated snaps
jfecher Jul 18, 2025
fcaad39
Format
jfecher Jul 18, 2025
6b2293e
Format test
jfecher Jul 18, 2025
cfe5fd6
Format test
jfecher Jul 18, 2025
8bd4cc1
Formatted test needs updated snap
jfecher Jul 18, 2025
1c840b3
Copy values
jfecher Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions compiler/noirc_frontend/src/hir/comptime/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,8 @@ impl Display for ValuePrinter<'_, '_> {
Value::Function(..) => write!(f, "(function)"),
Value::Closure(..) => write!(f, "(closure)"),
Value::Tuple(fields) => {
let fields = vecmap(fields, |field| field.display(self.interner).to_string());
let fields =
vecmap(fields, |field| field.borrow().display(self.interner).to_string());
if fields.len() == 1 {
write!(f, "({},)", fields[0])
} else {
Expand All @@ -399,7 +400,7 @@ impl Display for ValuePrinter<'_, '_> {
other => other.to_string(),
};
let fields = vecmap(fields, |(name, value)| {
format!("{}: {}", name, value.display(self.interner))
format!("{}: {}", name, value.borrow().display(self.interner))
});
write!(f, "{typename} {{ {} }}", fields.join(", "))
}
Expand Down
72 changes: 57 additions & 15 deletions compiler/noirc_frontend/src/hir/comptime/interpreter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
for ((pattern, typ), argument) in
pattern_fields.iter().zip(type_fields).zip(fields)
{
let argument = argument.borrow().clone();
self.define_pattern(pattern, typ, argument, location)?;
}
Ok(())
Expand Down Expand Up @@ -450,6 +451,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
}
})?;

let field = field.borrow();
let field_type = field.get_type().into_owned();
let result = self.define_pattern(
field_pattern,
Expand Down Expand Up @@ -492,7 +494,10 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
if let Entry::Occupied(mut entry) = scope.entry(id) {
match entry.get() {
Value::Pointer(reference, true, _) => {
*reference.borrow_mut() = argument;
// We can't store to the reference directly, we need to check if the value
// is a struct or tuple to store to each field instead. This is so any
// references to these fields are also updated.
Self::store_flattened(reference.clone(), argument);
}
_ => {
entry.insert(argument);
Expand Down Expand Up @@ -898,8 +903,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
/// - `<=`: `ordering != Ordering::Less`
fn evaluate_ordering(&self, ordering: Value, operator: BinaryOpKind) -> IResult<Value> {
let ordering = match ordering {
Value::Struct(fields, _) => match fields.into_iter().next().unwrap().1 {
Value::Field(ordering) => ordering,
Value::Struct(fields, _) => match &*fields.into_iter().next().unwrap().1.borrow() {
Value::Field(ordering) => *ordering,
_ => unreachable!("`cmp` should always return an Ordering value"),
},
_ => unreachable!("`cmp` should always return an Ordering value"),
Expand Down Expand Up @@ -938,7 +943,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
.fields
.into_iter()
.map(|(name, expr)| {
let field_value = self.evaluate(expr)?;
let field_value = Shared::new(self.evaluate(expr)?);
Ok((Rc::new(name.into_string()), field_value))
})
.collect::<Result<_, _>>()?;
Expand All @@ -961,11 +966,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
let (fields, struct_type) = match self.evaluate(access.lhs)? {
Value::Struct(fields, typ) => (fields, typ),
Value::Tuple(fields) => {
let (fields, field_types): (HashMap<Rc<String>, Value>, Vec<Type>) = fields
let (fields, field_types): (HashMap<Rc<String>, Shared<Value>>, Vec<Type>) = fields
.into_iter()
.enumerate()
.map(|(i, field)| {
let field_type = field.get_type().into_owned();
let field_type = field.borrow().get_type().into_owned();
let key_val_pair = (Rc::new(i.to_string()), field);
(key_val_pair, field_type)
})
Expand All @@ -979,19 +984,25 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
}
};

fields.get(access.rhs.as_string()).cloned().ok_or_else(|| {
let field = fields.get(access.rhs.as_string()).cloned().ok_or_else(|| {
let location = self.elaborator.interner.expr_location(&id);
let value = Value::Struct(fields, struct_type);
let field_name = access.rhs.into_string();
let typ = value.get_type().into_owned();
InterpreterError::ExpectedStructToHaveField { typ, field_name, location }
})
})?;

// Return a reference to the field so that `&mut foo.bar.baz` can use this reference.
// We set auto_deref to true so that when it is used elsewhere it is dereferenced
// automatically.
Ok(Value::Pointer(field, true, false))
}

fn evaluate_call(&mut self, call: HirCallExpression, id: ExprId) -> IResult<Value> {
let function = self.evaluate(call.func)?;
let arguments = try_vecmap(call.arguments, |arg| {
Ok((self.evaluate(arg)?, self.elaborator.interner.expr_location(&arg)))
let value = self.evaluate(arg)?.copy();
Ok((value, self.elaborator.interner.expr_location(&arg)))
})?;
let location = self.elaborator.interner.expr_location(&id);

Expand Down Expand Up @@ -1084,7 +1095,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
}

fn evaluate_tuple(&mut self, tuple: Vec<ExprId>) -> IResult<Value> {
let fields = try_vecmap(tuple, |field| self.evaluate(field))?;
let fields = try_vecmap(tuple, |field| Ok(Shared::new(self.evaluate(field)?)))?;
Ok(Value::Tuple(fields))
}

Expand Down Expand Up @@ -1165,7 +1176,7 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
HirLValue::Dereference { lvalue, element_type: _, location, implicitly_added: _ } => {
match self.evaluate_lvalue(&lvalue)? {
Value::Pointer(value, _, _) => {
*value.borrow_mut() = rhs;
Self::store_flattened(value, rhs);
Ok(())
}
value => {
Expand All @@ -1186,11 +1197,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {

match object_value {
Value::Tuple(mut fields) => {
fields[index] = rhs;
fields[index] = Shared::new(rhs);
self.store_lvalue(*object, Value::Tuple(fields))
}
Value::Struct(mut fields, typ) => {
fields.insert(Rc::new(field_name.into_string()), rhs);
fields.insert(Rc::new(field_name.into_string()), Shared::new(rhs));
self.store_lvalue(*object, Value::Struct(fields, typ.follow_bindings()))
}
value => {
Expand All @@ -1217,6 +1228,35 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
}
}

/// When we store to a struct such as in
/// ```noir
/// let mut a = (false,);
/// let b = &mut a.0;
/// a = (true,);
/// ```
/// we must flatten the store to store to each individual field so that any existing
/// references, such as `b` above, will also reflect the mutation.
fn store_flattened(lvalue: Shared<Value>, rvalue: Value) {
let lvalue_ref = lvalue.borrow();
match (&*lvalue_ref, rvalue) {
(Value::Struct(lvalue_fields, _), Value::Struct(mut rvalue_fields, _)) => {
for (name, lvalue) in lvalue_fields.iter() {
let Some(rvalue) = rvalue_fields.remove(name) else { continue };
Self::store_flattened(lvalue.clone(), rvalue.unwrap_or_clone());
}
}
(Value::Tuple(lvalue_fields), Value::Tuple(rvalue_fields)) => {
for (lvalue, rvalue) in lvalue_fields.iter().zip(rvalue_fields) {
Self::store_flattened(lvalue.clone(), rvalue.unwrap_or_clone());
}
}
(_, rvalue) => {
drop(lvalue_ref);
*lvalue.borrow_mut() = rvalue;
}
}
}

fn evaluate_lvalue(&mut self, lvalue: &HirLValue) -> IResult<Value> {
match lvalue {
HirLValue::Ident(ident, _) => match self.lookup(ident)? {
Expand Down Expand Up @@ -1244,8 +1284,10 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
})?;

match object_value {
Value::Tuple(mut values) => Ok(values.swap_remove(index)),
Value::Struct(fields, _) => Ok(fields[field_name.as_string()].clone()),
Value::Tuple(mut values) => Ok(values.swap_remove(index).unwrap_or_clone()),
Value::Struct(fields, _) => {
Ok(fields[field_name.as_string()].clone().unwrap_or_clone())
}
value => Err(InterpreterError::NonTupleOrStructInMemberAccess {
typ: value.get_type().into_owned(),
location: *location,
Expand Down
Loading
Loading