From 7f76910ebbd20e3d7a1db7541f2b7f43cd9b546d Mon Sep 17 00:00:00 2001 From: jfecher Date: Fri, 22 Sep 2023 18:28:47 -0500 Subject: [PATCH] fix!: Issue an error when a module is declared twice & fix module search path (#2801) --- compiler/fm/src/lib.rs | 46 +++++++++++++------ .../src/hir/def_collector/dc_mod.rs | 24 ++++++++-- .../src/hir/def_collector/errors.rs | 12 ++--- compiler/noirc_frontend/src/hir/mod.rs | 6 +++ compiler/noirc_frontend/src/hir_def/types.rs | 4 +- 5 files changed, 67 insertions(+), 25 deletions(-) diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs index f615555601c..1703db8d048 100644 --- a/compiler/fm/src/lib.rs +++ b/compiler/fm/src/lib.rs @@ -78,6 +78,7 @@ 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 @@ -85,23 +86,38 @@ impl FileManager { } pub fn find_module(&mut self, anchor: FileId, mod_name: &str) -> Result { - 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(); 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 86e4eb50de3..98dfa74a655 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::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); diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index afd0599ef4e..1e3b36844f9 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/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,7 +76,7 @@ impl From 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 ); { @@ -84,19 +84,19 @@ impl From for Diagnostic { 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, ) diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 63f057a63d3..9c747bfead1 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/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, 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, + /// Maps a given (contract) module id to the next available storage slot /// for that contract. pub storage_slots: BTreeMap, @@ -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(), diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 29b8591d88d..e6c9d7bee9a 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/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)?; }