internal: a bit more of cwd safety for flycheck

matklad · matklad · commit 8d8c26e6f519 · 2021-07-17T18:13:35.000+03:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/flycheck/Cargo.toml b/crates/flycheck/Cargo.toml
@@ -18,3 +18,4 @@ jod-thread = "0.1.1"
 
 toolchain = { path = "../toolchain", version = "0.0.0" }
 stdx = { path = "../stdx", version = "0.0.0" }
+paths = { path = "../paths", version = "0.0.0" }
diff --git a/crates/flycheck/src/lib.rs b/crates/flycheck/src/lib.rs
@@ -5,12 +5,12 @@
 use std::{
     fmt,
     io::{self, BufRead, BufReader},
-    path::PathBuf,
     process::{self, Command, Stdio},
     time::Duration,
 };
 
 use crossbeam_channel::{never, select, unbounded, Receiver, Sender};
+use paths::AbsPathBuf;
 use serde::Deserialize;
 use stdx::JodChild;
 
@@ -63,7 +63,7 @@ impl FlycheckHandle {
         id: usize,
         sender: Box<dyn Fn(Message) + Send>,
         config: FlycheckConfig,
-        workspace_root: PathBuf,
+        workspace_root: AbsPathBuf,
     ) -> FlycheckHandle {
         let actor = FlycheckActor::new(id, sender, config, workspace_root);
         let (sender, receiver) = unbounded::<Restart>();
@@ -82,7 +82,7 @@ impl FlycheckHandle {
 
 pub enum Message {
     /// Request adding a diagnostic with fixes included to a file
-    AddDiagnostic { workspace_root: PathBuf, diagnostic: Diagnostic },
+    AddDiagnostic { workspace_root: AbsPathBuf, diagnostic: Diagnostic },
 
     /// Request check progress notification to client
     Progress {
@@ -121,7 +121,7 @@ struct FlycheckActor {
     id: usize,
     sender: Box<dyn Fn(Message) + Send>,
     config: FlycheckConfig,
-    workspace_root: PathBuf,
+    workspace_root: AbsPathBuf,
     /// WatchThread exists to wrap around the communication needed to be able to
     /// run `cargo check` without blocking. Currently the Rust standard library
     /// doesn't provide a way to read sub-process output without blocking, so we
@@ -140,7 +140,7 @@ impl FlycheckActor {
         id: usize,
         sender: Box<dyn Fn(Message) + Send>,
         config: FlycheckConfig,
-        workspace_root: PathBuf,
+        workspace_root: AbsPathBuf,
     ) -> FlycheckActor {
         FlycheckActor { id, sender, config, workspace_root, cargo_handle: None }
     }
@@ -220,7 +220,7 @@ impl FlycheckActor {
                 cmd.arg(command);
                 cmd.current_dir(&self.workspace_root);
                 cmd.args(&["--workspace", "--message-format=json", "--manifest-path"])
-                    .arg(self.workspace_root.join("Cargo.toml"));
+                    .arg(self.workspace_root.join("Cargo.toml").as_os_str());
 
                 if let Some(target) = target_triple {
                     cmd.args(&["--target", target.as_str()]);
diff --git a/crates/paths/src/lib.rs b/crates/paths/src/lib.rs
@@ -186,6 +186,9 @@ impl AbsPath {
     pub fn starts_with(&self, base: &AbsPath) -> bool {
         self.0.starts_with(&base.0)
     }
+    pub fn ends_with(&self, suffix: &RelPath) -> bool {
+        self.0.starts_with(&suffix.0)
+    }
 
     // region:delegate-methods
 
diff --git a/crates/proc_macro_api/src/rpc.rs b/crates/proc_macro_api/src/rpc.rs
@@ -271,7 +271,7 @@ mod tests {
             macro_body: tt.clone(),
             macro_name: Default::default(),
             attributes: None,
-            lib: Default::default(),
+            lib: AbsPathBuf::assert(std::env::current_dir().unwrap()),
             env: Default::default(),
         };
 
diff --git a/crates/proc_macro_srv/Cargo.toml b/crates/proc_macro_srv/Cargo.toml
@@ -15,6 +15,7 @@ memmap2 = "0.3.0"
 
 tt = { path = "../tt", version = "0.0.0" }
 mbe = { path = "../mbe", version = "0.0.0" }
+paths = { path = "../paths", version = "0.0.0" }
 proc_macro_api = { path = "../proc_macro_api", version = "0.0.0" }
 
 [dev-dependencies]
diff --git a/crates/proc_macro_srv/src/dylib.rs b/crates/proc_macro_srv/src/dylib.rs
@@ -1,6 +1,7 @@
 //! Handles dynamic library loading for proc macro
 
 use std::{
+    convert::TryInto,
     fmt,
     fs::File,
     io,
@@ -10,6 +11,7 @@ use std::{
 use libloading::Library;
 use memmap2::Mmap;
 use object::Object;
+use paths::AbsPath;
 use proc_macro_api::{read_dylib_info, ProcMacroKind};
 
 use super::abis::Abi;
@@ -116,7 +118,10 @@ impl ProcMacroLibraryLibloading {
             invalid_data_err(format!("Cannot find registrar symbol in file {}", file.display()))
         })?;
 
-        let version_info = read_dylib_info(file)?;
+        let abs_file: &AbsPath = file.try_into().map_err(|_| {
+            invalid_data_err(format!("expected an absolute path, got {}", file.display()))
+        })?;
+        let version_info = read_dylib_info(&abs_file)?;
 
         let lib = load_library(file).map_err(invalid_data_err)?;
         let abi = Abi::from_lib(&lib, symbol_name, version_info)?;
@@ -136,7 +141,7 @@ impl Expander {
 
         let lib = ensure_file_with_lock_free_access(&lib)?;
 
-        let library = ProcMacroLibraryLibloading::open(&lib)?;
+        let library = ProcMacroLibraryLibloading::open(lib.as_ref())?;
 
         Ok(Expander { inner: library })
     }
diff --git a/crates/proc_macro_srv/src/lib.rs b/crates/proc_macro_srv/src/lib.rs
@@ -30,7 +30,7 @@ pub(crate) struct ProcMacroSrv {
 
 impl ProcMacroSrv {
     pub fn expand(&mut self, task: &ExpansionTask) -> Result<ExpansionResult, String> {
-        let expander = self.expander(&task.lib)?;
+        let expander = self.expander(task.lib.as_ref())?;
 
         let mut prev_env = HashMap::new();
         for (k, v) in &task.env {
@@ -54,7 +54,7 @@ impl ProcMacroSrv {
     }
 
     pub fn list_macros(&mut self, task: &ListMacrosTask) -> Result<ListMacrosResult, String> {
-        let expander = self.expander(&task.lib)?;
+        let expander = self.expander(task.lib.as_ref())?;
         Ok(ListMacrosResult { macros: expander.list_macros() })
     }
 
diff --git a/crates/proc_macro_srv/src/tests/mod.rs b/crates/proc_macro_srv/src/tests/mod.rs
@@ -3,6 +3,7 @@
 #[macro_use]
 mod utils;
 use expect_test::expect;
+use paths::AbsPathBuf;
 use utils::*;
 
 #[test]
@@ -95,7 +96,7 @@ fn list_test_macros() {
 
 #[test]
 fn test_version_check() {
-    let path = fixtures::proc_macro_test_dylib_path();
+    let path = AbsPathBuf::assert(fixtures::proc_macro_test_dylib_path());
     let info = proc_macro_api::read_dylib_info(&path).unwrap();
     assert!(info.version.1 >= 50);
 }
diff --git a/crates/proc_macro_srv/src/tests/utils.rs b/crates/proc_macro_srv/src/tests/utils.rs
@@ -3,6 +3,7 @@
 use crate::dylib;
 use crate::ProcMacroSrv;
 use expect_test::Expect;
+use paths::AbsPathBuf;
 use proc_macro_api::ListMacrosTask;
 use std::str::FromStr;
 
@@ -41,7 +42,7 @@ fn assert_expand_impl(macro_name: &str, input: &str, attr: Option<&str>, expect:
 }
 
 pub fn list() -> Vec<String> {
-    let path = fixtures::proc_macro_test_dylib_path();
+    let path = AbsPathBuf::assert(fixtures::proc_macro_test_dylib_path());
     let task = ListMacrosTask { lib: path };
     let mut srv = ProcMacroSrv::default();
     let res = srv.list_macros(&task).unwrap();
diff --git a/crates/rust-analyzer/src/cli/load_cargo.rs b/crates/rust-analyzer/src/cli/load_cargo.rs
@@ -28,7 +28,9 @@ pub(crate) fn load_workspace_at(
     progress: &dyn Fn(String),
 ) -> Result<(AnalysisHost, vfs::Vfs, Option<ProcMacroClient>)> {
     let root = AbsPathBuf::assert(std::env::current_dir()?.join(root));
+    eprintln!("root = {:?}", root);
     let root = ProjectManifest::discover_single(&root)?;
+    eprintln!("root = {:?}", root);
     let workspace = ProjectWorkspace::load(root, cargo_config, progress)?;
 
     load_workspace(workspace, load_config, progress)
@@ -48,7 +50,7 @@ fn load_workspace(
     };
 
     let proc_macro_client = if config.with_proc_macro {
-        let path = std::env::current_exe()?;
+        let path = AbsPathBuf::assert(std::env::current_exe()?);
         Some(ProcMacroClient::extern_process(path, &["proc-macro"]).unwrap())
     } else {
         None
@@ -142,7 +144,7 @@ mod tests {
     use hir::Crate;
 
     #[test]
-    fn test_loading_rust_analyzer() -> Result<()> {
+    fn test_loading_rust_analyzer() {
         let path = Path::new(env!("CARGO_MANIFEST_DIR")).parent().unwrap().parent().unwrap();
         let cargo_config = Default::default();
         let load_cargo_config = LoadCargoConfig {
@@ -152,12 +154,10 @@ mod tests {
             prefill_caches: false,
         };
         let (host, _vfs, _proc_macro) =
-            load_workspace_at(path, &cargo_config, &load_cargo_config, &|_| {})?;
+            load_workspace_at(path, &cargo_config, &load_cargo_config, &|_| {}).unwrap();
 
         let n_crates = Crate::all(host.raw_database()).len();
         // RA has quite a few crates, but the exact count doesn't matter
         assert!(n_crates > 20);
-
-        Ok(())
     }
 }
diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs
@@ -597,12 +597,14 @@ impl Config {
     pub fn lru_capacity(&self) -> Option<usize> {
         self.data.lruCapacity
     }
-    pub fn proc_macro_srv(&self) -> Option<(PathBuf, Vec<OsString>)> {
+    pub fn proc_macro_srv(&self) -> Option<(AbsPathBuf, Vec<OsString>)> {
         if !self.data.procMacro_enable {
             return None;
         }
-
-        let path = self.data.procMacro_server.clone().or_else(|| std::env::current_exe().ok())?;
+        let path = match &self.data.procMacro_server {
+            Some(it) => self.root_path.join(it),
+            None => AbsPathBuf::assert(std::env::current_exe().ok()?),
+        };
         Some((path, vec!["proc-macro".into()]))
     }
     pub fn expand_proc_attr_macros(&self) -> bool {
diff --git a/crates/rust-analyzer/src/diagnostics/to_proto.rs b/crates/rust-analyzer/src/diagnostics/to_proto.rs
@@ -1,12 +1,10 @@
 //! This module provides the functionality needed to convert diagnostics from
 //! `cargo check` json format to the LSP diagnostic format.
-use std::{
-    collections::HashMap,
-    path::{Path, PathBuf},
-};
+use std::collections::HashMap;
 
 use flycheck::{DiagnosticLevel, DiagnosticSpan};
 use stdx::format_to;
+use vfs::{AbsPath, AbsPathBuf};
 
 use crate::{lsp_ext, to_proto::url_from_abs_path};
 
@@ -46,7 +44,7 @@ fn is_dummy_macro_file(file_name: &str) -> bool {
 /// Converts a Rust span to a LSP location
 fn location(
     config: &DiagnosticsMapConfig,
-    workspace_root: &Path,
+    workspace_root: &AbsPath,
     span: &DiagnosticSpan,
 ) -> lsp_types::Location {
     let file_name = resolve_path(config, workspace_root, &span.file_name);
@@ -67,7 +65,7 @@ fn location(
 /// workspace into account and tries to avoid those, in case macros are involved.
 fn primary_location(
     config: &DiagnosticsMapConfig,
-    workspace_root: &Path,
+    workspace_root: &AbsPath,
     span: &DiagnosticSpan,
 ) -> lsp_types::Location {
     let span_stack = std::iter::successors(Some(span), |span| Some(&span.expansion.as_ref()?.span));
@@ -88,7 +86,7 @@ fn primary_location(
 /// If the span is unlabelled this will return `None`.
 fn diagnostic_related_information(
     config: &DiagnosticsMapConfig,
-    workspace_root: &Path,
+    workspace_root: &AbsPath,
     span: &DiagnosticSpan,
 ) -> Option<lsp_types::DiagnosticRelatedInformation> {
     let message = span.label.clone()?;
@@ -98,7 +96,11 @@ fn diagnostic_related_information(
 
 /// Resolves paths applying any matching path prefix remappings, and then
 /// joining the path to the workspace root.
-fn resolve_path(config: &DiagnosticsMapConfig, workspace_root: &Path, file_name: &str) -> PathBuf {
+fn resolve_path(
+    config: &DiagnosticsMapConfig,
+    workspace_root: &AbsPath,
+    file_name: &str,
+) -> AbsPathBuf {
     match config
         .remap_prefix
         .iter()
@@ -121,7 +123,7 @@ enum MappedRustChildDiagnostic {
 
 fn map_rust_child_diagnostic(
     config: &DiagnosticsMapConfig,
-    workspace_root: &Path,
+    workspace_root: &AbsPath,
     rd: &flycheck::Diagnostic,
 ) -> MappedRustChildDiagnostic {
     let spans: Vec<&DiagnosticSpan> = rd.spans.iter().filter(|s| s.is_primary).collect();
@@ -191,7 +193,7 @@ pub(crate) struct MappedRustDiagnostic {
 pub(crate) fn map_rust_diagnostic_to_lsp(
     config: &DiagnosticsMapConfig,
     rd: &flycheck::Diagnostic,
-    workspace_root: &Path,
+    workspace_root: &AbsPath,
 ) -> Vec<MappedRustDiagnostic> {
     let primary_spans: Vec<&DiagnosticSpan> = rd.spans.iter().filter(|s| s.is_primary).collect();
     if primary_spans.is_empty() {
@@ -426,6 +428,8 @@ fn clippy_code_description(code: Option<&str>) -> Option<lsp_types::CodeDescript
 #[cfg(test)]
 #[cfg(not(windows))]
 mod tests {
+    use std::{convert::TryInto, path::Path};
+
     use super::*;
 
     use expect_test::{expect_file, ExpectFile};
@@ -436,7 +440,7 @@ mod tests {
 
     fn check_with_config(config: DiagnosticsMapConfig, diagnostics_json: &str, expect: ExpectFile) {
         let diagnostic: flycheck::Diagnostic = serde_json::from_str(diagnostics_json).unwrap();
-        let workspace_root = Path::new("/test/");
+        let workspace_root: &AbsPath = Path::new("/test/").try_into().unwrap();
         let actual = map_rust_diagnostic_to_lsp(&config, &diagnostic, workspace_root);
         expect.assert_debug_eq(&actual)
     }
diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs
@@ -73,8 +73,9 @@ impl GlobalState {
         fn is_interesting(path: &AbsPath, change_kind: ChangeKind) -> bool {
             const IMPLICIT_TARGET_FILES: &[&str] = &["build.rs", "src/main.rs", "src/lib.rs"];
             const IMPLICIT_TARGET_DIRS: &[&str] = &["src/bin", "examples", "tests", "benches"];
+            let file_name = path.file_name().unwrap_or_default();
 
-            if path.ends_with("Cargo.toml") || path.ends_with("Cargo.lock") {
+            if file_name == "Cargo.toml" || file_name == "Cargo.lock" {
                 return true;
             }
             if change_kind == ChangeKind::Modify {
@@ -83,22 +84,22 @@ impl GlobalState {
             if path.extension().unwrap_or_default() != "rs" {
                 return false;
             }
-            if IMPLICIT_TARGET_FILES.iter().any(|it| path.ends_with(it)) {
+            if IMPLICIT_TARGET_FILES.iter().any(|it| path.as_ref().ends_with(it)) {
                 return true;
             }
             let parent = match path.parent() {
                 Some(it) => it,
                 None => return false,
             };
-            if IMPLICIT_TARGET_DIRS.iter().any(|it| parent.ends_with(it)) {
+            if IMPLICIT_TARGET_DIRS.iter().any(|it| parent.as_ref().ends_with(it)) {
                 return true;
             }
-            if path.ends_with("main.rs") {
+            if file_name == "main.rs" {
                 let grand_parent = match parent.parent() {
                     Some(it) => it,
                     None => return false,
                 };
-                if IMPLICIT_TARGET_DIRS.iter().any(|it| grand_parent.ends_with(it)) {
+                if IMPLICIT_TARGET_DIRS.iter().any(|it| grand_parent.as_ref().ends_with(it)) {
                     return true;
                 }
             }
diff --git a/crates/rust-analyzer/src/to_proto.rs b/crates/rust-analyzer/src/to_proto.rs
diff --git a/crates/vfs-notify/src/lib.rs b/crates/vfs-notify/src/lib.rs

Original file line number	Diff line number	Diff line change
`@@ -18,3 +18,4 @@ jod-thread = "0.1.1"`
`18`	`18`
`19`	`19`	`toolchain = { path = "../toolchain", version = "0.0.0" }`
`20`	`20`	`stdx = { path = "../stdx", version = "0.0.0" }`
	`21`	`+paths = { path = "../paths", version = "0.0.0" }`
Original file line number	Diff line number	Diff line change
`@@ -186,6 +186,9 @@ impl AbsPath {`
`186`	`186`	`pub fn starts_with(&self, base: &AbsPath) -> bool {`
`187`	`187`	`self.0.starts_with(&base.0)`
`188`	`188`	`}`
	`189`	`+ pub fn ends_with(&self, suffix: &RelPath) -> bool {`
	`190`	`+ self.0.starts_with(&suffix.0)`
	`191`	`+ }`
`189`	`192`
`190`	`193`	`// region:delegate-methods`
`191`	`194`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ pub(crate) struct ProcMacroSrv {`
`30`	`30`
`31`	`31`	`impl ProcMacroSrv {`
`32`	`32`	`pub fn expand(&mut self, task: &ExpansionTask) -> Result<ExpansionResult, String> {`
`33`		`- let expander = self.expander(&task.lib)?;`
	`33`	`+ let expander = self.expander(task.lib.as_ref())?;`
`34`	`34`
`35`	`35`	`let mut prev_env = HashMap::new();`
`36`	`36`	`for (k, v) in &task.env {`
`@@ -54,7 +54,7 @@ impl ProcMacroSrv {`
`54`	`54`	`}`
`55`	`55`
`56`	`56`	`pub fn list_macros(&mut self, task: &ListMacrosTask) -> Result<ListMacrosResult, String> {`
`57`		`- let expander = self.expander(&task.lib)?;`
	`57`	`+ let expander = self.expander(task.lib.as_ref())?;`
`58`	`58`	`Ok(ListMacrosResult { macros: expander.list_macros() })`
`59`	`59`	`}`
`60`	`60`