fix: compiler identifying imported functions as being part of a contr…

…act (#1112)

* Separate items defined within a module from those just imported into the module

* Fix test

crates/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -138,8 +138,7 @@ impl DefCollector {
             let name = resolved_import.name;
             for ns in resolved_import.resolved_namespace.iter_defs() {
                 let result = current_def_map.modules[resolved_import.module_scope.0]
-                    .scope
-                    .add_item_to_namespace(name.clone(), ns);
+                    .import(name.clone(), ns);
 
                 if let Err((first_def, second_def)) = result {
                     let err = DefCollectorErrorKind::DuplicateImport { first_def, second_def };
@@ -224,14 +223,13 @@ fn collect_impls(
             extend_errors(errors, unresolved.file_id, resolver.take_errors());
 
             if let Some(type_module) = get_local_id_from_type(&typ) {
-                // Grab the scope defined by the struct type. Note that impls are a case
-                // where the scope the methods are added to is not the same as the scope
+                // Grab the module defined by the struct type. Note that impls are a case
+                // where the module the methods are added to is not the same as the module
                 // they are resolved in.
-                let scope = &mut def_maps.get_mut(&crate_id).unwrap().modules[type_module.0].scope;
+                let module = &mut def_maps.get_mut(&crate_id).unwrap().modules[type_module.0];
 
-                // .define_func_def(name, func_id);
                 for (_, method_id, method) in &unresolved.functions {
-                    let result = scope.define_func_def(method.name_ident().clone(), *method_id);
+                    let result = module.declare_function(method.name_ident().clone(), *method_id);
 
                     if let Err((first_def, second_def)) = result {
                         let err =

crates/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -75,9 +75,8 @@ impl<'a> ModCollector<'a> {
             let stmt_id = context.def_interner.push_empty_global();
 
             // Add the statement to the scope so its path can be looked up later
-            let result = self.def_collector.def_map.modules[self.module_id.0]
-                .scope
-                .define_global(name, stmt_id);
+            let result =
+                self.def_collector.def_map.modules[self.module_id.0].declare_global(name, stmt_id);
 
             if let Err((first_def, second_def)) = result {
                 let err = DefCollectorErrorKind::DuplicateGlobal { first_def, second_def };
@@ -137,8 +136,7 @@ impl<'a> ModCollector<'a> {
 
             // Add function to scope/ns of the module
             let result = self.def_collector.def_map.modules[self.module_id.0]
-                .scope
-                .define_func_def(name, func_id);
+                .declare_function(name, func_id);
 
             if let Err((first_def, second_def)) = result {
                 let error = DefCollectorErrorKind::DuplicateFunction { first_def, second_def };
@@ -167,9 +165,8 @@ impl<'a> ModCollector<'a> {
             };
 
             // Add the struct to scope so its path can be looked up later
-            let result = self.def_collector.def_map.modules[self.module_id.0]
-                .scope
-                .define_struct_def(name, id);
+            let result =
+                self.def_collector.def_map.modules[self.module_id.0].declare_struct(name, id);
 
             if let Err((first_def, second_def)) = result {
                 let err = DefCollectorErrorKind::DuplicateFunction { first_def, second_def };
@@ -288,7 +285,7 @@ impl<'a> ModCollector<'a> {
             };
 
             if let Err((first_def, second_def)) =
-                modules[self.module_id.0].scope.define_module_def(mod_name.to_owned(), mod_id)
+                modules[self.module_id.0].declare_child_module(mod_name.to_owned(), mod_id)
             {
                 let err = DefCollectorErrorKind::DuplicateModuleDecl { first_def, second_def };
                 errors.push(err.into_file_diagnostic(self.file_id));

crates/noirc_frontend/src/hir/def_map/item_scope.rs

@@ -1,8 +1,5 @@
 use super::{namespace::PerNs, ModuleDefId, ModuleId};
-use crate::{
-    node_interner::{FuncId, StmtId, StructId},
-    Ident,
-};
+use crate::{node_interner::FuncId, Ident};
 use std::collections::{hash_map::Entry, HashMap};
 
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
@@ -55,54 +52,34 @@ impl ItemScope {
         }
     }
 
-    pub fn define_module_def(
-        &mut self,
-        name: Ident,
-        mod_id: ModuleId,
-    ) -> Result<(), (Ident, Ident)> {
-        self.add_definition(name, mod_id.into())
-    }
-
-    pub fn define_func_def(&mut self, name: Ident, local_id: FuncId) -> Result<(), (Ident, Ident)> {
-        self.add_definition(name, local_id.into())
-    }
-
-    pub fn define_struct_def(
-        &mut self,
-        name: Ident,
-        local_id: StructId,
-    ) -> Result<(), (Ident, Ident)> {
-        self.add_definition(name, ModuleDefId::TypeId(local_id))
-    }
-
-    pub fn define_global(&mut self, name: Ident, stmt_id: StmtId) -> Result<(), (Ident, Ident)> {
-        self.add_definition(name, ModuleDefId::GlobalId(stmt_id))
-    }
-
     pub fn find_module_with_name(&self, mod_name: &Ident) -> Option<&ModuleId> {
         let (module_def, _) = self.types.get(mod_name)?;
         match module_def {
             ModuleDefId::ModuleId(id) => Some(id),
             _ => None,
         }
     }
+
     pub fn find_func_with_name(&self, func_name: &Ident) -> Option<FuncId> {
         let (module_def, _) = self.values.get(func_name)?;
         match module_def {
             ModuleDefId::FunctionId(id) => Some(*id),
             _ => None,
         }
     }
+
     pub fn find_name(&self, name: &Ident) -> PerNs {
         PerNs { types: self.types.get(name).cloned(), values: self.values.get(name).cloned() }
     }
 
     pub fn definitions(&self) -> Vec<ModuleDefId> {
         self.defs.clone()
     }
+
     pub fn types(&self) -> &HashMap<Ident, (ModuleDefId, Visibility)> {
         &self.types
     }
+
     pub fn values(&self) -> &HashMap<Ident, (ModuleDefId, Visibility)> {
         &self.values
     }

crates/noirc_frontend/src/hir/def_map/mod.rs

@@ -110,7 +110,7 @@ impl CrateDefMap {
 
         // This function accepts an Ident, so we attach a dummy span to
         // "main". Equality is implemented only on the contents.
-        root_module.scope.find_func_with_name(&MAIN_FUNCTION.into())
+        root_module.find_func_with_name(&MAIN_FUNCTION.into())
     }
 
     pub fn root_file_id(&self) -> FileId {
@@ -129,8 +129,10 @@ impl CrateDefMap {
         interner: &'a NodeInterner,
     ) -> impl Iterator<Item = FuncId> + 'a {
         self.modules.iter().flat_map(|(_, module)| {
-            let functions = module.scope.values().values().filter_map(|(id, _)| id.as_function());
-            functions.filter(|id| interner.function_meta(id).attributes == Some(Attribute::Test))
+            module
+                .value_definitions()
+                .filter_map(|id| id.as_function())
+                .filter(|id| interner.function_meta(id).attributes == Some(Attribute::Test))
         })
     }
 
@@ -141,13 +143,8 @@ impl CrateDefMap {
             .iter()
             .filter_map(|(id, module)| {
                 if module.is_contract {
-                    let functions = module
-                        .scope
-                        .values()
-                        .values()
-                        .filter_map(|(id, _)| id.as_function())
-                        .collect();
-
+                    let functions =
+                        module.value_definitions().filter_map(|id| id.as_function()).collect();
                     let name = self.get_module_path(id, module.parent);
                     Some(Contract { name, functions })
                 } else {

crates/noirc_frontend/src/hir/def_map/module_data.rs

@@ -2,17 +2,26 @@ use std::collections::HashMap;
 
 use fm::FileId;
 
-use crate::Ident;
+use crate::{
+    node_interner::{FuncId, StmtId, StructId},
+    Ident,
+};
 
-use super::{ItemScope, LocalModuleId};
+use super::{ItemScope, LocalModuleId, ModuleDefId, ModuleId, PerNs};
 
 /// Contains the actual contents of a module: its parent (if one exists),
 /// children, and scope with all definitions defined within the scope.
 #[derive(Debug, PartialEq, Eq)]
 pub struct ModuleData {
     pub parent: Option<LocalModuleId>,
     pub children: HashMap<Ident, LocalModuleId>,
-    pub scope: ItemScope,
+
+    /// Contains all definitions visible to the current module. This includes
+    /// all definitions in self.definitions as well as all imported definitions.
+    scope: ItemScope,
+
+    /// Contains only the definitions directly defined in the current module
+    definitions: ItemScope,
 
     pub origin: ModuleOrigin,
 
@@ -30,10 +39,57 @@ impl ModuleData {
             parent,
             children: HashMap::new(),
             scope: ItemScope::default(),
+            definitions: ItemScope::default(),
             origin,
             is_contract,
         }
     }
+
+    fn declare(&mut self, name: Ident, item_id: ModuleDefId) -> Result<(), (Ident, Ident)> {
+        self.scope.add_definition(name.clone(), item_id)?;
+
+        // definitions is a subset of self.scope so it is expected if self.scope.define_func_def
+        // returns without error, so will self.definitions.define_func_def.
+        self.definitions.add_definition(name, item_id)
+    }
+
+    pub fn declare_function(&mut self, name: Ident, id: FuncId) -> Result<(), (Ident, Ident)> {
+        self.declare(name, id.into())
+    }
+
+    pub fn declare_global(&mut self, name: Ident, id: StmtId) -> Result<(), (Ident, Ident)> {
+        self.declare(name, id.into())
+    }
+
+    pub fn declare_struct(&mut self, name: Ident, id: StructId) -> Result<(), (Ident, Ident)> {
+        self.declare(name, ModuleDefId::TypeId(id))
+    }
+
+    pub fn declare_child_module(
+        &mut self,
+        name: Ident,
+        child_id: ModuleId,
+    ) -> Result<(), (Ident, Ident)> {
+        self.declare(name, child_id.into())
+    }
+
+    pub fn find_func_with_name(&self, name: &Ident) -> Option<FuncId> {
+        self.scope.find_func_with_name(name)
+    }
+
+    pub fn import(&mut self, name: Ident, id: ModuleDefId) -> Result<(), (Ident, Ident)> {
+        self.scope.add_item_to_namespace(name, id)
+    }
+
+    pub fn find_name(&self, name: &Ident) -> PerNs {
+        self.scope.find_name(name)
+    }
+
+    /// Return an iterator over all definitions defined within this module,
+    /// excluding any type definitions.
+    pub fn value_definitions(&self) -> impl Iterator<Item = ModuleDefId> + '_ {
+        self.definitions.values().values().map(|(id, _)| *id)
+    }
 }
 
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]

crates/noirc_frontend/src/hir/resolution/import.rs

@@ -135,7 +135,7 @@ fn resolve_name_in_module(
 
     let mut import_path = import_path.iter();
     let first_segment = import_path.next().expect("ice: could not fetch first segment");
-    let mut current_ns = current_mod.scope.find_name(first_segment);
+    let mut current_ns = current_mod.find_name(first_segment);
     if current_ns.is_none() {
         return Err(PathResolutionError::Unresolved(first_segment.clone()));
     }
@@ -158,7 +158,7 @@ fn resolve_name_in_module(
         current_mod = &def_maps[&new_module_id.krate].modules[new_module_id.local_id.0];
 
         // Check if namespace
-        let found_ns = current_mod.scope.find_name(segment);
+        let found_ns = current_mod.find_name(segment);
         if found_ns.is_none() {
             return Err(PathResolutionError::Unresolved(segment.clone()));
         }

crates/noirc_frontend/src/main.rs

@@ -41,18 +41,15 @@ fn main() {
     // Get root module
     let root = def_map.root();
     let module = def_map.modules().get(root.0).unwrap();
-    for (name, (def_id, vis)) in module.scope.values() {
-        println!("func name is {:?}", name);
+    for def_id in module.value_definitions() {
         let func_id = match def_id {
             ModuleDefId::FunctionId(func_id) => func_id,
             _ => unreachable!(),
         };
 
         // Get the HirFunction for that Id
-        let hir = context.def_interner.function(func_id);
-
+        let hir = context.def_interner.function(&func_id);
         println!("func hir is {:?}", hir);
-        println!("func vis is {:?}", vis);
     }
     //