diff --git a/Cargo.lock b/Cargo.lock index d2606d211ab..442d9750be0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2734,6 +2734,7 @@ dependencies = [ "smol_str", "strum", "strum_macros", + "tempfile", "thiserror", ] diff --git a/Cargo.toml b/Cargo.toml index 7cd00ee0a60..bcc98debdc8 100644 --- a/Cargo.toml +++ b/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" tracing = "0.1.40" diff --git a/compiler/fm/Cargo.toml b/compiler/fm/Cargo.toml index 699f709e9b5..42e4b0c25d7 100644 --- a/compiler/fm/Cargo.toml +++ b/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 diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs index 2a54e58d3b9..47b245eea2e 100644 --- a/compiler/fm/src/lib.rs +++ b/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 { - 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 { 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 ./ diff --git a/compiler/noirc_frontend/Cargo.toml b/compiler/noirc_frontend/Cargo.toml index 1565a105522..aa3a8e9f6b8 100644 --- a/compiler/noirc_frontend/Cargo.toml +++ b/compiler/noirc_frontend/Cargo.toml @@ -27,3 +27,4 @@ log.workspace = true [dev-dependencies] strum = "0.24" strum_macros = "0.24" +tempfile.workspace = true diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index aa1c658bade..04791b11b2a 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/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::{ @@ -524,7 +524,7 @@ impl<'a> ModCollector<'a> { ) -> 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 @@ impl<'a> ModCollector<'a> { Ok(LocalModuleId(module_id)) } } + +fn find_module( + file_manager: &FileManager, + anchor: FileId, + mod_name: &str, +) -> Result { + 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(); + } +} diff --git a/tooling/backend_interface/Cargo.toml b/tooling/backend_interface/Cargo.toml index a9217af65d2..32c5d28e3b0 100644 --- a/tooling/backend_interface/Cargo.toml +++ b/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" diff --git a/tooling/nargo/Cargo.toml b/tooling/nargo/Cargo.toml index ec4e1488809..f0733d7ad44 100644 --- a/tooling/nargo/Cargo.toml +++ b/tooling/nargo/Cargo.toml @@ -30,4 +30,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 \ No newline at end of file diff --git a/tooling/nargo_cli/Cargo.toml b/tooling/nargo_cli/Cargo.toml index 008b1233cd8..2f99fefb778 100644 --- a/tooling/nargo_cli/Cargo.toml +++ b/tooling/nargo_cli/Cargo.toml @@ -62,7 +62,7 @@ tracing-appender = "0.2.3" 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"