diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index 43562103603..4c81179718b 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -14,10 +14,10 @@ pub enum InterpreterError { /// These errors are all the result from malformed input SSA #[error("{0}")] Internal(InternalError), - #[error("constrain {lhs_id} == {rhs_id} failed:\n {lhs} != {rhs}")] - ConstrainEqFailed { lhs: String, lhs_id: ValueId, rhs: String, rhs_id: ValueId }, - #[error("constrain {lhs_id} != {rhs_id} failed:\n {lhs} == {rhs}")] - ConstrainNeFailed { lhs: String, lhs_id: ValueId, rhs: String, rhs_id: ValueId }, + #[error("constrain {lhs_id} == {rhs_id}{msg} failed:\n {lhs} != {rhs}")] + ConstrainEqFailed { lhs: String, lhs_id: ValueId, rhs: String, rhs_id: ValueId, msg: String }, + #[error("constrain {lhs_id} != {rhs_id}{msg} failed:\n {lhs} == {rhs}")] + ConstrainNeFailed { lhs: String, lhs_id: ValueId, rhs: String, rhs_id: ValueId, msg: String }, #[error("static_assert `{condition}` failed: {message}")] StaticAssertFailed { condition: ValueId, message: String }, #[error( diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 187636dde60..9959a3473ee 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -5,7 +5,9 @@ use super::{ ir::{ dfg::DataFlowGraph, function::{Function, FunctionId, RuntimeType}, - instruction::{ArrayOffset, Binary, BinaryOp, Instruction, TerminatorInstruction}, + instruction::{ + ArrayOffset, Binary, BinaryOp, ConstrainError, Instruction, TerminatorInstruction, + }, types::Type, value::ValueId, }, @@ -413,7 +415,18 @@ impl<'ssa> Interpreter<'ssa> { let rhs = rhs.to_string(); let lhs_id = *lhs_id; let rhs_id = *rhs_id; - return Err(InterpreterError::ConstrainEqFailed { lhs, lhs_id, rhs, rhs_id }); + let msg = if let Some(ConstrainError::StaticString(msg)) = constrain_error { + format!(", \"{msg}\"") + } else { + "".to_string() + }; + return Err(InterpreterError::ConstrainEqFailed { + lhs, + lhs_id, + rhs, + rhs_id, + msg, + }); } Ok(()) } @@ -425,7 +438,18 @@ impl<'ssa> Interpreter<'ssa> { let rhs = rhs.to_string(); let lhs_id = *lhs_id; let rhs_id = *rhs_id; - return Err(InterpreterError::ConstrainNeFailed { lhs, lhs_id, rhs, rhs_id }); + let msg = if let Some(ConstrainError::StaticString(msg)) = constrain_error { + format!(", \"{msg}\"") + } else { + "".to_string() + }; + return Err(InterpreterError::ConstrainNeFailed { + lhs, + lhs_id, + rhs, + rhs_id, + msg, + }); } Ok(()) } diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 3f2067d0340..8e4d877f864 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -183,9 +183,15 @@ impl AstPrinter { )?; self.print_expr(&let_expr.expression, f) } - Expression::Constrain(expr, ..) => { - write!(f, "constrain ")?; - self.print_expr(expr, f) + Expression::Constrain(expr, _, payload) => { + write!(f, "assert(")?; + self.print_expr(expr, f)?; + if let Some(payload) = payload { + write!(f, ", ")?; + self.print_expr(&payload.as_ref().0, f)?; + } + write!(f, ")")?; + Ok(()) } Expression::Assign(assign) => { self.print_lvalue(&assign.lvalue, f)?; diff --git a/compiler/noirc_frontend/src/ownership/tests.rs b/compiler/noirc_frontend/src/ownership/tests.rs index 1ba46e81053..8ba497404f5 100644 --- a/compiler/noirc_frontend/src/ownership/tests.rs +++ b/compiler/noirc_frontend/src/ownership/tests.rs @@ -33,10 +33,10 @@ fn last_use_in_if_branches() { unconstrained fn main$f0(d$l0: [Field; 2]) -> () { if (len$f1(d$l0.clone()) == 2) { if (len$f1(d$l0.clone()) == 2) { - constrain eq$f2(d$l0, [5, 6]); + assert(eq$f2(d$l0, [5, 6])); } } else { - constrain eq$f2(d$l0, [5, 6]); + assert(eq$f2(d$l0, [5, 6])); } } unconstrained fn len$f1(arr$l1: [Field; 2]) -> u32 { diff --git a/tooling/ast_fuzzer/fuzz/src/targets/comptime_vs_brillig.rs b/tooling/ast_fuzzer/fuzz/src/targets/comptime_vs_brillig.rs index 552d8c39d0a..e21d56f17ec 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/comptime_vs_brillig.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/comptime_vs_brillig.rs @@ -14,6 +14,7 @@ use noir_ast_fuzzer::rewrite::change_all_functions_into_unconstrained; pub fn fuzz(u: &mut Unstructured) -> eyre::Result<()> { let config = Config { // We created enough bug tickets due to overflows + // TODO(#8817): Comptime code fails to compile if there is an overflow, which causes a panic. avoid_overflow: true, // also with negative values avoid_negative_int_literals: true, @@ -23,6 +24,8 @@ pub fn fuzz(u: &mut Unstructured) -> eyre::Result<()> { avoid_large_int_literals: true, // Avoid break/continue avoid_loop_control: true, + // TODO(#8817): Comptime code fails to compile if there is an assertion failure, which causes a panic. + avoid_constrain: true, // Has to only use expressions valid in comptime comptime_friendly: true, // Force brillig 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 71199df03a3..7ff4f71015c 100644 --- a/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs +++ b/tooling/ast_fuzzer/fuzz/src/targets/orig_vs_morph.rs @@ -1,16 +1,16 @@ //! Perform random equivalence mutations on the AST and check that the //! execution result does not change, a.k.a. metamorphic testing. -use std::collections::{HashSet, VecDeque}; - use crate::{compare_results_compiled, create_ssa_or_die, default_ssa_options}; use arbitrary::{Arbitrary, Unstructured}; use color_eyre::eyre; use noir_ast_fuzzer::compare::{CompareMorph, CompareOptions}; +use noir_ast_fuzzer::visitor; use noir_ast_fuzzer::{Config, visitor::visit_expr_mut}; -use noir_ast_fuzzer::{expr, visitor}; use noirc_frontend::ast::UnaryOp; -use noirc_frontend::monomorphization::ast::{Expression, FuncId, Function, Program, Unary}; +use noirc_frontend::monomorphization::ast::{ + Call, Definition, Expression, Function, Ident, Program, Unary, +}; pub fn fuzz(u: &mut Unstructured) -> eyre::Result<()> { let rules = rules::all(); @@ -42,9 +42,8 @@ fn rewrite_program( rules: &[rules::Rule], max_rewrites: usize, ) { - let reachable = reachable_functions(program); for func in program.functions.iter_mut() { - if func.name.ends_with("_proxy") || !reachable.contains(&func.id) { + if func.name.ends_with("_proxy") { continue; } rewrite_function(u, func, rules, max_rewrites); @@ -92,9 +91,11 @@ impl MorphContext<'_> { } match expr { Expression::For(for_) => { + // Separate context for just the ranges. let range_ctx = rules::Context { is_in_range: true, ..*ctx }; self.rewrite_expr(&range_ctx, u, &mut for_.start_range); self.rewrite_expr(&range_ctx, u, &mut for_.end_range); + // Original context for the body. self.rewrite_expr(ctx, u, &mut for_.block); // No need to visit children, we just visited them. false @@ -106,6 +107,14 @@ impl MorphContext<'_> { self.rewrite_expr(&ctx, u, &mut unary.rhs); false } + Expression::Call(call) if is_special_call(call) => { + let ctx = rules::Context { is_in_special_call: true, ..*ctx }; + for arg in call.arguments.iter_mut() { + self.rewrite_expr(&ctx, u, arg); + } + false + } + // The rest can just have the rules applied on them, using the same context. _ => { for rule in self.rules { match self.try_apply_rule(ctx, u, expr, rule) { @@ -174,29 +183,15 @@ fn estimate_applicable_rules( count } -/// Collect the functions reachable from `main`. -/// -/// We don't want to waste our time morphing functions that won't be called. -/// -/// It would be nice if they were removed during AST generation, but if we -/// remove an item from `Programs::functions`, the calls made to them would -/// need to be updated according to their new position in the vector. -fn reachable_functions(program: &Program) -> HashSet { - let mut reachable = HashSet::new(); - let mut queue = VecDeque::new(); - - queue.push_back(Program::main_id()); - - while let Some(func_id) = queue.pop_front() { - if !reachable.insert(func_id) { - continue; - } - let func = &program.functions[func_id.0 as usize]; - let callees = expr::callees(&func.body); - queue.extend(callees); - } - - reachable +/// Check if we are calling an oracle or builtin function. +fn is_special_call(call: &Call) -> bool { + matches!( + call.func.as_ref(), + Expression::Ident(Ident { + definition: Definition::Oracle(_) | Definition::Builtin(_) | Definition::LowLevel(_), + .. + }) + ) } /// Metamorphic transformation rules. @@ -212,10 +207,12 @@ mod rules { pub struct Context { /// Is the function we're rewriting unconstrained? pub unconstrained: bool, - /// Are we rewriting an expression which is a range of a `for` loop? + /// Are we rewriting an expression which is a `start` or `end` of a `for` loop? pub is_in_range: bool, /// Are we in an expression that we're just taking a mutable reference to? pub is_in_ref_mut: bool, + /// Are we processing the arguments of an non-user function call, such as an oracle or built-in? + pub is_in_special_call: bool, } /// Check if the rule can be applied on an expression. @@ -301,6 +298,11 @@ mod rules { if ctx.is_in_ref_mut { return false; } + // We don't want to mess with the arguments of a `println`, because the printer assumes they are bool literals. + // Similarly a `constrain` call is expected to have a single boolean expression. + if ctx.is_in_special_call { + return false; + } // We can apply boolean rule on anything that returns a bool, // unless the expression can have a side effect, which we don't want to duplicate. if let Some(typ) = expr.return_type() { diff --git a/tooling/ast_fuzzer/src/compare/interpreted.rs b/tooling/ast_fuzzer/src/compare/interpreted.rs index ebb474d8a1b..6f6c57fd7d7 100644 --- a/tooling/ast_fuzzer/src/compare/interpreted.rs +++ b/tooling/ast_fuzzer/src/compare/interpreted.rs @@ -134,6 +134,21 @@ impl Comparable for ssa::interpreter::errors::InterpreterError { } details_or_sanitize(i1) == details_or_sanitize(i2) } + ( + ConstrainEqFailed { lhs: _lhs1, rhs: _rhs1, msg: msg1, .. }, + ConstrainEqFailed { lhs: _lhs2, rhs: _rhs2, msg: msg2, .. }, + ) + | ( + ConstrainNeFailed { lhs: _lhs1, rhs: _rhs1, msg: msg1, .. }, + ConstrainNeFailed { lhs: _lhs2, rhs: _rhs2, msg: msg2, .. }, + ) => { + // The sides might be flipped: `u1 0 == u1 1` vs `u1 1 == u1 0`. + // Unfortunately we often see the type change as well, which makes it more difficult to compare, + // for example `Field 313339671284855045676773137498590239475 != Field 0` vs `u128 313339671284855045676773137498590239475 != u128 0`, + // or `i64 -1615928006 != i64 -5568658583620095790` vs `u64 18446744072093623610 != u64 12878085490089455826` + // (lhs1 == lhs2 && rhs1 == rhs2 || lhs1 == rhs2 && rhs1 == lhs2) && msg1 == msg2 + msg1 == msg2 + } (e1, e2) => { // The format strings contain SSA instructions, // where the only difference might be the value ID. diff --git a/tooling/ast_fuzzer/src/compare/mod.rs b/tooling/ast_fuzzer/src/compare/mod.rs index 88a7d2fca28..fc6584580b1 100644 --- a/tooling/ast_fuzzer/src/compare/mod.rs +++ b/tooling/ast_fuzzer/src/compare/mod.rs @@ -108,7 +108,7 @@ where // Both programs failed the same way. Ok(None) } else { - bail!("both programs failed: {e1} vs {e2}\n{e1:?}\n{e2:?}") + bail!("both programs failed:\n{e1}\n!=\n{e2}\n\n{e1:?}\n{e2:?}") } } CompareResult::LeftFailed(e, _) => { diff --git a/tooling/ast_fuzzer/src/lib.rs b/tooling/ast_fuzzer/src/lib.rs index 489869731b4..b10c2220c0c 100644 --- a/tooling/ast_fuzzer/src/lib.rs +++ b/tooling/ast_fuzzer/src/lib.rs @@ -63,6 +63,8 @@ pub struct Config { pub avoid_loop_control: bool, /// Avoid using function pointers in parameters. pub avoid_lambdas: bool, + /// Avoid using constrain statements. + pub avoid_constrain: bool, /// Only use comptime friendly expressions. pub comptime_friendly: bool, } @@ -85,6 +87,7 @@ impl Default for Config { ("for", 22), ("let", 25), ("call", 5), + ("constrain", 5), ]); let stmt_freqs_brillig = Freqs::new(&[ ("drop", 0), @@ -98,6 +101,7 @@ impl Default for Config { ("let", 20), ("call", 5), ("print", 15), + ("constrain", 10), ]); Self { max_globals: 3, @@ -122,6 +126,7 @@ impl Default for Config { avoid_negative_int_literals: false, avoid_loop_control: false, avoid_lambdas: false, + avoid_constrain: false, comptime_friendly: false, } } diff --git a/tooling/ast_fuzzer/src/program/func.rs b/tooling/ast_fuzzer/src/program/func.rs index 251f2ae73df..c4f9433a6d9 100644 --- a/tooling/ast_fuzzer/src/program/func.rs +++ b/tooling/ast_fuzzer/src/program/func.rs @@ -30,6 +30,9 @@ use super::{ types, }; +/// Use random strings to identify constraints. +const CONSTRAIN_MSG_TYPE: Type = Type::String(3); + /// Something akin to a forward declaration of a function, capturing the details required to: /// 1. call the function from the other function bodies /// 2. generate the final HIR function signature @@ -693,7 +696,6 @@ impl<'a> FunctionContext<'a> { Freq::new(u, &self.ctx.config.stmt_freqs_acir)? }; // TODO(#7926): Match - // TODO(#7932): Constrain // Start with `drop`, it doesn't need to be frequent even if others are disabled. if freq.enabled("drop") { @@ -702,6 +704,13 @@ impl<'a> FunctionContext<'a> { } } + // We don't want constraints to get too frequent, as it could dominate all outcome. + if freq.enabled_when("constrain", !self.ctx.config.avoid_constrain) { + if let Some(e) = self.gen_constrain(u)? { + return Ok(e); + } + } + // Require a positive budget, so that we have some for the block itself and its contents. if freq.enabled_when("if", self.budget > 1) { return self.gen_if(u, &Type::Unit, self.max_depth(), Flags::TOP); @@ -906,6 +915,25 @@ impl<'a> FunctionContext<'a> { Ok(Some(call)) } + /// Generate a `constrain` statement, if there is some local variable we can do it on. + /// + /// Arbitrary constraints are very likely to fail, so we don't want too many of them, + /// otherwise they might mask disagreements in return values. + fn gen_constrain(&mut self, u: &mut Unstructured) -> arbitrary::Result> { + // Generate a condition that evaluates to bool. + let Some(cond) = self.gen_binary(u, &Type::Bool, self.max_depth())? else { + return Ok(None); + }; + // Generate a unique message for the assertion, so it's easy to find which one failed. + let msg = expr::gen_literal(u, &CONSTRAIN_MSG_TYPE)?; + let cons = Expression::Constrain( + Box::new(cond), + Location::dummy(), + Some(Box::new((msg, types::to_hir_type(&CONSTRAIN_MSG_TYPE)))), + ); + Ok(Some(cons)) + } + /// Generate an if-then-else statement or expression. fn gen_if( &mut self, diff --git a/tooling/ast_fuzzer/tests/calibration.rs b/tooling/ast_fuzzer/tests/calibration.rs index 832aebb7b4c..58d3c363c94 100644 --- a/tooling/ast_fuzzer/tests/calibration.rs +++ b/tooling/ast_fuzzer/tests/calibration.rs @@ -8,7 +8,7 @@ //! ```shell //! cargo test -p noir_ast_fuzzer --test calibration -- --nocapture //! ``` -use std::collections::BTreeMap; +use std::{collections::BTreeMap, ops::RangeInclusive}; use arbtest::arbtest; use noir_ast_fuzzer::{Config, arb_program, visitor::visit_expr}; @@ -67,6 +67,13 @@ fn arb_program_freqs_in_expected_range() { keys.iter().map(|key| counts[&unconstrained][group][key] * 100 / total).sum::() }; + let assert_both = |group: &str, key: &str, range: RangeInclusive| { + let a = freq_100(false, group, &[key]); + let b = freq_100(true, group, &[key]); + assert!(range.contains(&a), "ACIR {group}/{key} should be in {range:?}: {a}"); + assert!(range.contains(&b), "Brillig {group}/{key} should be in {range:?}: {b}"); + }; + // Assert relative frequencies let loops_a = freq_100(false, "stmt", &["for"]); let loops_b = freq_100(true, "stmt", &["for", "loop", "while"]); @@ -76,6 +83,8 @@ fn arb_program_freqs_in_expected_range() { assert!(loop_range.contains(&loops_a), "ACIR loops should be ~10: {loops_a}"); assert!(loop_range.contains(&loops_b), "Brillig loops should be ~10: {loops_b}"); assert!(break_b >= loops_b, "Brillig should break out of loops: {break_b} >= {loops_b}"); + + assert_both("stmt", "constrain", 1..=3); } /// Classify the expression into "expr" or "stmt" for frequency settings.