chore: move module related code from fm to noirc-frontend

Cargo.toml

@@ -121,6 +121,7 @@ num-bigint = "0.4"
 num-traits = "0.2"
 similar-asserts = "1.5.0"
 log = "0.4.17"
+tempfile = "3.6.0"
 
 [profile.dev]
 # This is required to be able to run `cargo test` in acvm_js due to the `locals exceeds maximum` error.

compiler/fm/Cargo.toml

@@ -12,5 +12,5 @@ codespan-reporting.workspace = true
 serde.workspace = true
 
 [dev-dependencies]
-tempfile = "3.2.0"
+tempfile.workspace = true
 iter-extended.workspace = true

compiler/fm/src/lib.rs

@@ -99,54 +99,12 @@ impl FileManager {
         self.id_to_path.get(&file_id).unwrap().as_path()
     }
 
-    // TODO: This should also ideally not be here, so that the file manager
-    // TODO: does not know about rust modules.
-    // TODO: Ideally this is moved to def_collector_mod and we make this method accept a FileManager
-    pub fn find_module(&self, anchor: FileId, mod_name: &str) -> Result<FileId, String> {
-        let anchor_path = self.path(anchor).with_extension("");
-        let anchor_dir = anchor_path.parent().unwrap();
-
-        // if `anchor` is a `main.nr`, `lib.nr`, `mod.nr` or `{mod_name}.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}"))
-        };
-
-        self.name_to_id(candidate.clone())
-            .ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
-    }
-
     // TODO: This should accept a &Path instead of a PathBuf
     pub fn name_to_id(&self, file_name: PathBuf) -> Option<FileId> {
         self.file_map.get_file_id(&PathString::from_path(file_name))
     }
 }
 
