From 3fc7101b966822ffb3b77106a16af8f806fb2001 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Wed, 30 Oct 2024 12:03:08 +0100 Subject: [PATCH] Fix config guard lock for ratoml tests --- .../crates/load-cargo/src/lib.rs | 4 +- .../crates/rust-analyzer/src/config.rs | 25 ++++------ .../crates/rust-analyzer/src/global_state.rs | 10 ++-- .../crates/rust-analyzer/src/reload.rs | 4 +- .../rust-analyzer/tests/slow-tests/ratoml.rs | 29 +++-------- .../rust-analyzer/tests/slow-tests/support.rs | 48 +++++++++++++------ 6 files changed, 58 insertions(+), 62 deletions(-) diff --git a/src/tools/rust-analyzer/crates/load-cargo/src/lib.rs b/src/tools/rust-analyzer/crates/load-cargo/src/lib.rs index ab8a780363d9..cf26845b1191 100644 --- a/src/tools/rust-analyzer/crates/load-cargo/src/lib.rs +++ b/src/tools/rust-analyzer/crates/load-cargo/src/lib.rs @@ -156,7 +156,7 @@ impl ProjectFolders { pub fn new( workspaces: &[ProjectWorkspace], global_excludes: &[AbsPathBuf], - user_config_dir_path: Option<&'static AbsPath>, + user_config_dir_path: Option<&AbsPath>, ) -> ProjectFolders { let mut res = ProjectFolders::default(); let mut fsc = FileSetConfig::builder(); @@ -302,7 +302,7 @@ impl ProjectFolders { p }; - let file_set_roots: Vec = vec![VfsPath::from(ratoml_path.to_owned())]; + let file_set_roots = vec![VfsPath::from(ratoml_path.to_owned())]; let entry = vfs::loader::Entry::Files(vec![ratoml_path.to_owned()]); res.watch.push(res.load.len()); diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/config.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/config.rs index 921c9b991c5a..694748f82f33 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/config.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/config.rs @@ -3,11 +3,7 @@ //! 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::{ - env, fmt, iter, - ops::Not, - sync::{LazyLock, OnceLock}, -}; +use std::{env, fmt, iter, ops::Not, sync::OnceLock}; use cfg::{CfgAtom, CfgDiff}; use hir::Symbol; @@ -805,16 +801,13 @@ impl std::ops::Deref for Config { impl Config { /// Path to the user configuration dir. This can be seen as a generic way to define what would be `$XDG_CONFIG_HOME/rust-analyzer` in Linux. - pub fn user_config_dir_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()?.join("rust-analyzer") - }; - Some(AbsPathBuf::assert_utf8(user_config_path)) - }); - USER_CONFIG_PATH.as_deref() + pub fn user_config_dir_path() -> Option { + 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()?.join("rust-analyzer") + }; + Some(AbsPathBuf::assert_utf8(user_config_path)) } pub fn same_source_root_parent_map( @@ -1254,7 +1247,7 @@ pub struct NotificationsConfig { pub cargo_toml_not_found: bool, } -#[derive(Deserialize, Serialize, Debug, Clone)] +#[derive(Debug, Clone)] pub enum RustfmtConfig { Rustfmt { extra_args: Vec, enable_range_formatting: bool }, CustomCommand { command: String, args: Vec }, diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs index 8d5536e1b094..5f8357028403 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/global_state.rs @@ -392,13 +392,13 @@ impl GlobalState { || !self.config.same_source_root_parent_map(&self.local_roots_parent_map) { let config_change = { - let user_config_path = { - let mut p = Config::user_config_dir_path().unwrap().to_path_buf(); + let user_config_path = (|| { + let mut p = Config::user_config_dir_path()?; p.push("rust-analyzer.toml"); - p - }; + Some(p) + })(); - let user_config_abs_path = Some(user_config_path.as_path()); + let user_config_abs_path = user_config_path.as_deref(); let mut change = ConfigChange::default(); let db = self.analysis_host.raw_database(); diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/src/reload.rs b/src/tools/rust-analyzer/crates/rust-analyzer/src/reload.rs index b0b622035282..4549735fef84 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/src/reload.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/src/reload.rs @@ -590,7 +590,7 @@ impl GlobalState { } watchers.extend( - iter::once(Config::user_config_dir_path()) + iter::once(Config::user_config_dir_path().as_deref()) .chain(self.workspaces.iter().map(|ws| ws.manifest().map(ManifestPath::as_ref))) .flatten() .map(|glob_pattern| lsp_types::FileSystemWatcher { @@ -616,7 +616,7 @@ impl GlobalState { let project_folders = ProjectFolders::new( &self.workspaces, &files_config.exclude, - Config::user_config_dir_path().to_owned(), + Config::user_config_dir_path().as_deref(), ); if (self.proc_macro_clients.is_empty() || !same_workspaces) diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/ratoml.rs b/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/ratoml.rs index 623a9f76d14e..5dfaf0d36503 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/ratoml.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/ratoml.rs @@ -30,23 +30,6 @@ impl RatomlTest { fixtures: Vec<&str>, roots: Vec<&str>, client_config: Option, - ) -> Self { - RatomlTest::new_with_lock(fixtures, roots, client_config, false) - } - - fn new_locked( - fixtures: Vec<&str>, - roots: Vec<&str>, - client_config: Option, - ) -> Self { - RatomlTest::new_with_lock(fixtures, roots, client_config, true) - } - - fn new_with_lock( - fixtures: Vec<&str>, - roots: Vec<&str>, - client_config: Option, - prelock: bool, ) -> Self { let tmp_dir = TestDir::new(); let tmp_path = tmp_dir.path().to_owned(); @@ -63,7 +46,7 @@ impl RatomlTest { project = project.with_config(client_config); } - let server = project.server_with_lock(prelock).wait_until_workspace_is_loaded(); + let server = project.server_with_lock(true).wait_until_workspace_is_loaded(); let mut case = Self { urls: vec![], server, tmp_path }; let urls = fixtures.iter().map(|fixture| case.fixture_path(fixture)).collect::>(); @@ -89,7 +72,7 @@ impl RatomlTest { let mut spl = spl.into_iter(); if let Some(first) = spl.next() { if first == "$$CONFIG_DIR$$" { - path = Config::user_config_dir_path().unwrap().to_path_buf().into(); + path = Config::user_config_dir_path().unwrap().into(); } else { path = path.join(first); } @@ -307,7 +290,7 @@ fn ratoml_user_config_detected() { return; } - let server = RatomlTest::new_locked( + let server = RatomlTest::new( vec![ r#" //- /$$CONFIG_DIR$$/rust-analyzer.toml @@ -343,7 +326,7 @@ fn ratoml_create_user_config() { return; } - let mut server = RatomlTest::new_locked( + let mut server = RatomlTest::new( vec![ r#" //- /p1/Cargo.toml @@ -382,7 +365,7 @@ fn ratoml_modify_user_config() { return; } - let mut server = RatomlTest::new_locked( + let mut server = RatomlTest::new( vec![ r#" //- /p1/Cargo.toml @@ -423,7 +406,7 @@ fn ratoml_delete_user_config() { return; } - let mut server = RatomlTest::new_locked( + let mut server = RatomlTest::new( vec![ r#" //- /p1/Cargo.toml diff --git a/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/support.rs b/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/support.rs index 86137e3f2ccb..5a88a5515c7f 100644 --- a/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/support.rs +++ b/src/tools/rust-analyzer/crates/rust-analyzer/tests/slow-tests/support.rs @@ -1,7 +1,7 @@ use std::{ cell::{Cell, RefCell}, env, fs, - sync::Once, + sync::{Once, OnceLock}, time::Duration, }; @@ -140,13 +140,36 @@ impl Project<'_> { /// if there is a path to config dir in the test fixture. However, in certain cases we create a /// file in the config dir after server is run, something where our naive approach comes short. /// Using a `prelock` allows us to force a lock when we know we need it. - pub(crate) fn server_with_lock(self, prelock: bool) -> Server { - static CONFIG_DIR_LOCK: Mutex<()> = Mutex::new(()); + pub(crate) fn server_with_lock(self, config_lock: bool) -> Server { + static CONFIG_DIR_LOCK: OnceLock<(Utf8PathBuf, Mutex<()>)> = OnceLock::new(); - let mut config_dir_guard = if prelock { - let v = Some(CONFIG_DIR_LOCK.lock()); - env::set_var("__TEST_RA_USER_CONFIG_DIR", TestDir::new().path()); - v + let config_dir_guard = if config_lock { + Some({ + let (path, mutex) = CONFIG_DIR_LOCK.get_or_init(|| { + let value = TestDir::new().keep().path().to_owned(); + env::set_var("__TEST_RA_USER_CONFIG_DIR", &value); + (value, Mutex::new(())) + }); + #[allow(dyn_drop)] + (mutex.lock(), { + Box::new({ + struct Dropper(Utf8PathBuf); + impl Drop for Dropper { + fn drop(&mut self) { + for entry in fs::read_dir(&self.0).unwrap() { + let path = entry.unwrap().path(); + if path.is_file() { + fs::remove_file(path).unwrap(); + } else if path.is_dir() { + fs::remove_dir_all(path).unwrap(); + } + } + } + } + Dropper(path.clone()) + }) as Box + }) + }) } else { None }; @@ -185,11 +208,6 @@ impl Project<'_> { for entry in fixture { if let Some(pth) = entry.path.strip_prefix("/$$CONFIG_DIR$$") { - if config_dir_guard.is_none() { - config_dir_guard = Some(CONFIG_DIR_LOCK.lock()); - env::set_var("__TEST_RA_USER_CONFIG_DIR", TestDir::new().path()); - } - let path = Config::user_config_dir_path().unwrap().join(&pth['/'.len_utf8()..]); fs::create_dir_all(path.parent().unwrap()).unwrap(); fs::write(path.as_path(), entry.text.as_bytes()).unwrap(); @@ -293,12 +311,14 @@ pub(crate) struct Server { client: Connection, /// XXX: remove the tempdir last dir: TestDir, - _config_dir_guard: Option>, + #[allow(dyn_drop)] + _config_dir_guard: Option<(MutexGuard<'static, ()>, Box)>, } impl Server { + #[allow(dyn_drop)] fn new( - config_dir_guard: Option>, + config_dir_guard: Option<(MutexGuard<'static, ()>, Box)>, dir: TestDir, config: Config, ) -> Server {