chore(fuzz): Test AST print-and-parse roundtrip

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)

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<bool, std::fmt::Error> {
         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 {

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,

tooling/ast_fuzzer/src/program/expr.rs

@@ -97,7 +97,7 @@ pub fn gen_literal(u: &mut Unstructured, typ: &Type) -> arbitrary::Result<Expres
         Type::Reference(typ, mutable) => {
             // 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;

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))

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<u64> {
+    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<Program, Vec<CustomDiagnostic>> {
+    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<String> {
+    // 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);
+}