diff --git a/.noir-sync-commit b/.noir-sync-commit index 9eafde097d5..76a4a97e19e 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -8bb3908281d531160db7d7898c67fb2647792e6e +3c488f4b272f460383341c51270b87bfe2b94468 diff --git a/noir/noir-repo/.github/workflows/test-js-packages.yml b/noir/noir-repo/.github/workflows/test-js-packages.yml index 2782b1aa44a..5222b667c2a 100644 --- a/noir/noir-repo/.github/workflows/test-js-packages.yml +++ b/noir/noir-repo/.github/workflows/test-js-packages.yml @@ -597,7 +597,7 @@ jobs: - name: Run nargo test working-directory: ./test-repo/${{ matrix.project.path }} run: | - nargo test --silence-warnings --pedantic-solving --skip-brillig-constraints-check --format json ${{ matrix.project.nargo_args }} | tee ${{ github.workspace }}/noir-repo/.github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl + nargo test --silence-warnings --skip-brillig-constraints-check --format json ${{ matrix.project.nargo_args }} | tee ${{ github.workspace }}/noir-repo/.github/critical_libraries_status/${{ matrix.project.repo }}/${{ matrix.project.path }}.actual.jsonl env: NARGO_IGNORE_TEST_FAILURES_FROM_FOREIGN_CALLS: true diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/memory_op.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/memory_op.rs index 1a6519d19c5..2a83bf2531c 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/memory_op.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/memory_op.rs @@ -21,6 +21,19 @@ pub(crate) struct MemoryOpSolver { } impl MemoryOpSolver { + fn index_from_field(&self, index: F) -> Result> { + if index.num_bits() <= 32 { + let memory_index = index.try_to_u64().unwrap() as MemoryIndex; + Ok(memory_index) + } else { + Err(OpcodeResolutionError::IndexOutOfBounds { + opcode_location: ErrorLocation::Unresolved, + index, + array_size: self.block_len, + }) + } + } + fn write_memory_index( &mut self, index: MemoryIndex, @@ -29,7 +42,7 @@ impl MemoryOpSolver { if index >= self.block_len { return Err(OpcodeResolutionError::IndexOutOfBounds { opcode_location: ErrorLocation::Unresolved, - index, + index: F::from(index as u128), array_size: self.block_len, }); } @@ -40,7 +53,7 @@ impl MemoryOpSolver { fn read_memory_index(&self, index: MemoryIndex) -> Result> { self.block_value.get(&index).copied().ok_or(OpcodeResolutionError::IndexOutOfBounds { opcode_location: ErrorLocation::Unresolved, - index, + index: F::from(index as u128), array_size: self.block_len, }) } @@ -72,7 +85,7 @@ impl MemoryOpSolver { // Find the memory index associated with this memory operation. let index = get_value(&op.index, initial_witness)?; - let memory_index = index.try_to_u64().unwrap() as MemoryIndex; + let memory_index = self.index_from_field(index)?; // Calculate the value associated with this memory operation. // @@ -193,9 +206,9 @@ mod tests { err, Some(crate::pwg::OpcodeResolutionError::IndexOutOfBounds { opcode_location: _, - index: 2, + index, array_size: 2 - }) + }) if index == FieldElement::from(2u128) )); } diff --git a/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs b/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs index 104a15c17cc..6e0e28cf81d 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/pwg/mod.rs @@ -126,7 +126,7 @@ pub enum OpcodeResolutionError { payload: Option>, }, #[error("Index out of bounds, array has size {array_size:?}, but index was {index:?}")] - IndexOutOfBounds { opcode_location: ErrorLocation, index: u32, array_size: u32 }, + IndexOutOfBounds { opcode_location: ErrorLocation, index: F, array_size: u32 }, #[error("Cannot solve opcode: {invalid_input_bit_size}")] InvalidInputBitSize { opcode_location: ErrorLocation, diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index e7fa601fbe3..7ecc4d28032 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -1984,14 +1984,7 @@ impl<'a> Context<'a> { if let NumericType::Unsigned { bit_size } = &num_type { // Check for integer overflow - self.check_unsigned_overflow( - result, - *bit_size, - binary.lhs, - binary.rhs, - dfg, - binary.operator, - )?; + self.check_unsigned_overflow(result, *bit_size, binary, dfg)?; } Ok(result) @@ -2002,47 +1995,18 @@ impl<'a> Context<'a> { &mut self, result: AcirVar, bit_size: u32, - lhs: ValueId, - rhs: ValueId, + binary: &Binary, dfg: &DataFlowGraph, - op: BinaryOp, ) -> Result<(), RuntimeError> { - // We try to optimize away operations that are guaranteed not to overflow - let max_lhs_bits = dfg.get_value_max_num_bits(lhs); - let max_rhs_bits = dfg.get_value_max_num_bits(rhs); - - let msg = match op { - BinaryOp::Add => { - if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { - // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. - return Ok(()); - } - "attempt to add with overflow".to_string() - } - BinaryOp::Sub => { - if dfg.is_constant(lhs) && max_lhs_bits > max_rhs_bits { - // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` - // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. - return Ok(()); - } - "attempt to subtract with overflow".to_string() - } - BinaryOp::Mul => { - if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { - // Either performing boolean multiplication (which cannot overflow), - // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. - return Ok(()); - } - "attempt to multiply with overflow".to_string() - } - _ => return Ok(()), + let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) else { + return Ok(()); }; let with_pred = self.acir_context.mul_var(result, self.current_side_effects_enabled_var)?; self.acir_context.range_constrain_var( with_pred, &NumericType::Unsigned { bit_size }, - Some(msg), + Some(msg.to_string()), )?; Ok(()) } @@ -2888,8 +2852,9 @@ mod test { use acvm::{ acir::{ circuit::{ - brillig::BrilligFunctionId, opcodes::AcirFunctionId, ExpressionWidth, Opcode, - OpcodeLocation, + brillig::BrilligFunctionId, + opcodes::{AcirFunctionId, BlackBoxFuncCall}, + ExpressionWidth, Opcode, OpcodeLocation, }, native_types::Witness, }, @@ -2913,6 +2878,8 @@ mod test { }, }; + use super::Ssa; + fn build_basic_foo_with_return( builder: &mut FunctionBuilder, foo_id: FunctionId, @@ -3659,4 +3626,36 @@ mod test { "Should have {expected_num_normal_calls} BrilligCall opcodes to normal Brillig functions but got {num_normal_brillig_calls}" ); } + + #[test] + fn multiply_with_bool_should_not_emit_range_check() { + let src = " + acir(inline) fn main f0 { + b0(v0: bool, v1: u32): + enable_side_effects v0 + v2 = cast v0 as u32 + v3 = mul v2, v1 + return v3 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + let brillig = ssa.to_brillig(false); + + let (mut acir_functions, _brillig_functions, _, _) = ssa + .into_acir(&brillig, ExpressionWidth::default()) + .expect("Should compile manually written SSA into ACIR"); + + assert_eq!(acir_functions.len(), 1); + + let opcodes = acir_functions[0].take_opcodes(); + + for opcode in opcodes { + if let Opcode::BlackBoxFuncCall(BlackBoxFuncCall::RANGE { input }) = opcode { + assert!( + input.to_witness().0 <= 1, + "only input witnesses should have range checks: {opcode:?}" + ); + } + } + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 66cc213a986..31c99bf433e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1478,88 +1478,70 @@ impl<'block> BrilligBlock<'block> { is_signed: bool, ) { let bit_size = left.bit_size; - let max_lhs_bits = dfg.get_value_max_num_bits(binary.lhs); - let max_rhs_bits = dfg.get_value_max_num_bits(binary.rhs); - if bit_size == FieldElement::max_num_bits() { + if bit_size == FieldElement::max_num_bits() || is_signed { return; } - match (binary_operation, is_signed) { - (BrilligBinaryOp::Add, false) => { - if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { - // `left` and `right` have both been casted up from smaller types and so cannot overflow. - return; - } - - let condition = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // Check that lhs <= result - self.brillig_context.binary_instruction( - left, - result, - condition, - BrilligBinaryOp::LessThanEquals, - ); - self.brillig_context - .codegen_constrain(condition, Some("attempt to add with overflow".to_string())); - self.brillig_context.deallocate_single_addr(condition); - } - (BrilligBinaryOp::Sub, false) => { - if dfg.is_constant(binary.lhs) && max_lhs_bits > max_rhs_bits { - // `left` is a fixed constant and `right` is restricted such that `left - right > 0` - // Note strict inequality as `right > left` while `max_lhs_bits == max_rhs_bits` is possible. - return; - } - - let condition = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - // Check that rhs <= lhs - self.brillig_context.binary_instruction( - right, - left, - condition, - BrilligBinaryOp::LessThanEquals, - ); - self.brillig_context.codegen_constrain( - condition, - Some("attempt to subtract with overflow".to_string()), - ); - self.brillig_context.deallocate_single_addr(condition); - } - (BrilligBinaryOp::Mul, false) => { - if bit_size == 1 || max_lhs_bits + max_rhs_bits <= bit_size { - // Either performing boolean multiplication (which cannot overflow), - // or `left` and `right` have both been casted up from smaller types and so cannot overflow. - return; + if let Some(msg) = binary.check_unsigned_overflow_msg(dfg, bit_size) { + match binary_operation { + BrilligBinaryOp::Add => { + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // Check that lhs <= result + self.brillig_context.binary_instruction( + left, + result, + condition, + BrilligBinaryOp::LessThanEquals, + ); + self.brillig_context.codegen_constrain(condition, Some(msg.to_string())); + self.brillig_context.deallocate_single_addr(condition); } - - let is_right_zero = - SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); - let zero = self.brillig_context.make_constant_instruction(0_usize.into(), bit_size); - self.brillig_context.binary_instruction( - zero, - right, - is_right_zero, - BrilligBinaryOp::Equals, - ); - self.brillig_context.codegen_if_not(is_right_zero.address, |ctx| { - let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); - let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size); - // Check that result / rhs == lhs - ctx.binary_instruction(result, right, division, BrilligBinaryOp::UnsignedDiv); - ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); - ctx.codegen_constrain( + BrilligBinaryOp::Sub => { + let condition = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + // Check that rhs <= lhs + self.brillig_context.binary_instruction( + right, + left, condition, - Some("attempt to multiply with overflow".to_string()), + BrilligBinaryOp::LessThanEquals, ); - ctx.deallocate_single_addr(condition); - ctx.deallocate_single_addr(division); - }); - self.brillig_context.deallocate_single_addr(is_right_zero); - self.brillig_context.deallocate_single_addr(zero); + self.brillig_context.codegen_constrain(condition, Some(msg.to_string())); + self.brillig_context.deallocate_single_addr(condition); + } + BrilligBinaryOp::Mul => { + let is_right_zero = + SingleAddrVariable::new(self.brillig_context.allocate_register(), 1); + let zero = + self.brillig_context.make_constant_instruction(0_usize.into(), bit_size); + self.brillig_context.binary_instruction( + zero, + right, + is_right_zero, + BrilligBinaryOp::Equals, + ); + self.brillig_context.codegen_if_not(is_right_zero.address, |ctx| { + let condition = SingleAddrVariable::new(ctx.allocate_register(), 1); + let division = SingleAddrVariable::new(ctx.allocate_register(), bit_size); + // Check that result / rhs == lhs + ctx.binary_instruction( + result, + right, + division, + BrilligBinaryOp::UnsignedDiv, + ); + ctx.binary_instruction(division, left, condition, BrilligBinaryOp::Equals); + ctx.codegen_constrain(condition, Some(msg.to_string())); + ctx.deallocate_single_addr(condition); + ctx.deallocate_single_addr(division); + }); + self.brillig_context.deallocate_single_addr(is_right_zero); + self.brillig_context.deallocate_single_addr(zero); + } + _ => {} } - _ => {} } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index ab37080ac1d..28e58e2cbb1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -319,6 +319,49 @@ impl Binary { }; SimplifyResult::None } + + /// Check if unsigned overflow is possible, and if so return some message to be used if it fails. + pub(crate) fn check_unsigned_overflow_msg( + &self, + dfg: &DataFlowGraph, + bit_size: u32, + ) -> Option<&'static str> { + // We try to optimize away operations that are guaranteed not to overflow + let max_lhs_bits = dfg.get_value_max_num_bits(self.lhs); + let max_rhs_bits = dfg.get_value_max_num_bits(self.rhs); + + let msg = match self.operator { + BinaryOp::Add => { + if std::cmp::max(max_lhs_bits, max_rhs_bits) < bit_size { + // `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return None; + } + "attempt to add with overflow" + } + BinaryOp::Sub => { + if dfg.is_constant(self.lhs) && max_lhs_bits > max_rhs_bits { + // `lhs` is a fixed constant and `rhs` is restricted such that `lhs - rhs > 0` + // Note strict inequality as `rhs > lhs` while `max_lhs_bits == max_rhs_bits` is possible. + return None; + } + "attempt to subtract with overflow" + } + BinaryOp::Mul => { + if bit_size == 1 + || max_lhs_bits + max_rhs_bits <= bit_size + || max_lhs_bits == 1 + || max_rhs_bits == 1 + { + // Either performing boolean multiplication (which cannot overflow), + // or `lhs` and `rhs` have both been casted up from smaller types and so cannot overflow. + return None; + } + "attempt to multiply with overflow" + } + _ => return None, + }; + Some(msg) + } } /// Evaluate a binary operation with constant arguments. diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs index d664f856262..aeee417789e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1451,7 +1451,7 @@ impl<'context> Elaborator<'context> { let method_name = self.interner.function_name(method_id).to_owned(); if let Some(first_fn) = - self.interner.add_method(self_type, method_name.clone(), *method_id, false) + self.interner.add_method(self_type, method_name.clone(), *method_id, None) { let error = ResolverError::DuplicateDefinition { name: method_name, diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/path_resolution.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/path_resolution.rs index f9f6fff56e0..09bd8f372b1 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/path_resolution.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/path_resolution.rs @@ -2,7 +2,7 @@ use iter_extended::vecmap; use noirc_errors::{Location, Span}; use crate::ast::{Ident, Path, PathKind, UnresolvedType}; -use crate::hir::def_map::{fully_qualified_module_path, ModuleData, ModuleDefId, ModuleId, PerNs}; +use crate::hir::def_map::{ModuleData, ModuleDefId, ModuleId, PerNs}; use crate::hir::resolution::import::{resolve_path_kind, PathResolutionError}; use crate::hir::resolution::errors::ResolverError; @@ -448,10 +448,6 @@ impl<'context> Elaborator<'context> { let per_ns = PerNs { types: None, values: Some(*item) }; StructMethodLookupResult::FoundTraitMethod(per_ns, trait_id) } - - fn fully_qualified_trait_path(&self, trait_: &Trait) -> String { - fully_qualified_module_path(self.def_maps, self.crate_graph, &trait_.crate_id, trait_.id.0) - } } fn merge_intermediate_path_resolution_item_with_module_def_id( diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs index c3375fc35ad..e01cda3a2e4 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs @@ -12,6 +12,7 @@ use crate::{ }, hir::{ def_collector::dc_crate::CompilationError, + def_map::{fully_qualified_module_path, ModuleDefId}, resolution::{errors::ResolverError, import::PathResolutionError}, type_check::{ generics::{Generic, TraitGenerics}, @@ -32,7 +33,8 @@ use crate::{ TraitImplKind, TraitMethodId, }, token::SecondaryAttribute, - Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, UnificationError, + Generics, Kind, ResolvedGeneric, Shared, StructType, Type, TypeBinding, TypeBindings, + UnificationError, }; use super::{lints, path_resolution::PathResolutionItem, Elaborator, UnsafeBlockStatus}; @@ -1309,7 +1311,7 @@ impl<'context> Elaborator<'context> { } } - pub(super) fn lookup_method( + pub(crate) fn lookup_method( &mut self, object_type: &Type, method_name: &str, @@ -1318,31 +1320,7 @@ impl<'context> Elaborator<'context> { ) -> Option { match object_type.follow_bindings() { Type::Struct(typ, _args) => { - let id = typ.borrow().id; - match self.interner.lookup_method(object_type, id, method_name, false, has_self_arg) - { - Some(method_id) => Some(HirMethodReference::FuncId(method_id)), - None => { - let has_field_with_function_type = - typ.borrow().get_fields_as_written().into_iter().any(|field| { - field.name.0.contents == method_name && field.typ.is_function() - }); - if has_field_with_function_type { - self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); - } else { - self.push_err(TypeCheckError::UnresolvedMethodCall { - method_name: method_name.to_string(), - object_type: object_type.clone(), - span, - }); - } - None - } - } + self.lookup_struct_method(object_type, method_name, span, has_self_arg, typ) } // TODO: We should allow method calls on `impl Trait`s eventually. // For now it is fine since they are only allowed on return types. @@ -1388,6 +1366,125 @@ impl<'context> Elaborator<'context> { } } + fn lookup_struct_method( + &mut self, + object_type: &Type, + method_name: &str, + span: Span, + has_self_arg: bool, + typ: Shared, + ) -> Option { + let id = typ.borrow().id; + + // First search in the struct methods + if let Some(method_id) = + self.interner.lookup_direct_method(object_type, id, method_name, has_self_arg) + { + return Some(HirMethodReference::FuncId(method_id)); + } + + // Next lookup all matching trait methods. + let trait_methods = + self.interner.lookup_trait_methods(object_type, id, method_name, has_self_arg); + + if trait_methods.is_empty() { + // If we couldn't find any trait methods, search in + // impls for all types `T`, e.g. `impl Foo for T` + if let Some(func_id) = + self.interner.lookup_generic_method(object_type, method_name, has_self_arg) + { + return Some(HirMethodReference::FuncId(func_id)); + } + + // Otherwise it's an error + let has_field_with_function_type = typ + .borrow() + .get_fields_as_written() + .into_iter() + .any(|field| field.name.0.contents == method_name && field.typ.is_function()); + if has_field_with_function_type { + self.push_err(TypeCheckError::CannotInvokeStructFieldFunctionType { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } else { + self.push_err(TypeCheckError::UnresolvedMethodCall { + method_name: method_name.to_string(), + object_type: object_type.clone(), + span, + }); + } + return None; + } + + // We found some trait methods... but is only one of them currently in scope? + let module_id = self.module_id(); + let module_data = self.get_module(module_id); + + let trait_methods_in_scope: Vec<_> = trait_methods + .iter() + .filter_map(|(func_id, trait_id)| { + let trait_ = self.interner.get_trait(*trait_id); + let trait_name = &trait_.name; + let Some(map) = module_data.scope().types().get(trait_name) else { + return None; + }; + let Some(imported_item) = map.get(&None) else { + return None; + }; + if imported_item.0 == ModuleDefId::TraitId(*trait_id) { + Some((*func_id, *trait_id, trait_name)) + } else { + None + } + }) + .collect(); + + for (_, _, trait_name) in &trait_methods_in_scope { + self.usage_tracker.mark_as_used(module_id, trait_name); + } + + if trait_methods_in_scope.is_empty() { + if trait_methods.len() == 1 { + // This is the backwards-compatible case where there's a single trait method but it's not in scope + let (func_id, trait_id) = trait_methods[0]; + let trait_ = self.interner.get_trait(trait_id); + let trait_name = self.fully_qualified_trait_path(trait_); + self.push_err(PathResolutionError::TraitMethodNotInScope { + ident: Ident::new(method_name.into(), span), + trait_name, + }); + return Some(HirMethodReference::FuncId(func_id)); + } else { + let traits = vecmap(trait_methods, |(_, trait_id)| { + let trait_ = self.interner.get_trait(trait_id); + self.fully_qualified_trait_path(trait_) + }); + self.push_err(PathResolutionError::UnresolvedWithPossibleTraitsToImport { + ident: Ident::new(method_name.into(), span), + traits, + }); + return None; + } + } + + if trait_methods_in_scope.len() > 1 { + let traits = vecmap(trait_methods, |(_, trait_id)| { + let trait_ = self.interner.get_trait(trait_id); + self.fully_qualified_trait_path(trait_) + }); + self.push_err(PathResolutionError::MultipleTraitsInScope { + ident: Ident::new(method_name.into(), span), + traits, + }); + return None; + } + + let func_id = trait_methods_in_scope[0].0; + Some(HirMethodReference::FuncId(func_id)) + } + fn lookup_method_in_trait_constraints( &mut self, object_type: &Type, @@ -1823,6 +1920,10 @@ impl<'context> Elaborator<'context> { ..*parent_trait_bound } } + + pub(crate) fn fully_qualified_trait_path(&self, trait_: &Trait) -> String { + fully_qualified_module_path(self.def_maps, self.crate_graph, &trait_.crate_id, trait_.id.0) + } } pub(crate) fn bind_ordered_generics( diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 12b99818c46..13a9400bd2e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1379,17 +1379,10 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let typ = object.get_type().follow_bindings(); let method_name = &call.method.0.contents; - // TODO: Traits - let method = match &typ { - Type::Struct(struct_def, _) => self.elaborator.interner.lookup_method( - &typ, - struct_def.borrow().id, - method_name, - false, - true, - ), - _ => self.elaborator.interner.lookup_primitive_method(&typ, method_name, true), - }; + let method = self + .elaborator + .lookup_method(&typ, method_name, location.span, true) + .and_then(|method| method.func_id(self.elaborator.interner)); if let Some(method) = method { self.call_function(method, arguments, TypeBindings::new(), location) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs index 3ed183df49c..25bc507da1e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -366,6 +366,7 @@ pub struct TraitImplMethod { // under TypeMethodKey::FieldOrInt pub typ: Option, pub method: FuncId, + pub trait_id: TraitId, } /// All the information from a function that is filled out during definition collection rather than @@ -1390,15 +1391,18 @@ impl NodeInterner { self_type: &Type, method_name: String, method_id: FuncId, - is_trait_method: bool, + trait_id: Option, ) -> Option { match self_type { Type::Struct(struct_type, _generics) => { let id = struct_type.borrow().id; - if let Some(existing) = self.lookup_method(self_type, id, &method_name, true, true) - { - return Some(existing); + if trait_id.is_none() { + if let Some(existing) = + self.lookup_direct_method(self_type, id, &method_name, true) + { + return Some(existing); + } } self.struct_methods @@ -1406,12 +1410,12 @@ impl NodeInterner { .or_default() .entry(method_name) .or_default() - .add_method(method_id, None, is_trait_method); + .add_method(method_id, None, trait_id); None } Type::Error => None, Type::MutableReference(element) => { - self.add_method(element, method_name, method_id, is_trait_method) + self.add_method(element, method_name, method_id, trait_id) } other => { @@ -1427,7 +1431,7 @@ impl NodeInterner { .or_default() .entry(method_name) .or_default() - .add_method(method_id, typ, is_trait_method); + .add_method(method_id, typ, trait_id); None } } @@ -1765,7 +1769,7 @@ impl NodeInterner { for method in &trait_impl.borrow().methods { let method_name = self.function_name(method).to_owned(); - self.add_method(&object_type, method_name, *method, true); + self.add_method(&object_type, method_name, *method, Some(trait_id)); } // The object type is generalized so that a generic impl will apply @@ -1777,36 +1781,33 @@ impl NodeInterner { Ok(()) } - /// Search by name for a method on the given struct. - /// - /// If `check_type` is true, this will force `lookup_method` to check the type - /// of each candidate instead of returning only the first candidate if there is exactly one. - /// This is generally only desired when declaring new methods to check if they overlap any - /// existing methods. - /// - /// Another detail is that this method does not handle auto-dereferencing through `&mut T`. - /// So if an object is of type `self : &mut T` but a method only accepts `self: T` (or - /// vice-versa), the call will not be selected. If this is ever implemented into this method, - /// we can remove the `methods.len() == 1` check and the `check_type` early return. - pub fn lookup_method( + /// Looks up a method that's directly defined in the given struct. + pub fn lookup_direct_method( &self, typ: &Type, id: StructId, method_name: &str, - force_type_check: bool, has_self_arg: bool, ) -> Option { - let methods = self.struct_methods.get(&id).and_then(|h| h.get(method_name)); - - // If there is only one method, just return it immediately. - // It will still be typechecked later. - if !force_type_check { - if let Some(method) = methods.and_then(|m| m.get_unambiguous()) { - return Some(method); - } - } + self.struct_methods + .get(&id) + .and_then(|h| h.get(method_name)) + .and_then(|methods| methods.find_direct_method(typ, has_self_arg, self)) + } - self.find_matching_method(typ, methods, method_name, has_self_arg) + /// Looks up a methods that apply to the given struct but are defined in traits. + pub fn lookup_trait_methods( + &self, + typ: &Type, + id: StructId, + method_name: &str, + has_self_arg: bool, + ) -> Vec<(FuncId, TraitId)> { + self.struct_methods + .get(&id) + .and_then(|h| h.get(method_name)) + .map(|methods| methods.find_trait_methods(typ, has_self_arg, self)) + .unwrap_or_default() } /// Select the 1 matching method with an object type matching `typ` @@ -1821,14 +1822,22 @@ impl NodeInterner { { Some(method) } else { - // Failed to find a match for the type in question, switch to looking at impls - // for all types `T`, e.g. `impl Foo for T` - let global_methods = - self.primitive_methods.get(&TypeMethodKey::Generic)?.get(method_name)?; - global_methods.find_matching_method(typ, has_self_arg, self) + self.lookup_generic_method(typ, method_name, has_self_arg) } } + /// Looks up a method at impls for all types `T`, e.g. `impl Foo for T` + pub fn lookup_generic_method( + &self, + typ: &Type, + method_name: &str, + has_self_arg: bool, + ) -> Option { + let global_methods = + self.primitive_methods.get(&TypeMethodKey::Generic)?.get(method_name)?; + global_methods.find_matching_method(typ, has_self_arg, self) + } + /// Looks up a given method name on the given primitive type. pub fn lookup_primitive_method( &self, @@ -2307,22 +2316,9 @@ impl NodeInterner { } impl Methods { - /// Get a single, unambiguous reference to a name if one exists. - /// If not, there may be multiple methods of the same name for a given - /// type or there may be no methods at all. - fn get_unambiguous(&self) -> Option { - if self.direct.len() == 1 { - Some(self.direct[0]) - } else if self.direct.is_empty() && self.trait_impl_methods.len() == 1 { - Some(self.trait_impl_methods[0].method) - } else { - None - } - } - - fn add_method(&mut self, method: FuncId, typ: Option, is_trait_method: bool) { - if is_trait_method { - let trait_impl_method = TraitImplMethod { typ, method }; + fn add_method(&mut self, method: FuncId, typ: Option, trait_id: Option) { + if let Some(trait_id) = trait_id { + let trait_impl_method = TraitImplMethod { typ, method, trait_id }; self.trait_impl_methods.push(trait_impl_method); } else { self.direct.push(method); @@ -2349,32 +2345,96 @@ impl Methods { // When adding methods we always check they do not overlap, so there should be // at most 1 matching method in this list. for (method, method_type) in self.iter() { - match interner.function_meta(&method).typ.instantiate(interner).0 { - Type::Function(args, _, _, _) => { - if has_self_param { - if let Some(object) = args.first() { + if Self::method_matches(typ, has_self_param, method, method_type, interner) { + return Some(method); + } + } + + None + } + + pub fn find_direct_method( + &self, + typ: &Type, + has_self_param: bool, + interner: &NodeInterner, + ) -> Option { + for method in &self.direct { + if Self::method_matches(typ, has_self_param, *method, &None, interner) { + return Some(*method); + } + } + + None + } + + pub fn find_trait_methods( + &self, + typ: &Type, + has_self_param: bool, + interner: &NodeInterner, + ) -> Vec<(FuncId, TraitId)> { + let mut results = Vec::new(); + + for trait_impl_method in &self.trait_impl_methods { + let method = trait_impl_method.method; + let method_type = &trait_impl_method.typ; + let trait_id = trait_impl_method.trait_id; + + if Self::method_matches(typ, has_self_param, method, method_type, interner) { + results.push((method, trait_id)); + } + } + + results + } + + fn method_matches( + typ: &Type, + has_self_param: bool, + method: FuncId, + method_type: &Option, + interner: &NodeInterner, + ) -> bool { + match interner.function_meta(&method).typ.instantiate(interner).0 { + Type::Function(args, _, _, _) => { + if has_self_param { + if let Some(object) = args.first() { + if object.unify(typ).is_ok() { + return true; + } + + // Handle auto-dereferencing `&mut T` into `T` + if let Type::MutableReference(object) = object { if object.unify(typ).is_ok() { - return Some(method); + return true; } } - } else { - // If we recorded the concrete type this trait impl method belongs to, - // and it matches typ, it's an exact match and we return that. - if let Some(method_type) = method_type { + } + } else { + // If we recorded the concrete type this trait impl method belongs to, + // and it matches typ, it's an exact match and we return that. + if let Some(method_type) = method_type { + if method_type.unify(typ).is_ok() { + return true; + } + + // Handle auto-dereferencing `&mut T` into `T` + if let Type::MutableReference(method_type) = method_type { if method_type.unify(typ).is_ok() { - return Some(method); + return true; } - } else { - return Some(method); } + } else { + return true; } } - Type::Error => (), - other => unreachable!("Expected function type, found {other}"), } + Type::Error => (), + other => unreachable!("Expected function type, found {other}"), } - None + false } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index dabfcfe9191..f1db0df9540 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -3933,3 +3933,22 @@ fn warns_on_nested_unsafe() { CompilationError::TypeError(TypeCheckError::NestedUnsafeBlock { .. }) )); } + +#[test] +fn mutable_self_call() { + let src = r#" + fn main() { + let mut bar = Bar {}; + let _ = bar.bar(); + } + + struct Bar {} + + impl Bar { + fn bar(&mut self) { + let _ = self; + } + } + "#; + assert_no_errors(src); +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs index ba70daa297d..82c8460d004 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs @@ -831,7 +831,7 @@ fn errors_if_trait_is_not_in_scope_for_function_call_and_there_are_multiple_cand } #[test] -fn errors_if_multiple_trait_methods_are_in_scope() { +fn errors_if_multiple_trait_methods_are_in_scope_for_function_call() { let src = r#" use private_mod::Foo; use private_mod::Foo2; @@ -879,6 +879,134 @@ fn errors_if_multiple_trait_methods_are_in_scope() { assert_eq!(traits, vec!["private_mod::Foo", "private_mod::Foo2"]); } +#[test] +fn calls_trait_method_if_it_is_in_scope() { + let src = r#" + use private_mod::Foo; + + fn main() { + let bar = Bar { x: 42 }; + let _ = bar.foo(); + } + + pub struct Bar { + x: i32, + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + } + "#; + assert_no_errors(src); +} + +#[test] +fn errors_if_trait_is_not_in_scope_for_method_call_and_there_are_multiple_candidates() { + let src = r#" + fn main() { + let bar = Bar { x: 42 }; + let _ = bar.foo(); + } + + pub struct Bar { + x: i32, + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + + pub trait Foo2 { + fn foo(self) -> i32; + } + + impl Foo2 for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + } + "#; + let mut errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::UnresolvedWithPossibleTraitsToImport { ident, mut traits }, + )) = errors.remove(0).0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + traits.sort(); + assert_eq!(traits, vec!["private_mod::Foo", "private_mod::Foo2"]); +} + +#[test] +fn errors_if_multiple_trait_methods_are_in_scope_for_method_call() { + let src = r#" + use private_mod::Foo; + use private_mod::Foo2; + + fn main() { + let bar = Bar { x : 42 }; + let _ = bar.foo(); + } + + pub struct Bar { + x: i32, + } + + mod private_mod { + pub trait Foo { + fn foo(self) -> i32; + } + + impl Foo for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + + pub trait Foo2 { + fn foo(self) -> i32; + } + + impl Foo2 for super::Bar { + fn foo(self) -> i32 { + self.x + } + } + } + "#; + let mut errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::MultipleTraitsInScope { ident, mut traits }, + )) = errors.remove(0).0 + else { + panic!("Expected a 'trait method not in scope' error"); + }; + assert_eq!(ident.to_string(), "foo"); + traits.sort(); + assert_eq!(traits, vec!["private_mod::Foo", "private_mod::Foo2"]); +} + #[test] fn type_checks_trait_default_method_and_errors() { let src = r#" @@ -891,6 +1019,7 @@ fn type_checks_trait_default_method_and_errors() { fn main() {} "#; + let errors = get_program_errors(src); assert_eq!(errors.len(), 1); diff --git a/noir/noir-repo/noir_stdlib/src/uint128.nr b/noir/noir-repo/noir_stdlib/src/uint128.nr index fab8cacc055..ffd6d60b6ca 100644 --- a/noir/noir-repo/noir_stdlib/src/uint128.nr +++ b/noir/noir-repo/noir_stdlib/src/uint128.nr @@ -1,6 +1,6 @@ use crate::cmp::{Eq, Ord, Ordering}; use crate::ops::{Add, BitAnd, BitOr, BitXor, Div, Mul, Not, Rem, Shl, Shr, Sub}; -use super::convert::AsPrimitive; +use super::{convert::AsPrimitive, default::Default}; global pow64: Field = 18446744073709551616; //2^64; global pow63: Field = 9223372036854775808; // 2^63; @@ -331,7 +331,15 @@ impl Shr for U128 { } } +impl Default for U128 { + fn default() -> Self { + U128::zero() + } +} + mod tests { + use crate::default::Default; + use crate::ops::Not; use crate::uint128::{pow63, pow64, U128}; #[test] @@ -564,4 +572,9 @@ mod tests { ), ); } + + #[test] + fn test_default() { + assert_eq(U128::default(), U128::zero()); + } } diff --git a/noir/noir-repo/test_programs/gates_report_brillig_execution.sh b/noir/noir-repo/test_programs/gates_report_brillig_execution.sh index 6a5a699e2d8..219fbb5c11a 100755 --- a/noir/noir-repo/test_programs/gates_report_brillig_execution.sh +++ b/noir/noir-repo/test_programs/gates_report_brillig_execution.sh @@ -3,10 +3,10 @@ set -e # These tests are incompatible with gas reporting excluded_dirs=( - "workspace" - "workspace_default_member" - "double_verify_nested_proof" - "overlapping_dep_and_mod" + "workspace" + "workspace_default_member" + "double_verify_nested_proof" + "overlapping_dep_and_mod" "comptime_println" # bit sizes for bigint operation doesn't match up. "bigint" @@ -33,6 +33,10 @@ for dir in $test_dirs; do continue fi + if [[ ! -f "${base_path}/${dir}/Nargo.toml" ]]; then + continue + fi + echo " \"execution_success/$dir\"," >> Nargo.toml done diff --git a/noir/noir-repo/tooling/nargo_cli/src/cli/info_cmd.rs b/noir/noir-repo/tooling/nargo_cli/src/cli/info_cmd.rs index 651ddbc0b56..8d0fc257e1c 100644 --- a/noir/noir-repo/tooling/nargo_cli/src/cli/info_cmd.rs +++ b/noir/noir-repo/tooling/nargo_cli/src/cli/info_cmd.rs @@ -257,7 +257,14 @@ fn profile_brillig_execution( initial_witness, &Bn254BlackBoxSolver(pedantic_solving), &mut DefaultForeignCallBuilder::default().build(), - )?; + ) + .map_err(|e| { + CliError::Generic(format!( + "failed to execute '{}': {}", + package.root_dir.to_string_lossy(), + e + )) + })?; let expression_width = get_target_width(package.expression_width, expression_width);