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/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 93776c12767..09cc7eb8a69 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( @@ -497,8 +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 { - return self.print_println(&call.arguments, f); + if print_oracle && self.show_print_as_std && self.print_println(&call.arguments, f)? { + return Ok(()); } if print_unsafe { write!(f, "unsafe {{ ")?; @@ -516,10 +524,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 +547,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 { 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..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, .. }) - ) && !expr::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) - && !expr::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, diff --git a/tooling/ast_fuzzer/src/program/expr.rs b/tooling/ast_fuzzer/src/program/expr.rs index a3c005ba6a0..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. @@ -380,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)) diff --git a/tooling/ast_fuzzer/tests/mono.rs b/tooling/ast_fuzzer/tests/mono.rs new file mode 100644 index 00000000000..4a0322e1445 --- /dev/null +++ b/tooling/ast_fuzzer/tests/mono.rs @@ -0,0 +1,114 @@ +//! 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_errors::CustomDiagnostic; +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, + // 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, + ..Default::default() + }; + let program1 = arb_program(u, config)?; + let src1 = format!("{}", DisplayAstAsNoir(&program1)); + 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)); + compare_sources(&src1, &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) -> Result> { + 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(); + + 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"); + + Ok(program) +} + +fn sanitize(src: &str) -> String { + // Sometimes `;` is removed, or duplicated. + src.replace(";", "").replace("{}", "()").replace("--", "") +} + +fn split_functions(src: &str) -> Vec { + // Split along the closing brace of the functions. + let sep = "\n}"; + src.split(sep).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); +}