From fd3fce260052463cb75df54409add9936ffa8470 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 19 Aug 2024 14:50:42 +0200 Subject: [PATCH 1/2] Remove the ability to configure the user config path --- crates/rust-analyzer/src/bin/main.rs | 2 +- crates/rust-analyzer/src/cli/scip.rs | 1 - crates/rust-analyzer/src/config.rs | 72 +++++++++---------- .../rust-analyzer/src/diagnostics/to_proto.rs | 1 - crates/rust-analyzer/src/global_state.rs | 2 +- crates/rust-analyzer/src/reload.rs | 9 ++- .../rust-analyzer/tests/slow-tests/ratoml.rs | 12 ++-- .../rust-analyzer/tests/slow-tests/support.rs | 34 +++++---- crates/vfs/src/vfs_path.rs | 14 ++++ 9 files changed, 81 insertions(+), 66 deletions(-) diff --git a/crates/rust-analyzer/src/bin/main.rs b/crates/rust-analyzer/src/bin/main.rs index 42953d3b83..21b481c1fa 100644 --- a/crates/rust-analyzer/src/bin/main.rs +++ b/crates/rust-analyzer/src/bin/main.rs @@ -227,7 +227,7 @@ fn run_server() -> anyhow::Result<()> { .filter(|workspaces| !workspaces.is_empty()) .unwrap_or_else(|| vec![root_path.clone()]); let mut config = - Config::new(root_path, capabilities, workspace_roots, visual_studio_code_version, None); + Config::new(root_path, capabilities, workspace_roots, visual_studio_code_version); if let Some(json) = initialization_options { let mut change = ConfigChange::default(); change.change_client_config(json); diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index 34b20851dd..a8a02712fe 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -37,7 +37,6 @@ impl flags::Scip { lsp_types::ClientCapabilities::default(), vec![], None, - None, ); if let Some(p) = self.config_path { diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index d7f24e6ce1..02a513f44b 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -3,10 +3,13 @@ //! Of particular interest is the `feature_flags` hash map: while other fields //! configure the server itself, feature flags are passed into analysis, and //! tweak things like automatic insertion of `()` in completions. -use std::{fmt, iter, ops::Not, sync::OnceLock}; +use std::{ + env, fmt, iter, + ops::Not, + sync::{LazyLock, OnceLock}, +}; use cfg::{CfgAtom, CfgDiff}; -use dirs::config_dir; use hir::Symbol; use ide::{ AssistConfig, CallableSnippets, CompletionConfig, DiagnosticsConfig, ExprFillDefaultMode, @@ -735,7 +738,6 @@ pub enum RatomlFileKind { } #[derive(Debug, Clone)] -// FIXME @alibektas : Seems like a clippy warning of this sort should tell that combining different ConfigInputs into one enum was not a good idea. #[allow(clippy::large_enum_variant)] enum RatomlFile { Workspace(GlobalLocalConfigInput), @@ -757,16 +759,6 @@ pub struct Config { /// by receiving a `lsp_types::notification::DidChangeConfiguration`. client_config: (FullConfigInput, ConfigErrors), - /// Path to the root configuration file. This can be seen as a generic way to define what would be `$XDG_CONFIG_HOME/rust-analyzer/rust-analyzer.toml` in Linux. - /// If not specified by init of a `Config` object this value defaults to : - /// - /// |Platform | Value | Example | - /// | ------- | ------------------------------------- | ---------------------------------------- | - /// | Linux | `$XDG_CONFIG_HOME` or `$HOME`/.config | /home/alice/.config | - /// | macOS | `$HOME`/Library/Application Support | /Users/Alice/Library/Application Support | - /// | Windows | `{FOLDERID_RoamingAppData}` | C:\Users\Alice\AppData\Roaming | - user_config_path: VfsPath, - /// Config node whose values apply to **every** Rust project. user_config: Option<(GlobalLocalConfigInput, ConfigErrors)>, @@ -794,8 +786,27 @@ impl std::ops::Deref for Config { } impl Config { - pub fn user_config_path(&self) -> &VfsPath { - &self.user_config_path + /// Path to the root configuration file. This can be seen as a generic way to define what would be `$XDG_CONFIG_HOME/rust-analyzer/rust-analyzer.toml` in Linux. + /// This path is equal to: + /// + /// |Platform | Value | Example | + /// | ------- | ------------------------------------- | ---------------------------------------- | + /// | Linux | `$XDG_CONFIG_HOME` or `$HOME`/.config | /home/alice/.config | + /// | macOS | `$HOME`/Library/Application Support | /Users/Alice/Library/Application Support | + /// | Windows | `{FOLDERID_RoamingAppData}` | C:\Users\Alice\AppData\Roaming | + pub fn user_config_path() -> &'static AbsPath { + static USER_CONFIG_PATH: LazyLock = LazyLock::new(|| { + let user_config_path = if let Some(path) = env::var_os("__TEST_RA_USER_CONFIG_DIR") { + std::path::PathBuf::from(path) + } else { + dirs::config_dir() + .expect("A config dir is expected to existed on all platforms ra supports.") + .join("rust-analyzer") + } + .join("rust-analyzer.toml"); + AbsPathBuf::assert_utf8(user_config_path) + }); + &USER_CONFIG_PATH } pub fn same_source_root_parent_map( @@ -1315,24 +1326,8 @@ impl Config { caps: lsp_types::ClientCapabilities, workspace_roots: Vec, visual_studio_code_version: Option, - user_config_path: Option, ) -> Self { static DEFAULT_CONFIG_DATA: OnceLock<&'static DefaultConfigData> = OnceLock::new(); - let user_config_path = if let Some(user_config_path) = user_config_path { - user_config_path.join("rust-analyzer").join("rust-analyzer.toml") - } else { - let p = config_dir() - .expect("A config dir is expected to existed on all platforms ra supports.") - .join("rust-analyzer") - .join("rust-analyzer.toml"); - Utf8PathBuf::from_path_buf(p).expect("Config dir expected to be abs.") - }; - - // A user config cannot be a virtual path as rust-analyzer cannot support watching changes in virtual paths. - // See `GlobalState::process_changes` to get more info. - // FIXME @alibektas : Temporary solution. I don't think this is right as at some point we may allow users to specify - // custom USER_CONFIG_PATHs which may also be relative. - let user_config_path = VfsPath::from(AbsPathBuf::assert(user_config_path)); Config { caps: ClientCapabilities::new(caps), @@ -1345,7 +1340,6 @@ impl Config { default_config: DEFAULT_CONFIG_DATA.get_or_init(|| Box::leak(Box::default())), source_root_parent_map: Arc::new(FxHashMap::default()), user_config: None, - user_config_path, detached_files: Default::default(), validation_errors: Default::default(), ratoml_file: Default::default(), @@ -3417,7 +3411,7 @@ mod tests { #[test] fn proc_macro_srv_null() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); change.change_client_config(serde_json::json!({ @@ -3432,7 +3426,7 @@ mod tests { #[test] fn proc_macro_srv_abs() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); change.change_client_config(serde_json::json!({ "procMacro" : { @@ -3446,7 +3440,7 @@ mod tests { #[test] fn proc_macro_srv_rel() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); @@ -3466,7 +3460,7 @@ mod tests { #[test] fn cargo_target_dir_unset() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); @@ -3484,7 +3478,7 @@ mod tests { #[test] fn cargo_target_dir_subdir() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); change.change_client_config(serde_json::json!({ @@ -3502,7 +3496,7 @@ mod tests { #[test] fn cargo_target_dir_relative_dir() { let mut config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); change.change_client_config(serde_json::json!({ @@ -3523,7 +3517,7 @@ mod tests { #[test] fn toml_unknown_key() { let config = - Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None, None); + Config::new(AbsPathBuf::assert(project_root()), Default::default(), vec![], None); let mut change = ConfigChange::default(); diff --git a/crates/rust-analyzer/src/diagnostics/to_proto.rs b/crates/rust-analyzer/src/diagnostics/to_proto.rs index e330cfc3cf..c3ab7f3ae7 100644 --- a/crates/rust-analyzer/src/diagnostics/to_proto.rs +++ b/crates/rust-analyzer/src/diagnostics/to_proto.rs @@ -548,7 +548,6 @@ mod tests { ClientCapabilities::default(), Vec::new(), None, - None, ), ); let snap = state.snapshot(); diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index 7a7ec1d77e..1e21249c12 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/crates/rust-analyzer/src/global_state.rs @@ -380,7 +380,7 @@ impl GlobalState { || !self.config.same_source_root_parent_map(&self.local_roots_parent_map) { let config_change = { - let user_config_path = self.config.user_config_path(); + let user_config_path = Config::user_config_path(); let mut change = ConfigChange::default(); let db = self.analysis_host.raw_database(); diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 510dc22603..5b58b332df 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -550,9 +550,12 @@ impl GlobalState { }; watchers.extend( - iter::once(self.config.user_config_path().as_path()) - .chain(self.workspaces.iter().map(|ws| ws.manifest().map(ManifestPath::as_ref))) - .flatten() + iter::once(Config::user_config_path()) + .chain( + self.workspaces + .iter() + .filter_map(|ws| ws.manifest().map(ManifestPath::as_ref)), + ) .map(|glob_pattern| lsp_types::FileSystemWatcher { glob_pattern: lsp_types::GlobPattern::String(glob_pattern.to_string()), kind: None, diff --git a/crates/rust-analyzer/tests/slow-tests/ratoml.rs b/crates/rust-analyzer/tests/slow-tests/ratoml.rs index c06ba9eee1..478840f104 100644 --- a/crates/rust-analyzer/tests/slow-tests/ratoml.rs +++ b/crates/rust-analyzer/tests/slow-tests/ratoml.rs @@ -8,6 +8,7 @@ use lsp_types::{ }; use paths::Utf8PathBuf; +use rust_analyzer::config::Config; use rust_analyzer::lsp::ext::{InternalTestingFetchConfig, InternalTestingFetchConfigParams}; use serde_json::json; use test_utils::skip_slow_tests; @@ -24,7 +25,6 @@ struct RatomlTest { urls: Vec, server: Server, tmp_path: Utf8PathBuf, - user_config_dir: Utf8PathBuf, } impl RatomlTest { @@ -41,11 +41,7 @@ impl RatomlTest { let full_fixture = fixtures.join("\n"); - let user_cnf_dir = TestDir::new(); - let user_config_dir = user_cnf_dir.path().to_owned(); - - let mut project = - Project::with_fixture(&full_fixture).tmp_dir(tmp_dir).user_config_dir(user_cnf_dir); + let mut project = Project::with_fixture(&full_fixture).tmp_dir(tmp_dir); for root in roots { project = project.root(root); @@ -57,7 +53,7 @@ impl RatomlTest { let server = project.server().wait_until_workspace_is_loaded(); - let mut case = Self { urls: vec![], server, tmp_path, user_config_dir }; + let mut case = Self { urls: vec![], server, tmp_path }; let urls = fixtures.iter().map(|fixture| case.fixture_path(fixture)).collect::>(); case.urls = urls; case @@ -81,7 +77,7 @@ impl RatomlTest { let mut spl = spl.into_iter(); if let Some(first) = spl.next() { if first == "$$CONFIG_DIR$$" { - path = self.user_config_dir.clone(); + path = Config::user_config_path().to_path_buf().into(); } else { path = path.join(first); } diff --git a/crates/rust-analyzer/tests/slow-tests/support.rs b/crates/rust-analyzer/tests/slow-tests/support.rs index 081ee5fa3e..5bd27133c2 100644 --- a/crates/rust-analyzer/tests/slow-tests/support.rs +++ b/crates/rust-analyzer/tests/slow-tests/support.rs @@ -8,6 +8,7 @@ use std::{ use crossbeam_channel::{after, select, Receiver}; use lsp_server::{Connection, Message, Notification, Request}; use lsp_types::{notification::Exit, request::Shutdown, TextDocumentIdentifier, Url}; +use parking_lot::{Mutex, MutexGuard}; use paths::{Utf8Path, Utf8PathBuf}; use rust_analyzer::{ config::{Config, ConfigChange, ConfigErrors}, @@ -27,7 +28,6 @@ pub(crate) struct Project<'a> { roots: Vec, config: serde_json::Value, root_dir_contains_symlink: bool, - user_config_path: Option, } impl Project<'_> { @@ -51,15 +51,9 @@ impl Project<'_> { } }), root_dir_contains_symlink: false, - user_config_path: None, } } - pub(crate) fn user_config_dir(mut self, config_path_dir: TestDir) -> Self { - self.user_config_path = Some(config_path_dir.path().to_owned()); - self - } - pub(crate) fn tmp_dir(mut self, tmp_dir: TestDir) -> Self { self.tmp_dir = Some(tmp_dir); self @@ -91,6 +85,7 @@ impl Project<'_> { } pub(crate) fn server(self) -> Server { + static CONFIG_DIR_LOCK: Mutex<()> = Mutex::new(()); let tmp_dir = self.tmp_dir.unwrap_or_else(|| { if self.root_dir_contains_symlink { TestDir::new_symlink() @@ -122,9 +117,13 @@ impl Project<'_> { assert!(mini_core.is_none()); assert!(toolchain.is_none()); + let mut config_dir_guard = None; for entry in fixture { if let Some(pth) = entry.path.strip_prefix("/$$CONFIG_DIR$$") { - let path = self.user_config_path.clone().unwrap().join(&pth['/'.len_utf8()..]); + if config_dir_guard.is_none() { + config_dir_guard = Some(CONFIG_DIR_LOCK.lock()); + } + let path = Config::user_config_path().join(&pth['/'.len_utf8()..]); fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write(path.as_path(), entry.text.as_bytes()).unwrap(); } else { @@ -201,7 +200,6 @@ impl Project<'_> { }, roots, None, - self.user_config_path, ); let mut change = ConfigChange::default(); @@ -213,7 +211,7 @@ impl Project<'_> { config.rediscover_workspaces(); - Server::new(tmp_dir.keep(), config) + Server::new(config_dir_guard, tmp_dir.keep(), config) } } @@ -228,10 +226,15 @@ pub(crate) struct Server { client: Connection, /// XXX: remove the tempdir last dir: TestDir, + _config_dir_guard: Option>, } impl Server { - fn new(dir: TestDir, config: Config) -> Server { + fn new( + config_dir_guard: Option>, + dir: TestDir, + config: Config, + ) -> Server { let (connection, client) = Connection::memory(); let _thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker) @@ -239,7 +242,14 @@ impl Server { .spawn(move || main_loop(config, connection).unwrap()) .expect("failed to spawn a thread"); - Server { req_id: Cell::new(1), dir, messages: Default::default(), client, _thread } + Server { + req_id: Cell::new(1), + dir, + messages: Default::default(), + client, + _thread, + _config_dir_guard: config_dir_guard, + } } pub(crate) fn doc_id(&self, rel_path: &str) -> TextDocumentIdentifier { diff --git a/crates/vfs/src/vfs_path.rs b/crates/vfs/src/vfs_path.rs index 92a49e0793..3c8e37413f 100644 --- a/crates/vfs/src/vfs_path.rs +++ b/crates/vfs/src/vfs_path.rs @@ -313,6 +313,20 @@ impl fmt::Debug for VfsPathRepr { } } +impl PartialEq for VfsPath { + fn eq(&self, other: &AbsPath) -> bool { + match &self.0 { + VfsPathRepr::PathBuf(lhs) => lhs == other, + VfsPathRepr::VirtualPath(_) => false, + } + } +} +impl PartialEq for AbsPath { + fn eq(&self, other: &VfsPath) -> bool { + other == self + } +} + /// `/`-separated virtual path. /// /// This is used to describe files that do not reside on the file system. From 90e08d3c9351c2ed813c3c9facaa92aa4e9530c4 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 19 Aug 2024 16:00:06 +0200 Subject: [PATCH 2/2] Allow user config to not exist --- crates/rust-analyzer/src/config.rs | 12 +++++------- crates/rust-analyzer/src/global_state.rs | 2 +- crates/rust-analyzer/src/reload.rs | 7 ++----- crates/rust-analyzer/tests/slow-tests/ratoml.rs | 2 +- crates/rust-analyzer/tests/slow-tests/support.rs | 2 +- 5 files changed, 10 insertions(+), 15 deletions(-) diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index 02a513f44b..36907c468b 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -794,19 +794,17 @@ impl Config { /// | Linux | `$XDG_CONFIG_HOME` or `$HOME`/.config | /home/alice/.config | /// | macOS | `$HOME`/Library/Application Support | /Users/Alice/Library/Application Support | /// | Windows | `{FOLDERID_RoamingAppData}` | C:\Users\Alice\AppData\Roaming | - pub fn user_config_path() -> &'static AbsPath { - static USER_CONFIG_PATH: LazyLock = LazyLock::new(|| { + pub fn user_config_path() -> Option<&'static AbsPath> { + static USER_CONFIG_PATH: LazyLock> = LazyLock::new(|| { let user_config_path = if let Some(path) = env::var_os("__TEST_RA_USER_CONFIG_DIR") { std::path::PathBuf::from(path) } else { - dirs::config_dir() - .expect("A config dir is expected to existed on all platforms ra supports.") - .join("rust-analyzer") + dirs::config_dir()?.join("rust-analyzer") } .join("rust-analyzer.toml"); - AbsPathBuf::assert_utf8(user_config_path) + Some(AbsPathBuf::assert_utf8(user_config_path)) }); - &USER_CONFIG_PATH + USER_CONFIG_PATH.as_deref() } pub fn same_source_root_parent_map( diff --git a/crates/rust-analyzer/src/global_state.rs b/crates/rust-analyzer/src/global_state.rs index 1e21249c12..8fcdfa5829 100644 --- a/crates/rust-analyzer/src/global_state.rs +++ b/crates/rust-analyzer/src/global_state.rs @@ -399,7 +399,7 @@ impl GlobalState { .collect_vec(); for (file_id, (_change_kind, vfs_path)) in modified_ratoml_files { - if vfs_path == *user_config_path { + if vfs_path.as_path() == user_config_path { change.change_user_config(Some(db.file_text(file_id))); continue; } diff --git a/crates/rust-analyzer/src/reload.rs b/crates/rust-analyzer/src/reload.rs index 5b58b332df..2b9bcf12a4 100644 --- a/crates/rust-analyzer/src/reload.rs +++ b/crates/rust-analyzer/src/reload.rs @@ -551,11 +551,8 @@ impl GlobalState { watchers.extend( iter::once(Config::user_config_path()) - .chain( - self.workspaces - .iter() - .filter_map(|ws| ws.manifest().map(ManifestPath::as_ref)), - ) + .chain(self.workspaces.iter().map(|ws| ws.manifest().map(ManifestPath::as_ref))) + .flatten() .map(|glob_pattern| lsp_types::FileSystemWatcher { glob_pattern: lsp_types::GlobPattern::String(glob_pattern.to_string()), kind: None, diff --git a/crates/rust-analyzer/tests/slow-tests/ratoml.rs b/crates/rust-analyzer/tests/slow-tests/ratoml.rs index 478840f104..f68b94fa66 100644 --- a/crates/rust-analyzer/tests/slow-tests/ratoml.rs +++ b/crates/rust-analyzer/tests/slow-tests/ratoml.rs @@ -77,7 +77,7 @@ impl RatomlTest { let mut spl = spl.into_iter(); if let Some(first) = spl.next() { if first == "$$CONFIG_DIR$$" { - path = Config::user_config_path().to_path_buf().into(); + path = Config::user_config_path().unwrap().to_path_buf().into(); } else { path = path.join(first); } diff --git a/crates/rust-analyzer/tests/slow-tests/support.rs b/crates/rust-analyzer/tests/slow-tests/support.rs index 5bd27133c2..06ce984681 100644 --- a/crates/rust-analyzer/tests/slow-tests/support.rs +++ b/crates/rust-analyzer/tests/slow-tests/support.rs @@ -123,7 +123,7 @@ impl Project<'_> { if config_dir_guard.is_none() { config_dir_guard = Some(CONFIG_DIR_LOCK.lock()); } - let path = Config::user_config_path().join(&pth['/'.len_utf8()..]); + let path = Config::user_config_path().unwrap().join(&pth['/'.len_utf8()..]); fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write(path.as_path(), entry.text.as_bytes()).unwrap(); } else {