diff --git a/.github/workflows/test-js-packages.yml b/.github/workflows/test-js-packages.yml index 2782b1aa44a..5222b667c2a 100644 --- a/.github/workflows/test-js-packages.yml +++ b/.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/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index d664f856262..aeee417789e 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/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/compiler/noirc_frontend/src/elaborator/path_resolution.rs b/compiler/noirc_frontend/src/elaborator/path_resolution.rs index f9f6fff56e0..010c5305fb3 100644 --- a/compiler/noirc_frontend/src/elaborator/path_resolution.rs +++ b/compiler/noirc_frontend/src/elaborator/path_resolution.rs @@ -2,13 +2,12 @@ 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; use crate::hir::resolution::visibility::item_in_module_is_visible; -use crate::hir_def::traits::Trait; use crate::locations::ReferencesTracker; use crate::node_interner::{FuncId, GlobalId, StructId, TraitId, TypeAliasId}; use crate::{Shared, Type, TypeAlias}; @@ -448,10 +447,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/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index c3375fc35ad..e01cda3a2e4 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/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/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 12b99818c46..13a9400bd2e 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/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/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 3ed183df49c..25bc507da1e 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/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/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index dabfcfe9191..f1db0df9540 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/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/compiler/noirc_frontend/src/tests/traits.rs b/compiler/noirc_frontend/src/tests/traits.rs index ba70daa297d..82c8460d004 100644 --- a/compiler/noirc_frontend/src/tests/traits.rs +++ b/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_stdlib/src/uint128.nr b/noir_stdlib/src/uint128.nr index fab8cacc055..c87cae98b3f 100644 --- a/noir_stdlib/src/uint128.nr +++ b/noir_stdlib/src/uint128.nr @@ -332,6 +332,7 @@ impl Shr for U128 { } mod tests { + use crate::ops::Not; use crate::uint128::{pow63, pow64, U128}; #[test]