From 5ed27231bc459b75648ef367f1077c30ee2eab8f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 30 Jun 2025 15:14:36 +0100 Subject: [PATCH 1/2] Assign to temp index variable to sequence side effects --- .../fuzz/src/targets/orig_vs_morph.rs | 18 +----- tooling/ast_fuzzer/src/program/expr.rs | 12 ++++ tooling/ast_fuzzer/src/program/func.rs | 63 +++++++++++++++---- tooling/ast_fuzzer/src/program/mod.rs | 2 +- 4 files changed, 66 insertions(+), 29 deletions(-) diff --git a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs index 9dcfb7ce789..49e3e8eb4ba 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs @@ -299,7 +299,7 @@ fn is_special_call(call: &Call) -> bool { mod rules { use crate::targets::orig_vs_morph::{VariableContext, helpers::reassign_ids}; - use super::helpers::{gen_expr, has_side_effect}; + use super::helpers::gen_expr; use acir::{AcirField, FieldElement}; use arbitrary::Unstructured; use noir_ast_fuzzer::{expr, types}; @@ -492,7 +492,7 @@ mod rules { operator: BinaryOpKind::Add | BinaryOpKind::Multiply, .. }) - ) && !has_side_effect(expr) + ) && !expr::has_side_effect(expr) }, |_u, _locals, expr| { let Expression::Binary(binary) = expr else { @@ -574,7 +574,7 @@ mod rules { // unless the expression can have a side effect, which we don't want to duplicate. if let Some(typ) = expr.return_type() { matches!(typ.as_ref(), Type::Bool) - && !has_side_effect(expr) + && !expr::has_side_effect(expr) && !expr::exists(expr, |expr| { matches!( expr, @@ -624,18 +624,6 @@ mod helpers { use crate::targets::orig_vs_morph::VariableContext; - /// Check if an expression can have a side effect, in which case duplicating or reordering it could - /// change the behavior of the program. - pub(super) fn has_side_effect(expr: &Expression) -> bool { - expr::exists(expr, |expr| { - matches!( - expr, - Expression::Call(_) // Functions can have side effects, maybe mutating some reference, printing - | Expression::Assign(_) // Assignment to a mutable variable could double up effects - ) - }) - } - /// Generate an arbitrary pure (free of side effects) expression, returning a specific type. pub(super) fn gen_expr( u: &mut Unstructured, diff --git a/tooling/ast_fuzzer/src/program/expr.rs b/tooling/ast_fuzzer/src/program/expr.rs index 026eb918b15..a3c005ba6a0 100644 --- a/tooling/ast_fuzzer/src/program/expr.rs +++ b/tooling/ast_fuzzer/src/program/expr.rs @@ -380,6 +380,18 @@ pub fn has_call(expr: &Expression) -> bool { }) } +/// Check if an expression can have a side effect, in which case duplicating or reordering it could +/// change the behavior of the program. +pub fn has_side_effect(expr: &Expression) -> bool { + exists(expr, |expr| { + matches!( + expr, + Expression::Call(_) // Functions can have side effects, maybe mutating some reference, printing + | Expression::Assign(_) // Assignment to a mutable variable could double up effects + ) + }) +} + /// Check if an `Expression` or any of its descendants match a predicate. pub fn exists(expr: &Expression, pred: impl Fn(&Expression) -> bool) -> bool { let mut exists = false; diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 71e138a3abf..9b4ec6cb26f 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -977,6 +977,29 @@ impl<'a> FunctionContext<'a> { expr::let_var(id, mutable, name, expr) } + /// Add a new local variable and return a `Let` expression along with an `Ident` to refer it by. + fn let_var_and_ident( + &mut self, + mutable: bool, + typ: Type, + expr: Expression, + add_to_scope: bool, + is_dynamic: bool, + ) -> (Expression, Ident) { + let v = self.let_var(mutable, typ.clone(), expr, add_to_scope, is_dynamic); + let Expression::Let(Let { id, name, .. }) = &v else { + unreachable!("expected to Let; got {v:?}"); + }; + let i = expr::ident_inner( + VariableId::Local(*id), + self.next_ident_id(), + mutable, + name.clone(), + typ, + ); + (v, i) + } + /// Assign to a mutable variable, if we have one in scope. /// /// It resets the dynamic flag of the assigned variable. @@ -1001,24 +1024,35 @@ impl<'a> FunctionContext<'a> { let ident = LValue::Ident(ident); // For arrays and tuples we can consider assigning to their items. - let (lvalue, typ, idx_dyn, is_compound) = match self.local_type(id).clone() { + let (lvalue, typ, idx_dyn, is_compound, prefix) = match self.local_type(id).clone() { Type::Array(len, typ) if len > 0 && bool::arbitrary(u)? => { let (idx, idx_dyn) = self.gen_index(u, len, self.max_depth())?; + + // If the index expressions can have side effects, we need to assign it to a + // temporary variable to match the sequencing done by the frontend; see #8384. + let (idx, prefix) = if expr::has_side_effect(&idx) { + let (idx, idx_ident) = + self.let_var_and_ident(false, types::U32, idx, false, idx_dyn); + (Expression::Ident(idx_ident), Some(idx)) + } else { + (idx, None) + }; + let lvalue = LValue::Index { array: Box::new(ident), index: Box::new(idx), element_type: typ.as_ref().clone(), location: Location::dummy(), }; - (lvalue, typ.deref().clone(), idx_dyn, true) + (lvalue, typ.deref().clone(), idx_dyn, true, prefix) } Type::Tuple(items) if bool::arbitrary(u)? => { let idx = u.choose_index(items.len())?; let typ = items[idx].clone(); let lvalue = LValue::MemberAccess { object: Box::new(ident), field_index: idx }; - (lvalue, typ, false, true) + (lvalue, typ, false, true, None) } - typ => (ident, typ, false, false), + typ => (ident, typ, false, false, None), }; // Generate the assigned value. @@ -1026,11 +1060,19 @@ impl<'a> FunctionContext<'a> { if idx_dyn || expr_dyn { self.dynamics.insert(id); - } else if !is_compound && !idx_dyn && !expr_dyn { + } else if !idx_dyn && !expr_dyn && !is_compound { + // This value is no longer considered dynamic, unless we assigned to a member of an array or tuple, + // in which case we don't know if other members have dynamic properties. self.dynamics.remove(&id); } - Ok(Some(Expression::Assign(Assign { lvalue, expression: Box::new(expr) }))) + let assign = Expression::Assign(Assign { lvalue, expression: Box::new(expr) }); + + if let Some(prefix) = prefix { + Ok(Some(Expression::Block(vec![prefix, assign]))) + } else { + Ok(Some(assign)) + } } /// Generate a `println` statement, if there is some printable local variable. @@ -1563,14 +1605,9 @@ impl<'a> FunctionContext<'a> { /// Create a block with a let binding, then return a mutable reference to it. /// This is used as a workaround when we need a mutable reference over an immutable value. fn indirect_ref_mut(&mut self, (expr, is_dyn): TrackedExpression, typ: Type) -> Expression { - self.locals.enter(); - let let_expr = self.let_var(true, typ.clone(), expr.clone(), true, is_dyn); - let Expression::Let(Let { id, .. }) = &let_expr else { - unreachable!("expected Let; got {let_expr}"); - }; - let let_ident = self.local_ident(*id); + let (let_expr, let_ident) = + self.let_var_and_ident(true, typ.clone(), expr.clone(), false, is_dyn); let ref_expr = expr::ref_mut(Expression::Ident(let_ident), typ); - self.locals.exit(); Expression::Block(vec![let_expr, ref_expr]) } } diff --git a/tooling/ast_fuzzer/src/program/mod.rs b/tooling/ast_fuzzer/src/program/mod.rs index 0c7ea9ace0f..071163f4d4e 100644 --- a/tooling/ast_fuzzer/src/program/mod.rs +++ b/tooling/ast_fuzzer/src/program/mod.rs @@ -572,7 +572,7 @@ impl std::fmt::Display for DisplayAstAsNoir<'_> { printer.show_type_in_let = true; // Most of the time it doesn't affect testing, except the comptime tests where // we parse back the code. For that we use `DisplayAstAsNoirComptime`. - printer.show_type_of_int_literal = false; + printer.show_type_of_int_literal = true; printer.print_program(self.0, f) } } From d046b2a1668030488c6e601a8b9dab5e8d5bc51d Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 30 Jun 2025 17:42:44 +0100 Subject: [PATCH 2/2] Don't print int type annotations yet --- tooling/ast_fuzzer/src/program/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/ast_fuzzer/src/program/mod.rs b/tooling/ast_fuzzer/src/program/mod.rs index 88d82e88aec..20fe5b949eb 100644 --- a/tooling/ast_fuzzer/src/program/mod.rs +++ b/tooling/ast_fuzzer/src/program/mod.rs @@ -572,7 +572,7 @@ impl std::fmt::Display for DisplayAstAsNoir<'_> { printer.show_type_in_let = true; // Most of the time it doesn't affect testing, except the comptime tests where // we parse back the code. For that we use `DisplayAstAsNoirComptime`. - printer.show_type_of_int_literal = true; + printer.show_type_of_int_literal = false; printer.print_program(self.0, f) } }