diff --git a/crates/nargo/src/errors.rs b/crates/nargo/src/errors.rs index 1c99d27ae42..8ffae3bf643 100644 --- a/crates/nargo/src/errors.rs +++ b/crates/nargo/src/errors.rs @@ -1,4 +1,4 @@ -use acvm::pwg::OpcodeResolutionError; +use acvm::{acir::circuit::OpcodeLocation, pwg::OpcodeResolutionError}; use noirc_printable_type::ForeignCallError; use thiserror::Error; @@ -8,10 +8,20 @@ pub enum NargoError { #[error("Failed to compile circuit")] CompilationError, - /// ACIR circuit solving error + /// ACIR circuit execution error #[error(transparent)] - SolvingError(#[from] OpcodeResolutionError), + ExecutionError(#[from] ExecutionError), + /// Oracle handling error #[error(transparent)] ForeignCallError(#[from] ForeignCallError), } + +#[derive(Debug, Error)] +pub enum ExecutionError { + #[error("Failed assertion: '{}'", .0)] + AssertionFailed(String, OpcodeLocation), + + #[error(transparent)] + SolvingError(#[from] OpcodeResolutionError), +} diff --git a/crates/nargo/src/lib.rs b/crates/nargo/src/lib.rs index 2bc62f9a96d..3bc1ecd7a80 100644 --- a/crates/nargo/src/lib.rs +++ b/crates/nargo/src/lib.rs @@ -9,7 +9,7 @@ pub mod artifacts; pub mod constants; -mod errors; +pub mod errors; pub mod ops; pub mod package; pub mod workspace; diff --git a/crates/nargo/src/ops/execute.rs b/crates/nargo/src/ops/execute.rs index 3914215de95..74b744efb4b 100644 --- a/crates/nargo/src/ops/execute.rs +++ b/crates/nargo/src/ops/execute.rs @@ -1,7 +1,8 @@ -use acvm::pwg::{ACVMStatus, ACVM}; +use acvm::pwg::{ACVMStatus, ErrorLocation, OpcodeResolutionError, ACVM}; use acvm::BlackBoxFunctionSolver; use acvm::{acir::circuit::Circuit, acir::native_types::WitnessMap}; +use crate::errors::ExecutionError; use crate::NargoError; use super::foreign_calls::ForeignCall; @@ -14,6 +15,15 @@ pub fn execute_circuit( ) -> Result { let mut acvm = ACVM::new(blackbox_solver, circuit.opcodes, initial_witness); + // Assert messages are not a map due to https://github.com/noir-lang/acvm/issues/522 + let get_assert_message = |opcode_location| { + circuit + .assert_messages + .iter() + .find(|(loc, _)| loc == opcode_location) + .map(|(_, message)| message.clone()) + }; + loop { let solver_status = acvm.solve(); @@ -22,7 +32,19 @@ pub fn execute_circuit( ACVMStatus::InProgress => { unreachable!("Execution should not stop while in `InProgress` state.") } - ACVMStatus::Failure(error) => return Err(error.into()), + ACVMStatus::Failure(error) => { + return Err(NargoError::ExecutionError(match error { + OpcodeResolutionError::UnsatisfiedConstrain { + opcode_location: ErrorLocation::Resolved(opcode_location), + } => match get_assert_message(&opcode_location) { + Some(assert_message) => { + ExecutionError::AssertionFailed(assert_message, opcode_location) + } + None => ExecutionError::SolvingError(error), + }, + _ => ExecutionError::SolvingError(error), + })) + } ACVMStatus::RequiresForeignCall(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_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index 159e7987992..1035840c65d 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -1,10 +1,10 @@ use acvm::acir::circuit::OpcodeLocation; use acvm::acir::{circuit::Circuit, native_types::WitnessMap}; -use acvm::pwg::ErrorLocation; +use acvm::pwg::{ErrorLocation, OpcodeResolutionError}; use clap::Args; use nargo::constants::PROVER_INPUT_FILE; +use nargo::errors::{ExecutionError, NargoError}; use nargo::package::Package; -use nargo::NargoError; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; use noirc_abi::{Abi, InputMap}; @@ -95,24 +95,25 @@ fn execute_package( /// If the location has been resolved we return the contained [OpcodeLocation]. fn extract_opcode_error_from_nargo_error( nargo_err: &NargoError, -) -> Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)> { - let solving_err = match nargo_err { - nargo::NargoError::SolvingError(err) => err, +) -> Option<(OpcodeLocation, &ExecutionError)> { + let execution_error = match nargo_err { + NargoError::ExecutionError(err) => err, _ => return None, }; - match solving_err { - acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { + match execution_error { + ExecutionError::AssertionFailed(_, location) => Some((*location, execution_error)), + ExecutionError::SolvingError(OpcodeResolutionError::IndexOutOfBounds { opcode_location: error_location, .. - } - | acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { + }) + | ExecutionError::SolvingError(OpcodeResolutionError::UnsatisfiedConstrain { opcode_location: error_location, - } => match error_location { + }) => match error_location { ErrorLocation::Unresolved => { unreachable!("Cannot resolve index for unsatisfied constraint") } - ErrorLocation::Resolved(opcode_location) => Some((*opcode_location, solving_err)), + ErrorLocation::Resolved(opcode_location) => Some((*opcode_location, execution_error)), }, _ => None, } @@ -122,7 +123,7 @@ fn extract_opcode_error_from_nargo_error( /// to determine an opcode's call stack. Then report the error using the resolved /// call stack and any other relevant error information returned from the ACVM. fn report_error_with_opcode_location( - opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>, + opcode_err_info: Option<(OpcodeLocation, &ExecutionError)>, debug: &DebugInfo, context: &Context, ) { @@ -132,18 +133,21 @@ fn report_error_with_opcode_location( // of the call stack (the last item in the Vec). if let Some(location) = locations.last() { let message = match opcode_err { - acvm::pwg::OpcodeResolutionError::IndexOutOfBounds { + ExecutionError::AssertionFailed(message, _) => { + format!("Assertion failed: '{message}'") + } + ExecutionError::SolvingError(OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. - } => { + }) => { format!( "Index out of bounds, array has size {array_size:?}, but index was {index:?}" ) } - acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { .. } => { - "Failed constraint".into() - } + ExecutionError::SolvingError(OpcodeResolutionError::UnsatisfiedConstrain { + .. + }) => "Failed constraint".into(), _ => { // All other errors that do not have corresponding opcode locations // should not be reported in this method. diff --git a/crates/nargo_cli/tests/execution_success/assert_statement/src/main.nr b/crates/nargo_cli/tests/execution_success/assert_statement/src/main.nr index 4ac9dd26c02..74e93249741 100644 --- a/crates/nargo_cli/tests/execution_success/assert_statement/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/assert_statement/src/main.nr @@ -2,5 +2,6 @@ // // The features being tested is assertion fn main(x : Field, y : Field) { - assert(x == y); + assert(x == y, "x and y are not equal"); + assert_eq(x, y, "x and y are not equal"); } diff --git a/crates/nargo_cli/tests/execution_success/brillig_assert/src/main.nr b/crates/nargo_cli/tests/execution_success/brillig_assert/src/main.nr index 801a818c816..632c72f2393 100644 --- a/crates/nargo_cli/tests/execution_success/brillig_assert/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/brillig_assert/src/main.nr @@ -6,6 +6,7 @@ fn main(x: Field) { } unconstrained fn conditional(x : bool) -> Field { - assert(x); + assert(x, "x is false"); + assert_eq(x, true, "x is false"); 1 } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index fc020cd3b5e..15195fc4dcb 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -214,7 +214,7 @@ impl<'block> BrilligBlock<'block> { ); self.convert_ssa_binary(binary, dfg, result_register); } - Instruction::Constrain(lhs, rhs) => { + Instruction::Constrain(lhs, rhs, assert_message) => { let condition = self.brillig_context.allocate_register(); self.convert_ssa_binary( @@ -223,7 +223,7 @@ impl<'block> BrilligBlock<'block> { condition, ); - self.brillig_context.constrain_instruction(condition); + self.brillig_context.constrain_instruction(condition, assert_message.clone()); self.brillig_context.deallocate_register(condition); } Instruction::Allocate => { diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs index 6b53b7c2069..ee7bbeed46e 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_directive.rs @@ -33,6 +33,7 @@ pub(crate) fn directive_invert() -> GeneratedBrillig { }, BrilligOpcode::Stop, ], + assert_messages: Default::default(), locations: Default::default(), } } @@ -101,6 +102,7 @@ pub(crate) fn directive_quotient(bit_size: u32) -> GeneratedBrillig { }, BrilligOpcode::Stop, ], + assert_messages: Default::default(), locations: Default::default(), } } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 41e52434009..a2c6a279b8d 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -361,13 +361,20 @@ impl BrilligContext { impl BrilligContext { /// Emits brillig bytecode to jump to a trap condition if `condition` /// is false. - pub(crate) fn constrain_instruction(&mut self, condition: RegisterIndex) { + pub(crate) fn constrain_instruction( + &mut self, + condition: RegisterIndex, + assert_message: Option, + ) { self.debug_show.constrain_instruction(condition); self.add_unresolved_jump( BrilligOpcode::JumpIf { condition, location: 0 }, self.next_section_label(), ); self.push_opcode(BrilligOpcode::Trap); + if let Some(assert_message) = assert_message { + self.obj.add_assert_message_to_last_opcode(assert_message); + } self.enter_next_section(); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 627e096bfc9..9b8c3913123 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -17,13 +17,16 @@ pub(crate) enum BrilligParameter { pub(crate) struct GeneratedBrillig { pub(crate) byte_code: Vec, pub(crate) locations: BTreeMap, + pub(crate) assert_messages: BTreeMap, } #[derive(Default, Debug, Clone)] /// Artifacts resulting from the compilation of a function into brillig byte code. -/// Currently it is just the brillig bytecode of the function. +/// It includes the bytecode of the function and all the metadata that allows linking with other functions. pub(crate) struct BrilligArtifact { - byte_code: Vec, + pub(crate) byte_code: Vec, + /// A map of bytecode positions to assertion messages + pub(crate) assert_messages: BTreeMap, /// The set of jumps that need to have their locations /// resolved. unresolved_jumps: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, @@ -68,7 +71,11 @@ impl BrilligArtifact { /// Resolves all jumps and generates the final bytecode pub(crate) fn finish(mut self) -> GeneratedBrillig { self.resolve_jumps(); - GeneratedBrillig { byte_code: self.byte_code, locations: self.locations } + GeneratedBrillig { + byte_code: self.byte_code, + locations: self.locations, + assert_messages: self.assert_messages, + } } /// Gets the first unresolved function call of this artifact. @@ -131,6 +138,10 @@ impl BrilligArtifact { .push((position_in_bytecode + offset, label_id.clone())); } + for (position_in_bytecode, message) in &obj.assert_messages { + self.assert_messages.insert(position_in_bytecode + offset, message.clone()); + } + for (position_in_bytecode, call_stack) in obj.locations.iter() { self.locations.insert(position_in_bytecode + offset, call_stack.clone()); } @@ -242,4 +253,9 @@ impl BrilligArtifact { pub(crate) fn set_call_stack(&mut self, call_stack: CallStack) { self.call_stack = call_stack; } + + pub(crate) fn add_assert_message_to_last_opcode(&mut self, message: String) { + let position = self.index_of_next_opcode() - 1; + self.assert_messages.insert(position, message); + } } diff --git a/crates/noirc_evaluator/src/errors.rs b/crates/noirc_evaluator/src/errors.rs index c3959fcf8a6..d7d04d521c5 100644 --- a/crates/noirc_evaluator/src/errors.rs +++ b/crates/noirc_evaluator/src/errors.rs @@ -16,11 +16,13 @@ use crate::ssa::ir::dfg::CallStack; #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum RuntimeError { - // We avoid showing the actual lhs and rhs since most of the time they are just 0 - // and 1 respectively. This would confuse users if a constraint such as - // assert(foo < bar) fails with "failed constraint: 0 = 1." - #[error("Failed constraint")] - FailedConstraint { lhs: Box, rhs: Box, call_stack: CallStack }, + #[error("{}", format_failed_constraint(.assert_message))] + FailedConstraint { + lhs: Box, + rhs: Box, + call_stack: CallStack, + assert_message: Option, + }, #[error(transparent)] InternalError(#[from] InternalError), #[error("Index out of bounds, array has size {index:?}, but index was {array_size:?}")] @@ -39,6 +41,16 @@ pub enum RuntimeError { AssertConstantFailed { call_stack: CallStack }, } +// We avoid showing the actual lhs and rhs since most of the time they are just 0 +// and 1 respectively. This would confuse users if a constraint such as +// assert(foo < bar) fails with "failed constraint: 0 = 1." +fn format_failed_constraint(message: &Option) -> String { + match message { + Some(message) => format!("Failed constraint: '{}'", message), + None => "Failed constraint".to_owned(), + } +} + #[derive(Debug, PartialEq, Eq, Clone, Error)] pub enum InternalError { #[error("ICE: Both expressions should have degree<=1")] diff --git a/crates/noirc_evaluator/src/ssa.rs b/crates/noirc_evaluator/src/ssa.rs index 1aa108371e3..92bbe21b20d 100644 --- a/crates/noirc_evaluator/src/ssa.rs +++ b/crates/noirc_evaluator/src/ssa.rs @@ -78,7 +78,12 @@ pub fn create_circuit( optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?; let opcodes = generated_acir.take_opcodes(); let GeneratedAcir { - current_witness_index, return_witnesses, locations, input_witnesses, .. + current_witness_index, + return_witnesses, + locations, + input_witnesses, + assert_messages, + .. } = generated_acir; let abi = gen_abi(context, func_sig, &input_witnesses, return_witnesses.clone()); @@ -99,7 +104,7 @@ pub fn create_circuit( private_parameters, public_parameters, return_values, - assert_messages: Default::default(), + assert_messages: assert_messages.into_iter().collect(), }; // This converts each im::Vector in the BTreeMap to a Vec diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index f9e6994859d..cc1888311e8 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -6,13 +6,13 @@ use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue}; use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::types::Type as SsaType; use crate::ssa::ir::{instruction::Endian, types::NumericType}; -use acvm::acir::brillig::Opcode as BrilligOpcode; use acvm::acir::circuit::brillig::{BrilligInputs, BrilligOutputs}; use acvm::acir::circuit::opcodes::{BlockId, MemOp}; use acvm::acir::circuit::Opcode; use acvm::brillig_vm::{brillig::Value, Registers, VMStatus, VM}; use acvm::{ acir::{ + brillig::Opcode as BrilligOpcode, circuit::opcodes::FunctionInput, native_types::{Expression, Witness}, BlackBoxFunc, @@ -313,9 +313,14 @@ impl AcirContext { } // Constrains `var` to be equal to the constant value `1` - pub(crate) fn assert_eq_one(&mut self, var: AcirVar) -> Result<(), RuntimeError> { + pub(crate) fn assert_eq_one( + &mut self, + var: AcirVar, + assert_message: Option, + ) -> Result<(), RuntimeError> { let one = self.add_constant(FieldElement::one()); - self.assert_eq_var(var, one) + self.assert_eq_var(var, one, assert_message)?; + Ok(()) } // Constrains `var` to be equal to predicate if the predicate is true @@ -328,7 +333,7 @@ impl AcirContext { predicate: AcirVar, ) -> Result<(), RuntimeError> { let pred_mul_var = self.mul_var(var, predicate)?; - self.assert_eq_var(pred_mul_var, predicate) + self.assert_eq_var(pred_mul_var, predicate, None) } // Returns the variable from the results, assuming it is the only result @@ -414,7 +419,12 @@ impl AcirContext { } /// Constrains the `lhs` and `rhs` to be equal. - pub(crate) fn assert_eq_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> Result<(), RuntimeError> { + pub(crate) fn assert_eq_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + assert_message: Option, + ) -> Result<(), RuntimeError> { let lhs_expr = self.var_to_expression(lhs)?; let rhs_expr = self.var_to_expression(rhs)?; @@ -433,11 +443,15 @@ impl AcirContext { lhs: Box::new(lhs_expr), rhs: Box::new(rhs_expr), call_stack: self.get_call_stack(), + assert_message, }); }; } self.acir_ir.assert_is_zero(diff_expr); + if let Some(message) = assert_message { + self.acir_ir.assert_messages.insert(self.acir_ir.last_acir_opcode_location(), message); + } self.mark_variables_equivalent(lhs, rhs)?; Ok(()) diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index f76afa031e0..8304ce1fbee 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -44,12 +44,15 @@ pub(crate) struct GeneratedAcir { /// All witness indices which are inputs to the main function pub(crate) input_witnesses: Vec, - /// Correspondance between an opcode index (in opcodes) and the source code call stack which generated it + /// Correspondence between an opcode index (in opcodes) and the source code call stack which generated it pub(crate) locations: BTreeMap, /// Source code location of the current instruction being processed /// None if we do not know the location pub(crate) call_stack: CallStack, + + /// Correspondence between an opcode index and the error message associated with it. + pub(crate) assert_messages: BTreeMap, } impl GeneratedAcir { @@ -62,8 +65,7 @@ impl GeneratedAcir { pub(crate) fn push_opcode(&mut self, opcode: AcirOpcode) { self.opcodes.push(opcode); if !self.call_stack.is_empty() { - self.locations - .insert(OpcodeLocation::Acir(self.opcodes.len() - 1), self.call_stack.clone()); + self.locations.insert(self.last_acir_opcode_location(), self.call_stack.clone()); } } @@ -863,6 +865,12 @@ impl GeneratedAcir { call_stack, ); } + for (brillig_index, message) in generated_brillig.assert_messages { + self.assert_messages.insert( + OpcodeLocation::Brillig { acir_index: self.opcodes.len() - 1, brillig_index }, + message, + ); + } } /// Generate gates and control bits witnesses which ensure that out_expr is a permutation of in_expr @@ -898,6 +906,10 @@ impl GeneratedAcir { } Ok(()) } + + pub(crate) fn last_acir_opcode_location(&self) -> OpcodeLocation { + OpcodeLocation::Acir(self.opcodes.len() - 1) + } } /// This function will return the number of inputs that a blackbox function diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs index 157cccf06f5..92619c06f39 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -328,7 +328,7 @@ impl Context { let result_acir_var = self.convert_ssa_binary(binary, dfg)?; self.define_result_var(dfg, instruction_id, result_acir_var); } - Instruction::Constrain(lhs, rhs) => { + Instruction::Constrain(lhs, rhs, assert_message) => { let lhs = self.convert_value(*lhs, dfg); let rhs = self.convert_value(*rhs, dfg); @@ -384,7 +384,7 @@ impl Context { for (lhs, rhs) in get_var_equality_assertions(lhs, rhs, &mut read_dynamic_array_index)? { - self.acir_context.assert_eq_var(lhs, rhs)?; + self.acir_context.assert_eq_var(lhs, rhs, assert_message.clone())?; } } Instruction::Cast(value_id, typ) => { diff --git a/crates/noirc_evaluator/src/ssa/function_builder/mod.rs b/crates/noirc_evaluator/src/ssa/function_builder/mod.rs index fab94b43689..c8b67c94888 100644 --- a/crates/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -238,8 +238,13 @@ impl FunctionBuilder { } /// Insert a constrain instruction at the end of the current block. - pub(crate) fn insert_constrain(&mut self, lhs: ValueId, rhs: ValueId) { - self.insert_instruction(Instruction::Constrain(lhs, rhs), None); + pub(crate) fn insert_constrain( + &mut self, + lhs: ValueId, + rhs: ValueId, + assert_message: Option, + ) { + self.insert_instruction(Instruction::Constrain(lhs, rhs, assert_message), None); } /// Insert a call instruction at the end of the current block and return diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 5772a2b9548..e57ae55209b 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -140,7 +140,7 @@ pub(crate) enum Instruction { Truncate { value: ValueId, bit_size: u32, max_bit_size: u32 }, /// Constrains two values to be equal to one another. - Constrain(ValueId, ValueId), + Constrain(ValueId, ValueId, Option), /// Performs a function call with a list of its arguments. Call { func: ValueId, arguments: Vec }, @@ -189,7 +189,7 @@ impl Instruction { InstructionResultType::Operand(*value) } Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array), - Instruction::Constrain(_, _) + Instruction::Constrain(..) | Instruction::Store { .. } | Instruction::EnableSideEffects { .. } => InstructionResultType::None, Instruction::Load { .. } | Instruction::ArrayGet { .. } | Instruction::Call { .. } => { @@ -218,7 +218,7 @@ impl Instruction { Truncate { .. } => false, // These either have side-effects or interact with memory - Constrain(_, _) | EnableSideEffects { .. } | Allocate | Load { .. } | Store { .. } => { + Constrain(..) | EnableSideEffects { .. } | Allocate | Load { .. } | Store { .. } => { false } @@ -242,7 +242,7 @@ impl Instruction { | ArrayGet { .. } | ArraySet { .. } => false, - Constrain(_, _) | Store { .. } | EnableSideEffects { .. } => true, + Constrain(..) | Store { .. } | EnableSideEffects { .. } => true, // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. Call { func, .. } => match dfg[*func] { @@ -278,7 +278,9 @@ impl Instruction { bit_size: *bit_size, max_bit_size: *max_bit_size, }, - Instruction::Constrain(lhs, rhs) => Instruction::Constrain(f(*lhs), f(*rhs)), + Instruction::Constrain(lhs, rhs, assert_message) => { + Instruction::Constrain(f(*lhs), f(*rhs), assert_message.clone()) + } Instruction::Call { func, arguments } => Instruction::Call { func: f(*func), arguments: vecmap(arguments.iter().copied(), f), @@ -319,7 +321,7 @@ impl Instruction { | Instruction::Load { address: value } => { f(*value); } - Instruction::Constrain(lhs, rhs) => { + Instruction::Constrain(lhs, rhs, _) => { f(*lhs); f(*rhs); } @@ -374,7 +376,7 @@ impl Instruction { _ => None, } } - Instruction::Constrain(lhs, rhs) => { + Instruction::Constrain(lhs, rhs, ..) => { if dfg.resolve(*lhs) == dfg.resolve(*rhs) { // Remove trivial case `assert_eq(x, x)` SimplifyResult::Remove diff --git a/crates/noirc_evaluator/src/ssa/ir/printer.rs b/crates/noirc_evaluator/src/ssa/ir/printer.rs index e27e9bb11ab..037f8aaac2b 100644 --- a/crates/noirc_evaluator/src/ssa/ir/printer.rs +++ b/crates/noirc_evaluator/src/ssa/ir/printer.rs @@ -145,9 +145,10 @@ pub(crate) fn display_instruction( let value = show(*value); writeln!(f, "truncate {value} to {bit_size} bits, max_bit_size: {max_bit_size}",) } - Instruction::Constrain(lhs, rhs) => { - writeln!(f, "constrain {} == {}", show(*lhs), show(*rhs)) - } + Instruction::Constrain(lhs, rhs, message) => match message { + Some(message) => writeln!(f, "constrain {} == {} '{message}'", show(*lhs), show(*rhs)), + None => writeln!(f, "constrain {} == {}", show(*lhs), show(*rhs)), + }, Instruction::Call { func, arguments } => { writeln!(f, "call {}({})", show(*func), value_list(function, arguments)) } diff --git a/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs index cf2c4585e88..3143871e59b 100644 --- a/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/crates/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -305,7 +305,7 @@ fn create_apply_function( function_builder.switch_to_block(executor_block); } else { // Else just constrain the condition - function_builder.insert_constrain(target_id, function_id_constant); + function_builder.insert_constrain(target_id, function_id_constant, None); } // Find the target block or build it if necessary let current_block = function_builder.current_block(); diff --git a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index e0d1d508674..61fdc0bcd37 100644 --- a/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -766,7 +766,7 @@ impl<'f> Context<'f> { ) -> Instruction { if let Some((_, condition)) = self.conditions.last().copied() { match instruction { - Instruction::Constrain(lhs, rhs) => { + Instruction::Constrain(lhs, rhs, message) => { // Replace constraint `lhs == rhs` with `condition * lhs == condition * rhs`. // Condition needs to be cast to argument type in order to multiply them together. @@ -784,7 +784,7 @@ impl<'f> Context<'f> { Instruction::binary(BinaryOp::Mul, rhs, casted_condition), call_stack, ); - Instruction::Constrain(lhs, rhs) + Instruction::Constrain(lhs, rhs, message) } Instruction::Store { address, value } => { self.remember_store(address, value); @@ -901,7 +901,7 @@ mod test { builder.terminate_with_jmpif(v0, b1, b2); builder.switch_to_block(b1); - builder.insert_constrain(v1, v_true); + builder.insert_constrain(v1, v_true, None); builder.terminate_with_jmp(b2, vec![]); builder.switch_to_block(b2); @@ -1360,7 +1360,7 @@ mod test { builder.terminate_with_jmp(b2, vec![]); builder.switch_to_block(b2); - builder.insert_constrain(v_false, v_true); // should not be removed + builder.insert_constrain(v_false, v_true, None); // should not be removed builder.terminate_with_return(vec![]); let ssa = builder.finish().flatten_cfg(); @@ -1368,7 +1368,7 @@ mod test { // Assert we have not incorrectly removed a constraint: use Instruction::Constrain; - let constrain_count = count_instruction(main, |ins| matches!(ins, Constrain(_, _))); + let constrain_count = count_instruction(main, |ins| matches!(ins, Constrain(..))); assert_eq!(constrain_count, 1); } @@ -1443,7 +1443,7 @@ mod test { builder.switch_to_block(b3); let v_true = builder.numeric_constant(true, Type::bool()); let v12 = builder.insert_binary(v9, BinaryOp::Eq, v_true); - builder.insert_constrain(v12, v_true); + builder.insert_constrain(v12, v_true, None); builder.terminate_with_return(vec![]); let ssa = builder.finish().flatten_cfg(); @@ -1452,7 +1452,7 @@ mod test { // Now assert that there is not an always-false constraint after flattening: let mut constrain_count = 0; for instruction in main.dfg[main.entry_block()].instructions() { - if let Instruction::Constrain(lhs, rhs) = main.dfg[*instruction] { + if let Instruction::Constrain(lhs, rhs, ..) = main.dfg[*instruction] { if let (Some(lhs), Some(rhs)) = (main.dfg.get_numeric_constant(lhs), main.dfg.get_numeric_constant(rhs)) { diff --git a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs index 2788c8ecf31..4f8209ce12e 100644 --- a/crates/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/crates/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -536,7 +536,7 @@ mod tests { builder.switch_to_block(b5); let v4 = builder.insert_binary(v0, BinaryOp::Add, v2); let v5 = builder.insert_binary(ten, BinaryOp::Lt, v4); - builder.insert_constrain(v5, one); + builder.insert_constrain(v5, one, None); let v6 = builder.insert_binary(v2, BinaryOp::Add, one); builder.terminate_with_jmp(b4, vec![v6]); diff --git a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 9afae95116e..652869bdc9d 100644 --- a/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -80,7 +80,9 @@ impl<'a> FunctionContext<'a> { } Expression::Call(call) => self.codegen_call(call), Expression::Let(let_expr) => self.codegen_let(let_expr), - Expression::Constrain(expr, location) => self.codegen_constrain(expr, *location), + Expression::Constrain(expr, location, assert_message) => { + self.codegen_constrain(expr, *location, assert_message.clone()) + } Expression::Assign(assign) => self.codegen_assign(assign), Expression::Semi(semi) => self.codegen_semi(semi), } @@ -344,7 +346,7 @@ impl<'a> FunctionContext<'a> { let is_offset_out_of_bounds = self.builder.insert_binary(index, BinaryOp::Lt, array_len); let true_const = self.builder.numeric_constant(true, Type::bool()); - self.builder.insert_constrain(is_offset_out_of_bounds, true_const); + self.builder.insert_constrain(is_offset_out_of_bounds, true_const, None); } fn codegen_cast(&mut self, cast: &ast::Cast) -> Values { @@ -517,7 +519,12 @@ impl<'a> FunctionContext<'a> { Self::unit_value() } - fn codegen_constrain(&mut self, expr: &Expression, location: Location) -> Values { + fn codegen_constrain( + &mut self, + expr: &Expression, + location: Location, + assert_message: Option, + ) -> Values { match expr { // If we're constraining an equality to be true then constrain the two sides directly. Expression::Binary(Binary { lhs, operator, rhs, .. }) @@ -525,13 +532,17 @@ impl<'a> FunctionContext<'a> { { let lhs = self.codegen_non_tuple_expression(lhs); let rhs = self.codegen_non_tuple_expression(rhs); - self.builder.set_location(location).insert_constrain(lhs, rhs); + self.builder.set_location(location).insert_constrain(lhs, rhs, assert_message); } _ => { let expr = self.codegen_non_tuple_expression(expr); let true_literal = self.builder.numeric_constant(true, Type::bool()); - self.builder.set_location(location).insert_constrain(expr, true_literal); + self.builder.set_location(location).insert_constrain( + expr, + true_literal, + assert_message, + ); } } Self::unit_value() diff --git a/crates/noirc_frontend/src/ast/statement.rs b/crates/noirc_frontend/src/ast/statement.rs index 7d51ed60b46..e48cf7b5457 100644 --- a/crates/noirc_frontend/src/ast/statement.rs +++ b/crates/noirc_frontend/src/ast/statement.rs @@ -401,7 +401,7 @@ pub enum LValue { } #[derive(Debug, PartialEq, Eq, Clone)] -pub struct ConstrainStatement(pub Expression); +pub struct ConstrainStatement(pub Expression, pub Option); #[derive(Debug, PartialEq, Eq, Clone)] pub enum Pattern { diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index f73a3b85166..cf0f8f80751 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -919,7 +919,8 @@ impl<'a> Resolver<'a> { } Statement::Constrain(constrain_stmt) => { let expr_id = self.resolve_expression(constrain_stmt.0); - HirStatement::Constrain(HirConstrainStatement(expr_id, self.file)) + let assert_message = constrain_stmt.1; + HirStatement::Constrain(HirConstrainStatement(expr_id, self.file, assert_message)) } Statement::Expression(expr) => HirStatement::Expression(self.resolve_expression(expr)), Statement::Semi(expr) => HirStatement::Semi(self.resolve_expression(expr)), diff --git a/crates/noirc_frontend/src/hir_def/stmt.rs b/crates/noirc_frontend/src/hir_def/stmt.rs index 8cf9d82f580..0dcb7192be2 100644 --- a/crates/noirc_frontend/src/hir_def/stmt.rs +++ b/crates/noirc_frontend/src/hir_def/stmt.rs @@ -46,7 +46,7 @@ pub struct HirAssignStatement { /// originates from. This is used later in the SSA pass to issue /// an error if a constrain is found to be always false. #[derive(Debug, Clone)] -pub struct HirConstrainStatement(pub ExprId, pub FileId); +pub struct HirConstrainStatement(pub ExprId, pub FileId, pub Option); #[derive(Debug, Clone)] pub enum HirPattern { diff --git a/crates/noirc_frontend/src/monomorphization/ast.rs b/crates/noirc_frontend/src/monomorphization/ast.rs index 2324624b1df..bc59249b489 100644 --- a/crates/noirc_frontend/src/monomorphization/ast.rs +++ b/crates/noirc_frontend/src/monomorphization/ast.rs @@ -29,7 +29,7 @@ pub enum Expression { ExtractTupleField(Box, usize), Call(Call), Let(Let), - Constrain(Box, Location), + Constrain(Box, Location, Option), Assign(Assign), Semi(Box), } diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 466a3f95e81..7a25c5e6b77 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -415,7 +415,7 @@ impl<'interner> Monomorphizer<'interner> { HirStatement::Constrain(constrain) => { let expr = self.expr(constrain.0); let location = self.interner.expr_location(&constrain.0); - ast::Expression::Constrain(Box::new(expr), location) + ast::Expression::Constrain(Box::new(expr), location, constrain.2) } HirStatement::Assign(assign) => self.assign(assign), HirStatement::Expression(expr) => self.expr(expr), diff --git a/crates/noirc_frontend/src/parser/errors.rs b/crates/noirc_frontend/src/parser/errors.rs index ec3095a989d..80832abb3dc 100644 --- a/crates/noirc_frontend/src/parser/errors.rs +++ b/crates/noirc_frontend/src/parser/errors.rs @@ -31,6 +31,8 @@ pub enum ParserErrorReason { ExperimentalFeature(&'static str), #[error("Where clauses are allowed only on functions with generic parameters")] WhereClauseOnNonGenericFunction, + #[error("Assert statements can only accept string literals")] + AssertMessageNotString, } /// Represents a parsing error, or a parsing error in the making. diff --git a/crates/noirc_frontend/src/parser/parser.rs b/crates/noirc_frontend/src/parser/parser.rs index 4a2de3850d2..00eba5de3f3 100644 --- a/crates/noirc_frontend/src/parser/parser.rs +++ b/crates/noirc_frontend/src/parser/parser.rs @@ -711,7 +711,7 @@ where keyword(Keyword::Constrain).labelled(ParsingRuleLabel::Statement), expr_parser, ) - .map(|expr| Statement::Constrain(ConstrainStatement(expr))) + .map(|expr| Statement::Constrain(ConstrainStatement(expr, None))) .validate(|expr, span, emit| { emit(ParserError::with_reason(ParserErrorReason::ConstrainDeprecated, span)); expr @@ -722,20 +722,37 @@ fn assertion<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - ignore_then_commit(keyword(Keyword::Assert), parenthesized(expr_parser)) + let argument_parser = + expr_parser.separated_by(just(Token::Comma)).allow_trailing().at_least(1).at_most(2); + + ignore_then_commit(keyword(Keyword::Assert), parenthesized(argument_parser)) .labelled(ParsingRuleLabel::Statement) - .map(|expr| Statement::Constrain(ConstrainStatement(expr))) + .validate(|expressions, span, emit| { + let condition = expressions.get(0).unwrap_or(&Expression::error(span)).clone(); + let mut message_str = None; + + if let Some(message) = expressions.get(1) { + if let ExpressionKind::Literal(Literal::Str(message)) = &message.kind { + message_str = Some(message.clone()); + } else { + emit(ParserError::with_reason(ParserErrorReason::AssertMessageNotString, span)); + } + } + + Statement::Constrain(ConstrainStatement(condition, message_str)) + }) } fn assertion_eq<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - let argument_parser = expr_parser.separated_by(just(Token::Comma)).allow_trailing().exactly(2); + let argument_parser = + expr_parser.separated_by(just(Token::Comma)).allow_trailing().at_least(2).at_most(3); ignore_then_commit(keyword(Keyword::AssertEq), parenthesized(argument_parser)) .labelled(ParsingRuleLabel::Statement) - .validate(|exprs: Vec, span, _| { + .validate(|exprs: Vec, span, emit| { let predicate = Expression::new( ExpressionKind::Infix(Box::new(InfixExpression { lhs: exprs.get(0).unwrap_or(&Expression::error(span)).clone(), @@ -744,8 +761,16 @@ where })), span, ); + let mut message_str = None; - Statement::Constrain(ConstrainStatement(predicate)) + if let Some(message) = exprs.get(2) { + if let ExpressionKind::Literal(Literal::Str(message)) = &message.kind { + message_str = Some(message.clone()); + } else { + emit(ParserError::with_reason(ParserErrorReason::AssertMessageNotString, span)); + } + } + Statement::Constrain(ConstrainStatement(predicate, message_str)) }) } @@ -1839,6 +1864,14 @@ mod test { "assert(x + x ^ x == y | m)", ], ); + + match parse_with(assertion(expression()), "assert(x == y, \"assertion message\")").unwrap() + { + Statement::Constrain(ConstrainStatement(_, message)) => { + assert_eq!(message, Some("assertion message".to_owned())); + } + _ => unreachable!(), + } } /// This is the standard way to assert that two expressions are equivalent @@ -1855,6 +1888,14 @@ mod test { "assert_eq(x + x ^ x, y | m)", ], ); + match parse_with(assertion_eq(expression()), "assert_eq(x, y, \"assertion message\")") + .unwrap() + { + Statement::Constrain(ConstrainStatement(_, message)) => { + assert_eq!(message, Some("assertion message".to_owned())); + } + _ => unreachable!(), + } } #[test] @@ -2212,8 +2253,10 @@ mod test { ("assert", 1, "constrain Error"), ("constrain x ==", 2, "constrain (plain::x == Error)"), ("assert(x ==)", 1, "constrain (plain::x == Error)"), + ("assert(x == x, x)", 1, "constrain (plain::x == plain::x)"), ("assert_eq(x,)", 1, "constrain (Error == Error)"), - ("assert_eq(x, x, x)", 1, "constrain (Error == Error)"), + ("assert_eq(x, x, x, x)", 1, "constrain (Error == Error)"), + ("assert_eq(x, x, x)", 1, "constrain (plain::x == plain::x)"), ]; let show_errors = |v| vecmap(v, ToString::to_string).join("\n");