Merge branch 'master' into michaeljklein/global-as-type

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

compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs

@@ -13,7 +13,7 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 use crate::ssa::{
     ir::{
         basic_block::BasicBlockId,
-        function::{Function, RuntimeType},
+        function::Function,
         function_inserter::FunctionInserter,
         instruction::{Instruction, InstructionId},
         types::Type,
@@ -27,12 +27,7 @@ use super::unrolling::{Loop, Loops};
 impl Ssa {
     #[tracing::instrument(level = "trace", skip(self))]
     pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa {
-        let brillig_functions = self
-            .functions
-            .iter_mut()
-            .filter(|(_, func)| matches!(func.runtime(), RuntimeType::Brillig(_)));
-
-        for (_, function) in brillig_functions {
+        for function in self.functions.values_mut() {
             function.loop_invariant_code_motion();
         }
 
@@ -63,6 +58,7 @@ impl Loops {
         }
 
         context.map_dependent_instructions();
+        context.inserter.map_data_bus_in_place();
     }
 }
 

docs/docs/getting_started/noir_installation.md

@@ -93,7 +93,7 @@ step 2: Follow the [Noirup instructions](#installing-noirup).
 
 ## Setting up shell completions
 
-Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions).
+Once `nargo` is installed, you can [set up shell completions for it](setting_up_shell_completions.md).
 
 ## Uninstalling Nargo
 

test_programs/execution_success/diamond_deps_0/Prover.toml

@@ -1,3 +1,3 @@
 x = 1
 y = 1
-return = 5
+return = 7

test_programs/execution_success/return_twice/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "return_twice"
+version = "0.1.0"
+type = "bin"
+authors = [""]
+
+[dependencies]

test_programs/execution_success/return_twice/Prover.toml

@@ -0,0 +1,2 @@
+return = [100, 100]
+in0 = 10

test_programs/execution_success/return_twice/src/main.nr

@@ -0,0 +1,5 @@
+pub fn main(in0: Field) -> pub (Field, Field) {
+    let out0 = (in0 * in0);
+    let out1 = (in0 * in0);
+    (out0, out1)
+}

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 =
@@ -115,14 +120,20 @@ pub fn run_test<B: BlackBoxFunctionSolver<FieldElement>>(
                 {
                     use acvm::acir::circuit::Program;
                     use noir_fuzzer::FuzzedExecutor;
+                    use proptest::test_runner::Config;
                     use proptest::test_runner::TestRunner;
-                    let runner = TestRunner::default();
+
+                    let runner =
+                        TestRunner::new(Config { failure_persistence: None, ..Config::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 +143,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 +199,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 +221,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 {

tooling/nargo_cli/src/cli/execute_cmd.rs

@@ -64,8 +64,9 @@ pub(crate) fn run(args: ExecuteCommand, config: NargoConfig) -> Result<(), CliEr
         let program_artifact_path = workspace.package_build_path(package);
         let program: CompiledProgram =
             read_program_from_file(program_artifact_path.clone())?.into();
+        let abi = program.abi.clone();
 
-        let (return_value, witness_stack) = execute_program_and_decode(
+        let results = execute_program_and_decode(
             program,
             package,
             &args.prover_name,
@@ -75,14 +76,27 @@ pub(crate) fn run(args: ExecuteCommand, config: NargoConfig) -> Result<(), CliEr
         )?;
 
         println!("[{}] Circuit witness successfully solved", package.name);
-        if let Some(return_value) = return_value {
+        if let Some(ref return_value) = results.actual_return {
             println!("[{}] Circuit output: {return_value:?}", package.name);
         }
 
         let package_name = package.name.clone().into();
         let witness_name = args.witness_name.as_ref().unwrap_or(&package_name);
-        let witness_path = save_witness_to_dir(witness_stack, witness_name, target_dir)?;
+        let witness_path = save_witness_to_dir(results.witness_stack, witness_name, target_dir)?;
         println!("[{}] Witness saved to {}", package.name, witness_path.display());
+
+        // Sanity checks on the return value after the witness has been saved, so it can be inspected if necessary.
+        if let Some(expected) = results.expected_return {
+            if results.actual_return.as_ref() != Some(&expected) {
+                return Err(CliError::UnexpectedReturn { expected, actual: results.actual_return });
+            }
+        }
+        // We can expect that if the circuit returns something, it should be non-empty after execution.
+        if let Some(ref expected) = abi.return_type {
+            if results.actual_return.is_none() {
+                return Err(CliError::MissingReturn { expected: expected.clone() });
+            }
+        }
     }
     Ok(())
 }
@@ -94,18 +108,24 @@ fn execute_program_and_decode(
     foreign_call_resolver_url: Option<&str>,
     root_path: Option<PathBuf>,
     package_name: Option<String>,
-) -> Result<(Option<InputValue>, WitnessStack<FieldElement>), CliError> {
+) -> Result<ExecutionResults, CliError> {
     // Parse the initial witness values from Prover.toml
-    let (inputs_map, _) =
+    let (inputs_map, expected_return) =
         read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &program.abi)?;
     let witness_stack =
         execute_program(&program, &inputs_map, foreign_call_resolver_url, root_path, package_name)?;
     // Get the entry point witness for the ABI
     let main_witness =
         &witness_stack.peek().expect("Should have at least one witness on the stack").witness;
-    let (_, return_value) = program.abi.decode(main_witness)?;
+    let (_, actual_return) = program.abi.decode(main_witness)?;
+
+    Ok(ExecutionResults { expected_return, actual_return, witness_stack })
+}
 
-    Ok((return_value, witness_stack))
+struct ExecutionResults {
+    expected_return: Option<InputValue>,
+    actual_return: Option<InputValue>,
+    witness_stack: WitnessStack<FieldElement>,
 }
 
 pub(crate) fn execute_program(

tooling/nargo_cli/src/errors.rs

@@ -2,7 +2,11 @@ use acvm::{acir::native_types::WitnessStackError, FieldElement};
 use nargo::{errors::CompileError, NargoError};
 use nargo_toml::ManifestError;
 use noir_debugger::errors::DapError;
-use noirc_abi::errors::{AbiError, InputParserError};
+use noirc_abi::{
+    errors::{AbiError, InputParserError},
+    input_parser::InputValue,
+    AbiReturnType,
+};
 use std::path::PathBuf;
 use thiserror::Error;
 
@@ -32,6 +36,7 @@ pub(crate) enum FilesystemError {
 pub(crate) enum CliError {
     #[error("{0}")]
     Generic(String),
+
     #[error("Error: destination {} already exists", .0.display())]
     DestinationAlreadyExists(PathBuf),
 
@@ -63,4 +68,10 @@ pub(crate) enum CliError {
     /// Error from the compilation pipeline
     #[error(transparent)]
     CompileError(#[from] CompileError),
+
+    #[error("Unexpected return value: expected {expected:?}; got {actual:?}")]
+    UnexpectedReturn { expected: InputValue, actual: Option<InputValue> },
+
+    #[error("Missing return witnesses; expected {expected:?}")]
+    MissingReturn { expected: AbiReturnType },
 }