From d5ebba0848e2efcb071b2a0cc4d3acaf064c5d82 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 1 May 2025 11:12:32 -0500 Subject: [PATCH 1/7] Start adding error variants --- compiler/noirc_evaluator/src/errors.rs | 11 +++++- .../src/ssa/interpreter/mod.rs | 34 +++++++++++++------ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index c976bb592d5..5bd957e9e57 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -13,7 +13,7 @@ use noirc_errors::{CustomDiagnostic, Location, call_stack::CallStack}; use noirc_frontend::signed_field::SignedField; use thiserror::Error; -use crate::ssa::ir::types::NumericType; +use crate::ssa::ir::{basic_block::BasicBlockId, types::NumericType, value::ValueId}; use serde::{Deserialize, Serialize}; #[derive(Debug, PartialEq, Eq, Clone, Error)] @@ -66,6 +66,15 @@ pub enum RuntimeError { "Could not resolve some references to the array. All references must be resolved at compile time" )] UnknownReference { call_stack: CallStack }, + #[error("Argument count {arguments} to block {block} does not match the expected parameter count {parameters}")] + BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, + #[error("Block {block} is missing the terminator instruction")] + BlockMissingTerminator { block: BasicBlockId }, + #[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 }, + FailedRangeConstraint } #[derive(Debug, Clone, Serialize, Deserialize, Hash)] diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 95c79d05e94..a2e79ccbf19 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -153,7 +153,11 @@ impl<'ssa> Interpreter<'ssa> { let block = &dfg[block_id]; if arguments.len() != block.parameters().len() { - panic!("Block argument count does not match the expected parameter count"); + return Err(RuntimeError::BlockArgumentCountMismatch { + block: block_id, + arguments: arguments.len(), + parameters: block.parameters().len(), + }); } for (parameter, argument) in block.parameters().iter().zip(arguments) { @@ -166,7 +170,9 @@ impl<'ssa> Interpreter<'ssa> { } match block.terminator() { - None => panic!("No block terminator in block {block_id}"), + None => { + return Err(RuntimeError::BlockMissingTerminator { block: block_id }); + }, Some(TerminatorInstruction::Jmp { destination, arguments: jump_args, .. }) => { block_id = *destination; arguments = self.lookup_all(jump_args); @@ -250,18 +256,26 @@ impl<'ssa> Interpreter<'ssa> { Instruction::Truncate { value, bit_size, max_bit_size } => { self.interpret_truncate(*value, *bit_size, *max_bit_size, results[0]); } - Instruction::Constrain(lhs, rhs, constrain_error) => { - let lhs = self.lookup(*lhs); - let rhs = self.lookup(*rhs); + Instruction::Constrain(lhs_id, rhs_id, constrain_error) => { + let lhs = self.lookup(*lhs_id); + let rhs = self.lookup(*rhs_id); if self.side_effects_enabled() && lhs != rhs { - panic!("Constrain {lhs} == {rhs} failed!"); + let lhs = lhs.to_string(); + let rhs = rhs.to_string(); + let lhs_id = *lhs_id; + let rhs_id = *rhs_id; + return Err(RuntimeError::ConstrainEqFailed { lhs, lhs_id, rhs, rhs_id }) } } - Instruction::ConstrainNotEqual(lhs, rhs, constrain_error) => { - let lhs = self.lookup(*lhs); - let rhs = self.lookup(*rhs); + Instruction::ConstrainNotEqual(lhs_id, rhs_id, constrain_error) => { + let lhs = self.lookup(*lhs_id); + let rhs = self.lookup(*rhs_id); if self.side_effects_enabled() && lhs == rhs { - panic!("Constrain {lhs} != {rhs} failed!"); + let lhs = lhs.to_string(); + let rhs = rhs.to_string(); + let lhs_id = *lhs_id; + let rhs_id = *rhs_id; + return Err(RuntimeError::ConstrainNeFailed { lhs, lhs_id, rhs, rhs_id }) } } Instruction::RangeCheck { value, max_bit_size, assert_message } => { From fd5d66ad4990f987093a5dd9069d3c4b19ffa0ba Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 1 May 2025 15:11:30 -0500 Subject: [PATCH 2/7] Replace all reachable errors with error variants --- compiler/noirc_evaluator/src/errors.rs | 11 +- .../src/ssa/interpreter/errors.rs | 93 +++ .../src/ssa/interpreter/mod.rs | 666 ++++++++++++------ .../src/ssa/interpreter/tests/instructions.rs | 38 +- .../src/ssa/interpreter/tests/mod.rs | 13 +- .../src/ssa/interpreter/value.rs | 8 +- .../noirc_evaluator/src/ssa/ir/printer.rs | 6 +- 7 files changed, 595 insertions(+), 240 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/interpreter/errors.rs diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 5bd957e9e57..c976bb592d5 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -13,7 +13,7 @@ use noirc_errors::{CustomDiagnostic, Location, call_stack::CallStack}; use noirc_frontend::signed_field::SignedField; use thiserror::Error; -use crate::ssa::ir::{basic_block::BasicBlockId, types::NumericType, value::ValueId}; +use crate::ssa::ir::types::NumericType; use serde::{Deserialize, Serialize}; #[derive(Debug, PartialEq, Eq, Clone, Error)] @@ -66,15 +66,6 @@ pub enum RuntimeError { "Could not resolve some references to the array. All references must be resolved at compile time" )] UnknownReference { call_stack: CallStack }, - #[error("Argument count {arguments} to block {block} does not match the expected parameter count {parameters}")] - BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, - #[error("Block {block} is missing the terminator instruction")] - BlockMissingTerminator { block: BasicBlockId }, - #[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 }, - FailedRangeConstraint } #[derive(Debug, Clone, Serialize, Deserialize, Hash)] diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs new file mode 100644 index 00000000000..15544d8568e --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -0,0 +1,93 @@ +use crate::ssa::ir::{basic_block::BasicBlockId, instruction::BinaryOp, value::ValueId}; +use thiserror::Error; + +pub(super) const MAX_SIGNED_BIT_SIZE: u32 = 64; +pub(super) const MAX_UNSIGNED_BIT_SIZE: u32 = 128; + +#[derive(Debug, Error)] +pub(crate) enum InterpreterError { + #[error( + "Argument count {arguments} to block {block} does not match the expected parameter count {parameters}" + )] + BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, + #[error("Block {block} is missing the terminator instruction")] + BlockMissingTerminator { block: BasicBlockId }, + #[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( + "Range check of {value_id} = {value} failed.\n Max bits allowed by range check = {max_bits}\n Actual bit count = {actual_bits}" + )] + RangeCheckFailed { value: String, value_id: ValueId, actual_bits: u32, max_bits: u32 }, + #[error( + "Range check of {value_id} = {value} failed.\n Max bits allowed by range check = {max_bits}\n Actual bit count = {actual_bits}\n {message}" + )] + RangeCheckFailedWithMessage { + value: String, + value_id: ValueId, + actual_bits: u32, + max_bits: u32, + message: String, + }, + #[error("Call to unknown foreign function {name}")] + UnknownForeignFunctionCall { name: String }, + #[error("Cannot call non-function value {value_id} = {value}")] + CalledNonFunction { value: String, value_id: ValueId }, + // Note that we don't need to display the value_id because displaying a reference + // value shows the original value id anyway + #[error("Reference value `{value}` passed from a constrained to an unconstrained function")] + ReferenceValueCrossedUnconstrainedBoundary { value: String }, + #[error("Reference value `{value}` loaded before it was first stored to")] + UninitializedReferenceValueLoaded { value: String }, + #[error( + "Mismatched types in binary operator: `{operator} {lhs_id}, {rhs_id}` ({operator} {lhs}, {rhs})" + )] + MismatchedTypesInBinaryOperator { + lhs_id: ValueId, + lhs: String, + operator: BinaryOp, + rhs_id: ValueId, + rhs: String, + }, + #[error("Division by zero: `div {lhs_id}, {rhs_id}` ({lhs} / {rhs})")] + DivisionByZero { lhs_id: ValueId, lhs: String, rhs_id: ValueId, rhs: String }, + #[error("Unsupported operator `{operator}` for type `{typ}`")] + UnsupportedOperatorForType { operator: &'static str, typ: &'static str }, + #[error( + "Invalid bit size of `{bit_size}` given to truncate, maximum size allowed for unsigned values is {MAX_UNSIGNED_BIT_SIZE}" + )] + InvalidUnsignedTruncateBitSize { bit_size: u32 }, + #[error( + "Invalid bit size of `{bit_size}` given to truncate, maximum size allowed for signed values is {MAX_SIGNED_BIT_SIZE}" + )] + InvalidSignedTruncateBitSize { bit_size: u32 }, + #[error("Rhs of `{operator}` should be a u32 but found `{rhs_id} = {rhs}`")] + RhsOfBitShiftShouldBeU32 { operator: &'static str, rhs_id: ValueId, rhs: String }, + #[error( + "Expected {expected_type} value in {instruction} but instead found `{value_id} = {value}`" + )] + TypeError { + value_id: ValueId, + value: String, + expected_type: &'static str, + instruction: &'static str, + }, + #[error("Underflow in dec_rc when decrementing reference count of `{value_id} = {value}`")] + DecRcUnderflow { value_id: ValueId, value: String }, + #[error( + "Erroneously incremented reference count of value `{value_id} = {value}` from 0 back to 1" + )] + IncRcRevive { value_id: ValueId, value: String }, + #[error("An overflow occurred while evaluating {instruction}")] + Overflow { instruction: String }, + #[error( + "Function {function} ({function_name}) returned {actual} argument(s) but it was expected to return {expected}" + )] + FunctionReturnedIncorrectArgCount { + function: ValueId, + function_name: String, + expected: usize, + actual: usize, + }, +} diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index a2e79ccbf19..d0eb8740989 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -10,13 +10,15 @@ use super::{ value::ValueId, }, }; -use crate::{errors::RuntimeError, ssa::ir::instruction::binary::truncate_field}; +use crate::ssa::ir::{instruction::binary::truncate_field, printer::display_binary}; use acvm::{AcirField, FieldElement}; +use errors::{InterpreterError, MAX_SIGNED_BIT_SIZE, MAX_UNSIGNED_BIT_SIZE}; use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; use noirc_frontend::Shared; -use value::{ArrayValue, NumericValue}; +use value::{ArrayValue, NumericValue, ReferenceValue}; +mod errors; mod intrinsics; mod tests; pub mod value; @@ -56,7 +58,7 @@ impl CallContext { } } -type IResult = Result; +type IResult = Result; type IResults = IResult>; #[allow(unused)] @@ -153,7 +155,7 @@ impl<'ssa> Interpreter<'ssa> { let block = &dfg[block_id]; if arguments.len() != block.parameters().len() { - return Err(RuntimeError::BlockArgumentCountMismatch { + return Err(InterpreterError::BlockArgumentCountMismatch { block: block_id, arguments: arguments.len(), parameters: block.parameters().len(), @@ -171,8 +173,8 @@ impl<'ssa> Interpreter<'ssa> { match block.terminator() { None => { - return Err(RuntimeError::BlockMissingTerminator { block: block_id }); - }, + return Err(InterpreterError::BlockMissingTerminator { block: block_id }); + } Some(TerminatorInstruction::Jmp { destination, arguments: jump_args, .. }) => { block_id = *destination; arguments = self.lookup_all(jump_args); @@ -183,7 +185,7 @@ impl<'ssa> Interpreter<'ssa> { else_destination, call_stack: _, }) => { - block_id = if self.lookup(*condition).as_bool().unwrap() { + block_id = if self.lookup_bool(*condition, "jmpif condition")? { *then_destination } else { *else_destination @@ -224,6 +226,55 @@ impl<'ssa> Interpreter<'ssa> { } } + fn lookup_helper( + &self, + value_id: ValueId, + instruction: &'static str, + expected_type: &'static str, + convert: impl FnOnce(&Value) -> Option, + ) -> IResult { + let value = self.lookup(value_id); + match convert(&value) { + Some(value) => Ok(value), + None => { + let value = value.to_string(); + Err(InterpreterError::TypeError { value_id, value, expected_type, instruction }) + } + } + } + + fn lookup_bool(&self, value_id: ValueId, instruction: &'static str) -> IResult { + self.lookup_helper(value_id, instruction, "bool", Value::as_bool) + } + + fn lookup_u32(&self, value_id: ValueId, instruction: &'static str) -> IResult { + self.lookup_helper(value_id, instruction, "u32", Value::as_u32) + } + + fn lookup_numeric( + &self, + value_id: ValueId, + instruction: &'static str, + ) -> IResult { + self.lookup_helper(value_id, instruction, "numeric", Value::as_numeric) + } + + fn lookup_array_or_slice( + &self, + value_id: ValueId, + instruction: &'static str, + ) -> IResult { + self.lookup_helper(value_id, instruction, "array or slice", Value::as_array_or_slice) + } + + fn lookup_reference( + &self, + value_id: ValueId, + instruction: &'static str, + ) -> IResult { + self.lookup_helper(value_id, instruction, "reference", Value::as_reference) + } + fn lookup_all(&self, ids: &[ValueId]) -> Vec { vecmap(ids, |id| self.lookup(*id)) } @@ -240,21 +291,23 @@ impl<'ssa> Interpreter<'ssa> { &mut self, instruction: &Instruction, results: &[ValueId], - ) -> Result<(), RuntimeError> { + ) -> IResult<()> { match instruction { Instruction::Binary(binary) => { let result = self.interpret_binary(binary)?; self.define(results[0], result); + Ok(()) } // Cast in SSA changes the type without altering the value Instruction::Cast(value, numeric_type) => { - let field = self.lookup(*value).as_numeric().unwrap().convert_to_field(); + let field = self.lookup_numeric(*value, "cast")?.convert_to_field(); let result = Value::Numeric(NumericValue::from_constant(field, *numeric_type)); self.define(results[0], result); + Ok(()) } Instruction::Not(id) => self.interpret_not(*id, results[0]), Instruction::Truncate { value, bit_size, max_bit_size } => { - self.interpret_truncate(*value, *bit_size, *max_bit_size, results[0]); + self.interpret_truncate(*value, *bit_size, *max_bit_size, results[0]) } Instruction::Constrain(lhs_id, rhs_id, constrain_error) => { let lhs = self.lookup(*lhs_id); @@ -264,8 +317,9 @@ impl<'ssa> Interpreter<'ssa> { let rhs = rhs.to_string(); let lhs_id = *lhs_id; let rhs_id = *rhs_id; - return Err(RuntimeError::ConstrainEqFailed { lhs, lhs_id, rhs, rhs_id }) + return Err(InterpreterError::ConstrainEqFailed { lhs, lhs_id, rhs, rhs_id }); } + Ok(()) } Instruction::ConstrainNotEqual(lhs_id, rhs_id, constrain_error) => { let lhs = self.lookup(*lhs_id); @@ -275,26 +329,29 @@ impl<'ssa> Interpreter<'ssa> { let rhs = rhs.to_string(); let lhs_id = *lhs_id; let rhs_id = *rhs_id; - return Err(RuntimeError::ConstrainNeFailed { lhs, lhs_id, rhs, rhs_id }) + return Err(InterpreterError::ConstrainNeFailed { lhs, lhs_id, rhs, rhs_id }); } + Ok(()) } Instruction::RangeCheck { value, max_bit_size, assert_message } => { - self.interpret_range_check(*value, *max_bit_size, assert_message.as_ref()); + self.interpret_range_check(*value, *max_bit_size, assert_message.as_ref()) } - Instruction::Call { func, arguments } => { - self.interpret_call(*func, arguments, results)?; + Instruction::Call { func, arguments } => self.interpret_call(*func, arguments, results), + Instruction::Allocate => { + self.interpret_allocate(results[0]); + Ok(()) } - Instruction::Allocate => self.interpret_allocate(results[0]), Instruction::Load { address } => self.interpret_load(*address, results[0]), Instruction::Store { address, value } => self.interpret_store(*address, *value), Instruction::EnableSideEffectsIf { condition } => { - self.side_effects_enabled = self.lookup(*condition).as_bool().unwrap(); + self.side_effects_enabled = self.lookup_bool(*condition, "enable_side_effects")?; + Ok(()) } Instruction::ArrayGet { array, index } => { - self.interpret_array_get(*array, *index, results[0]); + self.interpret_array_get(*array, *index, results[0]) } Instruction::ArraySet { array, index, value, mutable } => { - self.interpret_array_set(*array, *index, *value, *mutable, results[0]); + self.interpret_array_set(*array, *index, *value, *mutable, results[0]) } Instruction::IncrementRc { value } => self.interpret_inc_rc(*value), Instruction::DecrementRc { value } => self.interpret_dec_rc(*value), @@ -308,16 +365,19 @@ impl<'ssa> Interpreter<'ssa> { ), Instruction::MakeArray { elements, typ } => { self.interpret_make_array(elements, results[0], typ); + Ok(()) } - Instruction::Noop => (), + Instruction::Noop => Ok(()), } - Ok(()) } - fn interpret_not(&mut self, id: ValueId, result: ValueId) { - let new_result = match self.lookup(id).as_numeric().unwrap() { - NumericValue::Field(field) => { - unreachable!("not: Expected integer value, found field {field}") + fn interpret_not(&mut self, id: ValueId, result: ValueId) -> IResult<()> { + let new_result = match self.lookup_numeric(id, "not instruction")? { + NumericValue::Field(_) => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "!", + typ: "Field", + }); } NumericValue::U1(value) => NumericValue::U1(!value), NumericValue::U8(value) => NumericValue::U8(!value), @@ -331,6 +391,7 @@ impl<'ssa> Interpreter<'ssa> { NumericValue::I64(value) => NumericValue::I64(!value), }; self.define(result, Value::Numeric(new_result)); + Ok(()) } fn interpret_truncate( @@ -339,39 +400,40 @@ impl<'ssa> Interpreter<'ssa> { bit_size: u32, _max_bit_size: u32, result: ValueId, - ) { - let value = self.lookup(value).as_numeric().unwrap(); + ) -> IResult<()> { + let value = self.lookup_numeric(value, "truncate")?; let bit_mask = (1u128 << bit_size) - 1; assert_ne!(bit_mask, 0); let truncated = match value { NumericValue::Field(value) => NumericValue::Field(truncate_field(value, bit_size)), NumericValue::U1(value) => NumericValue::U1(value), - NumericValue::U8(value) => NumericValue::U8(truncate_unsigned(value, bit_size)), - NumericValue::U16(value) => NumericValue::U16(truncate_unsigned(value, bit_size)), - NumericValue::U32(value) => NumericValue::U32(truncate_unsigned(value, bit_size)), - NumericValue::U64(value) => NumericValue::U64(truncate_unsigned(value, bit_size)), - NumericValue::U128(value) => NumericValue::U128(truncate_unsigned(value, bit_size)), - NumericValue::I8(value) => NumericValue::I8(truncate_signed(value, bit_size)), - NumericValue::I16(value) => NumericValue::I16(truncate_signed(value, bit_size)), - NumericValue::I32(value) => NumericValue::I32(truncate_signed(value, bit_size)), - NumericValue::I64(value) => NumericValue::I64(truncate_signed(value, bit_size)), + NumericValue::U8(value) => NumericValue::U8(truncate_unsigned(value, bit_size)?), + NumericValue::U16(value) => NumericValue::U16(truncate_unsigned(value, bit_size)?), + NumericValue::U32(value) => NumericValue::U32(truncate_unsigned(value, bit_size)?), + NumericValue::U64(value) => NumericValue::U64(truncate_unsigned(value, bit_size)?), + NumericValue::U128(value) => NumericValue::U128(truncate_unsigned(value, bit_size)?), + NumericValue::I8(value) => NumericValue::I8(truncate_signed(value, bit_size)?), + NumericValue::I16(value) => NumericValue::I16(truncate_signed(value, bit_size)?), + NumericValue::I32(value) => NumericValue::I32(truncate_signed(value, bit_size)?), + NumericValue::I64(value) => NumericValue::I64(truncate_signed(value, bit_size)?), }; self.define(result, Value::Numeric(truncated)); + Ok(()) } fn interpret_range_check( &mut self, - value: ValueId, + value_id: ValueId, max_bit_size: u32, error_message: Option<&String>, - ) { + ) -> IResult<()> { if !self.side_effects_enabled() { - return; + return Ok(()); } - let value = self.lookup(value).as_numeric().unwrap(); + let value = self.lookup_numeric(value_id, "range check")?; assert_ne!(max_bit_size, 0); fn bit_count(x: impl Into) -> u32 { @@ -382,7 +444,7 @@ impl<'ssa> Interpreter<'ssa> { let bit_count = match value { NumericValue::Field(value) => value.num_bits(), // max_bit_size > 0 so u1 should always pass these checks - NumericValue::U1(_) => return, + NumericValue::U1(_) => return Ok(()), NumericValue::U8(value) => bit_count(value), NumericValue::U16(value) => bit_count(value), NumericValue::U32(value) => bit_count(value), @@ -410,23 +472,33 @@ impl<'ssa> Interpreter<'ssa> { }; if bit_count > max_bit_size { + let value = value.to_string(); + let actual_bits = bit_count; + let max_bits = max_bit_size; + if let Some(message) = error_message { - panic!( - "bit count of {bit_count} exceeded max bit count of {max_bit_size}\n{message}" - ); + Err(InterpreterError::RangeCheckFailedWithMessage { + value, + value_id, + actual_bits, + max_bits, + message: message.clone(), + }) } else { - panic!("bit count of {bit_count} exceeded max bit count of {max_bit_size}"); + Err(InterpreterError::RangeCheckFailed { value, value_id, actual_bits, max_bits }) } + } else { + Ok(()) } } fn interpret_call( &mut self, - function: ValueId, + function_id: ValueId, argument_ids: &[ValueId], results: &[ValueId], ) -> IResult<()> { - let function = self.lookup(function); + let function = self.lookup(function_id); let mut arguments = vecmap(argument_ids, |argument| self.lookup(*argument)); let new_results = if self.side_effects_enabled() { @@ -438,7 +510,9 @@ impl<'ssa> Interpreter<'ssa> { if !self.in_unconstrained_context() && self.ssa.functions[&id].runtime().is_brillig() { - arguments.iter_mut().for_each(Self::reset_array_state); + for argument in arguments.iter_mut() { + Self::reset_array_state(argument)?; + } } self.call_function(id, arguments)? } @@ -447,9 +521,14 @@ impl<'ssa> Interpreter<'ssa> { } Value::ForeignFunction(name) if name == "print" => self.call_print(arguments)?, Value::ForeignFunction(name) => { - todo!("call: ForeignFunction({name}) is not yet implemented") + return Err(InterpreterError::UnknownForeignFunctionCall { name }); + } + other => { + return Err(InterpreterError::CalledNonFunction { + value: other.to_string(), + value_id: function_id, + }); } - other => panic!("call: Expected function, found {other:?}"), } } else { vecmap(results, |result| { @@ -458,33 +537,59 @@ impl<'ssa> Interpreter<'ssa> { }) }; - assert_eq!(new_results.len(), results.len()); + if new_results.len() != results.len() { + let function_name = self.try_get_function_name(function_id); + return Err(InterpreterError::FunctionReturnedIncorrectArgCount { + function: function_id, + function_name, + expected: results.len(), + actual: new_results.len(), + }); + } + for (result, new_result) in results.iter().zip(new_results) { self.define(*result, new_result); } Ok(()) } + /// Try to get a function's name or approximate it if it is not known + fn try_get_function_name(&self, function: ValueId) -> String { + match self.lookup(function) { + Value::Function(id) => match self.ssa.functions.get(&id) { + Some(function) => function.name().to_string(), + None => "unknown function".to_string(), + }, + Value::Intrinsic(intrinsic) => intrinsic.to_string(), + Value::ForeignFunction(name) => name, + _ => "non-function".to_string(), + } + } + /// Reset the value's `Shared` states in each array within. This is used to mimic each /// invocation of the brillig vm receiving fresh values. No matter the history of this value /// (e.g. even if they were previously returned from another brillig function) the reference /// count should always be 1 and it shouldn't alias any other arrays. - fn reset_array_state(value: &mut Value) { + fn reset_array_state(value: &mut Value) -> IResult<()> { match value { Value::Numeric(_) | Value::Function(_) | Value::Intrinsic(_) - | Value::ForeignFunction(_) => (), + | Value::ForeignFunction(_) => Ok(()), - Value::Reference(_) => panic!( - "No reference values are allowed when crossing the constrained -> unconstrained boundary" - ), + Value::Reference(value) => { + let value = value.to_string(); + Err(InterpreterError::ReferenceValueCrossedUnconstrainedBoundary { value }) + } Value::ArrayOrSlice(array_value) => { let mut elements = array_value.elements.borrow().to_vec(); - elements.iter_mut().for_each(Self::reset_array_state); + for element in elements.iter_mut() { + Self::reset_array_state(element)?; + } array_value.elements = Shared::new(elements); array_value.rc = Shared::new(1); + Ok(()) } } } @@ -500,39 +605,42 @@ impl<'ssa> Interpreter<'ssa> { self.define(result, Value::reference(result, element_type)); } - fn interpret_load(&mut self, address: ValueId, result: ValueId) { - let address = self.lookup(address); - let address = address.as_reference().unwrap(); + fn interpret_load(&mut self, address: ValueId, result: ValueId) -> IResult<()> { + let address = self.lookup_reference(address, "load")?; let element = address.element.borrow(); let Some(value) = &*element else { - panic!( - "reference value {} is being loaded before it was stored to", - address.original_id - ); + let value = address.to_string(); + return Err(InterpreterError::UninitializedReferenceValueLoaded { value }); }; self.define(result, value.clone()); + Ok(()) } - fn interpret_store(&mut self, address: ValueId, value: ValueId) { - let address = self.lookup(address); - let address = address.as_reference().unwrap(); + fn interpret_store(&mut self, address: ValueId, value: ValueId) -> IResult<()> { + let address = self.lookup_reference(address, "store")?; let value = self.lookup(value); *address.element.borrow_mut() = Some(value); + Ok(()) } - fn interpret_array_get(&mut self, array: ValueId, index: ValueId, result: ValueId) { + fn interpret_array_get( + &mut self, + array: ValueId, + index: ValueId, + result: ValueId, + ) -> IResult<()> { let element = if self.side_effects_enabled() { - let array = self.lookup(array); - let array = array.as_array_or_slice().unwrap(); - let index = self.lookup(index).as_u32().unwrap(); + let array = self.lookup_array_or_slice(array, "array get")?; + let index = self.lookup_u32(index, "array get index")?; array.elements.borrow()[index as usize].clone() } else { let typ = self.dfg().type_of_value(result); Value::uninitialized(&typ, result) }; self.define(result, element); + Ok(()) } fn interpret_array_set( @@ -542,11 +650,11 @@ impl<'ssa> Interpreter<'ssa> { value: ValueId, mutable: bool, result: ValueId, - ) { + ) -> IResult<()> { + let array = self.lookup_array_or_slice(array, "array set")?; + let result_array = if self.side_effects_enabled() { - let array = self.lookup(array); - let array = array.as_array_or_slice().unwrap(); - let index = self.lookup(index).as_u32().unwrap(); + let index = self.lookup_u32(index, "array set index")?; let value = self.lookup(value); let should_mutate = @@ -566,31 +674,37 @@ impl<'ssa> Interpreter<'ssa> { } } else { // Side effects are disabled, return the original array - self.lookup(array) + Value::ArrayOrSlice(array) }; self.define(result, result_array); + Ok(()) } - fn interpret_inc_rc(&self, array: ValueId) { + fn interpret_inc_rc(&self, value_id: ValueId) -> IResult<()> { if self.in_unconstrained_context() { - let array = self.lookup(array); - let array = array.as_array_or_slice().unwrap(); + let array = self.lookup_array_or_slice(value_id, "inc_rc")?; let mut rc = array.rc.borrow_mut(); - + if *rc == 0 { + let value = array.to_string(); + return Err(InterpreterError::IncRcRevive { value_id, value }); + } assert_ne!(*rc, 0, "inc_rc: increment from 0 back to 1 detected"); *rc += 1; } + Ok(()) } - fn interpret_dec_rc(&self, array: ValueId) { + fn interpret_dec_rc(&self, value_id: ValueId) -> IResult<()> { if self.in_unconstrained_context() { - let array = self.lookup(array); - let array = array.as_array_or_slice().unwrap(); + let array = self.lookup_array_or_slice(value_id, "dec_rc")?; let mut rc = array.rc.borrow_mut(); - - assert_ne!(*rc, 0, "dec_rc: underflow detected"); + if *rc == 0 { + let value = array.to_string(); + return Err(InterpreterError::DecRcUnderflow { value_id, value }); + } *rc -= 1; } + Ok(()) } fn interpret_if_else( @@ -600,9 +714,9 @@ impl<'ssa> Interpreter<'ssa> { else_condition: ValueId, else_value: ValueId, result: ValueId, - ) { - let then_condition = self.lookup(then_condition).as_bool().unwrap(); - let else_condition = self.lookup(else_condition).as_bool().unwrap(); + ) -> IResult<()> { + let then_condition = self.lookup_bool(then_condition, "then condition")?; + let else_condition = self.lookup_bool(else_condition, "else condition")?; let then_value = self.lookup(then_value); let else_value = self.lookup(else_value); @@ -625,6 +739,7 @@ impl<'ssa> Interpreter<'ssa> { }; self.define(result, new_result); + Ok(()) } fn interpret_make_array( @@ -647,11 +762,13 @@ impl<'ssa> Interpreter<'ssa> { } macro_rules! apply_int_binop { - ($lhs:expr, $rhs:expr, $f:expr) => {{ + ($lhs:expr, $rhs:expr, $binary:expr, $f:expr) => {{ use value::NumericValue::*; match ($lhs, $rhs) { - (Field(_), Field(_)) => panic!("Expected only integer values, found field values"), - (U1(_), U1(_)) => panic!("Expected only large integer values, found u1"), + (Field(_), Field(_)) => { + unreachable!("Expected only integer values, found field values") + } + (U1(_), U1(_)) => unreachable!("Expected only large integer values, found u1"), (U8(lhs), U8(rhs)) => U8($f(&lhs, &rhs)), (U16(lhs), U16(rhs)) => U16($f(&lhs, &rhs)), (U32(lhs), U32(rhs)) => U32($f(&lhs, &rhs)), @@ -661,38 +778,78 @@ macro_rules! apply_int_binop { (I16(lhs), I16(rhs)) => I16($f(&lhs, &rhs)), (I32(lhs), I32(rhs)) => I32($f(&lhs, &rhs)), (I64(lhs), I64(rhs)) => I64($f(&lhs, &rhs)), - (lhs, rhs) => panic!("Got mismatched types in binop: {lhs:?} and {rhs:?}"), + (lhs, rhs) => { + let binary = $binary; + return Err(InterpreterError::MismatchedTypesInBinaryOperator { + lhs: lhs.to_string(), + rhs: rhs.to_string(), + operator: binary.operator, + lhs_id: binary.lhs, + rhs_id: binary.rhs, + }); + } } }}; } macro_rules! apply_int_binop_opt { - ($lhs:expr, $rhs:expr, $f:expr) => {{ + ($dfg:expr, $lhs:expr, $rhs:expr, $binary:expr, $f:expr) => {{ use value::NumericValue::*; - // TODO: Error if None instead of unwrapping - match ($lhs, $rhs) { - (Field(_), Field(_)) => panic!("Expected only integer values, found field values"), - (U1(_), U1(_)) => panic!("Expected only large integer values, found u1"), - (U8(lhs), U8(rhs)) => U8($f(&lhs, &rhs).unwrap()), - (U16(lhs), U16(rhs)) => U16($f(&lhs, &rhs).unwrap()), - (U32(lhs), U32(rhs)) => U32($f(&lhs, &rhs).unwrap()), - (U64(lhs), U64(rhs)) => U64($f(&lhs, &rhs).unwrap()), - (U128(lhs), U128(rhs)) => U128($f(&lhs, &rhs).unwrap()), - (I8(lhs), I8(rhs)) => I8($f(&lhs, &rhs).unwrap()), - (I16(lhs), I16(rhs)) => I16($f(&lhs, &rhs).unwrap()), - (I32(lhs), I32(rhs)) => I32($f(&lhs, &rhs).unwrap()), - (I64(lhs), I64(rhs)) => I64($f(&lhs, &rhs).unwrap()), - (lhs, rhs) => panic!("Got mismatched types in binop: {lhs:?} and {rhs:?}"), + + let lhs = $lhs; + let rhs = $rhs; + let binary = $binary; + let operator = binary.operator; + + let overflow = || { + if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) { + let lhs_id = binary.lhs; + let rhs_id = binary.rhs; + let lhs = lhs.to_string(); + let rhs = rhs.to_string(); + InterpreterError::DivisionByZero { lhs_id, lhs, rhs_id, rhs } + } else { + let instruction = + format!("`{}` ({operator} {lhs}, {rhs})", display_binary(binary, $dfg)); + InterpreterError::Overflow { instruction } + } + }; + + match (lhs, rhs) { + (Field(_), Field(_)) => { + unreachable!("Expected only integer values, found field values") + } + (U1(_), U1(_)) => unreachable!("Expected only large integer values, found u1"), + (U8(lhs), U8(rhs)) => U8($f(&lhs, &rhs).ok_or_else(overflow)?), + (U16(lhs), U16(rhs)) => U16($f(&lhs, &rhs).ok_or_else(overflow)?), + (U32(lhs), U32(rhs)) => U32($f(&lhs, &rhs).ok_or_else(overflow)?), + (U64(lhs), U64(rhs)) => U64($f(&lhs, &rhs).ok_or_else(overflow)?), + (U128(lhs), U128(rhs)) => U128($f(&lhs, &rhs).ok_or_else(overflow)?), + (I8(lhs), I8(rhs)) => I8($f(&lhs, &rhs).ok_or_else(overflow)?), + (I16(lhs), I16(rhs)) => I16($f(&lhs, &rhs).ok_or_else(overflow)?), + (I32(lhs), I32(rhs)) => I32($f(&lhs, &rhs).ok_or_else(overflow)?), + (I64(lhs), I64(rhs)) => I64($f(&lhs, &rhs).ok_or_else(overflow)?), + (lhs, rhs) => { + return Err(InterpreterError::MismatchedTypesInBinaryOperator { + lhs: lhs.to_string(), + rhs: rhs.to_string(), + operator, + lhs_id: binary.lhs, + rhs_id: binary.rhs, + }); + } } }}; } macro_rules! apply_int_comparison_op { - ($lhs:expr, $rhs:expr, $f:expr) => {{ + ($lhs:expr, $rhs:expr, $binary:expr, $f:expr) => {{ use NumericValue::*; match ($lhs, $rhs) { - (Field(_), Field(_)) => panic!("Expected only integer values, found field values"), - (U1(_), U1(_)) => panic!("Expected only large integer values, found u1"), + (Field(_), Field(_)) => { + unreachable!("Expected only integer values, found field values") + } + (U1(_), U1(_)) => unreachable!("Expected only large integer values, found u1"), (U8(lhs), U8(rhs)) => U1($f(&lhs, &rhs)), (U16(lhs), U16(rhs)) => U1($f(&lhs, &rhs)), (U32(lhs), U32(rhs)) => U1($f(&lhs, &rhs)), @@ -702,28 +859,37 @@ macro_rules! apply_int_comparison_op { (I16(lhs), I16(rhs)) => U1($f(&lhs, &rhs)), (I32(lhs), I32(rhs)) => U1($f(&lhs, &rhs)), (I64(lhs), I64(rhs)) => U1($f(&lhs, &rhs)), - (lhs, rhs) => panic!("Got mismatched types in binop: {lhs:?} and {rhs:?}"), + (lhs, rhs) => { + let binary = $binary; + return Err(InterpreterError::MismatchedTypesInBinaryOperator { + lhs: lhs.to_string(), + rhs: rhs.to_string(), + operator: binary.operator, + lhs_id: binary.lhs, + rhs_id: binary.rhs, + }); + } } }}; } impl Interpreter<'_> { fn interpret_binary(&mut self, binary: &Binary) -> IResult { - // TODO: Replace unwrap with real error - let lhs = self.lookup(binary.lhs).as_numeric().unwrap(); - let rhs = self.lookup(binary.rhs).as_numeric().unwrap(); + let lhs_id = binary.lhs; + let rhs_id = binary.rhs; + let lhs = self.lookup_numeric(lhs_id, "binary op lhs")?; + let rhs = self.lookup_numeric(rhs_id, "binary op rhs")?; if lhs.get_type() != rhs.get_type() && !matches!(binary.operator, BinaryOp::Shl | BinaryOp::Shr) { - panic!( - "Type error in ({}: {}) {} ({}: {})", - binary.lhs, - lhs.get_type(), - binary.operator, - binary.rhs, - rhs.get_type() - ) + return Err(InterpreterError::MismatchedTypesInBinaryOperator { + lhs_id, + lhs: lhs.to_string(), + operator: binary.operator, + rhs_id, + rhs: rhs.to_string(), + }); } // Disable this instruction if it is side-effectful and side effects are disabled. @@ -733,75 +899,116 @@ impl Interpreter<'_> { } if let (Some(lhs), Some(rhs)) = (lhs.as_field(), rhs.as_field()) { - return self.interpret_field_binary_op(lhs, binary.operator, rhs); + return self.interpret_field_binary_op(lhs, binary.operator, rhs, lhs_id, rhs_id); } if let (Some(lhs), Some(rhs)) = (lhs.as_bool(), rhs.as_bool()) { return self.interpret_u1_binary_op(lhs, binary.operator, rhs); } + let dfg = self.dfg(); let result = match binary.operator { BinaryOp::Add { unchecked: false } => { - apply_int_binop_opt!(lhs, rhs, num_traits::CheckedAdd::checked_add) + apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedAdd::checked_add) } BinaryOp::Add { unchecked: true } => { - apply_int_binop!(lhs, rhs, num_traits::WrappingAdd::wrapping_add) + apply_int_binop!(lhs, rhs, binary, num_traits::WrappingAdd::wrapping_add) } BinaryOp::Sub { unchecked: false } => { - apply_int_binop_opt!(lhs, rhs, num_traits::CheckedSub::checked_sub) + apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedSub::checked_sub) } BinaryOp::Sub { unchecked: true } => { - apply_int_binop!(lhs, rhs, num_traits::WrappingSub::wrapping_sub) + apply_int_binop!(lhs, rhs, binary, num_traits::WrappingSub::wrapping_sub) } BinaryOp::Mul { unchecked: false } => { - apply_int_binop_opt!(lhs, rhs, num_traits::CheckedMul::checked_mul) + apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedMul::checked_mul) } BinaryOp::Mul { unchecked: true } => { - apply_int_binop!(lhs, rhs, num_traits::WrappingMul::wrapping_mul) + apply_int_binop!(lhs, rhs, binary, num_traits::WrappingMul::wrapping_mul) } BinaryOp::Div => { - apply_int_binop_opt!(lhs, rhs, num_traits::CheckedDiv::checked_div) + apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedDiv::checked_div) } BinaryOp::Mod => { - apply_int_binop_opt!(lhs, rhs, num_traits::CheckedRem::checked_rem) + apply_int_binop_opt!(dfg, lhs, rhs, binary, num_traits::CheckedRem::checked_rem) } - BinaryOp::Eq => apply_int_comparison_op!(lhs, rhs, |a, b| a == b), - BinaryOp::Lt => apply_int_comparison_op!(lhs, rhs, |a, b| a < b), + BinaryOp::Eq => apply_int_comparison_op!(lhs, rhs, binary, |a, b| a == b), + BinaryOp::Lt => apply_int_comparison_op!(lhs, rhs, binary, |a, b| a < b), BinaryOp::And => { - apply_int_binop!(lhs, rhs, std::ops::BitAnd::bitand) + apply_int_binop!(lhs, rhs, binary, std::ops::BitAnd::bitand) } BinaryOp::Or => { - apply_int_binop!(lhs, rhs, std::ops::BitOr::bitor) + apply_int_binop!(lhs, rhs, binary, std::ops::BitOr::bitor) } BinaryOp::Xor => { - apply_int_binop!(lhs, rhs, std::ops::BitXor::bitxor) + apply_int_binop!(lhs, rhs, binary, std::ops::BitXor::bitxor) } BinaryOp::Shl => { - let rhs = rhs.as_u32().expect("Expected rhs of shl to be a u32"); - let overflow_msg = "Overflow when evaluating `shl`, `rhs` is too large"; + let Some(rhs) = rhs.as_u32() else { + let rhs = rhs.to_string(); + return Err(InterpreterError::RhsOfBitShiftShouldBeU32 { + operator: "<<", + rhs_id, + rhs, + }); + }; + + let overflow = || { + let instruction = + format!("`{}` ({lhs} << {rhs})", display_binary(binary, self.dfg())); + InterpreterError::Overflow { instruction } + }; + use NumericValue::*; match lhs { - Field(_) => unreachable!("<< is not implemented for Field"), - U1(_) => unreachable!("<< is not implemented for u1"), - U8(value) => U8(value.checked_shl(rhs).expect(overflow_msg)), - U16(value) => U16(value.checked_shl(rhs).expect(overflow_msg)), - U32(value) => U32(value.checked_shl(rhs).expect(overflow_msg)), - U64(value) => U64(value.checked_shl(rhs).expect(overflow_msg)), - U128(value) => U128(value.checked_shl(rhs).expect(overflow_msg)), - I8(value) => I8(value.checked_shl(rhs).expect(overflow_msg)), - I16(value) => I16(value.checked_shl(rhs).expect(overflow_msg)), - I32(value) => I32(value.checked_shl(rhs).expect(overflow_msg)), - I64(value) => I64(value.checked_shl(rhs).expect(overflow_msg)), + Field(_) => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "<<", + typ: "Field", + }); + } + U1(_) => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "<<", + typ: "u1", + }); + } + U8(value) => U8(value.checked_shl(rhs).ok_or_else(overflow)?), + U16(value) => U16(value.checked_shl(rhs).ok_or_else(overflow)?), + U32(value) => U32(value.checked_shl(rhs).ok_or_else(overflow)?), + U64(value) => U64(value.checked_shl(rhs).ok_or_else(overflow)?), + U128(value) => U128(value.checked_shl(rhs).ok_or_else(overflow)?), + I8(value) => I8(value.checked_shl(rhs).ok_or_else(overflow)?), + I16(value) => I16(value.checked_shl(rhs).ok_or_else(overflow)?), + I32(value) => I32(value.checked_shl(rhs).ok_or_else(overflow)?), + I64(value) => I64(value.checked_shl(rhs).ok_or_else(overflow)?), } } BinaryOp::Shr => { let zero = || NumericValue::zero(lhs.get_type()); - let rhs = rhs.as_u32().expect("Expected rhs of shr to be a u32"); + let Some(rhs) = rhs.as_u32() else { + let rhs = rhs.to_string(); + return Err(InterpreterError::RhsOfBitShiftShouldBeU32 { + operator: ">>", + rhs_id, + rhs, + }); + }; use NumericValue::*; match lhs { - Field(_) => unreachable!(">> is not implemented for Field"), - U1(_) => unreachable!(">> is not implemented for u1"), + Field(_) => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: ">>", + typ: "Field", + }); + } + U1(_) => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: ">>", + typ: "u1", + }); + } U8(value) => value.checked_shr(rhs).map(U8).unwrap_or_else(zero), U16(value) => value.checked_shr(rhs).map(U16).unwrap_or_else(zero), U32(value) => value.checked_shr(rhs).map(U32).unwrap_or_else(zero), @@ -822,26 +1029,59 @@ impl Interpreter<'_> { lhs: FieldElement, operator: BinaryOp, rhs: FieldElement, + lhs_id: ValueId, + rhs_id: ValueId, ) -> IResult { let result = match operator { BinaryOp::Add { unchecked: _ } => NumericValue::Field(lhs + rhs), BinaryOp::Sub { unchecked: _ } => NumericValue::Field(lhs - rhs), BinaryOp::Mul { unchecked: _ } => NumericValue::Field(lhs * rhs), BinaryOp::Div => { - // FieldElement::div returns a value with panicking on divide by zero if rhs.is_zero() { - panic!("Field division by zero"); + let lhs = lhs.to_string(); + let rhs = rhs.to_string(); + return Err(InterpreterError::DivisionByZero { lhs_id, lhs, rhs_id, rhs }); } NumericValue::Field(lhs / rhs) } - BinaryOp::Mod => panic!("Unsupported operator `%` for Field"), + BinaryOp::Mod => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "%", + typ: "Field", + }); + } BinaryOp::Eq => NumericValue::U1(lhs == rhs), BinaryOp::Lt => NumericValue::U1(lhs < rhs), - BinaryOp::And => panic!("Unsupported operator `&` for Field"), - BinaryOp::Or => panic!("Unsupported operator `|` for Field"), - BinaryOp::Xor => panic!("Unsupported operator `^` for Field"), - BinaryOp::Shl => panic!("Unsupported operator `<<` for Field"), - BinaryOp::Shr => panic!("Unsupported operator `>>` for Field"), + BinaryOp::And => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "&", + typ: "Field", + }); + } + BinaryOp::Or => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "|", + typ: "Field", + }); + } + BinaryOp::Xor => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "^", + typ: "Field", + }); + } + BinaryOp::Shl => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "<<", + typ: "Field", + }); + } + BinaryOp::Shr => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: ">>", + typ: "Field", + }); + } }; Ok(Value::Numeric(result)) } @@ -853,44 +1093,76 @@ impl Interpreter<'_> { rhs: bool, ) -> IResult { let result = match operator { - BinaryOp::Add { unchecked: _ } => panic!("Unsupported operator `+` for u1"), - BinaryOp::Sub { unchecked: _ } => panic!("Unsupported operator `-` for u1"), + BinaryOp::Add { unchecked: _ } => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "+", + typ: "u1", + }); + } + BinaryOp::Sub { unchecked: _ } => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "-", + typ: "u1", + }); + } BinaryOp::Mul { unchecked: _ } => lhs & rhs, // (*) = (&) for u1 - BinaryOp::Div => panic!("Unsupported operator `/` for u1"), - BinaryOp::Mod => panic!("Unsupported operator `%` for u1"), + BinaryOp::Div => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "/", + typ: "u1", + }); + } + BinaryOp::Mod => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "%", + typ: "u1", + }); + } BinaryOp::Eq => lhs == rhs, // clippy complains when you do `lhs < rhs` and recommends this instead BinaryOp::Lt => !lhs & rhs, BinaryOp::And => lhs & rhs, BinaryOp::Or => lhs | rhs, BinaryOp::Xor => lhs ^ rhs, - BinaryOp::Shl => panic!("Unsupported operator `<<` for u1"), - BinaryOp::Shr => panic!("Unsupported operator `>>` for u1"), + BinaryOp::Shl => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: "<<", + typ: "u1", + }); + } + BinaryOp::Shr => { + return Err(InterpreterError::UnsupportedOperatorForType { + operator: ">>", + typ: "u1", + }); + } }; Ok(Value::Numeric(NumericValue::U1(result))) } } -fn truncate_unsigned(value: T, bit_size: u32) -> T +fn truncate_unsigned(value: T, bit_size: u32) -> IResult where u128: From, T: TryFrom, >::Error: std::fmt::Debug, { let value_u128 = u128::from(value); - let bit_mask = match bit_size.cmp(&128) { + let bit_mask = match bit_size.cmp(&MAX_UNSIGNED_BIT_SIZE) { Ordering::Less => (1u128 << bit_size) - 1, Ordering::Equal => u128::MAX, - Ordering::Greater => panic!("truncate: Invalid bit size: {bit_size}"), + Ordering::Greater => { + return Err(InterpreterError::InvalidUnsignedTruncateBitSize { bit_size }); + } }; let result = value_u128 & bit_mask; - T::try_from(result).expect( + Ok(T::try_from(result).expect( "The truncated result should always be smaller than or equal to the original `value`", - ) + )) } -fn truncate_signed(value: T, bit_size: u32) -> T +fn truncate_signed(value: T, bit_size: u32) -> IResult where i128: From, T: TryFrom + num_traits::Bounded, @@ -900,19 +1172,21 @@ where if value_i128 < 0 { let max = 1i128 << (bit_size - 1); value_i128 += max; - assert!(bit_size <= 64, "The maximum bit size for signed integers is 64"); + if bit_size > MAX_SIGNED_BIT_SIZE { + return Err(InterpreterError::InvalidSignedTruncateBitSize { bit_size }); + } let mask = (1i128 << bit_size) - 1; let result = (value_i128 & mask) - max; - T::try_from(result).expect( + Ok(T::try_from(result).expect( "The truncated result should always be smaller than or equal to the original `value`", - ) + )) } else { - let result = truncate_unsigned::(value_i128 as u128, bit_size) as i128; - T::try_from(result).expect( + let result = truncate_unsigned::(value_i128 as u128, bit_size)? as i128; + Ok(T::try_from(result).expect( "The truncated result should always be smaller than or equal to the original `value`", - ) + )) } } @@ -920,27 +1194,27 @@ where mod test { #[test] fn test_truncate_unsigned() { - assert_eq!(super::truncate_unsigned(57_u32, 8), 57); - assert_eq!(super::truncate_unsigned(257_u16, 8), 1); - assert_eq!(super::truncate_unsigned(130_u8, 7), 2); - assert_eq!(super::truncate_unsigned(u8::MAX, 8), u8::MAX); - assert_eq!(super::truncate_unsigned(u128::MAX, 128), u128::MAX); + assert_eq!(super::truncate_unsigned(57_u32, 8).unwrap(), 57); + assert_eq!(super::truncate_unsigned(257_u16, 8).unwrap(), 1); + assert_eq!(super::truncate_unsigned(130_u8, 7).unwrap(), 2); + assert_eq!(super::truncate_unsigned(u8::MAX, 8).unwrap(), u8::MAX); + assert_eq!(super::truncate_unsigned(u128::MAX, 128).unwrap(), u128::MAX); } #[test] fn test_truncate_signed() { - assert_eq!(super::truncate_signed(57_i32, 8), 57); - assert_eq!(super::truncate_signed(257_i16, 8), 1); - assert_eq!(super::truncate_signed(130_i64, 7), 2); - assert_eq!(super::truncate_signed(i16::MAX, 16), i16::MAX); - - assert_eq!(super::truncate_signed(-57_i32, 8), -57); - assert_eq!(super::truncate_signed(-1_i64, 3), -1_i64); - assert_eq!(super::truncate_signed(-258_i16, 8), -2); - assert_eq!(super::truncate_signed(-130_i16, 7), -2); - assert_eq!(super::truncate_signed(i8::MIN, 8), i8::MIN); - assert_eq!(super::truncate_signed(-8_i8, 4), -8); - assert_eq!(super::truncate_signed(-8_i8, 3), 0); - assert_eq!(super::truncate_signed(-129_i32, 8), 127); + assert_eq!(super::truncate_signed(57_i32, 8).unwrap(), 57); + assert_eq!(super::truncate_signed(257_i16, 8).unwrap(), 1); + assert_eq!(super::truncate_signed(130_i64, 7).unwrap(), 2); + assert_eq!(super::truncate_signed(i16::MAX, 16).unwrap(), i16::MAX); + + assert_eq!(super::truncate_signed(-57_i32, 8).unwrap(), -57); + assert_eq!(super::truncate_signed(-1_i64, 3).unwrap(), -1_i64); + assert_eq!(super::truncate_signed(-258_i16, 8).unwrap(), -2); + assert_eq!(super::truncate_signed(-130_i16, 7).unwrap(), -2); + assert_eq!(super::truncate_signed(i8::MIN, 8).unwrap(), i8::MIN); + assert_eq!(super::truncate_signed(-8_i8, 4).unwrap(), -8); + assert_eq!(super::truncate_signed(-8_i8, 3).unwrap(), 0); + assert_eq!(super::truncate_signed(-129_i32, 8).unwrap(), 127); } } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index 2e33a7ff6f9..512870c688b 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -5,7 +5,7 @@ use noirc_frontend::Shared; use crate::ssa::{ interpreter::{ - NumericValue, Value, + InterpreterError, NumericValue, Value, tests::{expect_value, expect_values}, value::ReferenceValue, }, @@ -31,11 +31,9 @@ fn add() { assert_eq!(value, Value::Numeric(NumericValue::I32(102))); } -/// TODO: Replace panic with error #[test] -#[should_panic(expected = "called `Option::unwrap()` on a `None` value")] fn add_overflow() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -44,6 +42,7 @@ fn add_overflow() { } ", ); + assert!(matches!(error, InterpreterError::Overflow { .. })) } #[test] @@ -73,11 +72,9 @@ fn sub() { assert_eq!(value, Value::Numeric(NumericValue::I32(10000))); } -/// TODO: Replace panic with error #[test] -#[should_panic(expected = "called `Option::unwrap()` on a `None` value")] fn sub_underflow() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -86,6 +83,7 @@ fn sub_underflow() { } ", ); + assert!(matches!(error, InterpreterError::Overflow { .. })) } #[test] @@ -116,11 +114,9 @@ fn mul() { assert_eq!(value, Value::Numeric(NumericValue::U64(200))); } -/// TODO: Replace panic with error #[test] -#[should_panic(expected = "called `Option::unwrap()` on a `None` value")] fn mul_overflow() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -129,6 +125,7 @@ fn mul_overflow() { } ", ); + assert!(matches!(error, InterpreterError::Overflow { .. })) } #[test] @@ -159,11 +156,9 @@ fn div() { assert_eq!(value, Value::Numeric(NumericValue::I16(64))); } -/// TODO: Replace panic with error #[test] -#[should_panic(expected = "Field division by zero")] fn div_zero() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -172,6 +167,7 @@ fn div_zero() { } ", ); + assert!(matches!(error, InterpreterError::DivisionByZero { .. })) } #[test] @@ -188,11 +184,9 @@ fn r#mod() { assert_eq!(value, Value::Numeric(NumericValue::I64(2))); } -/// TODO: Replace panic with error #[test] -#[should_panic(expected = "called `Option::unwrap()` on a `None` value")] fn mod_zero() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -201,6 +195,8 @@ fn mod_zero() { } ", ); + dbg!(&error); + assert!(matches!(error, InterpreterError::DivisionByZero { .. })) } #[test] @@ -304,11 +300,10 @@ fn shl() { assert_eq!(value, Value::from_constant(12_u128.into(), NumericType::unsigned(8))); } -#[test] -#[should_panic] /// shl should overflow if the rhs is greater than the bit count +#[test] fn shl_overflow() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -317,6 +312,7 @@ fn shl_overflow() { } ", ); + assert!(matches!(error, InterpreterError::Overflow { .. })); } #[test] @@ -477,9 +473,8 @@ fn range_check() { } #[test] -#[should_panic] fn range_check_fail() { - expect_error( + let error = expect_error( " acir(inline) fn main f0 { b0(): @@ -488,6 +483,7 @@ fn range_check_fail() { } ", ); + assert!(matches!(error, InterpreterError::RangeCheckFailed { .. })); } #[test] diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/mod.rs index 7e5cc229d58..36b27ca8cef 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/mod.rs @@ -4,15 +4,12 @@ use std::sync::Arc; use acvm::{AcirField, FieldElement}; -use crate::{ - errors::RuntimeError, - ssa::{ - interpreter::value::NumericValue, - ir::types::{NumericType, Type}, - }, +use crate::ssa::{ + interpreter::value::NumericValue, + ir::types::{NumericType, Type}, }; -use super::{Ssa, Value}; +use super::{InterpreterError, Ssa, Value}; mod instructions; @@ -33,7 +30,7 @@ fn expect_value(src: &str) -> Value { } #[track_caller] -fn expect_error(src: &str) -> RuntimeError { +fn expect_error(src: &str) -> InterpreterError { let ssa = Ssa::from_str(src).unwrap(); ssa.interpret(Vec::new()).unwrap_err() } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs index 10ad7a86c2c..04292d9822d 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs @@ -111,16 +111,16 @@ impl Value { } } - pub(crate) fn as_reference(&self) -> Option<&ReferenceValue> { + pub(crate) fn as_reference(&self) -> Option { match self { - Value::Reference(value) => Some(value), + Value::Reference(value) => Some(value.clone()), _ => None, } } - pub(crate) fn as_array_or_slice(&self) -> Option<&ArrayValue> { + pub(crate) fn as_array_or_slice(&self) -> Option { match self { - Value::ArrayOrSlice(value) => Some(value), + Value::ArrayOrSlice(value) => Some(value.clone()), _ => None, } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 76393d2b7b4..9d9d6579783 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -190,7 +190,7 @@ fn display_instruction_inner( match instruction { Instruction::Binary(binary) => { - writeln!(f, "{} {}, {}", binary.operator, show(binary.lhs), show(binary.rhs)) + writeln!(f, "{}", display_binary(binary, dfg)) } Instruction::Cast(lhs, typ) => writeln!(f, "cast {} as {typ}", show(*lhs)), Instruction::Not(rhs) => writeln!(f, "not {}", show(*rhs)), @@ -307,6 +307,10 @@ fn display_instruction_inner( } } +pub(crate) fn display_binary(binary: &super::instruction::Binary, dfg: &DataFlowGraph) -> String { + format!("{} {}, {}", binary.operator, value(dfg, binary.lhs), value(dfg, binary.rhs)) +} + fn try_byte_array_to_string(elements: &Vector, dfg: &DataFlowGraph) -> Option { let mut string = String::new(); for element in elements { From 5aca182388f65ef3080579acabb481d1d3d6c620 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 1 May 2025 15:22:11 -0500 Subject: [PATCH 3/7] Remove some last asserts --- .../src/ssa/interpreter/errors.rs | 12 +++++++ .../src/ssa/interpreter/mod.rs | 32 ++++++++++++------- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index 15544d8568e..751ad74a3ab 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -90,4 +90,16 @@ pub(crate) enum InterpreterError { expected: usize, actual: usize, }, + #[error( + "if-else instruction with then condition `{then_condition_id}` and else condition `{else_condition_id}` has both branches as true. This should be impossible except for malformed SSA code" + )] + DoubleTrueIfElse { then_condition_id: ValueId, else_condition_id: ValueId }, + #[error( + "`truncate {value_id} to 0 bits, max_bit_size: {max_bit_size}` has invalid bit size 0. This should only be possible in malformed SSA." + )] + TruncateToZeroBits { value_id: ValueId, max_bit_size: u32 }, + #[error( + "`range_check {value_id} to 0 bits` has invalid bit size 0. This should only be possible in malformed SSA." + )] + RangeCheckToZeroBits { value_id: ValueId }, } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index d0eb8740989..db4a47163ee 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -396,14 +396,15 @@ impl<'ssa> Interpreter<'ssa> { fn interpret_truncate( &mut self, - value: ValueId, + value_id: ValueId, bit_size: u32, - _max_bit_size: u32, + max_bit_size: u32, result: ValueId, ) -> IResult<()> { - let value = self.lookup_numeric(value, "truncate")?; - let bit_mask = (1u128 << bit_size) - 1; - assert_ne!(bit_mask, 0); + let value = self.lookup_numeric(value_id, "truncate")?; + if bit_size == 0 { + return Err(InterpreterError::TruncateToZeroBits { value_id, max_bit_size }); + } let truncated = match value { NumericValue::Field(value) => NumericValue::Field(truncate_field(value, bit_size)), @@ -433,8 +434,11 @@ impl<'ssa> Interpreter<'ssa> { return Ok(()); } + if max_bit_size == 0 { + return Err(InterpreterError::RangeCheckToZeroBits { value_id }); + } + let value = self.lookup_numeric(value_id, "range check")?; - assert_ne!(max_bit_size, 0); fn bit_count(x: impl Into) -> u32 { let x = x.into(); @@ -688,7 +692,6 @@ impl<'ssa> Interpreter<'ssa> { let value = array.to_string(); return Err(InterpreterError::IncRcRevive { value_id, value }); } - assert_ne!(*rc, 0, "inc_rc: increment from 0 back to 1 detected"); *rc += 1; } Ok(()) @@ -709,14 +712,14 @@ impl<'ssa> Interpreter<'ssa> { fn interpret_if_else( &mut self, - then_condition: ValueId, + then_condition_id: ValueId, then_value: ValueId, - else_condition: ValueId, + else_condition_id: ValueId, else_value: ValueId, result: ValueId, ) -> IResult<()> { - let then_condition = self.lookup_bool(then_condition, "then condition")?; - let else_condition = self.lookup_bool(else_condition, "else condition")?; + let then_condition = self.lookup_bool(then_condition_id, "then condition")?; + let else_condition = self.lookup_bool(else_condition_id, "else condition")?; let then_value = self.lookup(then_value); let else_value = self.lookup(else_value); @@ -725,7 +728,12 @@ impl<'ssa> Interpreter<'ssa> { // then_condition = outer_condition & a // else_condition = outer_condition & !a // If `outer_condition` is false, both will be false. - assert!(!then_condition || !else_condition); + if then_condition && else_condition { + return Err(InterpreterError::DoubleTrueIfElse { + then_condition_id, + else_condition_id, + }); + } let new_result = if !then_condition && !else_condition { // Returning uninitialized/zero if both conditions are false to match From b2119d71ee0cbb9e0483408fc0b2b9e91938c5ac Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 May 2025 08:07:52 -0500 Subject: [PATCH 4/7] PR feedback --- .../src/ssa/interpreter/errors.rs | 54 ++--- .../src/ssa/interpreter/mod.rs | 185 +++++++----------- .../src/ssa/interpreter/tests/instructions.rs | 20 +- .../src/ssa/interpreter/value.rs | 4 +- 4 files changed, 118 insertions(+), 145 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index 751ad74a3ab..b5329abb372 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -6,12 +6,9 @@ pub(super) const MAX_UNSIGNED_BIT_SIZE: u32 = 128; #[derive(Debug, Error)] pub(crate) enum InterpreterError { - #[error( - "Argument count {arguments} to block {block} does not match the expected parameter count {parameters}" - )] - BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, - #[error("Block {block} is missing the terminator instruction")] - BlockMissingTerminator { block: BasicBlockId }, + /// 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}")] @@ -30,8 +27,35 @@ pub(crate) enum InterpreterError { max_bits: u32, message: String, }, + /// This is not an internal error since the SSA is still valid. We're just not able to + /// interpret it since we lack the context of what the external function is. #[error("Call to unknown foreign function {name}")] UnknownForeignFunctionCall { name: String }, + #[error("Division by zero: `div {lhs_id}, {rhs_id}` ({lhs} / {rhs})")] + DivisionByZero { lhs_id: ValueId, lhs: String, rhs_id: ValueId, rhs: String }, + #[error("Underflow in dec_rc when decrementing reference count of `{value_id} = {value}`")] + DecRcUnderflow { value_id: ValueId, value: String }, + #[error( + "Erroneously incremented reference count of value `{value_id} = {value}` from 0 back to 1" + )] + IncRcRevive { value_id: ValueId, value: String }, + #[error("An overflow occurred while evaluating {instruction}")] + Overflow { instruction: String }, + #[error( + "if-else instruction with then condition `{then_condition_id}` and else condition `{else_condition_id}` has both branches as true. This should be impossible except for malformed SSA code" + )] + DoubleTrueIfElse { then_condition_id: ValueId, else_condition_id: ValueId }, +} + +/// These errors can only result from interpreting malformed SSA +#[derive(Debug, Error)] +pub(crate) enum InternalError { + #[error( + "Argument count {arguments} to block {block} does not match the expected parameter count {parameters}" + )] + BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, + #[error("Block {block} is missing the terminator instruction")] + BlockMissingTerminator { block: BasicBlockId }, #[error("Cannot call non-function value {value_id} = {value}")] CalledNonFunction { value: String, value_id: ValueId }, // Note that we don't need to display the value_id because displaying a reference @@ -50,8 +74,6 @@ pub(crate) enum InterpreterError { rhs_id: ValueId, rhs: String, }, - #[error("Division by zero: `div {lhs_id}, {rhs_id}` ({lhs} / {rhs})")] - DivisionByZero { lhs_id: ValueId, lhs: String, rhs_id: ValueId, rhs: String }, #[error("Unsupported operator `{operator}` for type `{typ}`")] UnsupportedOperatorForType { operator: &'static str, typ: &'static str }, #[error( @@ -62,8 +84,8 @@ pub(crate) enum InterpreterError { "Invalid bit size of `{bit_size}` given to truncate, maximum size allowed for signed values is {MAX_SIGNED_BIT_SIZE}" )] InvalidSignedTruncateBitSize { bit_size: u32 }, - #[error("Rhs of `{operator}` should be a u32 but found `{rhs_id} = {rhs}`")] - RhsOfBitShiftShouldBeU32 { operator: &'static str, rhs_id: ValueId, rhs: String }, + #[error("Rhs of `{operator}` should be a u8 but found `{rhs_id} = {rhs}`")] + RhsOfBitShiftShouldBeU8 { operator: &'static str, rhs_id: ValueId, rhs: String }, #[error( "Expected {expected_type} value in {instruction} but instead found `{value_id} = {value}`" )] @@ -73,14 +95,6 @@ pub(crate) enum InterpreterError { expected_type: &'static str, instruction: &'static str, }, - #[error("Underflow in dec_rc when decrementing reference count of `{value_id} = {value}`")] - DecRcUnderflow { value_id: ValueId, value: String }, - #[error( - "Erroneously incremented reference count of value `{value_id} = {value}` from 0 back to 1" - )] - IncRcRevive { value_id: ValueId, value: String }, - #[error("An overflow occurred while evaluating {instruction}")] - Overflow { instruction: String }, #[error( "Function {function} ({function_name}) returned {actual} argument(s) but it was expected to return {expected}" )] @@ -90,10 +104,6 @@ pub(crate) enum InterpreterError { expected: usize, actual: usize, }, - #[error( - "if-else instruction with then condition `{then_condition_id}` and else condition `{else_condition_id}` has both branches as true. This should be impossible except for malformed SSA code" - )] - DoubleTrueIfElse { then_condition_id: ValueId, else_condition_id: ValueId }, #[error( "`truncate {value_id} to 0 bits, max_bit_size: {max_bit_size}` has invalid bit size 0. This should only be possible in malformed SSA." )] diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index db4a47163ee..31ff650804d 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -12,7 +12,7 @@ use super::{ }; use crate::ssa::ir::{instruction::binary::truncate_field, printer::display_binary}; use acvm::{AcirField, FieldElement}; -use errors::{InterpreterError, MAX_SIGNED_BIT_SIZE, MAX_UNSIGNED_BIT_SIZE}; +use errors::{InternalError, InterpreterError, MAX_SIGNED_BIT_SIZE, MAX_UNSIGNED_BIT_SIZE}; use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; use noirc_frontend::Shared; @@ -155,11 +155,11 @@ impl<'ssa> Interpreter<'ssa> { let block = &dfg[block_id]; if arguments.len() != block.parameters().len() { - return Err(InterpreterError::BlockArgumentCountMismatch { + return Err(internal(InternalError::BlockArgumentCountMismatch { block: block_id, arguments: arguments.len(), parameters: block.parameters().len(), - }); + })); } for (parameter, argument) in block.parameters().iter().zip(arguments) { @@ -173,7 +173,9 @@ impl<'ssa> Interpreter<'ssa> { match block.terminator() { None => { - return Err(InterpreterError::BlockMissingTerminator { block: block_id }); + return Err(internal(InternalError::BlockMissingTerminator { + block: block_id, + })); } Some(TerminatorInstruction::Jmp { destination, arguments: jump_args, .. }) => { block_id = *destination; @@ -238,7 +240,12 @@ impl<'ssa> Interpreter<'ssa> { Some(value) => Ok(value), None => { let value = value.to_string(); - Err(InterpreterError::TypeError { value_id, value, expected_type, instruction }) + Err(internal(InternalError::TypeError { + value_id, + value, + expected_type, + instruction, + })) } } } @@ -374,10 +381,10 @@ impl<'ssa> Interpreter<'ssa> { fn interpret_not(&mut self, id: ValueId, result: ValueId) -> IResult<()> { let new_result = match self.lookup_numeric(id, "not instruction")? { NumericValue::Field(_) => { - return Err(InterpreterError::UnsupportedOperatorForType { + return Err(internal(InternalError::UnsupportedOperatorForType { operator: "!", typ: "Field", - }); + })); } NumericValue::U1(value) => NumericValue::U1(!value), NumericValue::U8(value) => NumericValue::U8(!value), @@ -403,7 +410,7 @@ impl<'ssa> Interpreter<'ssa> { ) -> IResult<()> { let value = self.lookup_numeric(value_id, "truncate")?; if bit_size == 0 { - return Err(InterpreterError::TruncateToZeroBits { value_id, max_bit_size }); + return Err(internal(InternalError::TruncateToZeroBits { value_id, max_bit_size })); } let truncated = match value { @@ -435,7 +442,7 @@ impl<'ssa> Interpreter<'ssa> { } if max_bit_size == 0 { - return Err(InterpreterError::RangeCheckToZeroBits { value_id }); + return Err(internal(InternalError::RangeCheckToZeroBits { value_id })); } let value = self.lookup_numeric(value_id, "range check")?; @@ -528,10 +535,10 @@ impl<'ssa> Interpreter<'ssa> { return Err(InterpreterError::UnknownForeignFunctionCall { name }); } other => { - return Err(InterpreterError::CalledNonFunction { + return Err(internal(InternalError::CalledNonFunction { value: other.to_string(), value_id: function_id, - }); + })); } } } else { @@ -543,12 +550,12 @@ impl<'ssa> Interpreter<'ssa> { if new_results.len() != results.len() { let function_name = self.try_get_function_name(function_id); - return Err(InterpreterError::FunctionReturnedIncorrectArgCount { + return Err(internal(InternalError::FunctionReturnedIncorrectArgCount { function: function_id, function_name, expected: results.len(), actual: new_results.len(), - }); + })); } for (result, new_result) in results.iter().zip(new_results) { @@ -583,7 +590,7 @@ impl<'ssa> Interpreter<'ssa> { Value::Reference(value) => { let value = value.to_string(); - Err(InterpreterError::ReferenceValueCrossedUnconstrainedBoundary { value }) + Err(internal(InternalError::ReferenceValueCrossedUnconstrainedBoundary { value })) } Value::ArrayOrSlice(array_value) => { @@ -615,7 +622,7 @@ impl<'ssa> Interpreter<'ssa> { let element = address.element.borrow(); let Some(value) = &*element else { let value = address.to_string(); - return Err(InterpreterError::UninitializedReferenceValueLoaded { value }); + return Err(internal(InternalError::UninitializedReferenceValueLoaded { value })); }; self.define(result, value.clone()); @@ -788,13 +795,13 @@ macro_rules! apply_int_binop { (I64(lhs), I64(rhs)) => I64($f(&lhs, &rhs)), (lhs, rhs) => { let binary = $binary; - return Err(InterpreterError::MismatchedTypesInBinaryOperator { + return Err(internal(InternalError::MismatchedTypesInBinaryOperator { lhs: lhs.to_string(), rhs: rhs.to_string(), operator: binary.operator, lhs_id: binary.lhs, rhs_id: binary.rhs, - }); + })); } } }}; @@ -838,13 +845,13 @@ macro_rules! apply_int_binop_opt { (I32(lhs), I32(rhs)) => I32($f(&lhs, &rhs).ok_or_else(overflow)?), (I64(lhs), I64(rhs)) => I64($f(&lhs, &rhs).ok_or_else(overflow)?), (lhs, rhs) => { - return Err(InterpreterError::MismatchedTypesInBinaryOperator { + return Err(internal(InternalError::MismatchedTypesInBinaryOperator { lhs: lhs.to_string(), rhs: rhs.to_string(), operator, lhs_id: binary.lhs, rhs_id: binary.rhs, - }); + })); } } }}; @@ -869,13 +876,13 @@ macro_rules! apply_int_comparison_op { (I64(lhs), I64(rhs)) => U1($f(&lhs, &rhs)), (lhs, rhs) => { let binary = $binary; - return Err(InterpreterError::MismatchedTypesInBinaryOperator { + return Err(internal(InternalError::MismatchedTypesInBinaryOperator { lhs: lhs.to_string(), rhs: rhs.to_string(), operator: binary.operator, lhs_id: binary.lhs, rhs_id: binary.rhs, - }); + })); } } }}; @@ -891,13 +898,13 @@ impl Interpreter<'_> { if lhs.get_type() != rhs.get_type() && !matches!(binary.operator, BinaryOp::Shl | BinaryOp::Shr) { - return Err(InterpreterError::MismatchedTypesInBinaryOperator { + return Err(internal(InternalError::MismatchedTypesInBinaryOperator { lhs_id, lhs: lhs.to_string(), operator: binary.operator, rhs_id, rhs: rhs.to_string(), - }); + })); } // Disable this instruction if it is side-effectful and side effects are disabled. @@ -952,13 +959,13 @@ impl Interpreter<'_> { apply_int_binop!(lhs, rhs, binary, std::ops::BitXor::bitxor) } BinaryOp::Shl => { - let Some(rhs) = rhs.as_u32() else { + let Some(rhs) = rhs.as_u8() else { let rhs = rhs.to_string(); - return Err(InterpreterError::RhsOfBitShiftShouldBeU32 { + return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 { operator: "<<", rhs_id, rhs, - }); + })); }; let overflow = || { @@ -967,19 +974,20 @@ impl Interpreter<'_> { InterpreterError::Overflow { instruction } }; + let rhs = rhs as u32; use NumericValue::*; match lhs { Field(_) => { - return Err(InterpreterError::UnsupportedOperatorForType { + return Err(internal(InternalError::UnsupportedOperatorForType { operator: "<<", typ: "Field", - }); + })); } U1(_) => { - return Err(InterpreterError::UnsupportedOperatorForType { + return Err(internal(InternalError::UnsupportedOperatorForType { operator: "<<", typ: "u1", - }); + })); } U8(value) => U8(value.checked_shl(rhs).ok_or_else(overflow)?), U16(value) => U16(value.checked_shl(rhs).ok_or_else(overflow)?), @@ -994,28 +1002,29 @@ impl Interpreter<'_> { } BinaryOp::Shr => { let zero = || NumericValue::zero(lhs.get_type()); - let Some(rhs) = rhs.as_u32() else { + let Some(rhs) = rhs.as_u8() else { let rhs = rhs.to_string(); - return Err(InterpreterError::RhsOfBitShiftShouldBeU32 { + return Err(internal(InternalError::RhsOfBitShiftShouldBeU8 { operator: ">>", rhs_id, rhs, - }); + })); }; + let rhs = rhs as u32; use NumericValue::*; match lhs { Field(_) => { - return Err(InterpreterError::UnsupportedOperatorForType { + return Err(internal(InternalError::UnsupportedOperatorForType { operator: ">>", typ: "Field", - }); + })); } U1(_) => { - return Err(InterpreterError::UnsupportedOperatorForType { + return Err(internal(InternalError::UnsupportedOperatorForType { operator: ">>", typ: "u1", - }); + })); } U8(value) => value.checked_shr(rhs).map(U8).unwrap_or_else(zero), U16(value) => value.checked_shr(rhs).map(U16).unwrap_or_else(zero), @@ -1040,6 +1049,11 @@ impl Interpreter<'_> { lhs_id: ValueId, rhs_id: ValueId, ) -> IResult { + let unsupported_operator = |operator| -> IResult { + let typ = "Field"; + Err(internal(InternalError::UnsupportedOperatorForType { operator, typ })) + }; + let result = match operator { BinaryOp::Add { unchecked: _ } => NumericValue::Field(lhs + rhs), BinaryOp::Sub { unchecked: _ } => NumericValue::Field(lhs - rhs), @@ -1052,44 +1066,14 @@ impl Interpreter<'_> { } NumericValue::Field(lhs / rhs) } - BinaryOp::Mod => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "%", - typ: "Field", - }); - } + BinaryOp::Mod => return unsupported_operator("%"), BinaryOp::Eq => NumericValue::U1(lhs == rhs), BinaryOp::Lt => NumericValue::U1(lhs < rhs), - BinaryOp::And => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "&", - typ: "Field", - }); - } - BinaryOp::Or => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "|", - typ: "Field", - }); - } - BinaryOp::Xor => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "^", - typ: "Field", - }); - } - BinaryOp::Shl => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "<<", - typ: "Field", - }); - } - BinaryOp::Shr => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: ">>", - typ: "Field", - }); - } + BinaryOp::And => return unsupported_operator("&"), + BinaryOp::Or => return unsupported_operator("|"), + BinaryOp::Xor => return unsupported_operator("^"), + BinaryOp::Shl => return unsupported_operator("<<"), + BinaryOp::Shr => return unsupported_operator(">>"), }; Ok(Value::Numeric(result)) } @@ -1100,50 +1084,25 @@ impl Interpreter<'_> { operator: BinaryOp, rhs: bool, ) -> IResult { + let unsupported_operator = |operator| -> IResult { + let typ = "u1"; + Err(internal(InternalError::UnsupportedOperatorForType { operator, typ })) + }; + let result = match operator { - BinaryOp::Add { unchecked: _ } => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "+", - typ: "u1", - }); - } - BinaryOp::Sub { unchecked: _ } => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "-", - typ: "u1", - }); - } + BinaryOp::Add { unchecked: _ } => return unsupported_operator("+"), + BinaryOp::Sub { unchecked: _ } => return unsupported_operator("-"), BinaryOp::Mul { unchecked: _ } => lhs & rhs, // (*) = (&) for u1 - BinaryOp::Div => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "/", - typ: "u1", - }); - } - BinaryOp::Mod => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "%", - typ: "u1", - }); - } + BinaryOp::Div => return unsupported_operator("/"), + BinaryOp::Mod => return unsupported_operator("%"), BinaryOp::Eq => lhs == rhs, // clippy complains when you do `lhs < rhs` and recommends this instead BinaryOp::Lt => !lhs & rhs, BinaryOp::And => lhs & rhs, BinaryOp::Or => lhs | rhs, BinaryOp::Xor => lhs ^ rhs, - BinaryOp::Shl => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: "<<", - typ: "u1", - }); - } - BinaryOp::Shr => { - return Err(InterpreterError::UnsupportedOperatorForType { - operator: ">>", - typ: "u1", - }); - } + BinaryOp::Shl => return unsupported_operator("<<"), + BinaryOp::Shr => return unsupported_operator(">>"), }; Ok(Value::Numeric(NumericValue::U1(result))) } @@ -1160,7 +1119,7 @@ where Ordering::Less => (1u128 << bit_size) - 1, Ordering::Equal => u128::MAX, Ordering::Greater => { - return Err(InterpreterError::InvalidUnsignedTruncateBitSize { bit_size }); + return Err(internal(InternalError::InvalidUnsignedTruncateBitSize { bit_size })); } }; @@ -1181,7 +1140,7 @@ where let max = 1i128 << (bit_size - 1); value_i128 += max; if bit_size > MAX_SIGNED_BIT_SIZE { - return Err(InterpreterError::InvalidSignedTruncateBitSize { bit_size }); + return Err(internal(InternalError::InvalidSignedTruncateBitSize { bit_size })); } let mask = (1i128 << bit_size) - 1; @@ -1198,6 +1157,10 @@ where } } +fn internal(error: InternalError) -> InterpreterError { + InterpreterError::Internal(error) +} + #[cfg(test)] mod test { #[test] diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index 512870c688b..8512e3b6dcb 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -292,12 +292,12 @@ fn shl() { " acir(inline) fn main f0 { b0(): - v0 = shl u8 3, u32 2 + v0 = shl i8 3, u8 2 return v0 } ", ); - assert_eq!(value, Value::from_constant(12_u128.into(), NumericType::unsigned(8))); + assert_eq!(value, Value::from_constant(12_u128.into(), NumericType::signed(8))); } /// shl should overflow if the rhs is greater than the bit count @@ -307,7 +307,7 @@ fn shl_overflow() { " acir(inline) fn main f0 { b0(): - v0 = shl u8 3, u32 9 + v0 = shl u8 3, u8 9 return v0 } ", @@ -321,16 +321,16 @@ fn shr() { " acir(inline) fn main f0 { b0(): - v0 = shr u8 12, u32 2 - v1 = shr u8 5, u32 1 - v2 = shr u8 5, u32 4 + v0 = shr u16 12, u8 2 + v1 = shr u16 5, u8 1 + v2 = shr u16 5, u8 4 return v0, v1, v2 } ", ); - assert_eq!(values[0], Value::from_constant(3_u128.into(), NumericType::unsigned(8))); - assert_eq!(values[1], Value::from_constant(2_u128.into(), NumericType::unsigned(8))); - assert_eq!(values[2], Value::from_constant(0_u128.into(), NumericType::unsigned(8))); + assert_eq!(values[0], Value::from_constant(3_u128.into(), NumericType::unsigned(16))); + assert_eq!(values[1], Value::from_constant(2_u128.into(), NumericType::unsigned(16))); + assert_eq!(values[2], Value::from_constant(0_u128.into(), NumericType::unsigned(16))); } #[test] @@ -340,7 +340,7 @@ fn shr_overflow() { " acir(inline) fn main f0 { b0(): - v0 = shr u8 3, u32 9 + v0 = shr u8 3, u8 9 return v0 } ", diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs index 04292d9822d..bafe384c10b 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/value.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/value.rs @@ -220,9 +220,9 @@ impl NumericValue { } } - pub(crate) fn as_u32(&self) -> Option { + pub(crate) fn as_u8(&self) -> Option { match self { - NumericValue::U32(value) => Some(*value), + NumericValue::U8(value) => Some(*value), _ => None, } } From 3eaa04a94361368a2cbe1adfa23883b6602a0c9a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 May 2025 09:17:24 -0500 Subject: [PATCH 5/7] Add forgotten error handling in intrinsics.rs --- .../src/ssa/interpreter/errors.rs | 26 ++- .../src/ssa/interpreter/intrinsics.rs | 180 +++++++++++------- .../src/ssa/interpreter/mod.rs | 10 +- 3 files changed, 143 insertions(+), 73 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs index b5329abb372..f0789dc1dc1 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/errors.rs @@ -1,4 +1,9 @@ -use crate::ssa::ir::{basic_block::BasicBlockId, instruction::BinaryOp, value::ValueId}; +use crate::ssa::ir::{ + basic_block::BasicBlockId, + instruction::{BinaryOp, Intrinsic}, + value::ValueId, +}; +use acvm::FieldElement; use thiserror::Error; pub(super) const MAX_SIGNED_BIT_SIZE: u32 = 64; @@ -13,6 +18,8 @@ pub(crate) enum InterpreterError { 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("static_assert `{condition}` failed: {message}")] + StaticAssertFailed { condition: ValueId, message: String }, #[error( "Range check of {value_id} = {value} failed.\n Max bits allowed by range check = {max_bits}\n Actual bit count = {actual_bits}" )] @@ -45,6 +52,10 @@ pub(crate) enum InterpreterError { "if-else instruction with then condition `{then_condition_id}` and else condition `{else_condition_id}` has both branches as true. This should be impossible except for malformed SSA code" )] DoubleTrueIfElse { then_condition_id: ValueId, else_condition_id: ValueId }, + #[error("Tried to pop from empty slice `{slice}` in `{instruction}`")] + PoppedFromEmptySlice { slice: ValueId, instruction: &'static str }, + #[error("Unable to convert `{field_id} = {field}` to radix {radix}")] + ToRadixFailed { field_id: ValueId, field: FieldElement, radix: u32 }, } /// These errors can only result from interpreting malformed SSA @@ -54,6 +65,10 @@ pub(crate) enum InternalError { "Argument count {arguments} to block {block} does not match the expected parameter count {parameters}" )] BlockArgumentCountMismatch { block: BasicBlockId, arguments: usize, parameters: usize }, + #[error( + "Argument count {arguments} to `{intrinsic}` does not match the expected parameter count {parameters}" + )] + IntrinsicArgumentCountMismatch { intrinsic: Intrinsic, arguments: usize, parameters: usize }, #[error("Block {block} is missing the terminator instruction")] BlockMissingTerminator { block: BasicBlockId }, #[error("Cannot call non-function value {value_id} = {value}")] @@ -112,4 +127,13 @@ pub(crate) enum InternalError { "`range_check {value_id} to 0 bits` has invalid bit size 0. This should only be possible in malformed SSA." )] RangeCheckToZeroBits { value_id: ValueId }, + #[error("`field_less_than` can only be called in unconstrained contexts")] + FieldLessThanCalledInConstrainedContext, + #[error("Slice `{slice_id} = {slice}` contains struct/tuple elements of types `({})` and thus needs a minimum length of {} to pop 1 struct/tuple, but it is only of length {actual_length}", element_types.join(", "), element_types.len())] + NotEnoughElementsToPopSliceOfStructs { + slice_id: ValueId, + slice: String, + actual_length: usize, + element_types: Vec, + }, } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs b/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs index 3eaa4e767a8..b836e7c71af 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/intrinsics.rs @@ -11,28 +11,29 @@ use crate::ssa::{ }, }; -use super::{IResults, Interpreter, Value}; +use super::{ArrayValue, IResult, IResults, InternalError, Interpreter, InterpreterError, Value}; impl Interpreter<'_> { pub(super) fn call_intrinsic( &mut self, intrinsic: Intrinsic, - mut args: Vec, + args: &[ValueId], results: &[ValueId], ) -> IResults { match intrinsic { Intrinsic::ArrayLen => { - assert_eq!(args.len(), 1); - let length = args[0].as_array_or_slice().unwrap().elements.borrow().len(); + check_argument_count(args, 1, intrinsic)?; + let array = self.lookup_array_or_slice(args[0], "call to array_len")?; + let length = array.elements.borrow().len(); Ok(vec![Value::Numeric(NumericValue::U32(length as u32))]) } Intrinsic::ArrayAsStrUnchecked => { - assert_eq!(args.len(), 1); - Ok(args) + check_argument_count(args, 1, intrinsic)?; + Ok(vec![self.lookup(args[0])]) } Intrinsic::AsSlice => { - assert_eq!(args.len(), 1); - let array = args[0].as_array_or_slice().unwrap(); + check_argument_count(args, 1, intrinsic)?; + let array = self.lookup_array_or_slice(args[0], "call to as_slice")?; let length = array.elements.borrow().len(); let length = Value::Numeric(NumericValue::U32(length as u32)); @@ -46,15 +47,15 @@ impl Interpreter<'_> { Ok(Vec::new()) } Intrinsic::StaticAssert => { - assert_eq!(args.len(), 2); - - let condition = args[0].as_bool().unwrap(); - if !condition { - let message = args[1].as_string().unwrap(); - panic!("static_assert failed: {message}"); + check_argument_count(args, 2, intrinsic)?; + + let condition = self.lookup_bool(args[0], "static_assert")?; + if condition { + Ok(Vec::new()) + } else { + let message = self.lookup_string(args[1], "static_assert")?; + Err(InterpreterError::StaticAssertFailed { condition: args[0], message }) } - - Ok(Vec::new()) } Intrinsic::SlicePushBack => self.slice_push_back(args), Intrinsic::SlicePushFront => self.slice_push_front(args), @@ -67,46 +68,44 @@ impl Interpreter<'_> { } // Both of these are no-ops Intrinsic::StrAsBytes | Intrinsic::AsWitness => { - assert_eq!(args.len(), 1); - let arg = args.pop().unwrap(); - Ok(vec![arg]) + check_argument_count(args, 1, intrinsic)?; + Ok(vec![self.lookup(args[0])]) } Intrinsic::ToBits(endian) => { - assert_eq!(args.len(), 1); - assert_eq!(results.len(), 1); - let field = args[0].as_field().unwrap(); - self.to_radix(endian, field, 2, results[0]) + check_argument_count(args, 1, intrinsic)?; + let field = self.lookup_field(args[0], "call to to_bits")?; + self.to_radix(endian, args[0], field, 2, results[0]) } Intrinsic::ToRadix(endian) => { - assert_eq!(args.len(), 2); - assert_eq!(results.len(), 1); - let field = args[0].as_field().unwrap(); - let radix = args[1].as_u32().unwrap(); - self.to_radix(endian, field, radix, results[0]) + check_argument_count(args, 2, intrinsic)?; + let field = self.lookup_field(args[0], "call to to_bits")?; + let radix = self.lookup_u32(args[1], "call to to_bits")?; + self.to_radix(endian, args[0], field, radix, results[0]) } Intrinsic::BlackBox(black_box_func) => { todo!("Intrinsic::BlackBox({black_box_func}) is currently unimplemented") } Intrinsic::Hint(_) => todo!("Intrinsic::Hint is currently unimplemented"), Intrinsic::IsUnconstrained => { - assert_eq!(args.len(), 0); + check_argument_count(args, 0, intrinsic)?; Ok(vec![Value::bool(self.in_unconstrained_context())]) } Intrinsic::DerivePedersenGenerators => { todo!("Intrinsic::DerivePedersenGenerators is currently unimplemented") } Intrinsic::FieldLessThan => { - assert!( - self.in_unconstrained_context(), - "FieldLessThan can only be called in unconstrained" - ); - assert_eq!(args.len(), 2); - let lhs = args[0].as_field().unwrap(); - let rhs = args[1].as_field().unwrap(); + if !self.in_unconstrained_context() { + return Err(InterpreterError::Internal( + InternalError::FieldLessThanCalledInConstrainedContext, + )); + } + check_argument_count(args, 2, intrinsic)?; + let lhs = self.lookup_field(args[0], "lhs of call to field less than")?; + let rhs = self.lookup_field(args[1], "rhs of call to field less than")?; Ok(vec![Value::bool(lhs < rhs)]) } Intrinsic::ArrayRefCount | Intrinsic::SliceRefCount => { - let array = args[0].as_array_or_slice().unwrap(); + let array = self.lookup_array_or_slice(args[0], "array/slice ref count")?; let rc = *array.rc.borrow(); Ok(vec![Value::from_constant(rc.into(), NumericType::unsigned(32))]) } @@ -116,16 +115,23 @@ impl Interpreter<'_> { fn to_radix( &self, endian: Endian, + field_id: ValueId, field: FieldElement, radix: u32, result: ValueId, ) -> IResults { - let Type::Array(_, limb_count) = self.dfg().type_of_value(result) else { - unreachable!("Expected result of to_radix/to_bytes to be an array") + let result_type = self.dfg().type_of_value(result); + let Type::Array(_, limb_count) = result_type else { + return Err(InterpreterError::Internal(InternalError::TypeError { + value_id: result, + value: result_type.to_string(), + expected_type: "array", + instruction: "call to to_radix", + })); }; let Some(limbs) = dfg::simplify::constant_to_radix(endian, field, radix, limb_count) else { - panic!("Unable to convert `{field}` to radix `{radix}`") + return Err(InterpreterError::ToRadixFailed { field_id, field, radix }); }; let elements = vecmap(limbs, |limb| Value::from_constant(limb, NumericType::unsigned(8))); @@ -133,16 +139,16 @@ impl Interpreter<'_> { } /// (length, slice, elem...) -> (length, slice) - fn slice_push_back(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); + fn slice_push_back(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_push_back")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_push_back")?; // The resulting slice should be cloned - should we check RC here to try mutating it? // It'd need to be brillig-only if so since RC is always 1 in acir. let mut new_elements = slice.elements.borrow().to_vec(); let element_types = slice.element_types.clone(); - new_elements.extend(args.into_iter().skip(2)); + new_elements.extend(args.iter().skip(2).map(|arg| self.lookup(*arg))); let new_length = Value::Numeric(NumericValue::U32(length + 1)); let new_slice = Value::slice(new_elements, element_types); @@ -150,13 +156,13 @@ impl Interpreter<'_> { } /// (length, slice, elem...) -> (length, slice) - fn slice_push_front(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); + fn slice_push_front(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_push_front")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_push_front")?; let slice_elements = slice.elements.clone(); let element_types = slice.element_types.clone(); - let mut new_elements = args.into_iter().skip(2).collect::>(); + let mut new_elements = vecmap(args.iter().skip(2), |arg| self.lookup(*arg)); new_elements.extend_from_slice(&slice_elements.borrow()); let new_length = Value::Numeric(NumericValue::U32(length + 1)); @@ -165,18 +171,18 @@ impl Interpreter<'_> { } /// (length, slice) -> (length, slice, elem...) - fn slice_pop_back(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); + fn slice_pop_back(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_pop_back")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_pop_back")?; let mut slice_elements = slice.elements.borrow().to_vec(); let element_types = slice.element_types.clone(); if slice_elements.is_empty() { - panic!("slice_pop_back: empty slice"); + let instruction = "slice_pop_back"; + return Err(InterpreterError::PoppedFromEmptySlice { slice: args[1], instruction }); } - - assert!(slice_elements.len() >= element_types.len()); + check_slice_can_pop_all_element_types(args[1], &slice)?; let mut popped_elements = vecmap(0..element_types.len(), |_| slice_elements.pop().unwrap()); popped_elements.reverse(); @@ -189,18 +195,19 @@ impl Interpreter<'_> { } /// (length, slice) -> (elem..., length, slice) - fn slice_pop_front(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); + fn slice_pop_front(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_pop_front")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_pop_front")?; let mut slice_elements = slice.elements.borrow().to_vec(); let element_types = slice.element_types.clone(); if slice_elements.is_empty() { - panic!("slice_pop_front: empty slice"); + let instruction = "slice_pop_front"; + return Err(InterpreterError::PoppedFromEmptySlice { slice: args[1], instruction }); } + check_slice_can_pop_all_element_types(args[1], &slice)?; - assert!(slice_elements.len() >= element_types.len()); let mut results = slice_elements.drain(0..element_types.len()).collect::>(); let new_length = Value::Numeric(NumericValue::U32(length - 1)); @@ -211,17 +218,17 @@ impl Interpreter<'_> { } /// (length, slice, index:u32, elem...) -> (length, slice) - fn slice_insert(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); - let index = args[2].as_u32().unwrap(); + fn slice_insert(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_insert")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_insert")?; + let index = self.lookup_u32(args[2], "call to slice_insert")?; let mut slice_elements = slice.elements.borrow().to_vec(); let element_types = slice.element_types.clone(); let mut index = index as usize * element_types.len(); - for arg in args.into_iter().skip(3) { - slice_elements.insert(index, arg); + for arg in args.iter().skip(3) { + slice_elements.insert(index, self.lookup(*arg)); index += 1; } @@ -231,18 +238,19 @@ impl Interpreter<'_> { } /// (length, slice, index:u32) -> (length, slice, elem...) - fn slice_remove(&self, args: Vec) -> IResults { - let length = args[0].as_u32().unwrap(); - let slice = args[1].as_array_or_slice().unwrap(); - let index = args[2].as_u32().unwrap(); + fn slice_remove(&self, args: &[ValueId]) -> IResults { + let length = self.lookup_u32(args[0], "call to slice_remove")?; + let slice = self.lookup_array_or_slice(args[1], "call to slice_remove")?; + let index = self.lookup_u32(args[2], "call to slice_remove")?; let mut slice_elements = slice.elements.borrow().to_vec(); let element_types = slice.element_types.clone(); if slice_elements.is_empty() { - panic!("slice_remove: empty slice"); + let instruction = "slice_remove"; + return Err(InterpreterError::PoppedFromEmptySlice { slice: args[1], instruction }); } - assert!(slice_elements.len() >= element_types.len()); + check_slice_can_pop_all_element_types(args[1], &slice)?; let index = index as usize * element_types.len(); let removed: Vec<_> = slice_elements.drain(index..index + element_types.len()).collect(); @@ -260,3 +268,33 @@ impl Interpreter<'_> { Ok(Vec::new()) } } + +fn check_argument_count( + args: &[ValueId], + expected_count: usize, + intrinsic: Intrinsic, +) -> IResult<()> { + if args.len() != expected_count { + Err(InterpreterError::Internal(InternalError::IntrinsicArgumentCountMismatch { + intrinsic, + arguments: args.len(), + parameters: expected_count, + })) + } else { + Ok(()) + } +} + +fn check_slice_can_pop_all_element_types(slice_id: ValueId, slice: &ArrayValue) -> IResult<()> { + let actual_length = slice.elements.borrow().len(); + if actual_length >= slice.element_types.len() { + Ok(()) + } else { + Err(InterpreterError::Internal(InternalError::NotEnoughElementsToPopSliceOfStructs { + slice_id, + slice: slice.to_string(), + actual_length, + element_types: vecmap(slice.element_types.iter(), ToString::to_string), + })) + } +} diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 31ff650804d..94ee799dac3 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -258,6 +258,10 @@ impl<'ssa> Interpreter<'ssa> { self.lookup_helper(value_id, instruction, "u32", Value::as_u32) } + fn lookup_field(&self, value_id: ValueId, instruction: &'static str) -> IResult { + self.lookup_helper(value_id, instruction, "Field", Value::as_field) + } + fn lookup_numeric( &self, value_id: ValueId, @@ -274,6 +278,10 @@ impl<'ssa> Interpreter<'ssa> { self.lookup_helper(value_id, instruction, "array or slice", Value::as_array_or_slice) } + fn lookup_string(&self, value_id: ValueId, instruction: &'static str) -> IResult { + self.lookup_helper(value_id, instruction, "string", Value::as_string) + } + fn lookup_reference( &self, value_id: ValueId, @@ -528,7 +536,7 @@ impl<'ssa> Interpreter<'ssa> { self.call_function(id, arguments)? } Value::Intrinsic(intrinsic) => { - self.call_intrinsic(intrinsic, arguments, results)? + self.call_intrinsic(intrinsic, argument_ids, results)? } Value::ForeignFunction(name) if name == "print" => self.call_print(arguments)?, Value::ForeignFunction(name) => { From 00e75fd621f6bf351c2041f9317b75d096cff4fd Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 May 2025 09:25:14 -0500 Subject: [PATCH 6/7] Clippy needs to run with '--all-targets' --- .../src/ssa/interpreter/tests/instructions.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index 8512e3b6dcb..40c72072995 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -42,7 +42,7 @@ fn add_overflow() { } ", ); - assert!(matches!(error, InterpreterError::Overflow { .. })) + assert!(matches!(error, InterpreterError::Overflow { .. })); } #[test] @@ -83,7 +83,7 @@ fn sub_underflow() { } ", ); - assert!(matches!(error, InterpreterError::Overflow { .. })) + assert!(matches!(error, InterpreterError::Overflow { .. })); } #[test] @@ -125,7 +125,7 @@ fn mul_overflow() { } ", ); - assert!(matches!(error, InterpreterError::Overflow { .. })) + assert!(matches!(error, InterpreterError::Overflow { .. })); } #[test] From a2162054762175ac53856eafd9bab71eed6ee3aa Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 2 May 2025 09:28:20 -0500 Subject: [PATCH 7/7] Clippy needs to run with that argument and in the exact crate --- .../src/ssa/interpreter/tests/instructions.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs index 40c72072995..d43f9998b15 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/tests/instructions.rs @@ -167,7 +167,7 @@ fn div_zero() { } ", ); - assert!(matches!(error, InterpreterError::DivisionByZero { .. })) + assert!(matches!(error, InterpreterError::DivisionByZero { .. })); } #[test] @@ -195,8 +195,7 @@ fn mod_zero() { } ", ); - dbg!(&error); - assert!(matches!(error, InterpreterError::DivisionByZero { .. })) + assert!(matches!(error, InterpreterError::DivisionByZero { .. })); } #[test]