fix!: Issue an error when a module is declared twice & fix module sea…

…rch path (#2801)

compiler/fm/src/lib.rs

@@ -78,30 +78,46 @@ impl FileManager {
         // Unwrap as we ensure that all file_id's map to a corresponding file in the file map
         self.file_map.get_file(file_id).unwrap()
     }
+
     pub fn path(&self, file_id: FileId) -> &Path {
         // Unwrap as we ensure that all file_ids are created by the file manager
         // So all file_ids will points to a corresponding path
         self.id_to_path.get(&file_id).unwrap().0.as_path()
     }
 
     pub fn find_module(&mut self, anchor: FileId, mod_name: &str) -> Result<FileId, String> {
-        let mut candidate_files = Vec::new();
-
         let anchor_path = self.path(anchor).to_path_buf();
         let anchor_dir = anchor_path.parent().unwrap();
 
-        // First we attempt to look at `base/anchor/mod_name.nr` (child of the anchor)
-        candidate_files.push(anchor_path.join(format!("{mod_name}.{FILE_EXTENSION}")));
-        // If not found, we attempt to look at `base/mod_name.nr` (sibling of the anchor)
-        candidate_files.push(anchor_dir.join(format!("{mod_name}.{FILE_EXTENSION}")));
+        // if `anchor` is a `main.nr`, `lib.nr`, `mod.nr` or `{modname}.nr`, we check siblings of
+        // the anchor at `base/mod_name.nr`.
+        let candidate = if should_check_siblings_for_module(&anchor_path, anchor_dir) {
+            anchor_dir.join(format!("{mod_name}.{FILE_EXTENSION}"))
+        } else {
+            // Otherwise, we check for children of the anchor at `base/anchor/mod_name.nr`
+            anchor_path.join(format!("{mod_name}.{FILE_EXTENSION}"))
+        };
 
-        for candidate in candidate_files.iter() {
-            if let Some(file_id) = self.add_file(candidate) {
-                return Ok(file_id);
-            }
-        }
+        self.add_file(&candidate).ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
+    }
+}
 
-        Err(candidate_files.remove(0).as_os_str().to_str().unwrap().to_owned())
+/// Returns true if a module's child module's are expected to be in the same directory.
+/// Returns false if they are expected to be in a subdirectory matching the name of the module.
+fn should_check_siblings_for_module(module_path: &Path, parent_path: &Path) -> bool {
+    if let Some(filename) = module_path.file_name() {
+        // This check also means a `main.nr` or `lib.nr` file outside of the crate root would
+        // check its same directory for child modules instead of a subdirectory. Should we prohibit
+        // `main.nr` and `lib.nr` files outside of the crate root?
+        filename == "main"
+            || filename == "lib"
+            || filename == "mod"
+            || Some(filename) == parent_path.file_name()
+    } else {
+        // If there's no filename, we arbitrarily return true.
+        // Alternatively, we could panic, but this is left to a different step where we
+        // ideally have some source location to issue an error.
+        true
     }
 }
 
@@ -215,12 +231,13 @@ mod tests {
 
         let dep_file_name = Path::new("foo.nr");
         create_dummy_file(&dir, dep_file_name);
-        fm.find_module(file_id, "foo").unwrap();
+        fm.find_module(file_id, "foo").unwrap_err();
     }
