feat!: require trait method calls (foo.bar()) to have the trait in …

…scope (imported) (#6895)

Co-authored-by: Tom French <[email protected]>

.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
 

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,

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(

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<HirMethodReference> {
         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<StructType>,
+    ) -> Option<HirMethodReference> {
+        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<T> 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(

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)