fix(fm): Create FileManager with a root and normalize filenames again…

…st it

crates/fm/src/lib.rs

@@ -10,7 +10,7 @@ pub use file_map::{File, FileId, FileMap};
 
 use std::{
     collections::HashMap,
-    path::{Path, PathBuf},
+    path::{Component, Path, PathBuf},
 };
 
 pub const FILE_EXTENSION: &str = "nr";
@@ -28,20 +28,34 @@ pub enum FileType {
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 struct VirtualPath(PathBuf);
 
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct FileManager {
+    root: PathBuf,
     file_map: file_map::FileMap,
     id_to_path: HashMap<FileId, VirtualPath>,
     path_to_id: HashMap<VirtualPath, FileId>,
 }
 
 impl FileManager {
-    // XXX: Maybe use a AsRef<Path> here, for API ergonomics
-    pub fn add_file(&mut self, path_to_file: &Path, file_type: FileType) -> Option<FileId> {
+    pub fn new(root: &Path) -> Self {
+        Self {
+            root: normalize_path(root),
+            file_map: Default::default(),
+            id_to_path: Default::default(),
+            path_to_id: Default::default(),
+        }
+    }
+
+    pub fn add_file(&mut self, file_name: &Path, file_type: FileType) -> Option<FileId> {
         // Handle both relative file paths and std/lib virtual paths.
-        let base = Path::new(".").canonicalize().expect("Base path canonicalize failed");
-        let res = path_to_file.canonicalize().unwrap_or_else(|_| path_to_file.to_path_buf());
-        let resolved_path = res.strip_prefix(base).unwrap_or(&res);
+        let absolute_path = normalize_path(&self.root.join(file_name));
+        let resolved_path = if file_name.starts_with("std/") || file_name.starts_with("std\\") {
+            // Special case for stdlib where we want to read specifically the `std/` relative path
+            // TODO: The stdlib path should probably be an absolute path rooted in something people would never create
+            file_name
+        } else {
+            &absolute_path
+        };
 
         // 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, file_type);
@@ -93,6 +107,36 @@ impl FileManager {
     }
 }
 
+// Replacement for std::fs::canonicalize that doesn't verify the path exists
+// Plucked from https://github.com/rust-lang/cargo/blob/fede83ccf973457de319ba6fa0e36ead454d2e20/src/cargo/util/paths.rs#L61
+// Advice from https://www.reddit.com/r/rust/comments/hkkquy/comment/fwtw53s/?utm_source=share&utm_medium=web2x&context=3
+fn normalize_path(path: &Path) -> PathBuf {
+    let mut components = path.components().peekable();
+    let mut ret = if let Some(c @ Component::Prefix(..)) = components.peek().cloned() {
+        components.next();
+        PathBuf::from(c.as_os_str())
+    } else {
+        PathBuf::new()
+    };
+
+    for component in components {
+        match component {
+            Component::Prefix(..) => unreachable!(),
+            Component::RootDir => {
+                ret.push(component.as_os_str());
+            }
+            Component::CurDir => {}
+            Component::ParentDir => {
+                ret.pop();
+            }
+            Component::Normal(c) => {
+                ret.push(c);
+            }
+        }
+    }
+    ret
+}
+
 /// Takes a path to a noir file. This will panic on paths to directories
 /// Returns
 /// For Normal filetypes, given "src/mod.nr" this method returns "src/mod"
@@ -118,46 +162,49 @@ mod tests {
     use super::*;
     use tempfile::{tempdir, TempDir};
 
-    fn dummy_file_path(dir: &TempDir, file_name: &str) -> PathBuf {
+    fn create_dummy_file(dir: &TempDir, file_name: &Path) {
         let file_path = dir.path().join(file_name);
         let _file = std::fs::File::create(file_path.clone()).unwrap();
-
-        file_path
     }
 
     #[test]
     fn path_resolve_file_module() {
         let dir = tempdir().unwrap();
-        let file_path = dummy_file_path(&dir, "my_dummy_file.nr");
 
-        let mut fm = FileManager::default();
+        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(&file_path, FileType::Root).unwrap();
+        let file_id = fm.add_file(entry_file_name, FileType::Root).unwrap();
 
-        let _foo_file_path = dummy_file_path(&dir, "foo.nr");
+        let dep_file_name = Path::new("foo.nr");
+        create_dummy_file(&dir, dep_file_name);
         fm.resolve_path(file_id, "foo").unwrap();
     }
     #[test]
     fn path_resolve_file_module_other_ext() {
         let dir = tempdir().unwrap();
-        let file_path = dummy_file_path(&dir, "foo.noir");
+        let file_name = Path::new("foo.noir");
+        create_dummy_file(&dir, file_name);
 
-        let mut fm = FileManager::default();
+        let mut fm = FileManager::new(dir.path());
 
-        let file_id = fm.add_file(&file_path, FileType::Normal).unwrap();
+        let file_id = fm.add_file(&file_name, FileType::Normal).unwrap();
 
         assert!(fm.path(file_id).ends_with("foo"));
     }
     #[test]
     fn path_resolve_sub_module() {
-        let mut fm = FileManager::default();
-
         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 file_path = dummy_file_path(&dir, "lib.nr");
+        let file_name = Path::new("lib.nr");
+        create_dummy_file(&dir, file_name);
 
-        let file_id = fm.add_file(&file_path, FileType::Root).unwrap();
+        let file_id = fm.add_file(&file_name, FileType::Root).unwrap();
 
         // Create a sub directory
         // we now have:
@@ -171,14 +218,14 @@ mod tests {
         // we no have:
         // - dir/lib.nr
         // - dir/sub_dir/foo.nr
-        let _foo_file_path = dummy_file_path(&sub_dir, "foo.nr");
+        create_dummy_file(&sub_dir, Path::new("foo.nr"));
 
         // 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_root_file_path = dummy_file_path(&dir, &format!("{}.nr", sub_dir_name));
+        create_dummy_file(&dir, Path::new(&format!("{}.nr", sub_dir_name)));
 
         // First check for the sub_dir.nr file and add it to the FileManager
         let sub_dir_file_id = fm.resolve_path(file_id, sub_dir_name).unwrap();
@@ -193,20 +240,22 @@ mod tests {
     /// they should both resolve to ../foo.nr
     #[test]
     fn path_resolve_modules_with_different_paths_as_same_file() {
-        let mut fm = FileManager::default();
-
-        // Create a lib.nr file at the root.
         let dir = tempdir().unwrap();
         let sub_dir = TempDir::new_in(&dir).unwrap();
         let sub_sub_dir = TempDir::new_in(&sub_dir).unwrap();
-        let file_path = dummy_file_path(&dir, "lib.nr");
 
-        // Create another file in a subdirectory with a convoluted path
-        let second_file_path = dummy_file_path(&sub_sub_dir, "./../../lib.nr");
+        let mut fm = FileManager::new(dir.path());
+
+        // Create a lib.nr file at the root.
+        let file_name = Path::new("lib.nr");
+        create_dummy_file(&dir, file_name);
+
+        // Create another path with `./` and `../` inside it
+        let second_file_name = PathBuf::from(sub_sub_dir.path()).join("./../../lib.nr");
 
         // Add both files to the file manager
-        let file_id = fm.add_file(&file_path, FileType::Root).unwrap();
-        let second_file_id = fm.add_file(&second_file_path, FileType::Root).unwrap();
+        let file_id = fm.add_file(&file_name, FileType::Root).unwrap();
+        let second_file_id = fm.add_file(&second_file_name, FileType::Root).unwrap();
 
         assert_eq!(file_id, second_file_id);
     }

crates/lsp/Cargo.toml

@@ -11,6 +11,7 @@ edition.workspace = true
 acvm.workspace = true
 codespan-lsp.workspace = true
 codespan-reporting.workspace = true
+fm.workspace = true
 lsp-types.workspace = true
 noirc_driver.workspace = true
 noirc_errors.workspace = true

crates/lsp/src/lib.rs

@@ -1,15 +1,16 @@
 use std::{
-    future::Future,
+    future::{self, Future},
     ops::{self, ControlFlow},
     pin::Pin,
     task::{self, Poll},
 };
 
 use async_lsp::{
-    router::Router, AnyEvent, AnyNotification, AnyRequest, ClientSocket, Error, LanguageClient,
-    LspService, ResponseError,
+    router::Router, AnyEvent, AnyNotification, AnyRequest, ClientSocket, Error, ErrorCode,
+    LanguageClient, LspService, ResponseError,
 };
 use codespan_reporting::files;
+use fm::FileManager;
 use lsp_types::{
     notification, request, CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic,
     DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams,
@@ -19,7 +20,10 @@ use lsp_types::{
 };
 use noirc_driver::{check_crate, create_local_crate};
 use noirc_errors::{DiagnosticKind, FileDiagnostic};
-use noirc_frontend::{graph::CrateType, hir::Context};
+use noirc_frontend::{
+    graph::{CrateGraph, CrateType},
+    hir::Context,
+};
 use serde_json::Value as JsonValue;
 use tower::Service;
 
@@ -28,14 +32,13 @@ const TEST_CODELENS_TITLE: &str = "▶\u{fe0e} Run Test";
 
 // State for the LSP gets implemented on this struct and is internal to the implementation
 struct LspState {
-    context: Context,
+    context: Option<Context>,
     client: ClientSocket,
 }
 
 impl LspState {
     fn new(client: &ClientSocket) -> Self {
-        // TODO: Do we want to build the Context here or when we get the initialize message?
-        Self { client: client.clone(), context: Context::default() }
+        Self { context: None, client: client.clone() }
     }
 }
 
@@ -101,9 +104,17 @@ impl LspService for NargoLspService {
 // and params passed in.
 
 fn on_initialize(
-    _state: &mut LspState,
-    _params: InitializeParams,
+    state: &mut LspState,
+    params: InitializeParams,
 ) -> impl Future<Output = Result<InitializeResult, ResponseError>> {
+    if let Some(root_uri) = params.root_uri {
+        let fm = FileManager::new(&root_uri.to_file_path().unwrap());
+        let graph = CrateGraph::default();
+        let context = Context::new(fm, graph);
+
+        state.context = Some(context);
+    }
+
     async {
         let text_document_sync =
             TextDocumentSyncOptions { save: Some(true.into()), ..Default::default() };
@@ -135,26 +146,34 @@ fn on_code_lens_request(
 ) -> impl Future<Output = Result<Option<Vec<CodeLens>>, ResponseError>> {
     let file_path = &params.text_document.uri.to_file_path().unwrap();
 
-    let crate_id = create_local_crate(&mut state.context, file_path, CrateType::Binary);
+    let context = match state.context.as_mut() {
+        Some(context) => context,
+        None => {
+            let err = ResponseError::new(ErrorCode::REQUEST_FAILED, "Project has not been built");
+            return future::ready(Err(err));
+        }
+    };
+
+    let crate_id = create_local_crate(context, file_path, CrateType::Binary);
 
     // We ignore the warnings and errors produced by compilation for producing codelenses
     // because we can still get the test functions even if compilation fails
-    let _ = check_crate(&mut state.context, false, false);
+    let _ = check_crate(context, false, false);
 
-    let fm = &state.context.file_manager;
+    let fm = &context.file_manager;
     let files = fm.as_simple_files();
-    let tests = state.context.get_all_test_functions_in_crate_matching(&crate_id, "");
+    let tests = context.get_all_test_functions_in_crate_matching(&crate_id, "");
 
     let mut lenses: Vec<CodeLens> = vec![];
     for func_id in tests {
-        let location = state.context.function_meta(&func_id).name.location;
+        let location = context.function_meta(&func_id).name.location;
         let file_id = location.file;
         // TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file
         if file_id.as_usize() != 0 {
             continue;
         }
 
-        let func_name = state.context.function_name(&func_id);
+        let func_name = context.function_name(&func_id);
 
         let range =
             byte_span_to_range(files, file_id.as_usize(), location.span.into()).unwrap_or_default();
@@ -170,13 +189,9 @@ fn on_code_lens_request(
         lenses.push(lens);
     }
 
-    async move {
-        if lenses.is_empty() {
-            Ok(None)
-        } else {
-            Ok(Some(lenses))
-        }
-    }
+    let res = if lenses.is_empty() { Ok(None) } else { Ok(Some(lenses)) };
+
+    future::ready(res)
 }
 
 fn on_initialized(
@@ -219,18 +234,25 @@ fn on_did_save_text_document(
     params: DidSaveTextDocumentParams,
 ) -> ControlFlow<Result<(), async_lsp::Error>> {
     let file_path = &params.text_document.uri.to_file_path().unwrap();
+    let context = match state.context.as_mut() {
+        Some(context) => context,
+        None => {
+            let err = ResponseError::new(ErrorCode::REQUEST_FAILED, "Project has not been built");
+            return ControlFlow::Break(Err(err.into()));
+        }
+    };
 
-    create_local_crate(&mut state.context, file_path, CrateType::Binary);
+    create_local_crate(context, file_path, CrateType::Binary);
 
     let mut diagnostics = Vec::new();
 
-    let file_diagnostics = match check_crate(&mut state.context, false, false) {
+    let file_diagnostics = match check_crate(context, false, false) {
         Ok(warnings) => warnings,
         Err(errors_and_warnings) => errors_and_warnings,
     };
 
     if !file_diagnostics.is_empty() {
-        let fm = &state.context.file_manager;
+        let fm = &context.file_manager;
         let files = fm.as_simple_files();
 
         for FileDiagnostic { file_id, diagnostic } in file_diagnostics {

crates/nargo_cli/Cargo.toml

@@ -30,6 +30,7 @@ noirc_frontend.workspace = true
 noirc_abi.workspace = true
 noirc_errors.workspace = true
 acvm.workspace = true
+fm.workspace = true
 toml.workspace = true
 serde.workspace = true
 serde_json.workspace = true