From 5ed27231bc459b75648ef367f1077c30ee2eab8f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 30 Jun 2025 15:14:36 +0100 Subject: [PATCH 01/12] 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 02/12] 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) } } From a4f393f6666d58bfd48469b934ff8f593ff632a7 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 14:58:19 +0100 Subject: [PATCH 03/12] Fall back to normal printing of print_oracle if it's not the direct version --- .../src/monomorphization/printer.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 93776c12767..62d037e493f 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -498,7 +498,9 @@ impl AstPrinter { }; // If this is the print oracle and we want to display it as Noir, we need to use the stdlib. if print_oracle && self.show_print_as_std { - return self.print_println(&call.arguments, f); + if self.print_println(&call.arguments, f)? { + return Ok(()); + } } if print_unsafe { write!(f, "unsafe {{ ")?; @@ -516,10 +518,18 @@ impl AstPrinter { /// Instead of printing a call to the print oracle as a regular function, /// print it in a way that makes it look like Noir: without the type /// information and bool flags. - fn print_println(&mut self, args: &[Expression], f: &mut Formatter) -> std::fmt::Result { + /// + /// This will only work if the AST bypassed the proxy functions created by + /// the monomorphizer. The returned flag indicates whether it managed to + /// do so, or false if the arguments were not as expected. + fn print_println( + &mut self, + args: &[Expression], + f: &mut Formatter, + ) -> Result { assert_eq!(args.len(), 4, "print has 4 arguments"); let Expression::Literal(Literal::Bool(with_newline)) = args[0] else { - unreachable!("the first arg of print is a bool"); + return Ok(false); }; if with_newline { write!(f, "println")?; @@ -531,7 +541,7 @@ impl AstPrinter { // they are inserted automatically by the monomorphizer in the AST. Here we ignore them. self.print_expr(&args[1], f)?; write!(f, ")")?; - Ok(()) + Ok(true) } fn print_lvalue(&mut self, lvalue: &LValue, f: &mut Formatter) -> std::fmt::Result { From 2157db9d9b0c779f635b4d47ae8fca21b2a0baa4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 15:38:31 +0100 Subject: [PATCH 04/12] Add fuzz test for monomorphizer roundtrip --- tooling/ast_fuzzer/tests/mono.rs | 94 ++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 tooling/ast_fuzzer/tests/mono.rs diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs new file mode 100644 index 00000000000..b784676c063 --- /dev/null +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -0,0 +1,94 @@ +//! Test that the AST of an arbitrary program can be printed, parsed, compiled, printed again, +//! and it results in the same AST. This should bring to light any assumptions the monomorphizer +//! puts in place during compilation that the direct AST generator does not respect. +//! +//! ```shell +//! cargo test -p noir_ast_fuzzer --test mono +//! ``` + +use std::{path::Path, time::Duration}; + +use arbtest::arbtest; +use nargo::parse_all; +use noir_ast_fuzzer::{Config, DisplayAstAsNoir, arb_program}; +use noirc_driver::{CompileOptions, file_manager_with_stdlib, prepare_crate}; +use noirc_frontend::{ + hir::Context, + monomorphization::{ast::Program, monomorphize}, +}; + +fn seed_from_env() -> Option { + let Ok(seed) = std::env::var("NOIR_AST_FUZZER_SEED") else { return None }; + let seed = u64::from_str_radix(seed.trim_start_matches("0x"), 16) + .unwrap_or_else(|e| panic!("failed to parse seed '{seed}': {e}")); + Some(seed) +} + +#[test] +fn arb_ast_roundtrip() { + let maybe_seed = seed_from_env(); + + let mut prop = arbtest(|u| { + let config = Config { + // The monomorphizer creates proxy functions, which the AST generator skips. + // Rather than try to match it, let's ignore prints in this test. + avoid_print: true, + // Negative literals can cause problems: --128_i8 is a compile error; --100_i32 is printed back as 100_i32. + avoid_negative_int_literals: true, + // The formatting of `unsafe { ` becomes `{ unsafe {` with extra line breaks. + // Let's stick to just Brillig so there is no need for `unsafe` at all. + force_brillig: true, + ..Default::default() + }; + let program1 = arb_program(u, config)?; + let src1 = format!("{}", DisplayAstAsNoir(&program1)); + println!("{src1}"); + let Some(program2) = monomorphize_snippet(src1.clone()) else { return Ok(()) }; + println!("{program2}"); + let src2 = format!("{}", DisplayAstAsNoir(&program2)); + similar_asserts::assert_eq!(sanitize(&src1), sanitize(&src2)); + Ok(()) + }) + .budget(Duration::from_secs(10)) + .size_min(1 << 12) + .size_max(1 << 20); + + if let Some(seed) = maybe_seed { + prop = prop.seed(seed); + } + + prop.run(); +} + +fn monomorphize_snippet(source: String) -> Option { + let root = Path::new(""); + let file_name = Path::new("main.nr"); + let mut file_manager = file_manager_with_stdlib(root); + file_manager.add_file_with_source(file_name, source).expect( + "Adding source buffer to file manager should never fail when file manager is empty", + ); + let parsed_files = parse_all(&file_manager); + + let mut context = Context::new(file_manager, parsed_files); + let crate_id = prepare_crate(&mut context, file_name); + + context.disable_comptime_printing(); + + // Things that generate compile-time rejections are possible, e.g. `--128`. + // In other tests we handle those already, so here we can just ignore them. + // We could also avoid generating them, but it would require avoiding all potential overflowing ops, or negative literals. + if let Err(_) = noirc_driver::check_crate(&mut context, crate_id, &CompileOptions::default()) { + return None; + } + + let main_id = context.get_main_function(&crate_id).expect("get_main_function"); + + let program = monomorphize(main_id, &mut context.def_interner, false).expect("monomorphize"); + + Some(program) +} + +fn sanitize(src: &str) -> String { + // Sometimes `;` is removed, or duplicated. + src.replace(";", "") +} From 48c192e98ac072022c893b5ca6adae93c1a9b0af Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 16:13:18 +0100 Subject: [PATCH 05/12] Expression::Unary with Reference has the target type of Reference, not the underlying --- compiler/noirc_frontend/src/monomorphization/ast.rs | 10 ++-------- tooling/ast_fuzzer/src/program/expr.rs | 8 ++++++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index 3ce51e21f49..541f753aaf9 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -7,7 +7,7 @@ use noirc_errors::{ }; use crate::{ - ast::{BinaryOpKind, IntegerBitSize, UnaryOp}, + ast::{BinaryOpKind, IntegerBitSize}, hir_def::expr::Constructor, shared::Signedness, signed_field::SignedField, @@ -81,13 +81,7 @@ impl Expression { }), }, Expression::Block(xs) => xs.last().and_then(|x| x.return_type()), - Expression::Unary(unary) => { - if let UnaryOp::Reference { mutable } = unary.operator { - owned(Type::Reference(Box::new(unary.result_type.clone()), mutable)) - } else { - borrowed(&unary.result_type) - } - } + Expression::Unary(unary) => borrowed(&unary.result_type), Expression::Binary(binary) => { if binary.operator.is_comparator() { borrowed(&Type::Bool) diff --git a/tooling/ast_fuzzer/src/program/expr.rs b/tooling/ast_fuzzer/src/program/expr.rs index 026eb918b15..e72a16dcba0 100644 --- a/tooling/ast_fuzzer/src/program/expr.rs +++ b/tooling/ast_fuzzer/src/program/expr.rs @@ -97,7 +97,7 @@ pub fn gen_literal(u: &mut Unstructured, typ: &Type) -> arbitrary::Result { // In Noir we can return a reference for a value created in a function. let value = gen_literal(u, typ.as_ref())?; - unary(UnaryOp::Reference { mutable: *mutable }, value, typ.as_ref().clone()) + ref_with_mut(value, typ.as_ref().clone(), *mutable) } _ => unreachable!("unexpected type to generate a literal for: {typ}"), }; @@ -336,7 +336,11 @@ pub fn deref(rhs: Expression, tgt_type: Type) -> Expression { /// Reference an expression as a target type pub fn ref_mut(rhs: Expression, tgt_type: Type) -> Expression { - unary(UnaryOp::Reference { mutable: true }, rhs, tgt_type) + ref_with_mut(rhs, tgt_type, true) +} + +fn ref_with_mut(rhs: Expression, tgt_type: Type, mutable: bool) -> Expression { + unary(UnaryOp::Reference { mutable }, rhs, Type::Reference(Box::new(tgt_type), mutable)) } /// Make a unary expression. From 56d7d86434c4753cbb2bfd3c8284586807553ed6 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 16:24:19 +0100 Subject: [PATCH 06/12] Fail test on compilation errors --- tooling/ast_fuzzer/tests/mono.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs index b784676c063..a769334b5ba 100644 --- a/tooling/ast_fuzzer/tests/mono.rs +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -12,6 +12,7 @@ use arbtest::arbtest; use nargo::parse_all; use noir_ast_fuzzer::{Config, DisplayAstAsNoir, arb_program}; use noirc_driver::{CompileOptions, file_manager_with_stdlib, prepare_crate}; +use noirc_errors::CustomDiagnostic; use noirc_frontend::{ hir::Context, monomorphization::{ast::Program, monomorphize}, @@ -42,9 +43,9 @@ fn arb_ast_roundtrip() { }; let program1 = arb_program(u, config)?; let src1 = format!("{}", DisplayAstAsNoir(&program1)); - println!("{src1}"); - let Some(program2) = monomorphize_snippet(src1.clone()) else { return Ok(()) }; - println!("{program2}"); + let program2 = monomorphize_snippet(src1.clone()).unwrap_or_else(|errors| { + panic!("the program did not compile:\n{src1}\n\n{errors:?}"); + }); let src2 = format!("{}", DisplayAstAsNoir(&program2)); similar_asserts::assert_eq!(sanitize(&src1), sanitize(&src2)); Ok(()) @@ -60,7 +61,7 @@ fn arb_ast_roundtrip() { prop.run(); } -fn monomorphize_snippet(source: String) -> Option { +fn monomorphize_snippet(source: String) -> Result> { let root = Path::new(""); let file_name = Path::new("main.nr"); let mut file_manager = file_manager_with_stdlib(root); @@ -74,18 +75,13 @@ fn monomorphize_snippet(source: String) -> Option { context.disable_comptime_printing(); - // Things that generate compile-time rejections are possible, e.g. `--128`. - // In other tests we handle those already, so here we can just ignore them. - // We could also avoid generating them, but it would require avoiding all potential overflowing ops, or negative literals. - if let Err(_) = noirc_driver::check_crate(&mut context, crate_id, &CompileOptions::default()) { - return None; - } + let _ = noirc_driver::check_crate(&mut context, crate_id, &CompileOptions::default())?; let main_id = context.get_main_function(&crate_id).expect("get_main_function"); let program = monomorphize(main_id, &mut context.def_interner, false).expect("monomorphize"); - Some(program) + Ok(program) } fn sanitize(src: &str) -> String { From 541327eada86309662833a11bbcebcd9ebd3c08c Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 17:02:36 +0100 Subject: [PATCH 07/12] Don't print () around negation --- .../noirc_frontend/src/monomorphization/printer.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 62d037e493f..1c037eb63b4 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -335,12 +335,20 @@ impl AstPrinter { unary: &super::ast::Unary, f: &mut Formatter, ) -> Result<(), std::fmt::Error> { - write!(f, "({}", unary.operator)?; + // "(-1)" parses back as the literal -1, so if we are printing with the intention of parsing, omit the (), to avoid ambiguity. + let print_parens = self.show_id || !matches!(unary.operator, UnaryOp::Minus); + if print_parens { + write!(f, "(")?; + } + write!(f, "{}", unary.operator)?; if matches!(&unary.operator, UnaryOp::Reference { mutable: true }) { write!(f, " ")?; } self.print_expr(&unary.rhs, f)?; - write!(f, ")") + if print_parens { + write!(f, ")")?; + } + Ok(()) } fn print_binary( From 0986ebe393a0767b4e95bbf43bfde23d1f98170b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 17:03:25 +0100 Subject: [PATCH 08/12] Split functions before comparing --- tooling/ast_fuzzer/tests/mono.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs index a769334b5ba..8ee0d61b591 100644 --- a/tooling/ast_fuzzer/tests/mono.rs +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -47,7 +47,7 @@ fn arb_ast_roundtrip() { panic!("the program did not compile:\n{src1}\n\n{errors:?}"); }); let src2 = format!("{}", DisplayAstAsNoir(&program2)); - similar_asserts::assert_eq!(sanitize(&src1), sanitize(&src2)); + compare_sources(&src1, &src2); Ok(()) }) .budget(Duration::from_secs(10)) @@ -86,5 +86,27 @@ fn monomorphize_snippet(source: String) -> Result fn sanitize(src: &str) -> String { // Sometimes `;` is removed, or duplicated. - src.replace(";", "") + src.replace(";", "").replace("{}", "()") +} + +fn split_functions(src: &str) -> Vec { + // Split along the closing brace of the functions. + let sep = "\n}"; + src.split(sep).into_iter().map(|f| format!("{f}{sep}")).collect() +} + +fn compare_sources(src1: &str, src2: &str) { + let prepare = |src| { + let mut v = split_functions(src); + let main = v.remove(0); + // Unused globals are not rendered. Ignore all globals. + let (_globals, main) = main.split_once("unconstrained fn main").unwrap(); + let main = format!("unconstrained fn main {main}"); + // Sort the other functions alphabetically. + v.sort(); + sanitize(&format!("{main}\n{}", v.join("\n"))) + }; + let src1 = prepare(src1); + let src2 = prepare(src2); + similar_asserts::assert_eq!(src1, src2); } From a5154a707078d1d46980bac1047f0780ac8ba169 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 1 Jul 2025 17:12:15 +0100 Subject: [PATCH 09/12] Avoid large numbers --- tooling/ast_fuzzer/tests/mono.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs index 8ee0d61b591..7ca107e578f 100644 --- a/tooling/ast_fuzzer/tests/mono.rs +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -36,6 +36,8 @@ fn arb_ast_roundtrip() { avoid_print: true, // Negative literals can cause problems: --128_i8 is a compile error; --100_i32 is printed back as 100_i32. avoid_negative_int_literals: true, + // Large ints are rejected in for loops, unless we use suffixes. + avoid_large_int_literals: true, // The formatting of `unsafe { ` becomes `{ unsafe {` with extra line breaks. // Let's stick to just Brillig so there is no need for `unsafe` at all. force_brillig: true, @@ -86,7 +88,7 @@ fn monomorphize_snippet(source: String) -> Result fn sanitize(src: &str) -> String { // Sometimes `;` is removed, or duplicated. - src.replace(";", "").replace("{}", "()") + src.replace(";", "").replace("{}", "()").replace("--", "") } fn split_functions(src: &str) -> Vec { From f930568f7b4a2cba039f249d6f55ac3b792695a8 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 2 Jul 2025 09:06:16 +0100 Subject: [PATCH 10/12] Break out the index unless it's a literal or an ident --- .../ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs | 16 ++++++++++++++-- tooling/ast_fuzzer/src/program/expr.rs | 12 ------------ tooling/ast_fuzzer/src/program/func.rs | 9 ++++++++- 3 files changed, 22 insertions(+), 15 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 49e3e8eb4ba..97bb6d176a1 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs @@ -492,7 +492,7 @@ mod rules { operator: BinaryOpKind::Add | BinaryOpKind::Multiply, .. }) - ) && !expr::has_side_effect(expr) + ) && !super::helpers::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) - && !expr::has_side_effect(expr) + && !super::helpers::has_side_effect(expr) && !expr::exists(expr, |expr| { matches!( expr, @@ -790,6 +790,18 @@ mod helpers { }, ); } + + /// 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 + ) + }) + } } #[cfg(test)] diff --git a/tooling/ast_fuzzer/src/program/expr.rs b/tooling/ast_fuzzer/src/program/expr.rs index ffa092af51f..e72a16dcba0 100644 --- a/tooling/ast_fuzzer/src/program/expr.rs +++ b/tooling/ast_fuzzer/src/program/expr.rs @@ -384,18 +384,6 @@ 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 9b4ec6cb26f..0d4168c0331 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -1030,7 +1030,14 @@ impl<'a> FunctionContext<'a> { // 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) { + // In the compiler the `Elaborator::fresh_definition_for_lvalue_index` decides. + let needs_prefix = !matches!(idx, Expression::Ident(_) | Expression::Literal(_)); + + // We could consider 2D arrays here and pick indexes for sub-arrays. + // That is, even if we have `a: [[T; N]; M]` it currently only assigns + // to `a[i]`, not `a[i][j]`. Should we do that, instead of a single prefix, + // we would have to collect all index assignments in a list first. + let (idx, prefix) = if needs_prefix { let (idx, idx_ident) = self.let_var_and_ident(false, types::U32, idx, false, idx_dyn); (Expression::Ident(idx_ident), Some(idx)) From 408642dd3fe110079c6af03d72b4311c3df4ae5f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 2 Jul 2025 09:22:04 +0100 Subject: [PATCH 11/12] Move has_side_effect back where it was --- .../fuzz/src/targets/orig_vs_morph.rs | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 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 97bb6d176a1..47083b16a1c 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs @@ -297,7 +297,10 @@ fn is_special_call(call: &Call) -> bool { /// Metamorphic transformation rules. mod rules { - use crate::targets::orig_vs_morph::{VariableContext, helpers::reassign_ids}; + use crate::targets::orig_vs_morph::{ + VariableContext, + helpers::{has_side_effect, reassign_ids}, + }; use super::helpers::gen_expr; use acir::{AcirField, FieldElement}; @@ -492,7 +495,7 @@ mod rules { operator: BinaryOpKind::Add | BinaryOpKind::Multiply, .. }) - ) && !super::helpers::has_side_effect(expr) + ) && !has_side_effect(expr) }, |_u, _locals, expr| { let Expression::Binary(binary) = expr else { @@ -574,7 +577,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) - && !super::helpers::has_side_effect(expr) + && !has_side_effect(expr) && !expr::exists(expr, |expr| { matches!( expr, @@ -624,6 +627,18 @@ 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, @@ -790,18 +805,6 @@ mod helpers { }, ); } - - /// 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 - ) - }) - } } #[cfg(test)] From 564d00fcaf59a84b7c7bf285626e8dc24ae72755 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Wed, 2 Jul 2025 09:24:25 +0100 Subject: [PATCH 12/12] Clippy --- compiler/noirc_frontend/src/monomorphization/printer.rs | 6 ++---- tooling/ast_fuzzer/tests/mono.rs | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 1c037eb63b4..09cc7eb8a69 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -505,10 +505,8 @@ impl AstPrinter { _ => (false, false), }; // If this is the print oracle and we want to display it as Noir, we need to use the stdlib. - if print_oracle && self.show_print_as_std { - if self.print_println(&call.arguments, f)? { - return Ok(()); - } + if print_oracle && self.show_print_as_std && self.print_println(&call.arguments, f)? { + return Ok(()); } if print_unsafe { write!(f, "unsafe {{ ")?; diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs index 7ca107e578f..4a0322e1445 100644 --- a/tooling/ast_fuzzer/tests/mono.rs +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -94,7 +94,7 @@ fn sanitize(src: &str) -> String { fn split_functions(src: &str) -> Vec { // Split along the closing brace of the functions. let sep = "\n}"; - src.split(sep).into_iter().map(|f| format!("{f}{sep}")).collect() + src.split(sep).map(|f| format!("{f}{sep}")).collect() } fn compare_sources(src1: &str, src2: &str) {