chore: remove path virtualization from FileManager (#3169)

compiler/fm/src/lib.rs

@@ -17,14 +17,11 @@ use std::{
 
 pub const FILE_EXTENSION: &str = "nr";
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
-struct VirtualPath(PathBuf);
-
 pub struct FileManager {
     root: PathBuf,
     file_map: file_map::FileMap,
-    id_to_path: HashMap<FileId, VirtualPath>,
-    path_to_id: HashMap<VirtualPath, FileId>,
+    id_to_path: HashMap<FileId, PathBuf>,
+    path_to_id: HashMap<PathBuf, FileId>,
     file_reader: Box<FileReader>,
 }
 
@@ -65,19 +62,18 @@ impl FileManager {
         };
 
         // Check that the resolved path already exists in the file map, if it is, we return it.
-        let path_to_file = virtualize_path(&resolved_path);
-        if let Some(file_id) = self.path_to_id.get(&path_to_file) {
+        if let Some(file_id) = self.path_to_id.get(&resolved_path) {
             return Some(*file_id);
         }
 
         // Otherwise we add the file
         let source = file_reader::read_file_to_string(&resolved_path, &self.file_reader).ok()?;
-        let file_id = self.file_map.add_file(resolved_path.into(), source);
-        self.register_path(file_id, path_to_file);
+        let file_id = self.file_map.add_file(resolved_path.clone().into(), source);
+        self.register_path(file_id, resolved_path);
         Some(file_id)
     }
 
-    fn register_path(&mut self, file_id: FileId, path: VirtualPath) {
+    fn register_path(&mut self, file_id: FileId, path: PathBuf) {
         let old_value = self.id_to_path.insert(file_id, path.clone());
         assert!(
             old_value.is_none(),
@@ -95,11 +91,11 @@ impl FileManager {
     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()
+        self.id_to_path.get(&file_id).unwrap().as_path()
     }
 
     pub fn find_module(&mut self, anchor: FileId, mod_name: &str) -> Result<FileId, String> {
-        let anchor_path = self.path(anchor).to_path_buf();
+        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
@@ -118,14 +114,14 @@ impl FileManager {
 /// 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() {
+    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_name()
+            || 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
@@ -211,16 +207,6 @@ mod path_normalization {
     }
 }
 
-/// Takes a path to a noir file. This will panic on paths to directories
-/// Returns the file path with the extension removed
-fn virtualize_path(path: &Path) -> VirtualPath {
-    let path = path.to_path_buf();
-    let base = path.parent().unwrap();
-    let path_no_ext: PathBuf =
-        path.file_stem().expect("ice: this should have been the path to a file").into();
-    let path = base.join(path_no_ext);
-    VirtualPath(path)
-}
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -257,7 +243,7 @@ mod tests {
 
         let file_id = fm.add_file(file_name).unwrap();
 
-        assert!(fm.path(file_id).ends_with("foo"));
+        assert!(fm.path(file_id).ends_with("foo.nr"));
     }
 
     #[test]

tooling/debugger/src/lib.rs

@@ -84,7 +84,7 @@ impl<'backend, B: BlackBoxFunctionSolver> DebugContext<'backend, B> {
                 let source = &file.source.as_str();
                 let start = loc.span.start() as usize;
                 let end = loc.span.end() as usize;
-                println!("At {}.nr:{start}-{end}", file.path.as_path().display());
+                println!("At {}:{start}-{end}", file.path.as_path().display());
                 println!("\n{}\n", &source[start..end]);
             }
         }

tooling/lsp/Cargo.toml

@@ -12,7 +12,6 @@ license.workspace = true
 acvm.workspace = true
 codespan-lsp.workspace = true
 codespan-reporting.workspace = true
-fm.workspace = true
 lsp-types.workspace = true
 nargo.workspace = true
 nargo_toml.workspace = true

tooling/lsp/src/lib.rs

@@ -17,7 +17,6 @@ use async_lsp::{
     ResponseError,
 };
 use codespan_reporting::files;
-use fm::FILE_EXTENSION;
 use noirc_frontend::{
     graph::{CrateId, CrateName},
     hir::{Context, FunctionNameMatch},
@@ -122,15 +121,15 @@ fn get_package_tests_in_crate(
             let location = context.function_meta(&test_function.get_id()).name.location;
             let file_id = location.file;
 
-            let file_path = fm.path(file_id).with_extension(FILE_EXTENSION);
             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))
+                .expect("Expected a valid file path that can be converted into a URI");
 
             NargoTest {
                 id: NargoTestId::new(crate_name.clone(), func_name.clone()),
                 label: func_name,
-                uri: Url::from_file_path(file_path)
-                    .expect("Expected a valid file path that can be converted into a URI"),
+                uri: file_uri,
                 range,
             }
         })

tooling/lsp/src/notifications/mod.rs

@@ -1,7 +1,6 @@
 use std::ops::ControlFlow;
 
 use async_lsp::{ErrorCode, LanguageClient, ResponseError};
-use fm::FILE_EXTENSION;
 use nargo::prepare_package;
 use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
 use noirc_driver::check_crate;
@@ -127,9 +126,7 @@ pub(super) fn on_did_save_text_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
-                    // TODO: This currently just appends the `.nr` file extension that we store as a constant,
-                    // but that won't work if we accept other extensions
-                    if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
+                    if fm.path(file_id) != file_path {
                         return None;
                     }
 

tooling/lsp/src/requests/code_lens_request.rs

@@ -2,7 +2,6 @@ use std::future::{self, Future};
 
 use async_lsp::{ErrorCode, LanguageClient, ResponseError};
 
-use fm::FILE_EXTENSION;
 use nargo::{package::Package, prepare_package, workspace::Workspace};
 use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
 use noirc_driver::check_crate;
@@ -93,9 +92,7 @@ fn on_code_lens_request_inner(
 
             // Ignore diagnostics for any file that wasn't the file we saved
             // TODO: In the future, we could create "related" diagnostics for these files
-            // TODO: This currently just appends the `.nr` file extension that we store as a constant,
-            // but that won't work if we accept other extensions
-            if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
+            if fm.path(file_id) != file_path {
                 continue;
             }
 
@@ -126,9 +123,7 @@ fn on_code_lens_request_inner(
 
                 // Ignore diagnostics for any file that wasn't the file we saved
                 // TODO: In the future, we could create "related" diagnostics for these files
-                // TODO: This currently just appends the `.nr` file extension that we store as a constant,
-                // but that won't work if we accept other extensions
-                if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
+                if fm.path(file_id) != file_path {
                     continue;
                 }
 
@@ -175,9 +170,7 @@ fn on_code_lens_request_inner(
 
                 // Ignore diagnostics for any file that wasn't the file we saved
                 // TODO: In the future, we could create "related" diagnostics for these files
-                // TODO: This currently just appends the `.nr` file extension that we store as a constant,
-                // but that won't work if we accept other extensions
-                if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
+                if fm.path(file_id) != file_path {
                     continue;
                 }