feat: require trait function calls (Foo::bar()) to have the trait i…

…n scope (imported) (#6882)

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

compiler/noirc_frontend/src/elaborator/comptime.rs

@@ -93,6 +93,7 @@ impl<'context> Elaborator<'context> {
             self.interner,
             self.def_maps,
             self.usage_tracker,
+            self.crate_graph,
             self.crate_id,
             self.debug_comptime_in_file,
             self.interpreter_call_stack.clone(),

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -4,8 +4,8 @@ use std::{
 };
 
 use crate::{
-    ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, node_interner::GlobalValue,
-    usage_tracker::UsageTracker, StructField, StructType, TypeBindings,
+    ast::ItemVisibility, graph::CrateGraph, hir_def::traits::ResolvedTraitBound,
+    node_interner::GlobalValue, usage_tracker::UsageTracker, StructField, StructType, TypeBindings,
 };
 use crate::{
     ast::{
@@ -97,6 +97,7 @@ pub struct Elaborator<'context> {
     pub(crate) interner: &'context mut NodeInterner,
     pub(crate) def_maps: &'context mut DefMaps,
     pub(crate) usage_tracker: &'context mut UsageTracker,
+    pub(crate) crate_graph: &'context CrateGraph,
 
     pub(crate) file: FileId,
 
@@ -201,6 +202,7 @@ impl<'context> Elaborator<'context> {
         interner: &'context mut NodeInterner,
         def_maps: &'context mut DefMaps,
         usage_tracker: &'context mut UsageTracker,
+        crate_graph: &'context CrateGraph,
         crate_id: CrateId,
         debug_comptime_in_file: Option<FileId>,
         interpreter_call_stack: im::Vector<Location>,
@@ -211,6 +213,7 @@ impl<'context> Elaborator<'context> {
             interner,
             def_maps,
             usage_tracker,
+            crate_graph,
             file: FileId::dummy(),
             unsafe_block_status: UnsafeBlockStatus::NotInUnsafeBlock,
             nested_loops: 0,
@@ -242,6 +245,7 @@ impl<'context> Elaborator<'context> {
             &mut context.def_interner,
             &mut context.def_maps,
             &mut context.usage_tracker,
+            &context.crate_graph,
             crate_id,
             debug_comptime_in_file,
             im::Vector::new(),
@@ -1239,7 +1243,7 @@ impl<'context> Elaborator<'context> {
             self.file = unresolved.file_id;
             let old_generic_count = self.generics.len();
             self.add_generics(generics);
-            self.declare_methods_on_struct(false, unresolved, *span);
+            self.declare_methods_on_struct(None, unresolved, *span);
             self.generics.truncate(old_generic_count);
         }
     }
@@ -1269,7 +1273,7 @@ impl<'context> Elaborator<'context> {
             self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause);
 
             let span = trait_impl.object_type.span;
-            self.declare_methods_on_struct(true, &mut trait_impl.methods, span);
+            self.declare_methods_on_struct(Some(trait_id), &mut trait_impl.methods, span);
 
             let methods = trait_impl.methods.function_ids();
             for func_id in &methods {
@@ -1328,7 +1332,7 @@ impl<'context> Elaborator<'context> {
 
     fn declare_methods_on_struct(
         &mut self,
-        is_trait_impl: bool,
+        trait_id: Option<TraitId>,
         functions: &mut UnresolvedFunctions,
         span: Span,
     ) {
@@ -1342,7 +1346,7 @@ impl<'context> Elaborator<'context> {
             let struct_ref = struct_type.borrow();
 
             // `impl`s are only allowed on types defined within the current crate
-            if !is_trait_impl && struct_ref.id.krate() != self.crate_id {
+            if trait_id.is_none() && struct_ref.id.krate() != self.crate_id {
                 let type_name = struct_ref.name.to_string();
                 self.push_err(DefCollectorErrorKind::ForeignImpl { span, type_name });
                 return;
@@ -1359,7 +1363,12 @@ impl<'context> Elaborator<'context> {
                 // object types in each method overlap or not. If they do, we issue an error.
                 // If not, that is specialization which is allowed.
                 let name = method.name_ident().clone();
-                if module.declare_function(name, method.def.visibility, *method_id).is_err() {
+                let result = if let Some(trait_id) = trait_id {
+                    module.declare_trait_function(name, *method_id, trait_id)
+                } else {
+                    module.declare_function(name, method.def.visibility, *method_id)
+                };
+                if result.is_err() {
                     let existing = module.find_func_with_name(method.name_ident()).expect(
                         "declare_function should only error if there is an existing function",
                     );
@@ -1374,14 +1383,14 @@ impl<'context> Elaborator<'context> {
             }
 
             // Trait impl methods are already declared in NodeInterner::add_trait_implementation
-            if !is_trait_impl {
+            if trait_id.is_none() {
                 self.declare_methods(self_type, &function_ids);
             }
         // We can define methods on primitive types only if we're in the stdlib
-        } else if !is_trait_impl && *self_type != Type::Error {
+        } else if trait_id.is_none() && *self_type != Type::Error {
             if self.crate_id.is_stdlib() {
                 // Trait impl methods are already declared in NodeInterner::add_trait_implementation
-                if !is_trait_impl {
+                if trait_id.is_none() {
                     self.declare_methods(self_type, &function_ids);
                 }
             } else {

compiler/noirc_frontend/src/elaborator/path_resolution.rs

@@ -1,12 +1,14 @@
+use iter_extended::vecmap;
 use noirc_errors::{Location, Span};
 
-use crate::ast::{Path, PathKind, UnresolvedType};
-use crate::hir::def_map::{ModuleDefId, ModuleId};
+use crate::ast::{Ident, Path, PathKind, UnresolvedType};
+use crate::hir::def_map::{fully_qualified_module_path, 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};
@@ -86,6 +88,21 @@ enum IntermediatePathResolutionItem {
 
 pub(crate) type PathResolutionResult = Result<PathResolution, PathResolutionError>;
 
+enum StructMethodLookupResult {
+    /// The method could not be found. There might be trait methods that could be imported,
+    /// but none of them are.
+    NotFound(Vec<TraitId>),
+    /// Found a struct method.
+    FoundStructMethod(PerNs),
+    /// Found a trait method and it's currently in scope.
+    FoundTraitMethod(PerNs, TraitId),
+    /// There's only one trait method that matches, but it's not in scope
+    /// (we'll warn about this to avoid introducing a large breaking change)
+    FoundOneTraitMethodButNotInScope(PerNs, TraitId),
+    /// Multiple trait method matches were found and they are all in scope.
+    FoundMultipleTraitMethods(Vec<TraitId>),
+}
+
 impl<'context> Elaborator<'context> {
     pub(super) fn resolve_path_or_error(
         &mut self,
@@ -195,7 +212,9 @@ impl<'context> Elaborator<'context> {
                 last_segment.ident.is_self_type_name(),
             );
 
-            (current_module_id, intermediate_item) = match typ {
+            let current_module_id_is_struct;
+
+            (current_module_id, current_module_id_is_struct, intermediate_item) = match typ {
                 ModuleDefId::ModuleId(id) => {
                     if last_segment_generics.is_some() {
                         errors.push(PathResolutionError::TurbofishNotAllowedOnItem {
@@ -204,10 +223,11 @@ impl<'context> Elaborator<'context> {
                         });
                     }
 
-                    (id, IntermediatePathResolutionItem::Module(id))
+                    (id, false, IntermediatePathResolutionItem::Module(id))
                 }
                 ModuleDefId::TypeId(id) => (
                     id.module_id(),
+                    true,
                     IntermediatePathResolutionItem::Struct(
                         id,
                         last_segment_generics.as_ref().map(|generics| Turbofish {
@@ -224,6 +244,7 @@ impl<'context> Elaborator<'context> {
 
                     (
                         module_id,
+                        true,
                         IntermediatePathResolutionItem::TypeAlias(
                             id,
                             last_segment_generics.as_ref().map(|generics| Turbofish {
@@ -235,6 +256,7 @@ impl<'context> Elaborator<'context> {
                 }
                 ModuleDefId::TraitId(id) => (
                     id.0,
+                    false,
                     IntermediatePathResolutionItem::Trait(
                         id,
                         last_segment_generics.as_ref().map(|generics| Turbofish {
@@ -263,7 +285,57 @@ impl<'context> Elaborator<'context> {
             current_module = self.get_module(current_module_id);
 
             // Check if namespace
-            let found_ns = current_module.find_name(current_ident);
+            let found_ns = if current_module_id_is_struct {
+                match self.resolve_struct_function(starting_module, current_module, current_ident) {
+                    StructMethodLookupResult::NotFound(vec) => {
+                        if vec.is_empty() {
+                            return Err(PathResolutionError::Unresolved(current_ident.clone()));
+                        } else {
+                            let traits = vecmap(vec, |trait_id| {
+                                let trait_ = self.interner.get_trait(trait_id);
+                                self.fully_qualified_trait_path(trait_)
+                            });
+                            return Err(
+                                PathResolutionError::UnresolvedWithPossibleTraitsToImport {
+                                    ident: current_ident.clone(),
+                                    traits,
+                                },
+                            );
+                        }
+                    }
+                    StructMethodLookupResult::FoundStructMethod(per_ns) => per_ns,
+                    StructMethodLookupResult::FoundTraitMethod(per_ns, trait_id) => {
+                        let trait_ = self.interner.get_trait(trait_id);
+                        self.usage_tracker.mark_as_used(starting_module, &trait_.name);
+                        per_ns
+                    }
+                    StructMethodLookupResult::FoundOneTraitMethodButNotInScope(
+                        per_ns,
+                        trait_id,
+                    ) => {
+                        let trait_ = self.interner.get_trait(trait_id);
+                        let trait_name = self.fully_qualified_trait_path(trait_);
+                        errors.push(PathResolutionError::TraitMethodNotInScope {
+                            ident: current_ident.clone(),
+                            trait_name,
+                        });
+                        per_ns
+                    }
+                    StructMethodLookupResult::FoundMultipleTraitMethods(vec) => {
+                        let traits = vecmap(vec, |trait_id| {
+                            let trait_ = self.interner.get_trait(trait_id);
+                            self.usage_tracker.mark_as_used(starting_module, &trait_.name);
+                            self.fully_qualified_trait_path(trait_)
+                        });
+                        return Err(PathResolutionError::MultipleTraitsInScope {
+                            ident: current_ident.clone(),
+                            traits,
+                        });
+                    }
+                }
+            } else {
+                current_module.find_name(current_ident)
+            };
             if found_ns.is_none() {
                 return Err(PathResolutionError::Unresolved(current_ident.clone()));
             }
@@ -307,6 +379,78 @@ impl<'context> Elaborator<'context> {
             None
         }
     }
+
+    fn resolve_struct_function(
+        &self,
+        starting_module_id: ModuleId,
+        current_module: &ModuleData,
+        ident: &Ident,
+    ) -> StructMethodLookupResult {
+        // If the current module is a struct, next we need to find a function for it.
+        // The function could be in the struct itself, or it could be defined in traits.
+        let item_scope = current_module.scope();
+        let Some(values) = item_scope.values().get(ident) else {
+            return StructMethodLookupResult::NotFound(vec![]);
+        };
+
+        // First search if the function is defined in the struct itself
+        if let Some(item) = values.get(&None) {
+            return StructMethodLookupResult::FoundStructMethod(PerNs {
+                types: None,
+                values: Some(*item),
+            });
+        }
+
+        // Otherwise, the function could be defined in zero, one or more traits.
+        let starting_module = self.get_module(starting_module_id);
+
+        // Gather a list of items for which their trait is in scope.
+        let mut results = Vec::new();
+
+        for (trait_id, item) in values.iter() {
+            let trait_id = trait_id.expect("The None option was already considered before");
+            let trait_ = self.interner.get_trait(trait_id);
+            let Some(map) = starting_module.scope().types().get(&trait_.name) else {
+                continue;
+            };
+            let Some(imported_item) = map.get(&None) else {
+                continue;
+            };
+            if imported_item.0 == ModuleDefId::TraitId(trait_id) {
+                results.push((trait_id, item));
+            }
+        }
+
+        if results.is_empty() {
+            if values.len() == 1 {
+                // This is the backwards-compatible case where there's a single trait method but it's not in scope
+                let (trait_id, item) = values.iter().next().expect("Expected an item");
+                let trait_id = trait_id.expect("The None option was already considered before");
+                let per_ns = PerNs { types: None, values: Some(*item) };
+                return StructMethodLookupResult::FoundOneTraitMethodButNotInScope(
+                    per_ns, trait_id,
+                );
+            } else {
+                let trait_ids = vecmap(values, |(trait_id, _)| {
+                    trait_id.expect("The none option was already considered before")
+                });
+                return StructMethodLookupResult::NotFound(trait_ids);
+            }
+        }
+
+        if results.len() > 1 {
+            let trait_ids = vecmap(results, |(trait_id, _)| trait_id);
+            return StructMethodLookupResult::FoundMultipleTraitMethods(trait_ids);
+        }
+
+        let (trait_id, item) = results.remove(0);
+        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/graph/mod.rs

@@ -113,6 +113,41 @@ pub struct CrateGraph {
     arena: FxHashMap<CrateId, CrateData>,
 }
 
+impl CrateGraph {
+    /// Tries to find the requested crate in the current one's dependencies,
+    /// otherwise walks down the crate dependency graph from crate_id until we reach it.
+    /// This is needed in case a library (lib1) re-export a structure defined in another library (lib2)
+    /// In that case, we will get [lib1,lib2] when looking for a struct defined in lib2,
+    /// re-exported by lib1 and used by the main crate.
+    /// Returns the path from crate_id to target_crate_id
+    pub(crate) fn find_dependencies(
+        &self,
+        crate_id: &CrateId,
+        target_crate_id: &CrateId,
+    ) -> Option<Vec<String>> {
+        self[crate_id]
+            .dependencies
+            .iter()
+            .find_map(|dep| {
+                if &dep.crate_id == target_crate_id {
+                    Some(vec![dep.name.to_string()])
+                } else {
+                    None
+                }
+            })
+            .or_else(|| {
+                self[crate_id].dependencies.iter().find_map(|dep| {
+                    if let Some(mut path) = self.find_dependencies(&dep.crate_id, target_crate_id) {
+                        path.insert(0, dep.name.to_string());
+                        Some(path)
+                    } else {
+                        None
+                    }
+                })
+            })
+    }
+}
+
 /// List of characters that are not allowed in a crate name
 /// For example, Hyphen(-) is disallowed as it is similar to underscore(_)
 /// and we do not want names that differ by a hyphen