-// TODO: This should not be here because the file manager should not know about the
-// TODO: rust modules. See comment on `find_module``
-// TODO: Moreover, the check for main, lib, mod should ideally not be done here
-/// 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_stem() {
-        // 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_stem()
-    } 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
-    }
-}
-
 pub trait NormalizePath {
     /// Replacement for `std::fs::canonicalize` that doesn't verify the path exists.
     ///
@@ -236,22 +194,6 @@ mod tests {
         file_path
     }
 
-    #[test]
-    fn path_resolve_file_module() {
-        let dir = tempdir().unwrap();
-
-        let entry_file_name = Path::new("my_dummy_file.nr");
-        create_dummy_file(&dir, entry_file_name);
-
-        let mut fm = FileManager::new(dir.path());
-
-        let file_id = fm.add_file_with_source(entry_file_name, "fn foo() {}".to_string()).unwrap();
-
-        let dep_file_name = Path::new("foo.nr");
-        create_dummy_file(&dir, dep_file_name);
-        fm.find_module(file_id, "foo").unwrap_err();
-    }
-
     #[test]
     fn path_resolve_file_module_other_ext() {
         let dir = tempdir().unwrap();
@@ -265,47 +207,6 @@ mod tests {
         assert!(fm.path(file_id).ends_with("foo.nr"));
     }
 
-    #[test]
-    fn path_resolve_sub_module() {
-        let dir = tempdir().unwrap();
-        let mut fm = FileManager::new(dir.path());
-
-        // Create a lib.nr file at the root.
-        // we now have dir/lib.nr
-        let lib_nr_path = create_dummy_file(&dir, Path::new("lib.nr"));
-        let file_id = fm
-            .add_file_with_source(lib_nr_path.as_path(), "fn foo() {}".to_string())
-            .expect("could not add file to file manager and obtain a FileId");
-
-        // Create a sub directory
-        // we now have:
-        // - dir/lib.nr
-        // - dir/sub_dir
-        let sub_dir = TempDir::new_in(&dir).unwrap();
-        let sub_dir_name = sub_dir.path().file_name().unwrap().to_str().unwrap();
-
-        // Add foo.nr to the subdirectory
-        // we no have:
-        // - dir/lib.nr
-        // - dir/sub_dir/foo.nr
-        let foo_nr_path = create_dummy_file(&sub_dir, Path::new("foo.nr"));
-        fm.add_file_with_source(foo_nr_path.as_path(), "fn foo() {}".to_string());
-
-        // Add a parent module for the sub_dir
-        // we no have:
-        // - dir/lib.nr
-        // - dir/sub_dir.nr
-        // - dir/sub_dir/foo.nr
-        let sub_dir_nr_path = create_dummy_file(&dir, Path::new(&format!("{sub_dir_name}.nr")));
-        fm.add_file_with_source(sub_dir_nr_path.as_path(), "fn foo() {}".to_string());
-
-        // First check for the sub_dir.nr file and add it to the FileManager
-        let sub_dir_file_id = fm.find_module(file_id, sub_dir_name).unwrap();
-
-        // Now check for files in it's subdirectory
-        fm.find_module(sub_dir_file_id, "foo").unwrap();
-    }
-
     /// Tests that two identical files that have different paths are treated as the same file
     /// e.g. if we start in the dir ./src and have a file ../../foo.nr
     /// that should be treated as the same file as ../ starting in ./

compiler/noirc_frontend/Cargo.toml

@@ -26,3 +26,4 @@ regex = "1.9.1"
 [dev-dependencies]
 strum = "0.24"
 strum_macros = "0.24"
+tempfile.workspace = true

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

@@ -1,7 +1,7 @@
-use std::{collections::HashMap, vec};
+use std::{collections::HashMap, path::Path, vec};
 
 use acvm::acir::acir_field::FieldOptions;
-use fm::FileId;
+use fm::{FileId, FileManager, FILE_EXTENSION};
 use noirc_errors::Location;
 
 use crate::{
@@ -396,7 +396,7 @@
                         let modifiers = FunctionModifiers {
                             name: name.to_string(),
                             visibility: crate::FunctionVisibility::Public,
                             // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629
                             attributes: crate::token::Attributes::empty(),
                             is_unconstrained: false,
                             contract_function_type: None,
@@ -455,7 +455,7 @@
                         }
                     }
                     TraitItem::Type { name } => {
                         // TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id()
                         if let Err((first_def, second_def)) = self.def_collector.def_map.modules
                             [trait_id.0.local_id.0]
                             .declare_type_alias(name.clone(), TypeAliasId::dummy_id())
@@ -524,7 +524,7 @@
     ) -> Vec<(CompilationError, FileId)> {
         let mut errors: Vec<(CompilationError, FileId)> = vec![];
         let child_file_id =
-            match context.file_manager.find_module(self.file_id, &mod_name.0.contents) {
+            match find_module(&context.file_manager, self.file_id, &mod_name.0.contents) {
                 Ok(child_file_id) => child_file_id,
                 Err(expected_path) => {
                     let mod_name = mod_name.clone();
@@ -628,3 +628,116 @@
         Ok(LocalModuleId(module_id))
     }
 }
+
+fn find_module(
+    file_manager: &FileManager,
+    anchor: FileId,
+    mod_name: &str,
+) -> Result<FileId, String> {
+    let anchor_path = file_manager.path(anchor).with_extension("");
+    let anchor_dir = anchor_path.parent().unwrap();
+
+    // if `anchor` is a `main.nr`, `lib.nr`, `mod.nr` or `{mod_name}.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}"))
+    };
+
+    file_manager
+        .name_to_id(candidate.clone())
+        .ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
+}
+
+/// Returns true if a module's child modules 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_stem() {
+        // 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_stem()
+    } 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
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use std::path::PathBuf;
+    use tempfile::{tempdir, TempDir};
+
+    // Returns the absolute path to the file
+    fn create_dummy_file(dir: &TempDir, file_name: &Path) -> PathBuf {
+        let file_path = dir.path().join(file_name);
+        let _file = std::fs::File::create(&file_path).unwrap();
+        file_path
+    }
+
+    #[test]
+    fn path_resolve_file_module() {
+        let dir = tempdir().unwrap();
+
+        let entry_file_name = Path::new("my_dummy_file.nr");
+        create_dummy_file(&dir, entry_file_name);
+
+        let mut fm = FileManager::new(dir.path());
+
+        let file_id = fm.add_file_with_source(entry_file_name, "fn foo() {}".to_string()).unwrap();
+
+        let dep_file_name = Path::new("foo.nr");
+        create_dummy_file(&dir, dep_file_name);
+        find_module(&fm, file_id, "foo").unwrap_err();
+    }
+
+    #[test]
+    fn path_resolve_sub_module() {
+        let dir = tempdir().unwrap();
+        let mut fm = FileManager::new(dir.path());
+
+        // Create a lib.nr file at the root.
+        // we now have dir/lib.nr
+        let lib_nr_path = create_dummy_file(&dir, Path::new("lib.nr"));
+        let file_id = fm
+            .add_file_with_source(lib_nr_path.as_path(), "fn foo() {}".to_string())
+            .expect("could not add file to file manager and obtain a FileId");
+
+        // Create a sub directory
+        // we now have:
+        // - dir/lib.nr
+        // - dir/sub_dir
+        let sub_dir = TempDir::new_in(&dir).unwrap();
+        let sub_dir_name = sub_dir.path().file_name().unwrap().to_str().unwrap();
+
+        // Add foo.nr to the subdirectory
+        // we no have:
+        // - dir/lib.nr
+        // - dir/sub_dir/foo.nr
+        let foo_nr_path = create_dummy_file(&sub_dir, Path::new("foo.nr"));
+        fm.add_file_with_source(foo_nr_path.as_path(), "fn foo() {}".to_string());
+
+        // Add a parent module for the sub_dir
+        // we no have:
+        // - dir/lib.nr
+        // - dir/sub_dir.nr
+        // - dir/sub_dir/foo.nr
+        let sub_dir_nr_path = create_dummy_file(&dir, Path::new(&format!("{sub_dir_name}.nr")));
+        fm.add_file_with_source(sub_dir_nr_path.as_path(), "fn foo() {}".to_string());
+
+        // First check for the sub_dir.nr file and add it to the FileManager
+        let sub_dir_file_id = find_module(&fm, file_id, sub_dir_name).unwrap();
+
+        // Now check for files in it's subdirectory
+        find_module(&fm, sub_dir_file_id, "foo").unwrap();
+    }
+}

tooling/backend_interface/Cargo.toml

@@ -18,7 +18,7 @@ serde_json.workspace = true
 bb_abstraction_leaks.workspace = true
 log.workspace = true
 
-tempfile = "3.6.0"
+tempfile.workspace = true
 
 ## bb binary downloading
 tar = "~0.4.15"

tooling/nargo/Cargo.toml

@@ -29,4 +29,4 @@ rayon = "1.8.0"
 [dev-dependencies]
 # TODO: This dependency is used to generate unit tests for `get_all_paths_in_dir`
 # TODO: once that method is moved to nargo_cli, we can move this dependency to nargo_cli
-tempfile = "3.2.0"
+tempfile.workspace = true

tooling/nargo_cli/Cargo.toml

@@ -57,7 +57,7 @@ bb_abstraction_leaks.workspace = true
 tokio-util = { version = "0.7.8", features = ["compat"] }
 
 [dev-dependencies]
-tempfile = "3.6.0"
+tempfile.workspace = true
 dirs.workspace = true
 assert_cmd = "2.0.8"
 assert_fs = "1.0.10"