From f7819e553f0dbb22afcf31ea77938d529fe52818 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Feb 2025 14:50:55 +0000 Subject: [PATCH] Add `user_configuration_directory` to `System` (#16020) ## Summary This PR adds a new `user_configuration_directory` method to `System`. We need it to resolve where to lookup a user-level `knot.toml` configuration file. The method belongs to `System` because not all platforms have a convention of where to store such configuration files (e.g. wasm). I refactored `TestSystem` to be a simple wrapper around an `Arc` and use the `System.as_any` method instead to cast it down to an `InMemory` system. I also removed some `System` specific methods from `InMemoryFileSystem`, they don't belong there. This PR removes the `os` feature as a default feature from `ruff_db`. Most crates depending on `ruff_db` don't need it because they only depend on `System` or only depend on `os` for testing. This was necessary to fix a compile error with `red_knot_wasm` ## Test Plan I'll make use of the method in my next PR. So I guess we won't know if it works before then but I copied the code from Ruff/uv, so I have high confidence that it is correct. `cargo test` --- Cargo.lock | 1 + crates/red_knot_project/Cargo.toml | 2 +- crates/red_knot_python_semantic/Cargo.toml | 2 +- crates/red_knot_server/Cargo.toml | 2 +- crates/red_knot_server/src/system.rs | 4 + crates/red_knot_wasm/Cargo.toml | 2 +- crates/red_knot_wasm/src/lib.rs | 4 + crates/ruff_db/Cargo.toml | 6 +- crates/ruff_db/src/system.rs | 7 +- crates/ruff_db/src/system/memory_fs.rs | 18 -- crates/ruff_db/src/system/os.rs | 15 ++ crates/ruff_db/src/system/test.rs | 245 +++++++++++++-------- 12 files changed, 186 insertions(+), 122 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 288a668931..b4df9d6250 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2752,6 +2752,7 @@ dependencies = [ "countme", "dashmap 6.1.0", "dunce", + "etcetera", "filetime", "glob", "ignore", diff --git a/crates/red_knot_project/Cargo.toml b/crates/red_knot_project/Cargo.toml index 43e1b2d190..ea31f99f79 100644 --- a/crates/red_knot_project/Cargo.toml +++ b/crates/red_knot_project/Cargo.toml @@ -13,7 +13,7 @@ license.workspace = true [dependencies] ruff_cache = { workspace = true } -ruff_db = { workspace = true, features = ["os", "cache", "serde"] } +ruff_db = { workspace = true, features = ["cache", "serde"] } ruff_macros = { workspace = true } ruff_python_ast = { workspace = true, features = ["serde"] } ruff_text_size = { workspace = true } diff --git a/crates/red_knot_python_semantic/Cargo.toml b/crates/red_knot_python_semantic/Cargo.toml index cafd00e9c0..47847e8307 100644 --- a/crates/red_knot_python_semantic/Cargo.toml +++ b/crates/red_knot_python_semantic/Cargo.toml @@ -44,7 +44,7 @@ test-case = { workspace = true } memchr = { workspace = true } [dev-dependencies] -ruff_db = { workspace = true, features = ["os", "testing"] } +ruff_db = { workspace = true, features = ["testing"] } ruff_python_parser = { workspace = true } red_knot_test = { workspace = true } red_knot_vendored = { workspace = true } diff --git a/crates/red_knot_server/Cargo.toml b/crates/red_knot_server/Cargo.toml index 87e8126b8c..f14aa19350 100644 --- a/crates/red_knot_server/Cargo.toml +++ b/crates/red_knot_server/Cargo.toml @@ -12,7 +12,7 @@ license = { workspace = true } [dependencies] red_knot_project = { workspace = true } -ruff_db = { workspace = true } +ruff_db = { workspace = true, features = ["os"] } ruff_notebook = { workspace = true } ruff_python_ast = { workspace = true } ruff_source_file = { workspace = true } diff --git a/crates/red_knot_server/src/system.rs b/crates/red_knot_server/src/system.rs index e2d4438567..faf13e6e1e 100644 --- a/crates/red_knot_server/src/system.rs +++ b/crates/red_knot_server/src/system.rs @@ -187,6 +187,10 @@ impl System for LSPSystem { self.os_system.current_directory() } + fn user_config_directory(&self) -> Option { + self.os_system.user_config_directory() + } + fn read_directory<'a>( &'a self, path: &SystemPath, diff --git a/crates/red_knot_wasm/Cargo.toml b/crates/red_knot_wasm/Cargo.toml index d6363f1446..7cf0838834 100644 --- a/crates/red_knot_wasm/Cargo.toml +++ b/crates/red_knot_wasm/Cargo.toml @@ -22,7 +22,7 @@ default = ["console_error_panic_hook"] red_knot_python_semantic = { workspace = true } red_knot_project = { workspace = true, default-features = false, features = ["deflate"] } -ruff_db = { workspace = true, features = [] } +ruff_db = { workspace = true, default-features = false, features = [] } ruff_notebook = { workspace = true } console_error_panic_hook = { workspace = true, optional = true } diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index ad9d422566..6801e566eb 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -262,6 +262,10 @@ impl System for WasmSystem { self.fs.current_directory() } + fn user_config_directory(&self) -> Option { + None + } + fn read_directory<'a>( &'a self, path: &SystemPath, diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index b5443ceeb9..d771fd8a8a 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -43,14 +43,16 @@ zip = { workspace = true } [target.'cfg(target_arch="wasm32")'.dependencies] web-time = { version = "1.1.0" } +[target.'cfg(not(target_arch="wasm32"))'.dependencies] +etcetera = { workspace = true, optional = true } + [dev-dependencies] insta = { workspace = true } tempfile = { workspace = true } [features] -default = ["os"] cache = ["ruff_cache"] -os = ["ignore"] +os = ["ignore", "dep:etcetera"] serde = ["dep:serde", "camino/serde1"] # Exposes testing utilities. testing = ["tracing-subscriber", "tracing-tree"] diff --git a/crates/ruff_db/src/system.rs b/crates/ruff_db/src/system.rs index 4a7edc8d0b..91bcb03756 100644 --- a/crates/ruff_db/src/system.rs +++ b/crates/ruff_db/src/system.rs @@ -7,7 +7,7 @@ use std::error::Error; use std::fmt::Debug; use std::path::{Path, PathBuf}; use std::{fmt, io}; -pub use test::{DbWithTestSystem, TestSystem}; +pub use test::{DbWithTestSystem, InMemorySystem, TestSystem}; use walk_directory::WalkDirectoryBuilder; use crate::file_revision::FileRevision; @@ -99,6 +99,11 @@ pub trait System: Debug { /// Returns the current working directory fn current_directory(&self) -> &SystemPath; + /// Returns the directory path where user configurations are stored. + /// + /// Returns `None` if no such convention exists for the system. + fn user_config_directory(&self) -> Option; + /// Iterate over the contents of the directory at `path`. /// /// The returned iterator must have the following properties: diff --git a/crates/ruff_db/src/system/memory_fs.rs b/crates/ruff_db/src/system/memory_fs.rs index ba3157cfb0..0c70c09c48 100644 --- a/crates/ruff_db/src/system/memory_fs.rs +++ b/crates/ruff_db/src/system/memory_fs.rs @@ -6,8 +6,6 @@ use camino::{Utf8Path, Utf8PathBuf}; use filetime::FileTime; use rustc_hash::FxHashMap; -use ruff_notebook::{Notebook, NotebookError}; - use crate::system::{ walk_directory, DirectoryEntry, FileType, GlobError, GlobErrorKind, Metadata, Result, SystemPath, SystemPathBuf, SystemVirtualPath, SystemVirtualPathBuf, @@ -131,14 +129,6 @@ impl MemoryFileSystem { read_to_string(self, path.as_ref()) } - pub(crate) fn read_to_notebook( - &self, - path: impl AsRef, - ) -> std::result::Result { - let content = self.read_to_string(path)?; - ruff_notebook::Notebook::from_source_code(&content) - } - pub(crate) fn read_virtual_path_to_string( &self, path: impl AsRef, @@ -151,14 +141,6 @@ impl MemoryFileSystem { Ok(file.content.clone()) } - pub(crate) fn read_virtual_path_to_notebook( - &self, - path: &SystemVirtualPath, - ) -> std::result::Result { - let content = self.read_virtual_path_to_string(path)?; - ruff_notebook::Notebook::from_source_code(&content) - } - pub fn exists(&self, path: &SystemPath) -> bool { let by_path = self.inner.by_path.read().unwrap(); let normalized = self.normalize_path(path); diff --git a/crates/ruff_db/src/system/os.rs b/crates/ruff_db/src/system/os.rs index 843558f8f6..fea57fea16 100644 --- a/crates/ruff_db/src/system/os.rs +++ b/crates/ruff_db/src/system/os.rs @@ -98,6 +98,21 @@ impl System for OsSystem { &self.inner.cwd } + #[cfg(not(target_arch = "wasm32"))] + fn user_config_directory(&self) -> Option { + use etcetera::BaseStrategy as _; + + let strategy = etcetera::base_strategy::choose_base_strategy().ok()?; + SystemPathBuf::from_path_buf(strategy.config_dir()).ok() + } + + // TODO: Remove this feature gating once `ruff_wasm` no longer indirectly depends on `ruff_db` with the + // `os` feature enabled (via `ruff_workspace` -> `ruff_graph` -> `ruff_db`). + #[cfg(target_arch = "wasm32")] + fn user_config_directory(&self) -> Option { + None + } + /// Creates a builder to recursively walk `path`. /// /// The walker ignores files according to [`ignore::WalkBuilder::standard_filters`] diff --git a/crates/ruff_db/src/system/test.rs b/crates/ruff_db/src/system/test.rs index bb15ffe62b..fa199fb97e 100644 --- a/crates/ruff_db/src/system/test.rs +++ b/crates/ruff_db/src/system/test.rs @@ -1,9 +1,8 @@ use glob::PatternError; use ruff_notebook::{Notebook, NotebookError}; use ruff_python_trivia::textwrap; -use std::any::Any; use std::panic::RefUnwindSafe; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use crate::files::File; use crate::system::{ @@ -21,104 +20,91 @@ use super::walk_directory::WalkDirectoryBuilder; /// /// ## Warning /// Don't use this system for production code. It's intended for testing only. -#[derive(Default, Debug, Clone)] +#[derive(Debug, Clone)] pub struct TestSystem { - inner: TestSystemInner, + inner: Arc, } impl TestSystem { + /// Returns the [`InMemorySystem`]. + /// + /// ## Panics + /// If the underlying test system isn't the [`InMemorySystem`]. + pub fn in_memory(&self) -> &InMemorySystem { + self.as_in_memory() + .expect("The test db is not using a memory file system") + } + + /// Returns the `InMemorySystem` or `None` if the underlying test system isn't the [`InMemorySystem`]. + pub fn as_in_memory(&self) -> Option<&InMemorySystem> { + self.system().as_any().downcast_ref::() + } + /// Returns the memory file system. /// /// ## Panics - /// If this test db isn't using a memory file system. + /// If the underlying test system isn't the [`InMemorySystem`]. pub fn memory_file_system(&self) -> &MemoryFileSystem { - if let TestSystemInner::Stub(fs) = &self.inner { - fs - } else { - panic!("The test db is not using a memory file system"); - } + self.in_memory().fs() } fn use_system(&mut self, system: S) where S: System + Send + Sync + RefUnwindSafe + 'static, { - self.inner = TestSystemInner::System(Arc::new(system)); + self.inner = Arc::new(system); + } + + pub fn system(&self) -> &dyn System { + &*self.inner } } impl System for TestSystem { - fn path_metadata(&self, path: &SystemPath) -> crate::system::Result { - match &self.inner { - TestSystemInner::Stub(fs) => fs.metadata(path), - TestSystemInner::System(system) => system.path_metadata(path), - } + fn path_metadata(&self, path: &SystemPath) -> Result { + self.system().path_metadata(path) } - fn read_to_string(&self, path: &SystemPath) -> crate::system::Result { - match &self.inner { - TestSystemInner::Stub(fs) => fs.read_to_string(path), - TestSystemInner::System(system) => system.read_to_string(path), - } + fn canonicalize_path(&self, path: &SystemPath) -> Result { + self.system().canonicalize_path(path) + } + + fn read_to_string(&self, path: &SystemPath) -> Result { + self.system().read_to_string(path) } fn read_to_notebook(&self, path: &SystemPath) -> std::result::Result { - match &self.inner { - TestSystemInner::Stub(fs) => fs.read_to_notebook(path), - TestSystemInner::System(system) => system.read_to_notebook(path), - } + self.system().read_to_notebook(path) } fn read_virtual_path_to_string(&self, path: &SystemVirtualPath) -> Result { - match &self.inner { - TestSystemInner::Stub(fs) => fs.read_virtual_path_to_string(path), - TestSystemInner::System(system) => system.read_virtual_path_to_string(path), - } + self.system().read_virtual_path_to_string(path) } fn read_virtual_path_to_notebook( &self, path: &SystemVirtualPath, ) -> std::result::Result { - match &self.inner { - TestSystemInner::Stub(fs) => fs.read_virtual_path_to_notebook(path), - TestSystemInner::System(system) => system.read_virtual_path_to_notebook(path), - } - } - - fn path_exists(&self, path: &SystemPath) -> bool { - match &self.inner { - TestSystemInner::Stub(fs) => fs.exists(path), - TestSystemInner::System(system) => system.path_exists(path), - } - } - - fn is_directory(&self, path: &SystemPath) -> bool { - match &self.inner { - TestSystemInner::Stub(fs) => fs.is_directory(path), - TestSystemInner::System(system) => system.is_directory(path), - } - } - - fn is_file(&self, path: &SystemPath) -> bool { - match &self.inner { - TestSystemInner::Stub(fs) => fs.is_file(path), - TestSystemInner::System(system) => system.is_file(path), - } + self.system().read_virtual_path_to_notebook(path) } fn current_directory(&self) -> &SystemPath { - match &self.inner { - TestSystemInner::Stub(fs) => fs.current_directory(), - TestSystemInner::System(system) => system.current_directory(), - } + self.system().current_directory() + } + + fn user_config_directory(&self) -> Option { + self.system().user_config_directory() + } + + fn read_directory<'a>( + &'a self, + path: &SystemPath, + ) -> Result> + 'a>> { + self.system().read_directory(path) } fn walk_directory(&self, path: &SystemPath) -> WalkDirectoryBuilder { - match &self.inner { - TestSystemInner::Stub(fs) => fs.walk_directory(path), - TestSystemInner::System(system) => system.walk_directory(path), - } + self.system().walk_directory(path) } fn glob( @@ -128,37 +114,22 @@ impl System for TestSystem { Box>>, PatternError, > { - match &self.inner { - TestSystemInner::Stub(fs) => { - let iterator = fs.glob(pattern)?; - Ok(Box::new(iterator)) - } - TestSystemInner::System(system) => system.glob(pattern), - } + self.system().glob(pattern) } - fn as_any(&self) -> &dyn Any { + fn as_any(&self) -> &dyn std::any::Any { self } - fn as_any_mut(&mut self) -> &mut dyn Any { + fn as_any_mut(&mut self) -> &mut dyn std::any::Any { self } +} - fn read_directory<'a>( - &'a self, - path: &SystemPath, - ) -> Result> + 'a>> { - match &self.inner { - TestSystemInner::System(fs) => fs.read_directory(path), - TestSystemInner::Stub(fs) => Ok(Box::new(fs.read_directory(path)?)), - } - } - - fn canonicalize_path(&self, path: &SystemPath) -> Result { - match &self.inner { - TestSystemInner::System(fs) => fs.canonicalize_path(path), - TestSystemInner::Stub(fs) => fs.canonicalize(path), +impl Default for TestSystem { + fn default() -> Self { + Self { + inner: Arc::new(InMemorySystem::default()), } } } @@ -173,8 +144,8 @@ pub trait DbWithTestSystem: Db + Sized { /// Writes the content of the given file and notifies the Db about the change. /// - /// # Panics - /// If the system isn't using the memory file system. + /// ## Panics + /// If the db isn't using the [`InMemorySystem`]. fn write_file(&mut self, path: impl AsRef, content: impl ToString) -> Result<()> { let path = path.as_ref(); @@ -201,6 +172,9 @@ pub trait DbWithTestSystem: Db + Sized { } /// Writes the content of the given virtual file. + /// + /// ## Panics + /// If the db isn't using the [`InMemorySystem`]. fn write_virtual_file(&mut self, path: impl AsRef, content: impl ToString) { let path = path.as_ref(); self.test_system() @@ -209,6 +183,9 @@ pub trait DbWithTestSystem: Db + Sized { } /// Writes auto-dedented text to a file. + /// + /// ## Panics + /// If the db isn't using the [`InMemorySystem`]. fn write_dedented(&mut self, path: &str, content: &str) -> crate::system::Result<()> { self.write_file(path, textwrap::dedent(content))?; Ok(()) @@ -216,8 +193,8 @@ pub trait DbWithTestSystem: Db + Sized { /// Writes the content of the given files and notifies the Db about the change. /// - /// # Panics - /// If the system isn't using the memory file system for testing. + /// ## Panics + /// If the db isn't using the [`InMemorySystem`]. fn write_files(&mut self, files: I) -> crate::system::Result<()> where I: IntoIterator, @@ -246,20 +223,94 @@ pub trait DbWithTestSystem: Db + Sized { /// Returns the memory file system. /// /// ## Panics - /// If this system isn't using a memory file system. + /// If the underlying test system isn't the [`InMemorySystem`]. fn memory_file_system(&self) -> &MemoryFileSystem { self.test_system().memory_file_system() } } -#[derive(Debug, Clone)] -enum TestSystemInner { - Stub(MemoryFileSystem), - System(Arc), +#[derive(Default, Debug)] +pub struct InMemorySystem { + user_config_directory: Mutex>, + memory_fs: MemoryFileSystem, } -impl Default for TestSystemInner { - fn default() -> Self { - Self::Stub(MemoryFileSystem::default()) +impl InMemorySystem { + pub fn fs(&self) -> &MemoryFileSystem { + &self.memory_fs + } + + pub fn set_user_configuration_directory(&self, directory: Option) { + let mut user_directory = self.user_config_directory.lock().unwrap(); + *user_directory = directory; + } +} + +impl System for InMemorySystem { + fn path_metadata(&self, path: &SystemPath) -> Result { + self.memory_fs.metadata(path) + } + + fn canonicalize_path(&self, path: &SystemPath) -> Result { + self.memory_fs.canonicalize(path) + } + + fn read_to_string(&self, path: &SystemPath) -> Result { + self.memory_fs.read_to_string(path) + } + + fn read_to_notebook(&self, path: &SystemPath) -> std::result::Result { + let content = self.read_to_string(path)?; + Notebook::from_source_code(&content) + } + + fn read_virtual_path_to_string(&self, path: &SystemVirtualPath) -> Result { + self.memory_fs.read_virtual_path_to_string(path) + } + + fn read_virtual_path_to_notebook( + &self, + path: &SystemVirtualPath, + ) -> std::result::Result { + let content = self.read_virtual_path_to_string(path)?; + Notebook::from_source_code(&content) + } + + fn current_directory(&self) -> &SystemPath { + self.memory_fs.current_directory() + } + + fn user_config_directory(&self) -> Option { + self.user_config_directory.lock().unwrap().clone() + } + + fn read_directory<'a>( + &'a self, + path: &SystemPath, + ) -> Result> + 'a>> { + Ok(Box::new(self.memory_fs.read_directory(path)?)) + } + + fn walk_directory(&self, path: &SystemPath) -> WalkDirectoryBuilder { + self.memory_fs.walk_directory(path) + } + + fn glob( + &self, + pattern: &str, + ) -> std::result::Result< + Box>>, + PatternError, + > { + let iterator = self.memory_fs.glob(pattern)?; + Ok(Box::new(iterator)) + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } + + fn as_any_mut(&mut self) -> &mut dyn std::any::Any { + self } }