+
     #[test]
     fn path_resolve_file_module_other_ext() {
         let dir = tempdir().unwrap();
-        let file_name = Path::new("foo.noir");
+        let file_name = Path::new("foo.nr");
         create_dummy_file(&dir, file_name);
 
         let mut fm = FileManager::new(dir.path());
@@ -229,6 +246,7 @@ mod tests {
 
         assert!(fm.path(file_id).ends_with("foo"));
     }
+
     #[test]
     fn path_resolve_sub_module() {
         let dir = tempdir().unwrap();

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

@@ -1,7 +1,7 @@
 use std::collections::HashSet;
 
 use fm::FileId;
-use noirc_errors::{FileDiagnostic, Location};
+use noirc_errors::{CustomDiagnostic, FileDiagnostic, Location};
 
 use crate::{
     graph::CrateId,
@@ -528,14 +528,32 @@ impl<'a> ModCollector<'a> {
         let child_file_id =
             match context.file_manager.find_module(self.file_id, &mod_name.0.contents) {
                 Ok(child_file_id) => child_file_id,
-                Err(_) => {
+                Err(expected_path) => {
+                    let mod_name = mod_name.clone();
                     let err =
-                        DefCollectorErrorKind::UnresolvedModuleDecl { mod_name: mod_name.clone() };
+                        DefCollectorErrorKind::UnresolvedModuleDecl { mod_name, expected_path };
                     errors.push(err.into_file_diagnostic(self.file_id));
                     return;
                 }
             };
 
+        let location = Location { file: self.file_id, span: mod_name.span() };
+
+        if let Some(old_location) = context.visited_files.get(&child_file_id) {
+            let message = format!("Module '{mod_name}' is already part of the crate");
+            let secondary = String::new();
+            let error = CustomDiagnostic::simple_error(message, secondary, location.span);
+            errors.push(error.in_file(location.file));
+
+            let message = format!("Note: {mod_name} was originally declared here");
+            let secondary = String::new();
+            let error = CustomDiagnostic::simple_error(message, secondary, old_location.span);
+            errors.push(error.in_file(old_location.file));
+            return;
+        }
+
+        context.visited_files.insert(child_file_id, location);
+
         // Parse the AST for the module we just found and then recursively look for it's defs
         let ast = parse_file(&context.file_manager, child_file_id, errors);
 

compiler/noirc_frontend/src/hir/def_collector/errors.rs

@@ -24,7 +24,7 @@ pub enum DefCollectorErrorKind {
     #[error("duplicate {typ} found in namespace")]
     Duplicate { typ: DuplicateType, first_def: Ident, second_def: Ident },
     #[error("unresolved import")]
-    UnresolvedModuleDecl { mod_name: Ident },
+    UnresolvedModuleDecl { mod_name: Ident, expected_path: String },
     #[error("path resolution error")]
     PathResolutionError(PathResolutionError),
     #[error("Non-struct type used in impl")]
@@ -76,27 +76,27 @@ impl From<DefCollectorErrorKind> for Diagnostic {
         match error {
             DefCollectorErrorKind::Duplicate { typ, first_def, second_def } => {
                 let primary_message = format!(
-                    "duplicate definitions of {} with name {} found",
+                    "Duplicate definitions of {} with name {} found",
                     &typ, &first_def.0.contents
                 );
                 {
                     let first_span = first_def.0.span();
                     let second_span = second_def.0.span();
                     let mut diag = Diagnostic::simple_error(
                         primary_message,
-                        format!("first {} found here", &typ),
+                        format!("First {} found here", &typ),
                         first_span,
                     );
-                    diag.add_secondary(format!("second {} found here", &typ), second_span);
+                    diag.add_secondary(format!("Second {} found here", &typ), second_span);
                     diag
                 }
             }
-            DefCollectorErrorKind::UnresolvedModuleDecl { mod_name } => {
+            DefCollectorErrorKind::UnresolvedModuleDecl { mod_name, expected_path } => {
                 let span = mod_name.0.span();
                 let mod_name = &mod_name.0.contents;
 
                 Diagnostic::simple_error(
-                    format!("could not resolve module `{mod_name}` "),
+                    format!("No module `{mod_name}` at path `{expected_path}`"),
                     String::new(),
                     span,
                 )

compiler/noirc_frontend/src/hir/mod.rs

@@ -9,6 +9,7 @@ use crate::hir_def::function::FuncMeta;
 use crate::node_interner::{FuncId, NodeInterner, StructId};
 use def_map::{Contract, CrateDefMap};
 use fm::FileManager;
+use noirc_errors::Location;
 use std::collections::BTreeMap;
 
 use self::def_map::TestFunction;
@@ -22,6 +23,10 @@ pub struct Context {
     pub(crate) def_maps: BTreeMap<CrateId, CrateDefMap>,
     pub file_manager: FileManager,
 
+    /// A map of each file that already has been visited from a prior `mod foo;` declaration.
+    /// This is used to issue an error if a second `mod foo;` is declared to the same file.
+    pub visited_files: BTreeMap<fm::FileId, Location>,
+
     /// Maps a given (contract) module id to the next available storage slot
     /// for that contract.
     pub storage_slots: BTreeMap<def_map::ModuleId, StorageSlot>,
@@ -41,6 +46,7 @@ impl Context {
         Context {
             def_interner: NodeInterner::default(),
             def_maps: BTreeMap::new(),
+            visited_files: BTreeMap::new(),
             crate_graph,
             file_manager,
             storage_slots: BTreeMap::new(),

compiler/noirc_frontend/src/hir_def/types.rs

@@ -844,8 +844,8 @@ impl Type {
             // No recursive try_unify call for struct fields. Don't want
             // to mutate shared type variables within struct definitions.
             // This isn't possible currently but will be once noir gets generic types
-            (Struct(fields_a, args_a), Struct(fields_b, args_b)) => {
-                if fields_a == fields_b {
+            (Struct(id_a, args_a), Struct(id_b, args_b)) => {
+                if id_a == id_b && args_a.len() == args_b.len() {
                     for (a, b) in args_a.iter().zip(args_b) {
                         a.try_unify(b)?;
                     }