From 8d8c26e6f5197ee25c69cbf1341ffceca55b9301 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 17 Jul 2021 17:40:13 +0300 Subject: [PATCH] internal: a bit more of cwd safety for flycheck --- Cargo.lock | 2 ++ crates/flycheck/Cargo.toml | 1 + crates/flycheck/src/lib.rs | 12 ++++----- crates/paths/src/lib.rs | 3 +++ crates/proc_macro_api/src/rpc.rs | 2 +- crates/proc_macro_srv/Cargo.toml | 1 + crates/proc_macro_srv/src/dylib.rs | 9 +++++-- crates/proc_macro_srv/src/lib.rs | 4 +-- crates/proc_macro_srv/src/tests/mod.rs | 3 ++- crates/proc_macro_srv/src/tests/utils.rs | 3 ++- crates/rust-analyzer/src/cli/load_cargo.rs | 10 +++---- crates/rust-analyzer/src/config.rs | 8 +++--- .../rust-analyzer/src/diagnostics/to_proto.rs | 26 +++++++++++-------- crates/rust-analyzer/src/reload.rs | 11 ++++---- crates/rust-analyzer/src/to_proto.rs | 22 +++++++--------- crates/vfs-notify/src/lib.rs | 7 ++--- 16 files changed, 72 insertions(+), 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0e2665c7b1..82c7c143ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -381,6 +381,7 @@ dependencies = [ "crossbeam-channel", "jod-thread", "log", + "paths", "serde", "serde_json", "stdx", @@ -1131,6 +1132,7 @@ dependencies = [ "mbe", "memmap2", "object", + "paths", "proc_macro_api", "proc_macro_test", "test_utils", diff --git a/crates/flycheck/Cargo.toml b/crates/flycheck/Cargo.toml index e939d39325..8e0ef8f6d6 100644 --- 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 index 3f98f14592..6a3b4a45a6 100644 --- 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, config: FlycheckConfig, - workspace_root: PathBuf, + workspace_root: AbsPathBuf, ) -> FlycheckHandle { let actor = FlycheckActor::new(id, sender, config, workspace_root); let (sender, receiver) = unbounded::(); @@ -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, 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, 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 index afdfed7fd5..f976783dea 100644 --- 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 index 4bb77eb7f1..d56136196e 100644 --- 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 index fb80522b94..df4680c6ef 100644 --- 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 index 5f0b0b061e..ab13e66fb8 100644 --- 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 index eb9080e998..1c39455112 100644 --- 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 { - 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 { - 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 index d9037ee57f..9356e6dcb0 100644 --- 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 index 150e26fe75..d78e14a2dd 100644 --- 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 { - 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 index b5f5519b43..b8f47f2b98 100644 --- 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)> { 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 index 415ae708b0..7df6022964 100644 --- 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 { self.data.lruCapacity } - pub fn proc_macro_srv(&self) -> Option<(PathBuf, Vec)> { + pub fn proc_macro_srv(&self) -> Option<(AbsPathBuf, Vec)> { 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 index 8594d923cd..d64909add3 100644 --- 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 { 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 { 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 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 index 060b69e088..8bd3f7a9eb 100644 --- a/crates/rust-analyzer/src/to_proto.rs +++ b/crates/rust-analyzer/src/to_proto.rs @@ -1,7 +1,7 @@ //! Conversion of rust-analyzer specific types to lsp_types equivalents. use std::{ iter::once, - path::{self, Path}, + path, sync::atomic::{AtomicU32, Ordering}, }; @@ -14,6 +14,7 @@ use ide::{ }; use itertools::Itertools; use serde_json::to_value; +use vfs::AbsPath; use crate::{ cargo_target_spec::CargoTargetSpec, @@ -622,10 +623,9 @@ pub(crate) fn url(snap: &GlobalStateSnapshot, file_id: FileId) -> lsp_types::Url /// This will only happen when processing windows paths. /// /// When processing non-windows path, this is essentially the same as `Url::from_file_path`. -pub(crate) fn url_from_abs_path(path: &Path) -> lsp_types::Url { - assert!(path.is_absolute()); +pub(crate) fn url_from_abs_path(path: &AbsPath) -> lsp_types::Url { let url = lsp_types::Url::from_file_path(path).unwrap(); - match path.components().next() { + match path.as_ref().components().next() { Some(path::Component::Prefix(prefix)) if matches!(prefix.kind(), path::Prefix::Disk(_) | path::Prefix::VerbatimDisk(_)) => { @@ -1328,15 +1328,13 @@ fn main() { // `Url` is not able to parse windows paths on unix machines. #[test] #[cfg(target_os = "windows")] - fn test_lowercase_drive_letter_with_drive() { - let url = url_from_abs_path(Path::new("C:\\Test")); - assert_eq!(url.to_string(), "file:///c:/Test"); - } + fn test_lowercase_drive_letter() { + use std::{convert::TryInto, path::Path}; - #[test] - #[cfg(target_os = "windows")] - fn test_drive_without_colon_passthrough() { - let url = url_from_abs_path(Path::new(r#"\\localhost\C$\my_dir"#)); + let url = url_from_abs_path(Path::new("C:\\Test").try_into().unwrap()); + assert_eq!(url.to_string(), "file:///c:/Test"); + + let url = url_from_abs_path(Path::new(r#"\\localhost\C$\my_dir"#).try_into().unwrap()); assert_eq!(url.to_string(), "file://localhost/C$/my_dir"); } } diff --git a/crates/vfs-notify/src/lib.rs b/crates/vfs-notify/src/lib.rs index 7815556f5a..d330fba331 100644 --- a/crates/vfs-notify/src/lib.rs +++ b/crates/vfs-notify/src/lib.rs @@ -6,7 +6,7 @@ //! //! Hopefully, one day a reliable file watching/walking crate appears on //! crates.io, and we can reduce this to trivial glue code. -use std::convert::TryFrom; +use std::{convert::TryFrom, fs}; use crossbeam_channel::{never, select, unbounded, Receiver, Sender}; use notify::{RecommendedWatcher, RecursiveMode, Watcher}; @@ -123,7 +123,8 @@ impl NotifyActor { .into_iter() .map(|path| AbsPathBuf::try_from(path).unwrap()) .filter_map(|path| { - if path.is_dir() + let meta = fs::metadata(&path).ok()?; + if meta.file_type().is_dir() && self .watched_entries .iter() @@ -133,7 +134,7 @@ impl NotifyActor { return None; } - if !path.is_file() { + if !meta.file_type().is_file() { return None; } if !self