fix: optimizer to keep track of changing opcode locations (#6781)

acvm-repo/acvm/src/compiler/mod.rs

@@ -82,19 +82,22 @@ pub fn compile<F: AcirField>(
     acir: Circuit<F>,
     expression_width: ExpressionWidth,
 ) -> (Circuit<F>, AcirTransformationMap) {
+    let acir_opcode_positions = (0..acir.opcodes.len()).collect::<Vec<_>>();
+
     if MAX_OPTIMIZER_PASSES == 0 {
-        let acir_opcode_positions = (0..acir.opcodes.len()).collect::<Vec<_>>();
-        let transformation_map = AcirTransformationMap::new(&acir_opcode_positions);
-        return (acir, transformation_map);
+        return (acir, AcirTransformationMap::new(&acir_opcode_positions));
     }
+
     let mut pass = 0;
     let mut prev_opcodes_hash = fxhash::hash64(&acir.opcodes);
     let mut prev_acir = acir;
+    let mut prev_acir_opcode_positions = acir_opcode_positions;
 
     // For most test programs it would be enough to only loop `transform_internal`,
     // but some of them don't stabilize unless we also repeat the backend agnostic optimizations.
     let (mut acir, acir_opcode_positions) = loop {
-        let (acir, acir_opcode_positions) = optimize_internal(prev_acir);
+        let (acir, acir_opcode_positions) =
+            optimize_internal(prev_acir, prev_acir_opcode_positions);
 
         // Stop if we have already done at least one transform and an extra optimization changed nothing.
         if pass > 0 && prev_opcodes_hash == fxhash::hash64(&acir.opcodes) {
@@ -114,6 +117,7 @@ pub fn compile<F: AcirField>(
         pass += 1;
         prev_acir = acir;
         prev_opcodes_hash = opcodes_hash;
+        prev_acir_opcode_positions = acir_opcode_positions;
     };
 
     let transformation_map = AcirTransformationMap::new(&acir_opcode_positions);

acvm-repo/acvm/src/compiler/optimizers/mod.rs

@@ -21,7 +21,11 @@ use super::{transform_assert_messages, AcirTransformationMap};
 
 /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`].
 pub fn optimize<F: AcirField>(acir: Circuit<F>) -> (Circuit<F>, AcirTransformationMap) {
-    let (mut acir, new_opcode_positions) = optimize_internal(acir);
+    // Track original acir opcode positions throughout the transformation passes of the compilation
+    // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert)
+    let acir_opcode_positions = (0..acir.opcodes.len()).collect();
+
+    let (mut acir, new_opcode_positions) = optimize_internal(acir, acir_opcode_positions);
 
     let transformation_map = AcirTransformationMap::new(&new_opcode_positions);
 
@@ -31,12 +35,13 @@ pub fn optimize<F: AcirField>(acir: Circuit<F>) -> (Circuit<F>, AcirTransformati
 }
 
 /// Applies [`ProofSystemCompiler`][crate::ProofSystemCompiler] independent optimizations to a [`Circuit`].
+///
+/// Accepts an injected `acir_opcode_positions` to allow optimizations to be applied in a loop.
 #[tracing::instrument(level = "trace", name = "optimize_acir" skip(acir))]
-pub(super) fn optimize_internal<F: AcirField>(acir: Circuit<F>) -> (Circuit<F>, Vec<usize>) {
-    // Track original acir opcode positions throughout the transformation passes of the compilation
-    // by applying the modifications done to the circuit opcodes and also to the opcode_positions (delete and insert)
-    let acir_opcode_positions = (0..acir.opcodes.len()).collect();
-
+pub(super) fn optimize_internal<F: AcirField>(
+    acir: Circuit<F>,
+    acir_opcode_positions: Vec<usize>,
+) -> (Circuit<F>, Vec<usize>) {
     if acir.opcodes.len() == 1 && matches!(acir.opcodes[0], Opcode::BrilligCall { .. }) {
         info!("Program is fully unconstrained, skipping optimization pass");
         return (acir, acir_opcode_positions);

test_programs/noir_test_success/assert_message/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "assert_message"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.23.0"
+
+[dependencies]

test_programs/noir_test_success/assert_message/src/main.nr

@@ -0,0 +1,15 @@
+// Have to use inputs otherwise the ACIR gets optimized away due to constants.
+// With the original ACIR the optimizations can rearrange or merge opcodes,
+// which might end up getting out of sync with assertion messages.
+#[test(should_fail_with = "first")]
+fn test_assert_message_preserved_during_optimization(a: Field, b: Field, c: Field) {
+    if (a + b) * (a - b) != c {
+        assert((a + b) * (a - b) == c, "first");
+        assert((a - b) * (a + b) == c, "second");
+        assert((a + b) * (a - b) == c, "third");
+        assert((2 * a + b) * (a - b / 2) == c * c, "fourth");
+    } else {
+        // The fuzzer might have generated a passing test.
+        assert((a + b) * (a - b) != c, "first");
+    }
+}

test_programs/noir_test_success/fuzzer_checks/src/main.nr

@@ -1,6 +1,9 @@
-
 #[test(should_fail_with = "42 is not allowed")]
 fn finds_magic_value(x: u32) {
     let x = x as u64;
-    assert(2 * x != 42, "42 is not allowed");
+    if x == 21 {
+        assert(2 * x != 42, "42 is not allowed");
+    } else {
+        assert(2 * x == 42, "42 is not allowed");
+    }
 }

tooling/nargo/src/ops/test.rs

@@ -9,7 +9,7 @@ use acvm::{
     AcirField, BlackBoxFunctionSolver, FieldElement,
 };
 use noirc_abi::Abi;
-use noirc_driver::{compile_no_check, CompileError, CompileOptions};
+use noirc_driver::{compile_no_check, CompileError, CompileOptions, DEFAULT_EXPRESSION_WIDTH};
 use noirc_errors::{debug_info::DebugInfo, FileDiagnostic};
 use noirc_frontend::hir::{def_map::TestFunction, Context};
 use noirc_printable_type::ForeignCallError;
@@ -29,6 +29,7 @@ use crate::{
 
 use super::execute_program;
 
+#[derive(Debug)]
 pub enum TestStatus {
     Pass,
     Fail { message: String, error_diagnostic: Option<FileDiagnostic> },
@@ -62,6 +63,10 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
 
     match compile_no_check(context, config, test_function.get_id(), None, false) {
         Ok(compiled_program) => {
+            // Do the same optimizations as `compile_cmd`.
+            let target_width = config.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH);
+            let compiled_program = crate::ops::transform_program(compiled_program, target_width);
+
             if test_function_has_no_arguments {
                 // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen,
                 // otherwise constraints involving these expressions will not error.
@@ -81,9 +86,9 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
 
                 let status = test_status_program_compile_pass(
                     test_function,
-                    compiled_program.abi,
-                    compiled_program.debug,
-                    circuit_execution,
+                    &compiled_program.abi,
+                    &compiled_program.debug,
+                    &circuit_execution,
                 );
 
                 let ignore_foreign_call_failures =
@@ -118,11 +123,14 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
                     use proptest::test_runner::TestRunner;
                     let runner = TestRunner::default();
 
+                    let abi = compiled_program.abi.clone();
+                    let debug = compiled_program.debug.clone();
+
                     let executor =
                         |program: &Program<FieldElement>,
                          initial_witness: WitnessMap<FieldElement>|
                          -> Result<WitnessStack<FieldElement>, String> {
-                            execute_program(
+                            let circuit_execution = execute_program(
                                 program,
                                 initial_witness,
                                 blackbox_solver,
@@ -132,9 +140,23 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
                                     root_path.clone(),
                                     package_name.clone(),
                                 ),
-                            )
-                            .map_err(|err| err.to_string())
+                            );
+
+                            let status = test_status_program_compile_pass(
+                                test_function,
+                                &abi,
+                                &debug,
+                                &circuit_execution,
+                            );
+
+                            if let TestStatus::Fail { message, error_diagnostic: _ } = status {
+                                Err(message)
+                            } else {
+                                // The fuzzer doesn't care about the actual result.
+                                Ok(WitnessStack::default())
+                            }
                         };
+
                     let fuzzer = FuzzedExecutor::new(compiled_program.into(), executor, runner);
 
                     let result = fuzzer.fuzz();
@@ -174,9 +196,9 @@ fn test_status_program_compile_fail(err: CompileError, test_function: &TestFunct
 /// passed/failed to determine the test status.
 fn test_status_program_compile_pass(
     test_function: &TestFunction,
-    abi: Abi,
-    debug: Vec<DebugInfo>,
-    circuit_execution: Result<WitnessStack<FieldElement>, NargoError<FieldElement>>,
+    abi: &Abi,
+    debug: &[DebugInfo],
+    circuit_execution: &Result<WitnessStack<FieldElement>, NargoError<FieldElement>>,
 ) -> TestStatus {
     let circuit_execution_err = match circuit_execution {
         // Circuit execution was successful; ie no errors or unsatisfied constraints
@@ -196,7 +218,7 @@ fn test_status_program_compile_pass(
     // If we reach here, then the circuit execution failed.
     //
     // Check if the function should have passed
-    let diagnostic = try_to_diagnose_runtime_error(&circuit_execution_err, &abi, &debug);
+    let diagnostic = try_to_diagnose_runtime_error(circuit_execution_err, abi, debug);
     let test_should_have_passed = !test_function.should_fail();
     if test_should_have_passed {
         return TestStatus::Fail {