From dfd57f1d705d439d199d4d8bf9c9580866b648e1 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 1 Aug 2023 20:53:33 +0000 Subject: [PATCH 1/6] move show-output to occur on execute rather than compilation --- crates/nargo/src/ops/execute.rs | 3 +- crates/nargo/src/ops/foreign_calls.rs | 5 ++- crates/nargo_cli/src/cli/execute_cmd.rs | 2 +- crates/nargo_cli/src/cli/test_cmd.rs | 4 +- .../tests/test_data/strings/src/main.nr | 1 + crates/noirc_driver/src/lib.rs | 8 ++-- crates/noirc_evaluator/src/ssa_refactor.rs | 6 +-- .../acir_gen/acir_ir/acir_variable.rs | 13 ------- .../src/ssa_refactor/acir_gen/mod.rs | 39 +++++-------------- 9 files changed, 25 insertions(+), 56 deletions(-) diff --git a/crates/nargo/src/ops/execute.rs b/crates/nargo/src/ops/execute.rs index 13ea64ed261..2a126443468 100644 --- a/crates/nargo/src/ops/execute.rs +++ b/crates/nargo/src/ops/execute.rs @@ -10,6 +10,7 @@ pub fn execute_circuit( _backend: &B, circuit: Circuit, initial_witness: WitnessMap, + show_output: bool, ) -> Result { let mut acvm = ACVM::new(B::default(), circuit.opcodes, initial_witness); @@ -23,7 +24,7 @@ pub fn execute_circuit( } ACVMStatus::Failure(error) => return Err(error.into()), ACVMStatus::RequiresForeignCall(foreign_call) => { - let foreign_call_result = ForeignCall::execute(&foreign_call)?; + let foreign_call_result = ForeignCall::execute(&foreign_call, show_output)?; acvm.resolve_pending_foreign_call(foreign_call_result); } } diff --git a/crates/nargo/src/ops/foreign_calls.rs b/crates/nargo/src/ops/foreign_calls.rs index 2abc62b1032..4d2f5988e38 100644 --- a/crates/nargo/src/ops/foreign_calls.rs +++ b/crates/nargo/src/ops/foreign_calls.rs @@ -42,11 +42,14 @@ impl ForeignCall { pub(crate) fn execute( foreign_call: &ForeignCallWaitInfo, + show_output: bool, ) -> Result { let foreign_call_name = foreign_call.function.as_str(); match Self::lookup(foreign_call_name) { Some(ForeignCall::Println) => { - Self::execute_println(&foreign_call.inputs)?; + if show_output { + Self::execute_println(&foreign_call.inputs)?; + } Ok(ForeignCallResult { values: vec![] }) } Some(ForeignCall::Sequence) => { diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index eaaea6d4ab3..289cd0714ba 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -123,7 +123,7 @@ pub(crate) fn execute_program( debug_data: Option<(DebugInfo, Context)>, ) -> Result> { let initial_witness = abi.encode(inputs_map, None)?; - let solved_witness_err = nargo::ops::execute_circuit(backend, circuit, initial_witness); + let solved_witness_err = nargo::ops::execute_circuit(backend, circuit, initial_witness, true); match solved_witness_err { Ok(solved_witness) => Ok(solved_witness), Err(err) => { diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index c1aa359e724..734387860ea 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -96,14 +96,14 @@ fn run_test( show_output: bool, config: &CompileOptions, ) -> Result<(), CliError> { - let mut program = compile_no_check(context, show_output, config, main) + let mut program = compile_no_check(context, config, main) .map_err(|_| CliError::Generic(format!("Test '{test_name}' failed to compile")))?; // Note: We could perform this test using the unoptimized ACIR as generated by `compile_no_check`. program.circuit = optimize_circuit(backend, program.circuit).unwrap().0; // Run the backend to ensure the PWG evaluates functions like std::hash::pedersen, // otherwise constraints involving these expressions will not error. - match execute_circuit(backend, program.circuit, WitnessMap::new()) { + match execute_circuit(backend, program.circuit, WitnessMap::new(), show_output) { Ok(_) => Ok(()), Err(error) => { let writer = StandardStream::stderr(ColorChoice::Always); diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index bee2370201c..3f84dfbd934 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -33,6 +33,7 @@ fn test_prints_strings() { std::println(message); std::println("goodbye world"); + assert(false); } #[test] diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index f2537bb88fe..c557b7ea68a 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -192,7 +192,7 @@ pub fn compile_main( } }; - let compiled_program = compile_no_check(context, true, options, main)?; + let compiled_program = compile_no_check(context, options, main)?; if options.print_acir { println!("Compiled ACIR for main (unoptimized):"); @@ -259,7 +259,7 @@ fn compile_contract( let mut errs = Vec::new(); for function_id in &contract.functions { let name = context.function_name(function_id).to_owned(); - let function = match compile_no_check(context, true, options, *function_id) { + let function = match compile_no_check(context, options, *function_id) { Ok(function) => function, Err(err) => { errs.push(err); @@ -296,14 +296,12 @@ fn compile_contract( #[allow(deprecated)] pub fn compile_no_check( context: &Context, - show_output: bool, options: &CompileOptions, main_function: FuncId, ) -> Result { let program = monomorphize(main_function, &context.def_interner); - let (circuit, debug, abi) = - create_circuit(program, options.show_ssa, options.show_brillig, show_output)?; + let (circuit, debug, abi) = create_circuit(program, options.show_ssa, options.show_brillig)?; Ok(CompiledProgram { circuit, debug, abi }) } diff --git a/crates/noirc_evaluator/src/ssa_refactor.rs b/crates/noirc_evaluator/src/ssa_refactor.rs index 6326b45554d..c57bb330b09 100644 --- a/crates/noirc_evaluator/src/ssa_refactor.rs +++ b/crates/noirc_evaluator/src/ssa_refactor.rs @@ -35,7 +35,6 @@ pub mod ssa_gen; /// convert the final SSA into ACIR and return it. pub(crate) fn optimize_into_acir( program: Program, - allow_log_ops: bool, print_ssa_passes: bool, print_brillig_trace: bool, ) -> Result { @@ -63,7 +62,7 @@ pub(crate) fn optimize_into_acir( .dead_instruction_elimination() .print(print_ssa_passes, "After Dead Instruction Elimination:"); } - ssa.into_acir(brillig, abi_distinctness, allow_log_ops) + ssa.into_acir(brillig, abi_distinctness) } /// Compiles the Program into ACIR and applies optimizations to the arithmetic gates @@ -74,7 +73,6 @@ pub fn create_circuit( program: Program, enable_ssa_logging: bool, enable_brillig_logging: bool, - show_output: bool, ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> { let func_sig = program.main_function_signature.clone(); let GeneratedAcir { @@ -84,7 +82,7 @@ pub fn create_circuit( locations, input_witnesses, .. - } = optimize_into_acir(program, show_output, enable_ssa_logging, enable_brillig_logging)?; + } = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; let abi = gen_abi(func_sig, &input_witnesses, return_witnesses.clone()); let public_abi = abi.clone().public_abi(); diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 9177dc9ae6c..d1479ef1f1b 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -827,19 +827,6 @@ impl AcirContext { self.radix_decompose(endian, input_var, two_var, limb_count_var, result_element_type) } - /// Prints the given `AcirVar`s as witnesses. - pub(crate) fn print(&mut self, input: Vec) -> Result<(), RuntimeError> { - let input = Self::flatten_values(input); - - let witnesses = vecmap(input, |acir_var| { - let var_data = &self.vars[&acir_var]; - let expr = var_data.to_expression(); - self.acir_ir.get_or_create_witness(&expr) - }); - self.acir_ir.call_print(witnesses); - Ok(()) - } - /// Flatten the given Vector of AcirValues into a single vector of only variables. /// Each AcirValue::Array in the vector is recursively flattened, so each element /// will flattened into the resulting Vec. E.g. flatten_values([1, [2, 3]) == [1, 2, 3]. diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 5253cb71875..e66dff4995a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -103,10 +103,9 @@ impl Ssa { self, brillig: Brillig, abi_distinctness: AbiDistinctness, - allow_log_ops: bool, ) -> Result { let context = Context::new(); - let mut generated_acir = context.convert_ssa(self, brillig, allow_log_ops)?; + let mut generated_acir = context.convert_ssa(self, brillig)?; match abi_distinctness { AbiDistinctness::Distinct => { @@ -144,15 +143,10 @@ impl Context { } /// Converts SSA into ACIR - fn convert_ssa( - self, - ssa: Ssa, - brillig: Brillig, - allow_log_ops: bool, - ) -> Result { + fn convert_ssa(self, ssa: Ssa, brillig: Brillig) -> Result { let main_func = ssa.main(); match main_func.runtime() { - RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig, allow_log_ops), + RuntimeType::Acir => self.convert_acir_main(main_func, &ssa, brillig), RuntimeType::Brillig => self.convert_brillig_main(main_func, brillig), } } @@ -162,14 +156,13 @@ impl Context { main_func: &Function, ssa: &Ssa, brillig: Brillig, - allow_log_ops: bool, ) -> Result { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?; for instruction_id in entry_block.instructions() { - self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, allow_log_ops)?; + self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig)?; } self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; @@ -294,7 +287,6 @@ impl Context { dfg: &DataFlowGraph, ssa: &Ssa, brillig: &Brillig, - allow_log_ops: bool, ) -> Result<(), RuntimeError> { let instruction = &dfg[instruction_id]; self.acir_context.set_location(dfg.get_location(&instruction_id)); @@ -339,13 +331,8 @@ impl Context { } } Value::Intrinsic(intrinsic) => { - let outputs = self.convert_ssa_intrinsic_call( - *intrinsic, - arguments, - dfg, - allow_log_ops, - result_ids, - )?; + let outputs = self + .convert_ssa_intrinsic_call(*intrinsic, arguments, dfg, result_ids)?; // Issue #1438 causes this check to fail with intrinsics that return 0 // results but the ssa form instead creates 1 unit result value. @@ -936,7 +923,6 @@ impl Context { intrinsic: Intrinsic, arguments: &[ValueId], dfg: &DataFlowGraph, - allow_log_ops: bool, result_ids: &[ValueId], ) -> Result, RuntimeError> { match intrinsic { @@ -966,13 +952,8 @@ impl Context { self.acir_context.bit_decompose(endian, field, bit_size, result_type) } - Intrinsic::Println => { - let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - if allow_log_ops { - self.acir_context.print(inputs)?; - } - Ok(Vec::new()) - } + // TODO(#2115): Remove the println intrinsic as the oracle println is now used instead + Intrinsic::Println => Ok(Vec::new()), Intrinsic::Sort => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); // We flatten the inputs and retrieve the bit_size of the elements @@ -1002,7 +983,7 @@ impl Context { } Intrinsic::ArrayLen => { let len = match self.convert_value(arguments[0], dfg) { - AcirValue::Var(_, _) => unreachable!("Non-array passed to array.len() method"), + AcirValue::Var(_, _) => unreachable!("Non-array passed to array.len() method"), AcirValue::Array(values) => (values.len() as u128).into(), AcirValue::DynamicArray(array) => (array.len as u128).into(), }; @@ -1140,7 +1121,7 @@ mod tests { let ssa = builder.finish(); let context = Context::new(); - let acir = context.convert_ssa(ssa, Brillig::default(), false).unwrap(); + let acir = context.convert_ssa(ssa, Brillig::default()).unwrap(); let expected_opcodes = vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))]; From 42b6d2e11158417c41d0b96b4eed9927b3ede23d Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 1 Aug 2023 20:57:58 +0000 Subject: [PATCH 2/6] remove assert(false) from test --- crates/nargo_cli/tests/test_data/strings/src/main.nr | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index 3f84dfbd934..bee2370201c 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -33,7 +33,6 @@ fn test_prints_strings() { std::println(message); std::println("goodbye world"); - assert(false); } #[test] From 496196f34c31ab298596f30fc1aec847afb9dbbf Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 1 Aug 2023 21:18:32 +0000 Subject: [PATCH 3/6] fix compile err --- crates/nargo_cli/tests/test_data/strings/src/main.nr | 5 ++--- crates/wasm/src/compile.rs | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index bee2370201c..06379f78f84 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -39,9 +39,8 @@ fn test_prints_strings() { fn test_prints_array() { let array = [1, 2, 3, 5, 8]; - // TODO: Printing structs currently not supported - // let s = Test { a: 1, b: 2, c: [3, 4] }; - // std::println(s); + let s = Test { a: 1, b: 2, c: [3, 4] }; + std::println(s); std::println(array); diff --git a/crates/wasm/src/compile.rs b/crates/wasm/src/compile.rs index c940f0ce246..041640d7892 100644 --- a/crates/wasm/src/compile.rs +++ b/crates/wasm/src/compile.rs @@ -107,8 +107,8 @@ pub fn compile(args: JsValue) -> JsValue { ::from_serde(&optimized_contracts).unwrap() } else { let main = context.get_main_function(&crate_id).expect("Could not find main function!"); - let mut compiled_program = compile_no_check(&context, true, &options.compile_options, main) - .expect("Compilation failed"); + let mut compiled_program = + compile_no_check(&context, &options.compile_options, main).expect("Compilation failed"); compiled_program.circuit = optimize_circuit(compiled_program.circuit); From 2aa33e45fddf916759744f77b826a37e468fcd28 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 2 Aug 2023 14:02:04 +0000 Subject: [PATCH 4/6] report compile errors in tests --- crates/nargo_cli/src/cli/test_cmd.rs | 12 +++++++++--- crates/nargo_cli/tests/test_data/strings/src/main.nr | 11 +++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index 734387860ea..39fd67b5cec 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -12,7 +12,10 @@ use crate::{ resolver::resolve_root_manifest, }; -use super::{compile_cmd::optimize_circuit, NargoConfig}; +use super::{ + compile_cmd::optimize_circuit, + NargoConfig, +}; /// Run the tests for this program #[derive(Debug, Clone, Args)] @@ -96,8 +99,11 @@ fn run_test( show_output: bool, config: &CompileOptions, ) -> Result<(), CliError> { - let mut program = compile_no_check(context, config, main) - .map_err(|_| CliError::Generic(format!("Test '{test_name}' failed to compile")))?; + let mut program = compile_no_check(context, config, main).map_err(|err| { + noirc_errors::reporter::report_all(&context.file_manager, &[err], config.deny_warnings); + CliError::Generic(format!("Test '{test_name}' failed to compile")) + })?; + // Note: We could perform this test using the unoptimized ACIR as generated by `compile_no_check`. program.circuit = optimize_circuit(backend, program.circuit).unwrap().0; diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index 06379f78f84..70962902743 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -48,6 +48,17 @@ fn test_prints_array() { std::println(hash); } +fn inner(hex_as_field: Field) { + assert(hex_as_field != 0x41); + let hash = std::hash::pedersen([hex_as_field]); + std::println(hash); +} + +#[test] +fn test_failed_constraint() { + inner(0x41); +} + struct Test { a: Field, b: Field, From 0cdc6de4519d09d47d6b574081de3aa87ccf1267 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 2 Aug 2023 14:29:56 +0000 Subject: [PATCH 5/6] aupdate failing constraint test --- crates/nargo_cli/src/cli/test_cmd.rs | 5 +---- crates/nargo_cli/tests/test_data/strings/src/main.nr | 12 ++++++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index 39fd67b5cec..38b5f4b4474 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -12,10 +12,7 @@ use crate::{ resolver::resolve_root_manifest, }; -use super::{ - compile_cmd::optimize_circuit, - NargoConfig, -}; +use super::{compile_cmd::optimize_circuit, NargoConfig}; /// Run the tests for this program #[derive(Debug, Clone, Args)] diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index 70962902743..2dfe7f08a7c 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -48,15 +48,19 @@ fn test_prints_array() { std::println(hash); } -fn inner(hex_as_field: Field) { +fn failed_constraint(hex_as_field: Field) { + // Note that when this method is called from a test method + // a `Failed constraint` compile error will be caught before this `println` + // is executed as the input will be a constant. + // In order to debug failing constraints with `println` calls `nargo execute` + // should be used instead + std::println(hex_as_field); assert(hex_as_field != 0x41); - let hash = std::hash::pedersen([hex_as_field]); - std::println(hash); } #[test] fn test_failed_constraint() { - inner(0x41); + failed_constraint(0x41); } struct Test { From 1d47ebcbfb92199d5c14058d269a21ff7cb5bde5 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 2 Aug 2023 17:50:22 +0000 Subject: [PATCH 6/6] change comment and link issue --- crates/nargo_cli/tests/test_data/strings/src/main.nr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/nargo_cli/tests/test_data/strings/src/main.nr b/crates/nargo_cli/tests/test_data/strings/src/main.nr index 2dfe7f08a7c..f55a4d744fc 100644 --- a/crates/nargo_cli/tests/test_data/strings/src/main.nr +++ b/crates/nargo_cli/tests/test_data/strings/src/main.nr @@ -49,11 +49,11 @@ fn test_prints_array() { } fn failed_constraint(hex_as_field: Field) { - // Note that when this method is called from a test method + // TODO(#2116): Note that `println` will not work if a failed constraint can be + // evaluated at compile time. + // When this method is called from a test method or with constant values // a `Failed constraint` compile error will be caught before this `println` - // is executed as the input will be a constant. - // In order to debug failing constraints with `println` calls `nargo execute` - // should be used instead + // is executed as the input will be a constant. std::println(hex_as_field); assert(hex_as_field != 0x41); }