chore: make panics for missing files explicit (#4051)

# Description ## Problem\* Step towards #3800 ## Summary\* This PR acts as a first step towards adding proper errors here. I've added concrete `expect`s in locations where I don't see the risk for a panic. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
noir-lang · Jan 16, 2024 · ca0766a · ca0766a
1 parent 27a8e68
commit ca0766a
Show file tree

Hide file tree

Showing 10 changed files with 33 additions and 25 deletions.
diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs
@@ -88,15 +88,15 @@ impl FileManager {
         assert!(old_value.is_none(), "ice: the same path was inserted into the file manager twice");
     }
 
-    pub fn fetch_file(&self, file_id: FileId) -> &str {
+    pub fn fetch_file(&self, file_id: FileId) -> Option<&str> {
         // 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().source()
+        self.file_map.get_file(file_id).map(|file| file.source())
     }
 
-    pub fn path(&self, file_id: FileId) -> &Path {
+    pub fn path(&self, file_id: FileId) -> Option<&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().as_path()
+        self.id_to_path.get(&file_id).map(|path| path.as_path())
     }
 
     // TODO: This should accept a &Path instead of a PathBuf
@@ -204,7 +204,7 @@ mod tests {
 
         let file_id = fm.add_file_with_source(file_name, "fn foo() {}".to_string()).unwrap();
 
-        assert!(fm.path(file_id).ends_with("foo.nr"));
+        assert!(fm.path(file_id).unwrap().ends_with("foo.nr"));
     }
 
     /// Tests that two identical files that have different paths are treated as the same file

diff --git a/compiler/noirc_driver/src/debug.rs b/compiler/noirc_driver/src/debug.rs
@@ -31,14 +31,12 @@ pub(crate) fn filter_relevant_files(
     let mut file_map = BTreeMap::new();
 
     for file_id in files_with_debug_symbols {
-        let file_source = file_manager.fetch_file(file_id);
+        let file_path = file_manager.path(file_id).expect("file should exist");
+        let file_source = file_manager.fetch_file(file_id).expect("file should exist");
 
         file_map.insert(
             file_id,
-            DebugFile {
-                source: file_source.to_string(),
-                path: file_manager.path(file_id).to_path_buf(),
-            },
+            DebugFile { source: file_source.to_string(), path: file_path.to_path_buf() },
         );
     }
     file_map

diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
@@ -634,7 +634,10 @@ fn find_module(
     anchor: FileId,
     mod_name: &str,
 ) -> Result<FileId, String> {
-    let anchor_path = file_manager.path(anchor).with_extension("");
+    let anchor_path = file_manager
+        .path(anchor)
+        .expect("File must exist in file manager in order for us to be resolving its imports.")
+        .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

diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs
@@ -295,7 +295,7 @@ pub struct Contract {
 
 /// Given a FileId, fetch the File, from the FileManager and parse it's content
 pub fn parse_file(fm: &FileManager, file_id: FileId) -> (ParsedModule, Vec<ParserError>) {
-    let file_source = fm.fetch_file(file_id);
+    let file_source = fm.fetch_file(file_id).expect("File does not exist");
     parse_program(file_source)
 }
 

diff --git a/compiler/wasm/src/errors.rs b/compiler/wasm/src/errors.rs
@@ -124,7 +124,10 @@ impl CompileError {
         let diagnostics: Vec<_> = file_diagnostics
             .iter()
             .map(|err| {
-                Diagnostic::new(err, file_manager.path(err.file_id).to_str().unwrap().to_string())
+                let file_path = file_manager
+                    .path(err.file_id)
+                    .expect("File must exist to have caused diagnostics");
+                Diagnostic::new(err, file_path.to_str().unwrap().to_string())
             })
             .collect();
 

diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs
@@ -152,10 +152,10 @@ fn get_package_tests_in_crate(
         .map(|(func_name, test_function)| {
             let location = context.function_meta(&test_function.get_id()).name.location;
             let file_id = location.file;
-
+            let file_path = fm.path(file_id).expect("file must exist to contain tests");
             let range =
                 byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default();
-            let file_uri = Url::from_file_path(fm.path(file_id))
+            let file_uri = Url::from_file_path(file_path)
                 .expect("Expected a valid file path that can be converted into a URI");
 
             NargoTest {

diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs
@@ -170,7 +170,9 @@ fn process_noir_document(
                 .filter_map(|FileDiagnostic { file_id, diagnostic, call_stack: _ }| {
                     // Ignore diagnostics for any file that wasn't the file we saved
                     // TODO: In the future, we could create "related" diagnostics for these files
-                    if fm.path(file_id) != file_path {
+                    if fm.path(file_id).expect("file must exist to have emitted diagnostic")
+                        != file_path
+                    {
                         return None;
                     }
 

diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs
@@ -102,7 +102,7 @@ pub(crate) fn collect_lenses_for_package(
         // Ignore diagnostics for any file that wasn't the file we saved
         // TODO: In the future, we could create "related" diagnostics for these files
         if let Some(file_path) = file_path {
-            if fm.path(file_id) != *file_path {
+            if fm.path(file_id).expect("file must exist to contain tests") != *file_path {
                 continue;
             }
         }
@@ -125,6 +125,7 @@ pub(crate) fn collect_lenses_for_package(
 
         lenses.push(test_lens);
     }
+
     if package.is_binary() {
         if let Some(main_func_id) = context.get_main_function(&crate_id) {
             let location = context.function_meta(&main_func_id).name.location;
@@ -133,7 +134,9 @@ pub(crate) fn collect_lenses_for_package(
             // Ignore diagnostics for any file that wasn't the file we saved
             // TODO: In the future, we could create "related" diagnostics for these files
             if let Some(file_path) = file_path {
-                if fm.path(file_id) != *file_path {
+                if fm.path(file_id).expect("file must exist to contain `main` function")
+                    != *file_path
+                {
                     return lenses;
                 }
             }
@@ -192,7 +195,7 @@ pub(crate) fn collect_lenses_for_package(
             // Ignore diagnostics for any file that wasn't the file we saved
             // TODO: In the future, we could create "related" diagnostics for these files
             if let Some(file_path) = file_path {
-                if fm.path(file_id) != *file_path {
+                if fm.path(file_id).expect("file must exist to contain a contract") != *file_path {
                     continue;
                 }
             }

diff --git a/tooling/nargo/src/artifacts/debug.rs b/tooling/nargo/src/artifacts/debug.rs
@@ -34,14 +34,12 @@ impl DebugArtifact {
             .collect();
 
         for file_id in files_with_debug_symbols {
-            let file_source = file_manager.fetch_file(file_id);
+            let file_path = file_manager.path(file_id).expect("file should exist");
+            let file_source = file_manager.fetch_file(file_id).expect("file should exist");
 
             file_map.insert(
                 file_id,
-                DebugFile {
-                    source: file_source.to_string(),
-                    path: file_manager.path(file_id).to_path_buf(),
-                },
+                DebugFile { source: file_source.to_string(), path: file_path.to_path_buf() },
             );
         }
 

diff --git a/tooling/nargo_cli/src/cli/fmt_cmd.rs b/tooling/nargo_cli/src/cli/fmt_cmd.rs
@@ -40,6 +40,7 @@ pub(crate) fn run(args: FormatCommand, config: NargoConfig) -> Result<(), CliErr
     for package in &workspace {
         visit_noir_files(&package.root_dir.join("src"), &mut |entry| {
             let file_id = workspace_file_manager.name_to_id(entry.path().to_path_buf()).expect("The file should exist since we added all files in the package into the file manager");
+
             let (parsed_module, errors) = parse_file(&workspace_file_manager, file_id);
 
             let is_all_warnings = errors.iter().all(ParserError::is_warning);
@@ -61,7 +62,7 @@ pub(crate) fn run(args: FormatCommand, config: NargoConfig) -> Result<(), CliErr
                 return Ok(());
             }
 
-            let original = workspace_file_manager.fetch_file(file_id);
+            let original = workspace_file_manager.fetch_file(file_id).expect("The file should exist since we added all files in the package into the file manager");
             let formatted = nargo_fmt::format(original, parsed_module, &config);
 
             if check_mode {