From f7819e553f0dbb22afcf31ea77938d529fe52818 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Feb 2025 14:50:55 +0000 Subject: [PATCH 01/12] 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 | 239 +++++++++++++-------- 12 files changed, 183 insertions(+), 119 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 288a668931e9b..b4df9d62503f8 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 43e1b2d190d8b..ea31f99f79a36 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 cafd00e9c050b..47847e8307cc8 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 87e8126b8cd03..f14aa19350910 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 e2d44385677d3..faf13e6e1eafd 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 d6363f144695c..7cf08388348a2 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 ad9d422566b38..6801e566ebafa 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 b5443ceeb9a04..d771fd8a8a7c1 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 4a7edc8d0b857..91bcb03756829 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 ba3157cfb0390..0c70c09c486e6 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 843558f8f631d..fea57fea16265 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 bb15ffe62bea9..fa199fb97ea0f 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), - } + self.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 current_directory(&self) -> &SystemPath { + self.system().current_directory() } - fn is_file(&self, path: &SystemPath) -> bool { - match &self.inner { - TestSystemInner::Stub(fs) => fs.is_file(path), - TestSystemInner::System(system) => system.is_file(path), - } + fn user_config_directory(&self) -> Option { + self.system().user_config_directory() } - fn current_directory(&self) -> &SystemPath { - match &self.inner { - TestSystemInner::Stub(fs) => fs.current_directory(), - TestSystemInner::System(system) => system.current_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 } } From af832560fc6986580927c7654f2634c3e8dce80e Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Feb 2025 15:44:23 +0000 Subject: [PATCH 02/12] [red-knot] User-level configuration (#16021) ## Summary This PR adds support for user-level configurations (`~/.config/knot/knot.toml`) to Red Knot. Red Knot will watch the user-level configuration file for changes but only if it exists when the process start. It doesn't watch for new configurations, mainly to simplify things for now (it would require watching the entire `.config` directory because the `knot` subfolder might not exist either). The new `ConfigurationFile` struct seems a bit overkill for now but I plan to use it for hierarchical configurations as well. Red Knot uses the same strategy as uv and Ruff by using the etcetera crate. ## Test Plan Added CLI and file watching test --- crates/red_knot/src/main.rs | 7 +- crates/red_knot/tests/cli.rs | 111 ++++++++- crates/red_knot/tests/file_watching.rs | 217 ++++++++++++++---- crates/red_knot_project/src/db/changes.rs | 20 +- crates/red_knot_project/src/metadata.rs | 45 +++- .../src/metadata/configuration_file.rs | 69 ++++++ .../src/watch/project_watcher.rs | 14 +- crates/red_knot_python_semantic/Cargo.toml | 2 +- crates/red_knot_server/src/session.rs | 4 +- crates/ruff_db/src/system.rs | 5 + crates/ruff_db/src/system/os.rs | 80 +++++++ 11 files changed, 511 insertions(+), 63 deletions(-) create mode 100644 crates/red_knot_project/src/metadata/configuration_file.rs diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 3c1751e5408cc..a8bb2850f8523 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -99,10 +99,11 @@ fn run_check(args: CheckCommand) -> anyhow::Result { let exit_zero = args.exit_zero; let cli_options = args.into_options(); - let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; - workspace_metadata.apply_cli_options(cli_options.clone()); + let mut project_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; + project_metadata.apply_cli_options(cli_options.clone()); + project_metadata.apply_configuration_files(&system)?; - let mut db = ProjectDatabase::new(workspace_metadata, system)?; + let mut db = ProjectDatabase::new(project_metadata, system)?; let (main_loop, main_loop_cancellation_token) = MainLoop::new(cli_options); diff --git a/crates/red_knot/tests/cli.rs b/crates/red_knot/tests/cli.rs index 28360dfc878a1..3ac2d260bf8a5 100644 --- a/crates/red_knot/tests/cli.rs +++ b/crates/red_knot/tests/cli.rs @@ -98,7 +98,7 @@ fn cli_arguments_are_relative_to_the_current_directory() -> anyhow::Result<()> { ])?; // Make sure that the CLI fails when the `libs` directory is not in the search path. - assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")), @r###" + assert_cmd_snapshot!(case.command().current_dir(case.root().join("child")), @r###" success: false exit_code: 1 ----- stdout ----- @@ -115,7 +115,7 @@ fn cli_arguments_are_relative_to_the_current_directory() -> anyhow::Result<()> { ----- stderr ----- "###); - assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")).arg("--extra-search-path").arg("../libs"), @r" + assert_cmd_snapshot!(case.command().current_dir(case.root().join("child")).arg("--extra-search-path").arg("../libs"), @r" success: true exit_code: 0 ----- stdout ----- @@ -167,7 +167,7 @@ fn paths_in_configuration_files_are_relative_to_the_project_root() -> anyhow::Re ), ])?; - assert_cmd_snapshot!(case.command().current_dir(case.project_dir().join("child")), @r" + assert_cmd_snapshot!(case.command().current_dir(case.root().join("child")), @r" success: true exit_code: 0 ----- stdout ----- @@ -717,6 +717,109 @@ fn exit_code_exit_zero_is_true() -> anyhow::Result<()> { Ok(()) } +#[test] +fn user_configuration() -> anyhow::Result<()> { + let case = TestCase::with_files([ + ( + "project/knot.toml", + r#" + [rules] + division-by-zero = "warn" + "#, + ), + ( + "project/main.py", + r#" + y = 4 / 0 + + for a in range(0, y): + x = a + + print(x) + "#, + ), + ])?; + + let config_directory = case.root().join("home/.config"); + let config_env_var = if cfg!(windows) { + "APPDATA" + } else { + "XDG_CONFIG_HOME" + }; + + assert_cmd_snapshot!( + case.command().current_dir(case.root().join("project")).env(config_env_var, config_directory.as_os_str()), + @r###" + success: true + exit_code: 0 + ----- stdout ----- + warning: lint:division-by-zero + --> /project/main.py:2:5 + | + 2 | y = 4 / 0 + | ----- Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + | + + warning: lint:possibly-unresolved-reference + --> /project/main.py:7:7 + | + 5 | x = a + 6 | + 7 | print(x) + | - Name `x` used when possibly not defined + | + + + ----- stderr ----- + "### + ); + + // The user-level configuration promotes `possibly-unresolved-reference` to an error. + // Changing the level for `division-by-zero` has no effect, because the project-level configuration + // has higher precedence. + case.write_file( + config_directory.join("knot/knot.toml"), + r#" + [rules] + division-by-zero = "error" + possibly-unresolved-reference = "error" + "#, + )?; + + assert_cmd_snapshot!( + case.command().current_dir(case.root().join("project")).env(config_env_var, config_directory.as_os_str()), + @r###" + success: false + exit_code: 1 + ----- stdout ----- + warning: lint:division-by-zero + --> /project/main.py:2:5 + | + 2 | y = 4 / 0 + | ----- Cannot divide object of type `Literal[4]` by zero + 3 | + 4 | for a in range(0, y): + | + + error: lint:possibly-unresolved-reference + --> /project/main.py:7:7 + | + 5 | x = a + 6 | + 7 | print(x) + | ^ Name `x` used when possibly not defined + | + + + ----- stderr ----- + "### + ); + + Ok(()) +} + struct TestCase { _temp_dir: TempDir, _settings_scope: SettingsBindDropGuard, @@ -784,7 +887,7 @@ impl TestCase { Ok(()) } - fn project_dir(&self) -> &Path { + fn root(&self) -> &Path { &self.project_dir } diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index eaad5ed0dd87f..ff1f21b982465 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -12,7 +12,9 @@ use red_knot_project::{Db, ProjectDatabase, ProjectMetadata}; use red_knot_python_semantic::{resolve_module, ModuleName, PythonPlatform, PythonVersion}; use ruff_db::files::{system_path_to_file, File, FileError}; use ruff_db::source::source_text; -use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf}; +use ruff_db::system::{ + OsSystem, System, SystemPath, SystemPathBuf, UserConfigDirectoryOverrideGuard, +}; use ruff_db::Upcast; struct TestCase { @@ -220,17 +222,44 @@ where } trait SetupFiles { - fn setup(self, root_path: &SystemPath, project_path: &SystemPath) -> anyhow::Result<()>; + fn setup(self, context: &SetupContext) -> anyhow::Result<()>; +} + +struct SetupContext<'a> { + system: &'a OsSystem, + root_path: &'a SystemPath, +} + +impl<'a> SetupContext<'a> { + fn system(&self) -> &'a OsSystem { + self.system + } + + fn join_project_path(&self, relative: impl AsRef) -> SystemPathBuf { + self.project_path().join(relative) + } + + fn project_path(&self) -> &SystemPath { + self.system.current_directory() + } + + fn root_path(&self) -> &'a SystemPath { + self.root_path + } + + fn join_root_path(&self, relative: impl AsRef) -> SystemPathBuf { + self.root_path().join(relative) + } } impl SetupFiles for [(P, &'static str); N] where P: AsRef, { - fn setup(self, _root_path: &SystemPath, project_path: &SystemPath) -> anyhow::Result<()> { + fn setup(self, context: &SetupContext) -> anyhow::Result<()> { for (relative_path, content) in self { let relative_path = relative_path.as_ref(); - let absolute_path = project_path.join(relative_path); + let absolute_path = context.join_project_path(relative_path); if let Some(parent) = absolute_path.parent() { std::fs::create_dir_all(parent).with_context(|| { format!("Failed to create parent directory for file `{relative_path}`") @@ -250,10 +279,10 @@ where impl SetupFiles for F where - F: FnOnce(&SystemPath, &SystemPath) -> anyhow::Result<()>, + F: FnOnce(&SetupContext) -> anyhow::Result<()>, { - fn setup(self, root_path: &SystemPath, project_path: &SystemPath) -> anyhow::Result<()> { - self(root_path, project_path) + fn setup(self, context: &SetupContext) -> anyhow::Result<()> { + self(context) } } @@ -261,13 +290,12 @@ fn setup(setup_files: F) -> anyhow::Result where F: SetupFiles, { - setup_with_options(setup_files, |_root, _project_path| None) + setup_with_options(setup_files, |_context| None) } -// TODO: Replace with configuration? fn setup_with_options( setup_files: F, - create_options: impl FnOnce(&SystemPath, &SystemPath) -> Option, + create_options: impl FnOnce(&SetupContext) -> Option, ) -> anyhow::Result where F: SetupFiles, @@ -295,13 +323,17 @@ where std::fs::create_dir_all(project_path.as_std_path()) .with_context(|| format!("Failed to create project directory `{project_path}`"))?; + let system = OsSystem::new(&project_path); + let setup_context = SetupContext { + system: &system, + root_path: &root_path, + }; + setup_files - .setup(&root_path, &project_path) + .setup(&setup_context) .context("Failed to setup test files")?; - let system = OsSystem::new(&project_path); - - if let Some(options) = create_options(&root_path, &project_path) { + if let Some(options) = create_options(&setup_context) { std::fs::write( project_path.join("pyproject.toml").as_std_path(), toml::to_string(&PyProject { @@ -315,7 +347,9 @@ where .context("Failed to write configuration")?; } - let project = ProjectMetadata::discover(&project_path, &system)?; + let mut project = ProjectMetadata::discover(&project_path, &system)?; + project.apply_configuration_files(&system)?; + let program_settings = project.to_program_settings(&system); for path in program_settings @@ -789,10 +823,12 @@ fn directory_deleted() -> anyhow::Result<()> { #[test] fn search_path() -> anyhow::Result<()> { - let mut case = setup_with_options([("bar.py", "import sub.a")], |root_path, _project_path| { + let mut case = setup_with_options([("bar.py", "import sub.a")], |context| { Some(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![RelativePathBuf::cli(root_path.join("site_packages"))]), + extra_paths: Some(vec![RelativePathBuf::cli( + context.join_root_path("site_packages"), + )]), ..EnvironmentOptions::default() }), ..Options::default() @@ -853,10 +889,12 @@ fn add_search_path() -> anyhow::Result<()> { #[test] fn remove_search_path() -> anyhow::Result<()> { - let mut case = setup_with_options([("bar.py", "import sub.a")], |root_path, _project_path| { + let mut case = setup_with_options([("bar.py", "import sub.a")], |context| { Some(Options { environment: Some(EnvironmentOptions { - extra_paths: Some(vec![RelativePathBuf::cli(root_path.join("site_packages"))]), + extra_paths: Some(vec![RelativePathBuf::cli( + context.join_root_path("site_packages"), + )]), ..EnvironmentOptions::default() }), ..Options::default() @@ -894,7 +932,7 @@ import os print(sys.last_exc, os.getegid()) "#, )], - |_root_path, _project_path| { + |_context| { Some(Options { environment: Some(EnvironmentOptions { python_version: Some(RangedValue::cli(PythonVersion::PY311)), @@ -942,21 +980,31 @@ print(sys.last_exc, os.getegid()) #[test] fn changed_versions_file() -> anyhow::Result<()> { let mut case = setup_with_options( - |root_path: &SystemPath, project_path: &SystemPath| { - std::fs::write(project_path.join("bar.py").as_std_path(), "import sub.a")?; - std::fs::create_dir_all(root_path.join("typeshed/stdlib").as_std_path())?; - std::fs::write(root_path.join("typeshed/stdlib/VERSIONS").as_std_path(), "")?; + |context: &SetupContext| { + std::fs::write( + context.join_project_path("bar.py").as_std_path(), + "import sub.a", + )?; + std::fs::create_dir_all(context.join_root_path("typeshed/stdlib").as_std_path())?; + std::fs::write( + context + .join_root_path("typeshed/stdlib/VERSIONS") + .as_std_path(), + "", + )?; std::fs::write( - root_path.join("typeshed/stdlib/os.pyi").as_std_path(), + context + .join_root_path("typeshed/stdlib/os.pyi") + .as_std_path(), "# not important", )?; Ok(()) }, - |root_path, _project_path| { + |context| { Some(Options { environment: Some(EnvironmentOptions { - typeshed: Some(RelativePathBuf::cli(root_path.join("typeshed"))), + typeshed: Some(RelativePathBuf::cli(context.join_root_path("typeshed"))), ..EnvironmentOptions::default() }), ..Options::default() @@ -1007,12 +1055,12 @@ fn changed_versions_file() -> anyhow::Result<()> { /// we're seeing is that Windows only emits a single event, similar to Linux. #[test] fn hard_links_in_project() -> anyhow::Result<()> { - let mut case = setup(|_root: &SystemPath, project: &SystemPath| { - let foo_path = project.join("foo.py"); + let mut case = setup(|context: &SetupContext| { + let foo_path = context.join_project_path("foo.py"); std::fs::write(foo_path.as_std_path(), "print('Version 1')")?; // Create a hardlink to `foo` - let bar_path = project.join("bar.py"); + let bar_path = context.join_project_path("bar.py"); std::fs::hard_link(foo_path.as_std_path(), bar_path.as_std_path()) .context("Failed to create hard link from foo.py -> bar.py")?; @@ -1078,12 +1126,12 @@ fn hard_links_in_project() -> anyhow::Result<()> { ignore = "windows doesn't support observing changes to hard linked files." )] fn hard_links_to_target_outside_project() -> anyhow::Result<()> { - let mut case = setup(|root: &SystemPath, project: &SystemPath| { - let foo_path = root.join("foo.py"); + let mut case = setup(|context: &SetupContext| { + let foo_path = context.join_root_path("foo.py"); std::fs::write(foo_path.as_std_path(), "print('Version 1')")?; // Create a hardlink to `foo` - let bar_path = project.join("bar.py"); + let bar_path = context.join_project_path("bar.py"); std::fs::hard_link(foo_path.as_std_path(), bar_path.as_std_path()) .context("Failed to create hard link from foo.py -> bar.py")?; @@ -1186,9 +1234,9 @@ mod unix { ignore = "FSEvents doesn't emit change events for symlinked directories outside of the watched paths." )] fn symlink_target_outside_watched_paths() -> anyhow::Result<()> { - let mut case = setup(|root: &SystemPath, project: &SystemPath| { + let mut case = setup(|context: &SetupContext| { // Set up the symlink target. - let link_target = root.join("bar"); + let link_target = context.join_root_path("bar"); std::fs::create_dir_all(link_target.as_std_path()) .context("Failed to create link target directory")?; let baz_original = link_target.join("baz.py"); @@ -1196,7 +1244,7 @@ mod unix { .context("Failed to write link target file")?; // Create a symlink inside the project - let bar = project.join("bar"); + let bar = context.join_project_path("bar"); std::os::unix::fs::symlink(link_target.as_std_path(), bar.as_std_path()) .context("Failed to create symlink to bar package")?; @@ -1267,9 +1315,9 @@ mod unix { /// ``` #[test] fn symlink_inside_project() -> anyhow::Result<()> { - let mut case = setup(|_root: &SystemPath, project: &SystemPath| { + let mut case = setup(|context: &SetupContext| { // Set up the symlink target. - let link_target = project.join("patched/bar"); + let link_target = context.join_project_path("patched/bar"); std::fs::create_dir_all(link_target.as_std_path()) .context("Failed to create link target directory")?; let baz_original = link_target.join("baz.py"); @@ -1277,7 +1325,7 @@ mod unix { .context("Failed to write link target file")?; // Create a symlink inside site-packages - let bar_in_project = project.join("bar"); + let bar_in_project = context.join_project_path("bar"); std::os::unix::fs::symlink(link_target.as_std_path(), bar_in_project.as_std_path()) .context("Failed to create symlink to bar package")?; @@ -1358,9 +1406,9 @@ mod unix { #[test] fn symlinked_module_search_path() -> anyhow::Result<()> { let mut case = setup_with_options( - |root: &SystemPath, project: &SystemPath| { + |context: &SetupContext| { // Set up the symlink target. - let site_packages = root.join("site-packages"); + let site_packages = context.join_root_path("site-packages"); let bar = site_packages.join("bar"); std::fs::create_dir_all(bar.as_std_path()) .context("Failed to create bar directory")?; @@ -1369,7 +1417,8 @@ mod unix { .context("Failed to write baz.py")?; // Symlink the site packages in the venv to the global site packages - let venv_site_packages = project.join(".venv/lib/python3.12/site-packages"); + let venv_site_packages = + context.join_project_path(".venv/lib/python3.12/site-packages"); std::fs::create_dir_all(venv_site_packages.parent().unwrap()) .context("Failed to create .venv directory")?; std::os::unix::fs::symlink( @@ -1380,7 +1429,7 @@ mod unix { Ok(()) }, - |_root, _project| { + |_context| { Some(Options { environment: Some(EnvironmentOptions { extra_paths: Some(vec![RelativePathBuf::cli( @@ -1450,9 +1499,9 @@ mod unix { #[test] fn nested_projects_delete_root() -> anyhow::Result<()> { - let mut case = setup(|root: &SystemPath, project_root: &SystemPath| { + let mut case = setup(|context: &SetupContext| { std::fs::write( - project_root.join("pyproject.toml").as_std_path(), + context.join_project_path("pyproject.toml").as_std_path(), r#" [project] name = "inner" @@ -1462,7 +1511,7 @@ fn nested_projects_delete_root() -> anyhow::Result<()> { )?; std::fs::write( - root.join("pyproject.toml").as_std_path(), + context.join_root_path("pyproject.toml").as_std_path(), r#" [project] name = "outer" @@ -1487,3 +1536,79 @@ fn nested_projects_delete_root() -> anyhow::Result<()> { Ok(()) } + +#[test] +fn changes_to_user_configuration() -> anyhow::Result<()> { + let mut _config_dir_override: Option = None; + + let mut case = setup(|context: &SetupContext| { + std::fs::write( + context.join_project_path("pyproject.toml").as_std_path(), + r#" + [project] + name = "test" + "#, + )?; + + std::fs::write( + context.join_project_path("foo.py").as_std_path(), + "a = 10 / 0", + )?; + + let config_directory = context.join_root_path("home/.config"); + std::fs::create_dir_all(config_directory.join("knot").as_std_path())?; + std::fs::write( + config_directory.join("knot/knot.toml").as_std_path(), + r#" + [rules] + division-by-zero = "ignore" + "#, + )?; + + _config_dir_override = Some( + context + .system() + .with_user_config_directory(Some(config_directory)), + ); + + Ok(()) + })?; + + let foo = case + .system_file(case.project_path("foo.py")) + .expect("foo.py to exist"); + let diagnostics = case + .db() + .check_file(foo) + .context("Failed to check project.")?; + + assert!( + diagnostics.is_empty(), + "Expected no diagnostics but got: {diagnostics:#?}" + ); + + // Enable division-by-zero in the user configuration with warning severity + update_file( + case.root_path().join("home/.config/knot/knot.toml"), + r#" + [rules] + division-by-zero = "warn" + "#, + )?; + + let changes = case.stop_watch(event_for_file("knot.toml")); + + case.apply_changes(changes); + + let diagnostics = case + .db() + .check_file(foo) + .context("Failed to check project.")?; + + assert!( + diagnostics.len() == 1, + "Expected exactly one diagnostic but got: {diagnostics:#?}" + ); + + Ok(()) +} diff --git a/crates/red_knot_project/src/db/changes.rs b/crates/red_knot_project/src/db/changes.rs index a1caf26376128..8c7111ed43aba 100644 --- a/crates/red_knot_project/src/db/changes.rs +++ b/crates/red_knot_project/src/db/changes.rs @@ -8,6 +8,7 @@ use ruff_db::files::{system_path_to_file, File, Files}; use ruff_db::system::walk_directory::WalkState; use ruff_db::system::SystemPath; use ruff_db::Db as _; +use ruff_python_ast::PySourceType; use rustc_hash::FxHashSet; impl ProjectDatabase { @@ -47,7 +48,7 @@ impl ProjectDatabase { if let Some(path) = change.system_path() { if matches!( path.file_name(), - Some(".gitignore" | ".ignore" | "ruff.toml" | ".ruff.toml" | "pyproject.toml") + Some(".gitignore" | ".ignore" | "knot.toml" | "pyproject.toml") ) { // Changes to ignore files or settings can change the project structure or add/remove files. project_changed = true; @@ -144,6 +145,12 @@ impl ProjectDatabase { metadata.apply_cli_options(cli_options.clone()); } + if let Err(error) = metadata.apply_configuration_files(self.system()) { + tracing::error!( + "Failed to apply configuration files, continuing without applying them: {error}" + ); + } + let program_settings = metadata.to_program_settings(self.system()); let program = Program::get(self); @@ -201,9 +208,16 @@ impl ProjectDatabase { return WalkState::Continue; } - let mut paths = added_paths.lock().unwrap(); + if entry + .path() + .extension() + .and_then(PySourceType::try_from_extension) + .is_some() + { + let mut paths = added_paths.lock().unwrap(); - paths.push(entry.into_path()); + paths.push(entry.into_path()); + } WalkState::Continue }) diff --git a/crates/red_knot_project/src/metadata.rs b/crates/red_knot_project/src/metadata.rs index c7f2009c0e244..38217e89791f5 100644 --- a/crates/red_knot_project/src/metadata.rs +++ b/crates/red_knot_project/src/metadata.rs @@ -1,3 +1,4 @@ +use configuration_file::{ConfigurationFile, ConfigurationFileError}; use red_knot_python_semantic::ProgramSettings; use ruff_db::system::{System, SystemPath, SystemPathBuf}; use ruff_python_ast::name::Name; @@ -10,6 +11,7 @@ use crate::metadata::value::ValueSource; use options::KnotTomlError; use options::Options; +mod configuration_file; pub mod options; pub mod pyproject; pub mod settings; @@ -24,6 +26,15 @@ pub struct ProjectMetadata { /// The raw options pub(super) options: Options, + + /// Paths of configurations other than the project's configuration that were combined into [`Self::options`]. + /// + /// This field stores the paths of the configuration files, mainly for + /// knowing which files to watch for changes. + /// + /// The path ordering doesn't imply precedence. + #[cfg_attr(test, serde(skip_serializing_if = "Vec::is_empty"))] + pub(super) extra_configuration_paths: Vec, } impl ProjectMetadata { @@ -32,6 +43,7 @@ impl ProjectMetadata { Self { name, root, + extra_configuration_paths: Vec::default(), options: Options::default(), } } @@ -64,6 +76,7 @@ impl ProjectMetadata { name, root, options, + extra_configuration_paths: Vec::new(), } } @@ -192,6 +205,10 @@ impl ProjectMetadata { &self.options } + pub fn extra_configuration_paths(&self) -> &[SystemPathBuf] { + &self.extra_configuration_paths + } + pub fn to_program_settings(&self, system: &dyn System) -> ProgramSettings { self.options.to_program_settings(self.root(), system) } @@ -201,9 +218,31 @@ impl ProjectMetadata { self.options = options.combine(std::mem::take(&mut self.options)); } - /// Combine the project options with the user options where project options take precedence. - pub fn apply_user_options(&mut self, options: Options) { - self.options.combine_with(options); + /// Applies the options from the configuration files to the project's options. + /// + /// This includes: + /// + /// * The user-level configuration + pub fn apply_configuration_files( + &mut self, + system: &dyn System, + ) -> Result<(), ConfigurationFileError> { + if let Some(user) = ConfigurationFile::user(system)? { + tracing::debug!( + "Applying user-level configuration loaded from `{path}`.", + path = user.path() + ); + self.apply_configuration_file(user); + } + + Ok(()) + } + + /// Applies a lower-precedence configuration files to the project's options. + fn apply_configuration_file(&mut self, options: ConfigurationFile) { + self.extra_configuration_paths + .push(options.path().to_owned()); + self.options.combine_with(options.into_options()); } } diff --git a/crates/red_knot_project/src/metadata/configuration_file.rs b/crates/red_knot_project/src/metadata/configuration_file.rs new file mode 100644 index 0000000000000..03db373e36568 --- /dev/null +++ b/crates/red_knot_project/src/metadata/configuration_file.rs @@ -0,0 +1,69 @@ +use std::sync::Arc; + +use ruff_db::system::{System, SystemPath, SystemPathBuf}; +use thiserror::Error; + +use crate::metadata::value::ValueSource; + +use super::options::{KnotTomlError, Options}; + +/// A `knot.toml` configuration file with the options it contains. +pub(crate) struct ConfigurationFile { + path: SystemPathBuf, + options: Options, +} + +impl ConfigurationFile { + /// Loads the user-level configuration file if it exists. + /// + /// Returns `None` if the file does not exist or if the concept of user-level configurations + /// doesn't exist on `system`. + pub(crate) fn user(system: &dyn System) -> Result, ConfigurationFileError> { + let Some(configuration_directory) = system.user_config_directory() else { + return Ok(None); + }; + + let knot_toml_path = configuration_directory.join("knot").join("knot.toml"); + + tracing::debug!( + "Searching for a user-level configuration at `{path}`", + path = &knot_toml_path + ); + + let Ok(knot_toml_str) = system.read_to_string(&knot_toml_path) else { + return Ok(None); + }; + + match Options::from_toml_str( + &knot_toml_str, + ValueSource::File(Arc::new(knot_toml_path.clone())), + ) { + Ok(options) => Ok(Some(Self { + path: knot_toml_path, + options, + })), + Err(error) => Err(ConfigurationFileError::InvalidKnotToml { + source: Box::new(error), + path: knot_toml_path, + }), + } + } + + /// Returns the path to the configuration file. + pub(crate) fn path(&self) -> &SystemPath { + &self.path + } + + pub(crate) fn into_options(self) -> Options { + self.options + } +} + +#[derive(Debug, Error)] +pub enum ConfigurationFileError { + #[error("{path} is not a valid `knot.toml`: {source}")] + InvalidKnotToml { + source: Box, + path: SystemPathBuf, + }, +} diff --git a/crates/red_knot_project/src/watch/project_watcher.rs b/crates/red_knot_project/src/watch/project_watcher.rs index 25b4f6f08df3e..d9fb2d9610467 100644 --- a/crates/red_knot_project/src/watch/project_watcher.rs +++ b/crates/red_knot_project/src/watch/project_watcher.rs @@ -73,6 +73,13 @@ impl ProjectWatcher { .canonicalize_path(&project_path) .unwrap_or(project_path); + let config_paths = db + .project() + .metadata(db) + .extra_configuration_paths() + .iter() + .cloned(); + // Find the non-overlapping module search paths and filter out paths that are already covered by the project. // Module search paths are already canonicalized. let unique_module_paths = ruff_db::system::deduplicate_nested_paths( @@ -83,8 +90,11 @@ impl ProjectWatcher { .map(SystemPath::to_path_buf); // Now add the new paths, first starting with the project path and then - // adding the library search paths. - for path in std::iter::once(project_path).chain(unique_module_paths) { + // adding the library search paths, and finally the paths for configurations. + for path in std::iter::once(project_path) + .chain(unique_module_paths) + .chain(config_paths) + { // Log a warning. It's not worth aborting if registering a single folder fails because // Ruff otherwise stills works as expected. if let Err(error) = self.watcher.watch(&path) { diff --git a/crates/red_knot_python_semantic/Cargo.toml b/crates/red_knot_python_semantic/Cargo.toml index 47847e8307cc8..b12b3637dbdfd 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 = ["testing"] } +ruff_db = { workspace = true, features = ["testing", "os"] } ruff_python_parser = { workspace = true } red_knot_test = { workspace = true } red_knot_vendored = { workspace = true } diff --git a/crates/red_knot_server/src/session.rs b/crates/red_knot_server/src/session.rs index 3ad3869cc7519..6e370418bee7e 100644 --- a/crates/red_knot_server/src/session.rs +++ b/crates/red_knot_server/src/session.rs @@ -68,7 +68,9 @@ impl Session { let system = LSPSystem::new(index.clone()); // TODO(dhruvmanila): Get the values from the client settings - let metadata = ProjectMetadata::discover(system_path, &system)?; + let mut metadata = ProjectMetadata::discover(system_path, &system)?; + metadata.apply_configuration_files(&system)?; + // TODO(micha): Handle the case where the program settings are incorrect more gracefully. workspaces.insert(path, ProjectDatabase::new(metadata, system)?); } diff --git a/crates/ruff_db/src/system.rs b/crates/ruff_db/src/system.rs index 91bcb03756829..14946e8e59f76 100644 --- a/crates/ruff_db/src/system.rs +++ b/crates/ruff_db/src/system.rs @@ -1,7 +1,12 @@ pub use glob::PatternError; pub use memory_fs::MemoryFileSystem; + +#[cfg(all(feature = "testing", feature = "os"))] +pub use os::testing::UserConfigDirectoryOverrideGuard; + #[cfg(feature = "os")] pub use os::OsSystem; + use ruff_notebook::{Notebook, NotebookError}; use std::error::Error; use std::fmt::Debug; diff --git a/crates/ruff_db/src/system/os.rs b/crates/ruff_db/src/system/os.rs index fea57fea16265..d1703f0952e29 100644 --- a/crates/ruff_db/src/system/os.rs +++ b/crates/ruff_db/src/system/os.rs @@ -24,6 +24,11 @@ pub struct OsSystem { #[derive(Default, Debug)] struct OsSystemInner { cwd: SystemPathBuf, + + /// Overrides the user's configuration directory for testing. + /// This is an `Option>` to allow setting an override of `None`. + #[cfg(feature = "testing")] + user_config_directory_override: std::sync::Mutex>>, } impl OsSystem { @@ -32,8 +37,11 @@ impl OsSystem { assert!(cwd.as_utf8_path().is_absolute()); Self { + // Spreading `..Default` because it isn't possible to feature gate the initializer of a single field. + #[allow(clippy::needless_update)] inner: Arc::new(OsSystemInner { cwd: cwd.to_path_buf(), + ..Default::default() }), } } @@ -100,6 +108,15 @@ impl System for OsSystem { #[cfg(not(target_arch = "wasm32"))] fn user_config_directory(&self) -> Option { + // In testing, we allow overriding the user configuration directory by using a + // thread local because overriding the environment variables breaks test isolation + // (tests run concurrently) and mutating environment variable in a multithreaded + // application is inherently unsafe. + #[cfg(feature = "testing")] + if let Ok(directory_override) = self.try_get_user_config_directory_override() { + return directory_override; + } + use etcetera::BaseStrategy as _; let strategy = etcetera::base_strategy::choose_base_strategy().ok()?; @@ -110,6 +127,11 @@ impl System for OsSystem { // `os` feature enabled (via `ruff_workspace` -> `ruff_graph` -> `ruff_db`). #[cfg(target_arch = "wasm32")] fn user_config_directory(&self) -> Option { + #[cfg(feature = "testing")] + if let Ok(directory_override) = self.try_get_user_config_directory_override() { + return directory_override; + } + None } @@ -336,6 +358,64 @@ fn not_found() -> std::io::Error { std::io::Error::new(std::io::ErrorKind::NotFound, "No such file or directory") } +#[cfg(feature = "testing")] +pub(super) mod testing { + + use crate::system::{OsSystem, SystemPathBuf}; + + impl OsSystem { + /// Overrides the user configuration directory for the current scope + /// (for as long as the returned override is not dropped). + pub fn with_user_config_directory( + &self, + directory: Option, + ) -> UserConfigDirectoryOverrideGuard { + let mut directory_override = self.inner.user_config_directory_override.lock().unwrap(); + let previous = directory_override.replace(directory); + + UserConfigDirectoryOverrideGuard { + previous, + system: self.clone(), + } + } + + /// Returns [`Ok`] if any override is set and [`Err`] otherwise. + pub(super) fn try_get_user_config_directory_override( + &self, + ) -> Result, ()> { + let directory_override = self.inner.user_config_directory_override.lock().unwrap(); + match directory_override.as_ref() { + Some(directory_override) => Ok(directory_override.clone()), + None => Err(()), + } + } + } + + /// A scoped override of the [user's configuration directory](crate::System::user_config_directory) for the [`OsSystem`]. + /// + /// Prefer overriding the user's configuration directory for tests that require + /// spawning a new process (e.g. CLI tests) by setting the `APPDATA` (windows) + /// or `XDG_CONFIG_HOME` (unix and other platforms) environment variables. + /// For example, by setting the environment variables when invoking the CLI with insta. + /// + /// Requires the `testing` feature. + #[must_use] + pub struct UserConfigDirectoryOverrideGuard { + previous: Option>, + system: OsSystem, + } + + impl Drop for UserConfigDirectoryOverrideGuard { + fn drop(&mut self) { + if let Ok(mut directory_override) = + self.system.inner.user_config_directory_override.try_lock() + { + *directory_override = self.previous.take(); + } + } + } +} + #[cfg(test)] mod tests { use tempfile::TempDir; From a4c8c49ac2ac6dfd1a4b0eb13c30303598534aa5 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 10 Feb 2025 16:06:05 +0000 Subject: [PATCH 03/12] Delete left-over `verbosity.rs (#16081) --- crates/red_knot/src/main.rs | 1 - crates/red_knot/src/verbosity.rs | 1 - 2 files changed, 2 deletions(-) delete mode 100644 crates/red_knot/src/verbosity.rs diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index a8bb2850f8523..c030826cedfd5 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -22,7 +22,6 @@ use salsa::plumbing::ZalsaDatabase; mod args; mod logging; mod python_version; -mod verbosity; mod version; #[allow(clippy::print_stdout, clippy::unnecessary_wraps, clippy::print_stderr)] diff --git a/crates/red_knot/src/verbosity.rs b/crates/red_knot/src/verbosity.rs deleted file mode 100644 index 8b137891791fe..0000000000000 --- a/crates/red_knot/src/verbosity.rs +++ /dev/null @@ -1 +0,0 @@ - From f30fac632685c1edd200005d110cf407992bd69a Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Mon, 10 Feb 2025 11:30:07 -0600 Subject: [PATCH 04/12] [`ruff`] Skip singleton starred expressions for `incorrectly-parenthesized-tuple-in-subscript` (`RUF031`) (#16083) The index in subscript access like `d[*y]` will not be linted or autofixed with parentheses, even when `lint.ruff.parenthesize-tuple-in-subscript = true`. Closes #16077 --- crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py | 4 ++++ .../resources/test/fixtures/ruff/RUF031_prefer_parens.py | 4 ++++ .../rules/incorrectly_parenthesized_tuple_in_subscript.rs | 6 ++++++ 3 files changed, 14 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py index e784125680ffc..9e869b22dbddf 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py @@ -42,3 +42,7 @@ type Y = typing.Literal[1, 2] Z: typing.TypeAlias = dict[int, int] class Foo(dict[str, int]): pass + +# Skip tuples of length one that are single-starred expressions +# https://github.com/astral-sh/ruff/issues/16077 +d[*x] diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF031_prefer_parens.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF031_prefer_parens.py index 0d9afff34828a..3c4231c140881 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF031_prefer_parens.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF031_prefer_parens.py @@ -42,3 +42,7 @@ type Y = typing.Literal[1, 2] Z: typing.TypeAlias = dict[int, int] class Foo(dict[str, int]): pass + +# Skip tuples of length one that are single-starred expressions +# https://github.com/astral-sh/ruff/issues/16077 +d[*x] diff --git a/crates/ruff_linter/src/rules/ruff/rules/incorrectly_parenthesized_tuple_in_subscript.rs b/crates/ruff_linter/src/rules/ruff/rules/incorrectly_parenthesized_tuple_in_subscript.rs index 0242c1717f2b2..9764a8155cc8e 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/incorrectly_parenthesized_tuple_in_subscript.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/incorrectly_parenthesized_tuple_in_subscript.rs @@ -73,6 +73,12 @@ pub(crate) fn subscript_with_parenthesized_tuple(checker: &Checker, subscript: & return; } + // We should not handle single starred expressions + // (regardless of `prefer_parentheses`) + if matches!(&tuple_subscript.elts[..], &[Expr::Starred(_)]) { + return; + } + // Adding parentheses in the presence of a slice leads to a syntax error. if prefer_parentheses && tuple_subscript.iter().any(Expr::is_slice_expr) { return; From 0019d39f6e7a9588abf8645680cbef88bbcb22f4 Mon Sep 17 00:00:00 2001 From: David Peter Date: Mon, 10 Feb 2025 23:07:06 +0100 Subject: [PATCH 05/12] [red-knot] `T | object == object` (#16088) ## Summary - Simplify unions with `object` to `object`. - Add a new `Type::object(db)` constructor to abbreviate `KnownClass::Object.to_instance(db)` in some places. - Add a `Type::is_object` and `Class::is_object` function to make some tests for a bit easier to read. closes #16084 ## Test Plan New Markdown tests. --- .../resources/mdtest/union_types.md | 29 +++++++++++++++++-- crates/red_knot_python_semantic/src/types.rs | 26 ++++++++++++----- .../src/types/builder.rs | 29 ++++++++++++++----- .../red_knot_python_semantic/src/types/mro.rs | 6 ++-- .../src/types/narrow.rs | 4 +-- .../src/types/property_tests.rs | 6 ++-- .../src/types/signatures.rs | 2 +- .../src/types/subclass_of.rs | 2 +- 8 files changed, 75 insertions(+), 29 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/union_types.md b/crates/red_knot_python_semantic/resources/mdtest/union_types.md index 2fa27ca2113de..a215a6cff2879 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/union_types.md +++ b/crates/red_knot_python_semantic/resources/mdtest/union_types.md @@ -37,6 +37,31 @@ def noreturn(u1: int | NoReturn, u2: int | NoReturn | str) -> None: reveal_type(u2) # revealed: int | str ``` +## `object` subsumes everything + +Unions with `object` can be simplified to `object`: + +```py +from typing_extensions import Never, Any + +def _( + u1: int | object, + u2: object | int, + u3: Any | object, + u4: object | Any, + u5: object | Never, + u6: Never | object, + u7: int | str | object | bytes | Any, +) -> None: + reveal_type(u1) # revealed: object + reveal_type(u2) # revealed: object + reveal_type(u3) # revealed: object + reveal_type(u4) # revealed: object + reveal_type(u5) # revealed: object + reveal_type(u6) # revealed: object + reveal_type(u7) # revealed: object +``` + ## Flattening of nested unions ```py @@ -120,8 +145,8 @@ Simplifications still apply when `Unknown` is present. ```py from knot_extensions import Unknown -def _(u1: str | Unknown | int | object): - reveal_type(u1) # revealed: Unknown | object +def _(u1: int | Unknown | bool) -> None: + reveal_type(u1) # revealed: int | Unknown ``` ## Union of intersections diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index d9d8ac2e9e7a3..6c38c57b46e63 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -631,6 +631,10 @@ impl<'db> Type<'db> { Self::Dynamic(DynamicType::Unknown) } + pub fn object(db: &'db dyn Db) -> Self { + KnownClass::Object.to_instance(db) + } + pub const fn is_unknown(&self) -> bool { matches!(self, Type::Dynamic(DynamicType::Unknown)) } @@ -639,6 +643,11 @@ impl<'db> Type<'db> { matches!(self, Type::Never) } + pub fn is_object(&self, db: &'db dyn Db) -> bool { + self.into_instance() + .is_some_and(|instance| instance.class.is_object(db)) + } + pub const fn is_todo(&self) -> bool { matches!(self, Type::Dynamic(DynamicType::Todo(_))) } @@ -895,7 +904,7 @@ impl<'db> Type<'db> { // `object` is the only type that can be known to be a supertype of any intersection, // even an intersection with no positive elements (Type::Intersection(_), Type::Instance(InstanceType { class })) - if class.is_known(db, KnownClass::Object) => + if class.is_object(db) => { true } @@ -949,7 +958,7 @@ impl<'db> Type<'db> { (left, Type::AlwaysTruthy) => left.bool(db).is_always_true(), // Currently, the only supertype of `AlwaysFalsy` and `AlwaysTruthy` is the universal set (object instance). (Type::AlwaysFalsy | Type::AlwaysTruthy, _) => { - target.is_equivalent_to(db, KnownClass::Object.to_instance(db)) + target.is_equivalent_to(db, Type::object(db)) } // All `StringLiteral` types are a subtype of `LiteralString`. @@ -1088,11 +1097,7 @@ impl<'db> Type<'db> { // All types are assignable to `object`. // TODO this special case might be removable once the below cases are comprehensive - (_, Type::Instance(InstanceType { class })) - if class.is_known(db, KnownClass::Object) => - { - true - } + (_, Type::Instance(InstanceType { class })) if class.is_object(db) => true, // A union is assignable to a type T iff every element of the union is assignable to T. (Type::Union(union), ty) => union @@ -1801,7 +1806,7 @@ impl<'db> Type<'db> { // TODO should be `Callable[[], Literal[True/False]]` todo_type!("`__bool__` for `AlwaysTruthy`/`AlwaysFalsy` Type variants").into() } - _ => KnownClass::Object.to_instance(db).member(db, name), + _ => Type::object(db).member(db, name), }, } } @@ -3853,6 +3858,11 @@ impl<'db> Class<'db> { self.known(db) == Some(known_class) } + /// Return `true` if this class represents the builtin class `object` + pub fn is_object(self, db: &'db dyn Db) -> bool { + self.is_known(db, KnownClass::Object) + } + /// Return an iterator over the inferred types of this class's *explicit* bases. /// /// Note that any class (except for `object`) that has no explicit diff --git a/crates/red_knot_python_semantic/src/types/builder.rs b/crates/red_knot_python_semantic/src/types/builder.rs index c19a4f06cefa4..45be6ac48867e 100644 --- a/crates/red_knot_python_semantic/src/types/builder.rs +++ b/crates/red_knot_python_semantic/src/types/builder.rs @@ -43,6 +43,13 @@ impl<'db> UnionBuilder<'db> { } } + /// Collapse the union to a single type: `object`. + fn collapse_to_object(mut self) -> Self { + self.elements.clear(); + self.elements.push(Type::object(self.db)); + self + } + /// Adds a type to this union. pub(crate) fn add(mut self, ty: Type<'db>) -> Self { match ty { @@ -53,7 +60,12 @@ impl<'db> UnionBuilder<'db> { self = self.add(*element); } } + // Adding `Never` to a union is a no-op. Type::Never => {} + // Adding `object` to a union results in `object`. + ty if ty.is_object(self.db) => { + return self.collapse_to_object(); + } _ => { let bool_pair = if let Type::BooleanLiteral(b) = ty { Some(Type::BooleanLiteral(!b)) @@ -76,7 +88,10 @@ impl<'db> UnionBuilder<'db> { break; } - if ty.is_same_gradual_form(*element) || ty.is_subtype_of(self.db, *element) { + if ty.is_same_gradual_form(*element) + || ty.is_subtype_of(self.db, *element) + || element.is_object(self.db) + { return self; } else if element.is_subtype_of(self.db, ty) { to_remove.push(index); @@ -88,9 +103,7 @@ impl<'db> UnionBuilder<'db> { // `element | ty` must be `object` (object has no other supertypes). This means we can simplify // the whole union to just `object`, since all other potential elements would also be subtypes of // `object`. - self.elements.clear(); - self.elements.push(KnownClass::Object.to_instance(self.db)); - return self; + return self.collapse_to_object(); } } match to_remove[..] { @@ -416,7 +429,7 @@ impl<'db> InnerIntersectionBuilder<'db> { Type::Never => { // Adding ~Never to an intersection is a no-op. } - Type::Instance(instance) if instance.class.is_known(db, KnownClass::Object) => { + Type::Instance(instance) if instance.class.is_object(db) => { // Adding ~object to an intersection results in Never. *self = Self::default(); self.positive.insert(Type::Never); @@ -481,7 +494,7 @@ impl<'db> InnerIntersectionBuilder<'db> { fn build(mut self, db: &'db dyn Db) -> Type<'db> { match (self.positive.len(), self.negative.len()) { - (0, 0) => KnownClass::Object.to_instance(db), + (0, 0) => Type::object(db), (1, 0) => self.positive[0], _ => { self.positive.shrink_to_fit(); @@ -534,7 +547,7 @@ mod tests { let db = setup_db(); let intersection = IntersectionBuilder::new(&db).build(); - assert_eq!(intersection, KnownClass::Object.to_instance(&db)); + assert_eq!(intersection, Type::object(&db)); } #[test_case(Type::BooleanLiteral(true))] @@ -548,7 +561,7 @@ mod tests { // We add t_object in various orders (in first or second position) in // the tests below to ensure that the boolean simplification eliminates // everything from the intersection, not just `bool`. - let t_object = KnownClass::Object.to_instance(&db); + let t_object = Type::object(&db); let t_bool = KnownClass::Bool.to_instance(&db); let ty = IntersectionBuilder::new(&db) diff --git a/crates/red_knot_python_semantic/src/types/mro.rs b/crates/red_knot_python_semantic/src/types/mro.rs index 444335e1eedb9..90f3f47ade45e 100644 --- a/crates/red_knot_python_semantic/src/types/mro.rs +++ b/crates/red_knot_python_semantic/src/types/mro.rs @@ -4,7 +4,7 @@ use std::ops::Deref; use rustc_hash::FxHashSet; use crate::types::class_base::ClassBase; -use crate::types::{Class, KnownClass, Type}; +use crate::types::{Class, Type}; use crate::Db; /// The inferred method resolution order of a given class. @@ -52,9 +52,7 @@ impl<'db> Mro<'db> { match class_bases { // `builtins.object` is the special case: // the only class in Python that has an MRO with length <2 - [] if class.is_known(db, KnownClass::Object) => { - Ok(Self::from([ClassBase::Class(class)])) - } + [] if class.is_object(db) => Ok(Self::from([ClassBase::Class(class)])), // All other classes in Python have an MRO with length >=2. // Even if a class has no explicit base classes, diff --git a/crates/red_knot_python_semantic/src/types/narrow.rs b/crates/red_knot_python_semantic/src/types/narrow.rs index 9fef834db420f..6b0e9f8973490 100644 --- a/crates/red_knot_python_semantic/src/types/narrow.rs +++ b/crates/red_knot_python_semantic/src/types/narrow.rs @@ -152,13 +152,13 @@ fn merge_constraints_or<'db>( *entry.get_mut() = UnionBuilder::new(db).add(*entry.get()).add(*value).build(); } Entry::Vacant(entry) => { - entry.insert(KnownClass::Object.to_instance(db)); + entry.insert(Type::object(db)); } } } for (key, value) in into.iter_mut() { if !from.contains_key(key) { - *value = KnownClass::Object.to_instance(db); + *value = Type::object(db); } } } diff --git a/crates/red_knot_python_semantic/src/types/property_tests.rs b/crates/red_knot_python_semantic/src/types/property_tests.rs index 5f6e8edf06ce3..9d3db01a76f8b 100644 --- a/crates/red_knot_python_semantic/src/types/property_tests.rs +++ b/crates/red_knot_python_semantic/src/types/property_tests.rs @@ -329,7 +329,7 @@ fn union<'db>(db: &'db TestDb, tys: impl IntoIterator>) -> Type mod stable { use super::union; - use crate::types::{KnownClass, Type}; + use crate::types::Type; // Reflexivity: `T` is equivalent to itself. type_property_test!( @@ -419,13 +419,13 @@ mod stable { // All types should be assignable to `object` type_property_test!( all_types_assignable_to_object, db, - forall types t. t.is_assignable_to(db, KnownClass::Object.to_instance(db)) + forall types t. t.is_assignable_to(db, Type::object(db)) ); // And for fully static types, they should also be subtypes of `object` type_property_test!( all_fully_static_types_subtype_of_object, db, - forall types t. t.is_fully_static(db) => t.is_subtype_of(db, KnownClass::Object.to_instance(db)) + forall types t. t.is_fully_static(db) => t.is_subtype_of(db, Type::object(db)) ); // Never should be assignable to every type diff --git a/crates/red_knot_python_semantic/src/types/signatures.rs b/crates/red_knot_python_semantic/src/types/signatures.rs index 6174730becd27..9d44c22bbb3fe 100644 --- a/crates/red_knot_python_semantic/src/types/signatures.rs +++ b/crates/red_knot_python_semantic/src/types/signatures.rs @@ -411,7 +411,7 @@ mod tests { }, Parameter { name: Some(Name::new_static("args")), - annotated_ty: Some(KnownClass::Object.to_instance(&db)), + annotated_ty: Some(Type::object(&db)), kind: ParameterKind::Variadic, }, Parameter { diff --git a/crates/red_knot_python_semantic/src/types/subclass_of.rs b/crates/red_knot_python_semantic/src/types/subclass_of.rs index fe5cc5cd98629..4a0705dfcf5cc 100644 --- a/crates/red_knot_python_semantic/src/types/subclass_of.rs +++ b/crates/red_knot_python_semantic/src/types/subclass_of.rs @@ -26,7 +26,7 @@ impl<'db> SubclassOfType<'db> { ClassBase::Class(class) => { if class.is_final(db) { Type::ClassLiteral(ClassLiteralType { class }) - } else if class.is_known(db, KnownClass::Object) { + } else if class.is_object(db) { KnownClass::Type.to_instance(db) } else { Type::SubclassOf(Self { subclass_of }) From 7fbd89cb392dab6129a61acc6d4e523851a760f1 Mon Sep 17 00:00:00 2001 From: InSync Date: Tue, 11 Feb 2025 14:40:56 +0700 Subject: [PATCH 06/12] [`pyupgrade`] Handle micro version numbers correctly (`UP036`) (#16091) ## Summary Resolves #16082. `UP036` will now also take into consideration whether or not a micro version number is set: * If a third element doesn't exist, the existing logic is preserved. * If it exists but is not an integer literal, the check will not be reported. * If it is an integer literal but doesn't fit into a `u8`, the check will be reported as invalid. * Otherwise, the compared version is determined to always be less than the target version when: * The target's minor version is smaller than that of the comparator, or * The operator is `<`, the micro version is 0, and the two minor versions compare equal. As this is considered a bugfix, it is not preview-gated. ## Test Plan `cargo nextest run` and `cargo insta test`. --------- Co-authored-by: Micha Reiser --- .../test/fixtures/pyupgrade/UP036_5.py | 43 +++++++++ .../pyupgrade/rules/outdated_version_block.rs | 19 +++- ...__rules__pyupgrade__tests__UP036_5.py.snap | 91 +++++++++++++++++++ 3 files changed, 150 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP036_5.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP036_5.py index c7e1a8778da50..697c84a8047d6 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP036_5.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP036_5.py @@ -28,3 +28,46 @@ def a(): else: print(3) return None + + +# https://github.com/astral-sh/ruff/issues/16082 + +## Errors +if sys.version_info < (3, 12, 0): + print() + +if sys.version_info <= (3, 12, 0): + print() + +if sys.version_info < (3, 12, 11): + print() + +if sys.version_info < (3, 13, 0): + print() + +if sys.version_info <= (3, 13, 100000): + print() + + +## No errors + +if sys.version_info <= (3, 13, foo): + print() + +if sys.version_info <= (3, 13, 'final'): + print() + +if sys.version_info <= (3, 13, 0): + print() + +if sys.version_info < (3, 13, 37): + print() + +if sys.version_info <= (3, 13, 37): + print() + +if sys.version_info <= (3, 14, 0): + print() + +if sys.version_info <= (3, 14, 15): + print() diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/outdated_version_block.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/outdated_version_block.rs index 37b1c7a859f7f..8bf187a6ccd90 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/outdated_version_block.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/outdated_version_block.rs @@ -236,14 +236,27 @@ fn version_always_less_than( return Err(anyhow::anyhow!("invalid minor version: {if_minor}")); }; + let if_micro = match check_version_iter.next() { + None => None, + Some(micro) => match micro.as_u8() { + Some(micro) => Some(micro), + None => anyhow::bail!("invalid micro version: {micro}"), + }, + }; + Ok(if or_equal { // Ex) `sys.version_info <= 3.8`. If Python 3.8 is the minimum supported version, // the condition won't always evaluate to `false`, so we want to return `false`. if_minor < py_minor } else { - // Ex) `sys.version_info < 3.8`. If Python 3.8 is the minimum supported version, - // the condition _will_ always evaluate to `false`, so we want to return `true`. - if_minor <= py_minor + if let Some(if_micro) = if_micro { + // Ex) `sys.version_info < 3.8.3` + if_minor < py_minor || if_minor == py_minor && if_micro == 0 + } else { + // Ex) `sys.version_info < 3.8`. If Python 3.8 is the minimum supported version, + // the condition _will_ always evaluate to `false`, so we want to return `true`. + if_minor <= py_minor + } }) } } diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP036_5.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP036_5.py.snap index 478ed274d9c10..efd113261f901 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP036_5.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP036_5.py.snap @@ -66,3 +66,94 @@ UP036_5.py:18:4: UP036 [*] Version block is outdated for minimum Python version 23 |+ else: 24 |+ print(3) 25 |+ return None +31 26 | +32 27 | +33 28 | # https://github.com/astral-sh/ruff/issues/16082 + +UP036_5.py:36:4: UP036 [*] Version block is outdated for minimum Python version + | +35 | ## Errors +36 | if sys.version_info < (3, 12, 0): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +37 | print() + | + = help: Remove outdated version block + +ℹ Unsafe fix +33 33 | # https://github.com/astral-sh/ruff/issues/16082 +34 34 | +35 35 | ## Errors +36 |-if sys.version_info < (3, 12, 0): +37 |- print() +38 36 | +39 37 | if sys.version_info <= (3, 12, 0): +40 38 | print() + +UP036_5.py:39:4: UP036 [*] Version block is outdated for minimum Python version + | +37 | print() +38 | +39 | if sys.version_info <= (3, 12, 0): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +40 | print() + | + = help: Remove outdated version block + +ℹ Unsafe fix +36 36 | if sys.version_info < (3, 12, 0): +37 37 | print() +38 38 | +39 |-if sys.version_info <= (3, 12, 0): +40 |- print() +41 39 | +42 40 | if sys.version_info < (3, 12, 11): +43 41 | print() + +UP036_5.py:42:4: UP036 [*] Version block is outdated for minimum Python version + | +40 | print() +41 | +42 | if sys.version_info < (3, 12, 11): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +43 | print() + | + = help: Remove outdated version block + +ℹ Unsafe fix +39 39 | if sys.version_info <= (3, 12, 0): +40 40 | print() +41 41 | +42 |-if sys.version_info < (3, 12, 11): +43 |- print() +44 42 | +45 43 | if sys.version_info < (3, 13, 0): +46 44 | print() + +UP036_5.py:45:4: UP036 [*] Version block is outdated for minimum Python version + | +43 | print() +44 | +45 | if sys.version_info < (3, 13, 0): + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP036 +46 | print() + | + = help: Remove outdated version block + +ℹ Unsafe fix +42 42 | if sys.version_info < (3, 12, 11): +43 43 | print() +44 44 | +45 |-if sys.version_info < (3, 13, 0): +46 |- print() +47 45 | +48 46 | if sys.version_info <= (3, 13, 100000): +49 47 | print() + +UP036_5.py:48:24: UP036 Version specifier is invalid + | +46 | print() +47 | +48 | if sys.version_info <= (3, 13, 100000): + | ^^^^^^^^^^^^^^^ UP036 +49 | print() + | From 69d86d1d69f3ef48f691b626595a7d0c056b9e66 Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Tue, 11 Feb 2025 05:38:50 -0500 Subject: [PATCH 07/12] Transition to salsa coarse-grained tracked structs (#15763) ## Summary Transition to using coarse-grained tracked structs (depends on https://github.com/salsa-rs/salsa/pull/657). For now, this PR doesn't add any `#[tracked]` fields, meaning that any changes cause the entire struct to be invalidated. It also changes `AstNodeRef` to be compared/hashed by pointer address, instead of performing a deep AST comparison. ## Test Plan This yields a 10-15% improvement on my machine (though weirdly some runs were 5-10% without being flagged as inconsistent by criterion, is there some non-determinism involved?). It's possible that some of this is unrelated, I'll try applying the patch to the current salsa version to make sure. --------- Co-authored-by: Micha Reiser --- Cargo.lock | 17 ++- Cargo.toml | 2 +- crates/red_knot_python_semantic/Cargo.toml | 6 +- .../src/ast_node_ref.rs | 42 ++++-- .../src/module_resolver/resolver.rs | 2 +- .../src/semantic_index.rs | 9 +- .../src/semantic_index/ast_ids.rs | 5 +- .../semantic_index/attribute_assignment.rs | 2 +- .../src/semantic_index/constraint.rs | 12 +- .../src/semantic_index/definition.rs | 32 +++-- .../src/semantic_index/expression.rs | 5 +- .../src/semantic_index/symbol.rs | 11 +- .../src/semantic_index/use_def.rs | 4 +- crates/red_knot_python_semantic/src/symbol.rs | 2 +- crates/red_knot_python_semantic/src/types.rs | 9 +- .../src/types/infer.rs | 12 +- .../red_knot_python_semantic/src/types/mro.rs | 6 +- .../src/types/narrow.rs | 4 +- .../src/types/signatures.rs | 8 +- .../src/types/statistics.rs | 121 ------------------ .../src/types/unpacker.rs | 2 +- crates/red_knot_python_semantic/src/unpack.rs | 5 +- .../src/visibility_constraints.rs | 4 +- crates/ruff_db/src/parsed.rs | 8 ++ crates/ruff_db/src/testing.rs | 4 +- crates/ruff_index/Cargo.toml | 1 + crates/ruff_index/src/vec.rs | 13 ++ crates/ruff_python_ast/Cargo.toml | 9 +- crates/ruff_python_ast/src/name.rs | 1 + fuzz/Cargo.toml | 2 +- 30 files changed, 137 insertions(+), 223 deletions(-) delete mode 100644 crates/red_knot_python_semantic/src/types/statistics.rs diff --git a/Cargo.lock b/Cargo.lock index b4df9d62503f8..2172a3257cf38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -29,6 +29,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "allocator-api2" +version = "0.2.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923" + [[package]] name = "android-tzdata" version = "0.1.1" @@ -1096,6 +1102,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e5274423e17b7c9fc20b6e7e208532f9b19825d82dfd615708b70edd83df41f1" dependencies = [ "ahash", + "allocator-api2", ] [[package]] @@ -2870,6 +2877,7 @@ name = "ruff_index" version = "0.0.0" dependencies = [ "ruff_macros", + "salsa", "static_assertions", ] @@ -2980,6 +2988,7 @@ dependencies = [ "ruff_source_file", "ruff_text_size", "rustc-hash 2.1.1", + "salsa", "schemars", "serde", ] @@ -3304,12 +3313,14 @@ checksum = "6ea1a2d0a644769cc99faa24c3ad26b379b786fe7c36fd3c546254801650e6dd" [[package]] name = "salsa" version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=88a1d7774d78f048fbd77d40abca9ebd729fd1f0#88a1d7774d78f048fbd77d40abca9ebd729fd1f0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=351d9cf0037be949d17800d0c7b4838e533c2ed6#351d9cf0037be949d17800d0c7b4838e533c2ed6" dependencies = [ "append-only-vec", "arc-swap", + "compact_str", "crossbeam", "dashmap 6.1.0", + "hashbrown 0.14.5", "hashlink", "indexmap", "parking_lot", @@ -3324,12 +3335,12 @@ dependencies = [ [[package]] name = "salsa-macro-rules" version = "0.1.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=88a1d7774d78f048fbd77d40abca9ebd729fd1f0#88a1d7774d78f048fbd77d40abca9ebd729fd1f0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=351d9cf0037be949d17800d0c7b4838e533c2ed6#351d9cf0037be949d17800d0c7b4838e533c2ed6" [[package]] name = "salsa-macros" version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=88a1d7774d78f048fbd77d40abca9ebd729fd1f0#88a1d7774d78f048fbd77d40abca9ebd729fd1f0" +source = "git+https://github.com/salsa-rs/salsa.git?rev=351d9cf0037be949d17800d0c7b4838e533c2ed6#351d9cf0037be949d17800d0c7b4838e533c2ed6" dependencies = [ "heck", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 296cde103f8a5..0429bc37ccac2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -123,7 +123,7 @@ rayon = { version = "1.10.0" } regex = { version = "1.10.2" } rustc-hash = { version = "2.0.0" } # When updating salsa, make sure to also update the revision in `fuzz/Cargo.toml` -salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "88a1d7774d78f048fbd77d40abca9ebd729fd1f0" } +salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "351d9cf0037be949d17800d0c7b4838e533c2ed6" } schemars = { version = "0.8.16" } seahash = { version = "4.1.0" } serde = { version = "1.0.197", features = ["derive"] } diff --git a/crates/red_knot_python_semantic/Cargo.toml b/crates/red_knot_python_semantic/Cargo.toml index b12b3637dbdfd..e7af108fe0331 100644 --- a/crates/red_knot_python_semantic/Cargo.toml +++ b/crates/red_knot_python_semantic/Cargo.toml @@ -12,9 +12,9 @@ license = { workspace = true } [dependencies] ruff_db = { workspace = true } -ruff_index = { workspace = true } +ruff_index = { workspace = true, features = ["salsa"] } ruff_macros = { workspace = true } -ruff_python_ast = { workspace = true } +ruff_python_ast = { workspace = true, features = ["salsa"] } ruff_python_parser = { workspace = true } ruff_python_stdlib = { workspace = true } ruff_source_file = { workspace = true } @@ -31,7 +31,7 @@ drop_bomb = { workspace = true } indexmap = { workspace = true } itertools = { workspace = true } ordermap = { workspace = true } -salsa = { workspace = true } +salsa = { workspace = true, features = ["compact_str"] } thiserror = { workspace = true } tracing = { workspace = true } rustc-hash = { workspace = true } diff --git a/crates/red_knot_python_semantic/src/ast_node_ref.rs b/crates/red_knot_python_semantic/src/ast_node_ref.rs index da8f610683e0c..667b8f6363ecc 100644 --- a/crates/red_knot_python_semantic/src/ast_node_ref.rs +++ b/crates/red_knot_python_semantic/src/ast_node_ref.rs @@ -12,7 +12,29 @@ use ruff_db::parsed::ParsedModule; /// Holding on to any [`AstNodeRef`] prevents the [`ParsedModule`] from being released. /// /// ## Equality -/// Two `AstNodeRef` are considered equal if their wrapped nodes are equal. +/// Two `AstNodeRef` are considered equal if their pointer addresses are equal. +/// +/// ## Usage in salsa tracked structs +/// It's important that [`AstNodeRef`] fields in salsa tracked structs are tracked fields +/// (attributed with `#[tracked`]). It prevents that the tracked struct gets a new ID +/// everytime the AST changes, which in turn, invalidates the result of any query +/// that takes said tracked struct as a query argument or returns the tracked struct as part of its result. +/// +/// For example, marking the [`AstNodeRef`] as tracked on `Expression` +/// has the effect that salsa will consider the expression as "unchanged" for as long as it: +/// +/// * belongs to the same file +/// * belongs to the same scope +/// * has the same kind +/// * was created in the same order +/// +/// This means that changes to expressions in other scopes don't invalidate the expression's id, giving +/// us some form of scope-stable identity for expressions. Only queries accessing the node field +/// run on every AST change. All other queries only run when the expression's identity changes. +/// +/// The one exception to this is if it is known that all queries tacking the tracked struct +/// as argument or returning it as part of their result are known to access the node field. +/// Marking the field tracked is then unnecessary. #[derive(Clone)] pub struct AstNodeRef { /// Owned reference to the node's [`ParsedModule`]. @@ -67,23 +89,17 @@ where } } -impl PartialEq for AstNodeRef -where - T: PartialEq, -{ +impl PartialEq for AstNodeRef { fn eq(&self, other: &Self) -> bool { - self.node().eq(other.node()) + self.node.eq(&other.node) } } -impl Eq for AstNodeRef where T: Eq {} +impl Eq for AstNodeRef {} -impl Hash for AstNodeRef -where - T: Hash, -{ +impl Hash for AstNodeRef { fn hash(&self, state: &mut H) { - self.node().hash(state); + self.node.hash(state); } } @@ -117,7 +133,7 @@ mod tests { let stmt_cloned = &cloned.syntax().body[0]; let cloned_node = unsafe { AstNodeRef::new(cloned.clone(), stmt_cloned) }; - assert_eq!(node1, cloned_node); + assert_ne!(node1, cloned_node); let other_raw = parse_unchecked_source("2 + 2", PySourceType::Python); let other = ParsedModule::new(other_raw); diff --git a/crates/red_knot_python_semantic/src/module_resolver/resolver.rs b/crates/red_knot_python_semantic/src/module_resolver/resolver.rs index f311c23c676ee..5fb8b686b0616 100644 --- a/crates/red_knot_python_semantic/src/module_resolver/resolver.rs +++ b/crates/red_knot_python_semantic/src/module_resolver/resolver.rs @@ -133,7 +133,7 @@ pub(crate) fn search_paths(db: &dyn Db) -> SearchPathIterator { } #[derive(Debug, PartialEq, Eq)] -pub(crate) struct SearchPaths { +pub struct SearchPaths { /// Search paths that have been statically determined purely from reading Ruff's configuration settings. /// These shouldn't ever change unless the config settings themselves change. static_paths: Vec, diff --git a/crates/red_knot_python_semantic/src/semantic_index.rs b/crates/red_knot_python_semantic/src/semantic_index.rs index ce975fcb94a95..f2058e54b1c3c 100644 --- a/crates/red_knot_python_semantic/src/semantic_index.rs +++ b/crates/red_knot_python_semantic/src/semantic_index.rs @@ -1,13 +1,14 @@ use std::iter::FusedIterator; use std::sync::Arc; -use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; -use salsa::plumbing::AsId; - use ruff_db::files::File; use ruff_db::parsed::parsed_module; use ruff_index::{IndexSlice, IndexVec}; +use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; +use salsa::plumbing::AsId; +use salsa::Update; + use crate::module_name::ModuleName; use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey; use crate::semantic_index::ast_ids::AstIds; @@ -123,7 +124,7 @@ pub(crate) fn global_scope(db: &dyn Db, file: File) -> ScopeId<'_> { } /// The symbol tables and use-def maps for all scopes in a file. -#[derive(Debug)] +#[derive(Debug, Update)] pub(crate) struct SemanticIndex<'db> { /// List of all symbol tables in this file, indexed by scope. symbol_tables: IndexVec>, diff --git a/crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs b/crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs index ea87dc84098ad..160f3af7b9cd2 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/ast_ids.rs @@ -24,7 +24,7 @@ use crate::Db; /// /// x = foo() /// ``` -#[derive(Debug)] +#[derive(Debug, salsa::Update)] pub(crate) struct AstIds { /// Maps expressions to their expression id. expressions_map: FxHashMap, @@ -74,6 +74,7 @@ impl HasScopedUseId for ast::ExprRef<'_> { /// Uniquely identifies an [`ast::Expr`] in a [`crate::semantic_index::symbol::FileScopeId`]. #[newtype_index] +#[derive(salsa::Update)] pub struct ScopedExpressionId; pub trait HasScopedExpressionId { @@ -181,7 +182,7 @@ pub(crate) mod node_key { use crate::node_key::NodeKey; - #[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)] + #[derive(Copy, Clone, Eq, PartialEq, Hash, Debug, salsa::Update)] pub(crate) struct ExpressionNodeKey(NodeKey); impl From> for ExpressionNodeKey { diff --git a/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs b/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs index 53cc41e9dcfce..5970ede0d49a7 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/attribute_assignment.rs @@ -9,7 +9,7 @@ use rustc_hash::FxHashMap; /// Describes an (annotated) attribute assignment that we discovered in a method /// body, typically of the form `self.x: int`, `self.x: int = …` or `self.x = …`. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] pub(crate) enum AttributeAssignment<'db> { /// An attribute assignment with an explicit type annotation, either /// `self.x: ` or `self.x: = …`. diff --git a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs index ed0760a8bc3cf..cee7e472eddf3 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs @@ -5,20 +5,20 @@ use crate::db::Db; use crate::semantic_index::expression::Expression; use crate::semantic_index::symbol::{FileScopeId, ScopeId}; -#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, salsa::Update)] pub(crate) struct Constraint<'db> { pub(crate) node: ConstraintNode<'db>, pub(crate) is_positive: bool, } -#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, salsa::Update)] pub(crate) enum ConstraintNode<'db> { Expression(Expression<'db>), Pattern(PatternConstraint<'db>), } /// Pattern kinds for which we support type narrowing and/or static visibility analysis. -#[derive(Debug, Clone, Hash, PartialEq)] +#[derive(Debug, Clone, Hash, PartialEq, salsa::Update)] pub(crate) enum PatternConstraintKind<'db> { Singleton(Singleton, Option>), Value(Expression<'db>, Option>), @@ -28,21 +28,15 @@ pub(crate) enum PatternConstraintKind<'db> { #[salsa::tracked] pub(crate) struct PatternConstraint<'db> { - #[id] pub(crate) file: File, - #[id] pub(crate) file_scope: FileScopeId, - #[no_eq] - #[return_ref] pub(crate) subject: Expression<'db>, - #[no_eq] #[return_ref] pub(crate) kind: PatternConstraintKind<'db>, - #[no_eq] count: countme::Count>, } diff --git a/crates/red_knot_python_semantic/src/semantic_index/definition.rs b/crates/red_knot_python_semantic/src/semantic_index/definition.rs index 782d88f9904dc..830ad0ffd4b19 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/definition.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/definition.rs @@ -25,22 +25,19 @@ use crate::Db; #[salsa::tracked] pub struct Definition<'db> { /// The file in which the definition occurs. - #[id] pub(crate) file: File, /// The scope in which the definition occurs. - #[id] pub(crate) file_scope: FileScopeId, /// The symbol defined. - #[id] pub(crate) symbol: ScopedSymbolId, #[no_eq] #[return_ref] + #[tracked] pub(crate) kind: DefinitionKind<'db>, - #[no_eq] count: countme::Count>, } @@ -435,7 +432,14 @@ impl DefinitionCategory { } } -#[derive(Clone, Debug)] +/// The kind of a definition. +/// +/// ## Usage in salsa tracked structs +/// +/// [`DefinitionKind`] fields in salsa tracked structs should be tracked (attributed with `#[tracked]`) +/// because the kind is a thin wrapper around [`AstNodeRef`]. See the [`AstNodeRef`] documentation +/// for an in-depth explanation of why this is necessary. +#[derive(Clone, Debug, Hash)] pub enum DefinitionKind<'db> { Import(AstNodeRef), ImportFrom(ImportFromDefinitionKind), @@ -540,7 +544,7 @@ impl DefinitionKind<'_> { } } -#[derive(Copy, Clone, Debug, PartialEq)] +#[derive(Copy, Clone, Debug, PartialEq, Hash)] pub(crate) enum TargetKind<'db> { Sequence(Unpack<'db>), Name, @@ -555,7 +559,7 @@ impl<'db> From>> for TargetKind<'db> { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] #[allow(dead_code)] pub struct MatchPatternDefinitionKind { pattern: AstNodeRef, @@ -573,7 +577,7 @@ impl MatchPatternDefinitionKind { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct ComprehensionDefinitionKind { iterable: AstNodeRef, target: AstNodeRef, @@ -599,7 +603,7 @@ impl ComprehensionDefinitionKind { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct ImportFromDefinitionKind { node: AstNodeRef, alias_index: usize, @@ -615,7 +619,7 @@ impl ImportFromDefinitionKind { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct AssignmentDefinitionKind<'db> { target: TargetKind<'db>, value: AstNodeRef, @@ -641,7 +645,7 @@ impl<'db> AssignmentDefinitionKind<'db> { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct WithItemDefinitionKind { node: AstNodeRef, target: AstNodeRef, @@ -662,7 +666,7 @@ impl WithItemDefinitionKind { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct ForStmtDefinitionKind<'db> { target: TargetKind<'db>, iterable: AstNodeRef, @@ -693,7 +697,7 @@ impl<'db> ForStmtDefinitionKind<'db> { } } -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Hash)] pub struct ExceptHandlerDefinitionKind { handler: AstNodeRef, is_star: bool, @@ -713,7 +717,7 @@ impl ExceptHandlerDefinitionKind { } } -#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)] +#[derive(Copy, Clone, Eq, PartialEq, Hash, Debug, salsa::Update)] pub(crate) struct DefinitionNodeKey(NodeKey); impl From<&ast::Alias> for DefinitionNodeKey { diff --git a/crates/red_knot_python_semantic/src/semantic_index/expression.rs b/crates/red_knot_python_semantic/src/semantic_index/expression.rs index 6809dbef24928..6189c4cb68c56 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/expression.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/expression.rs @@ -33,23 +33,20 @@ pub(crate) enum ExpressionKind { #[salsa::tracked] pub(crate) struct Expression<'db> { /// The file in which the expression occurs. - #[id] pub(crate) file: File, /// The scope in which the expression occurs. - #[id] pub(crate) file_scope: FileScopeId, /// The expression node. #[no_eq] + #[tracked] #[return_ref] pub(crate) node_ref: AstNodeRef, /// Should this expression be inferred as a normal expression or a type expression? - #[id] pub(crate) kind: ExpressionKind, - #[no_eq] count: countme::Count>, } diff --git a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs index 3094bca7cd4f7..d2f93062411eb 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs @@ -96,18 +96,16 @@ impl From for ScopedSymbolId { /// Symbol ID that uniquely identifies a symbol inside a [`Scope`]. #[newtype_index] +#[derive(salsa::Update)] pub struct ScopedSymbolId; /// A cross-module identifier of a scope that can be used as a salsa query parameter. #[salsa::tracked] pub struct ScopeId<'db> { - #[id] pub file: File, - #[id] pub file_scope_id: FileScopeId, - #[no_eq] count: countme::Count>, } @@ -159,6 +157,7 @@ impl<'db> ScopeId<'db> { /// ID that uniquely identifies a scope inside of a module. #[newtype_index] +#[derive(salsa::Update)] pub struct FileScopeId; impl FileScopeId { @@ -177,7 +176,7 @@ impl FileScopeId { } } -#[derive(Debug)] +#[derive(Debug, salsa::Update)] pub struct Scope { pub(super) parent: Option, pub(super) node: NodeWithScopeKind, @@ -216,7 +215,7 @@ impl ScopeKind { } /// Symbol table for a specific [`Scope`]. -#[derive(Debug, Default)] +#[derive(Debug, Default, salsa::Update)] pub struct SymbolTable { /// The symbols in this scope. symbols: IndexVec, @@ -424,7 +423,7 @@ impl NodeWithScopeRef<'_> { } /// Node that introduces a new scope. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, salsa::Update)] pub enum NodeWithScopeKind { Module, Class(AstNodeRef), diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index 4d46c79152fad..e4d201170819a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -278,7 +278,7 @@ mod symbol_state; type AllConstraints<'db> = IndexVec>; /// Applicable definitions and constraints for every use of a name. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct UseDefMap<'db> { /// Array of [`Definition`] in this scope. Only the first entry should be `None`; /// this represents the implicit "unbound"/"undeclared" definition of every symbol. @@ -384,7 +384,7 @@ impl<'db> UseDefMap<'db> { } /// Either live bindings or live declarations for a symbol. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, salsa::Update)] enum SymbolDefinitions { Bindings(SymbolBindings), Declarations(SymbolDeclarations), diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 1b0dc20525370..3b8866eaf1ae8 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -26,7 +26,7 @@ pub(crate) enum Boundness { /// possibly_unbound: Symbol::Type(Type::IntLiteral(2), Boundness::PossiblyUnbound), /// non_existent: Symbol::Unbound, /// ``` -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] pub(crate) enum Symbol<'db> { Type(Type<'db>, Boundness), Unbound, diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 6c38c57b46e63..217337be85175 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -56,7 +56,6 @@ mod mro; mod narrow; mod signatures; mod slots; -mod statistics; mod string_annotation; mod subclass_of; mod type_ordering; @@ -2589,7 +2588,7 @@ bitflags! { /// /// Example: `Annotated[ClassVar[tuple[int]], "metadata"]` would have type `tuple[int]` and the /// qualifier `ClassVar`. -#[derive(Clone, Debug, Copy, Eq, PartialEq)] +#[derive(Clone, Debug, Copy, Eq, PartialEq, salsa::Update)] pub(crate) struct TypeAndQualifiers<'db> { inner: Type<'db>, qualifiers: TypeQualifiers, @@ -4400,7 +4399,7 @@ impl<'db> TypeAliasType<'db> { } /// Either the explicit `metaclass=` keyword of the class, or the inferred metaclass of one of its base classes. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] pub(super) struct MetaclassCandidate<'db> { metaclass: Class<'db>, explicit_metaclass_of: Class<'db>, @@ -4443,7 +4442,7 @@ impl<'db> From> for Type<'db> { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] pub(super) struct MetaclassError<'db> { kind: MetaclassErrorKind<'db>, } @@ -4455,7 +4454,7 @@ impl<'db> MetaclassError<'db> { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, salsa::Update)] pub(super) enum MetaclassErrorKind<'db> { /// The class has incompatible metaclasses in its inheritance hierarchy. /// diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index a9fe575c4d759..8fe12e1cee41b 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -61,7 +61,6 @@ use crate::types::diagnostic::{ UNDEFINED_REVEAL, UNRESOLVED_ATTRIBUTE, UNRESOLVED_IMPORT, UNSUPPORTED_OPERATOR, }; use crate::types::mro::MroErrorKind; -use crate::types::statistics::TypeStatistics; use crate::types::unpacker::{UnpackResult, Unpacker}; use crate::types::{ builtins_symbol, global_symbol, symbol, symbol_from_bindings, symbol_from_declarations, @@ -237,7 +236,7 @@ impl<'db> InferenceRegion<'db> { } /// The inferred types for a single region. -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug, Eq, PartialEq, salsa::Update)] pub(crate) struct TypeInference<'db> { /// The types of every expression in this region. expressions: FxHashMap>, @@ -300,14 +299,6 @@ impl<'db> TypeInference<'db> { self.diagnostics.shrink_to_fit(); self.deferred.shrink_to_fit(); } - - pub(super) fn statistics(&self) -> TypeStatistics { - let mut statistics = TypeStatistics::default(); - for ty in self.expressions.values() { - statistics.increment(*ty); - } - statistics - } } impl WithDiagnostics for TypeInference<'_> { @@ -6433,6 +6424,7 @@ mod tests { assert_eq!(attr_ty.display(&db).to_string(), "Unknown | str | None"); db.take_salsa_events() }; + assert_function_query_was_not_run( &db, infer_expression_types, diff --git a/crates/red_knot_python_semantic/src/types/mro.rs b/crates/red_knot_python_semantic/src/types/mro.rs index 90f3f47ade45e..b87361e9db49f 100644 --- a/crates/red_knot_python_semantic/src/types/mro.rs +++ b/crates/red_knot_python_semantic/src/types/mro.rs @@ -10,7 +10,7 @@ use crate::Db; /// The inferred method resolution order of a given class. /// /// See [`Class::iter_mro`] for more details. -#[derive(PartialEq, Eq, Clone, Debug)] +#[derive(PartialEq, Eq, Clone, Debug, salsa::Update)] pub(super) struct Mro<'db>(Box<[ClassBase<'db>]>); impl<'db> Mro<'db> { @@ -236,7 +236,7 @@ impl<'db> Iterator for MroIterator<'db> { impl std::iter::FusedIterator for MroIterator<'_> {} -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, salsa::Update)] pub(super) struct MroError<'db> { kind: MroErrorKind<'db>, fallback_mro: Mro<'db>, @@ -256,7 +256,7 @@ impl<'db> MroError<'db> { } /// Possible ways in which attempting to resolve the MRO of a class might fail. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, salsa::Update)] pub(super) enum MroErrorKind<'db> { /// The class inherits from one or more invalid bases. /// diff --git a/crates/red_knot_python_semantic/src/types/narrow.rs b/crates/red_knot_python_semantic/src/types/narrow.rs index 6b0e9f8973490..90ac0f2956d23 100644 --- a/crates/red_knot_python_semantic/src/types/narrow.rs +++ b/crates/red_knot_python_semantic/src/types/narrow.rs @@ -231,10 +231,10 @@ impl<'db> NarrowingConstraintsBuilder<'db> { match pattern.kind(self.db) { PatternConstraintKind::Singleton(singleton, _guard) => { - self.evaluate_match_pattern_singleton(*subject, *singleton) + self.evaluate_match_pattern_singleton(subject, *singleton) } PatternConstraintKind::Class(cls, _guard) => { - self.evaluate_match_pattern_class(*subject, *cls) + self.evaluate_match_pattern_class(subject, *cls) } // TODO: support more pattern kinds PatternConstraintKind::Value(..) | PatternConstraintKind::Unsupported => None, diff --git a/crates/red_knot_python_semantic/src/types/signatures.rs b/crates/red_knot_python_semantic/src/types/signatures.rs index 9d44c22bbb3fe..96d259a76bbc3 100644 --- a/crates/red_knot_python_semantic/src/types/signatures.rs +++ b/crates/red_knot_python_semantic/src/types/signatures.rs @@ -4,7 +4,7 @@ use crate::{semantic_index::definition::Definition, types::todo_type}; use ruff_python_ast::{self as ast, name::Name}; /// A typed callable signature. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct Signature<'db> { /// Parameters, in source order. /// @@ -60,7 +60,7 @@ impl<'db> Signature<'db> { } // TODO: use SmallVec here once invariance bug is fixed -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct Parameters<'db>(Vec>); impl<'db> Parameters<'db> { @@ -218,7 +218,7 @@ impl<'db> std::ops::Index for Parameters<'db> { } } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct Parameter<'db> { /// Parameter name. /// @@ -304,7 +304,7 @@ impl<'db> Parameter<'db> { } } -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, salsa::Update)] pub(crate) enum ParameterKind<'db> { /// Positional-only parameter, e.g. `def f(x, /): ...` PositionalOnly { default_ty: Option> }, diff --git a/crates/red_knot_python_semantic/src/types/statistics.rs b/crates/red_knot_python_semantic/src/types/statistics.rs deleted file mode 100644 index 625a38d52adad..0000000000000 --- a/crates/red_knot_python_semantic/src/types/statistics.rs +++ /dev/null @@ -1,121 +0,0 @@ -use crate::types::{infer_scope_types, semantic_index, Type}; -use crate::Db; -use ruff_db::files::File; -use rustc_hash::FxHashMap; - -/// Get type-coverage statistics for a file. -#[salsa::tracked(return_ref)] -pub fn type_statistics<'db>(db: &'db dyn Db, file: File) -> TypeStatistics<'db> { - let _span = tracing::trace_span!("type_statistics", file=?file.path(db)).entered(); - - tracing::debug!( - "Gathering statistics for file '{path}'", - path = file.path(db) - ); - - let index = semantic_index(db, file); - let mut statistics = TypeStatistics::default(); - - for scope_id in index.scope_ids() { - let result = infer_scope_types(db, scope_id); - statistics.extend(&result.statistics()); - } - - statistics -} - -/// Map each type to count of expressions with that type. -#[derive(Debug, Default, Eq, PartialEq)] -pub(super) struct TypeStatistics<'db>(FxHashMap, u32>); - -impl<'db> TypeStatistics<'db> { - fn extend(&mut self, other: &TypeStatistics<'db>) { - for (ty, count) in &other.0 { - self.0 - .entry(*ty) - .and_modify(|my_count| *my_count += count) - .or_insert(*count); - } - } - - pub(super) fn increment(&mut self, ty: Type<'db>) { - self.0 - .entry(ty) - .and_modify(|count| *count += 1) - .or_insert(1); - } - - #[allow(unused)] - fn expression_count(&self) -> u32 { - self.0.values().sum() - } - - #[allow(unused)] - fn todo_count(&self) -> u32 { - self.0 - .iter() - .filter(|(key, _)| key.is_todo()) - .map(|(_, count)| count) - .sum() - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::db::tests::{setup_db, TestDb}; - use ruff_db::files::system_path_to_file; - use ruff_db::system::DbWithTestSystem; - - fn get_stats<'db>( - db: &'db mut TestDb, - filename: &str, - source: &str, - ) -> &'db TypeStatistics<'db> { - db.write_dedented(filename, source).unwrap(); - - type_statistics(db, system_path_to_file(db, filename).unwrap()) - } - - #[test] - fn all_static() { - let mut db = setup_db(); - - let stats = get_stats(&mut db, "src/foo.py", "1"); - - assert_eq!(stats.0, FxHashMap::from_iter([(Type::IntLiteral(1), 1)])); - } - - #[test] - fn todo_and_expression_count() { - let mut db = setup_db(); - - let stats = get_stats( - &mut db, - "src/foo.py", - r#" - x = [x for x in [1]] - "#, - ); - - assert_eq!(stats.todo_count(), 4); - assert_eq!(stats.expression_count(), 6); - } - - #[test] - fn sum() { - let mut db = setup_db(); - - let stats = get_stats( - &mut db, - "src/foo.py", - r#" - 1 - def f(): - 1 - "#, - ); - - assert_eq!(stats.0[&Type::IntLiteral(1)], 2); - } -} diff --git a/crates/red_knot_python_semantic/src/types/unpacker.rs b/crates/red_knot_python_semantic/src/types/unpacker.rs index bd5baa982d04e..a70312e5cbcc2 100644 --- a/crates/red_knot_python_semantic/src/types/unpacker.rs +++ b/crates/red_knot_python_semantic/src/types/unpacker.rs @@ -261,7 +261,7 @@ impl<'db> Unpacker<'db> { } } -#[derive(Debug, Default, PartialEq, Eq)] +#[derive(Debug, Default, PartialEq, Eq, salsa::Update)] pub(crate) struct UnpackResult<'db> { targets: FxHashMap>, diagnostics: TypeCheckDiagnostics, diff --git a/crates/red_knot_python_semantic/src/unpack.rs b/crates/red_knot_python_semantic/src/unpack.rs index 470041a4dc9c7..05a396d4dc062 100644 --- a/crates/red_knot_python_semantic/src/unpack.rs +++ b/crates/red_knot_python_semantic/src/unpack.rs @@ -28,10 +28,8 @@ use crate::Db; /// * an argument of a cross-module query #[salsa::tracked] pub(crate) struct Unpack<'db> { - #[id] pub(crate) file: File, - #[id] pub(crate) file_scope: FileScopeId, /// The target expression that is being unpacked. For example, in `(a, b) = (1, 2)`, the target @@ -45,7 +43,6 @@ pub(crate) struct Unpack<'db> { #[no_eq] pub(crate) value: UnpackValue<'db>, - #[no_eq] count: countme::Count>, } @@ -62,7 +59,7 @@ impl<'db> Unpack<'db> { } /// The expression that is being unpacked. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, Hash)] pub(crate) enum UnpackValue<'db> { /// An iterable expression like the one in a `for` loop or a comprehension. Iterable(Expression<'db>), diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 2acd79ffa677f..aa4c5e6971694 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -338,7 +338,7 @@ const SMALLEST_TERMINAL: ScopedVisibilityConstraintId = ALWAYS_FALSE; /// A collection of visibility constraints. This is currently stored in `UseDefMap`, which means we /// maintain a separate set of visibility constraints for each scope in file. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct VisibilityConstraints<'db> { constraints: IndexVec>, interiors: IndexVec, @@ -627,7 +627,7 @@ impl<'db> VisibilityConstraints<'db> { ConstraintNode::Pattern(inner) => match inner.kind(db) { PatternConstraintKind::Value(value, guard) => { let subject_expression = inner.subject(db); - let inference = infer_expression_types(db, *subject_expression); + let inference = infer_expression_types(db, subject_expression); let scope = subject_expression.scope(db); let subject_ty = inference.expression_type( subject_expression diff --git a/crates/ruff_db/src/parsed.rs b/crates/ruff_db/src/parsed.rs index e93d5e55178c2..fcc10b5844008 100644 --- a/crates/ruff_db/src/parsed.rs +++ b/crates/ruff_db/src/parsed.rs @@ -73,6 +73,14 @@ impl std::fmt::Debug for ParsedModule { } } +impl PartialEq for ParsedModule { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.inner, &other.inner) + } +} + +impl Eq for ParsedModule {} + #[cfg(test)] mod tests { use crate::files::{system_path_to_file, vendored_path_to_file}; diff --git a/crates/ruff_db/src/testing.rs b/crates/ruff_db/src/testing.rs index cbba5b3cff18b..880a6fccc8867 100644 --- a/crates/ruff_db/src/testing.rs +++ b/crates/ruff_db/src/testing.rs @@ -18,7 +18,7 @@ pub fn assert_function_query_was_not_run( db.attach(|_| { if let Some(will_execute_event) = will_execute_event { - panic!("Expected query {query_name}({id}) not to have run but it did: {will_execute_event:?}"); + panic!("Expected query {query_name}({id}) not to have run but it did: {will_execute_event:?}\n\n{events:#?}"); } }); } @@ -46,7 +46,7 @@ pub fn assert_const_function_query_was_not_run( db.attach(|_| { if let Some(will_execute_event) = event { panic!( - "Expected query {query_name}() not to have run but it did: {will_execute_event:?}" + "Expected query {query_name}() not to have run but it did: {will_execute_event:?}\n\n{events:#?}" ); } }); diff --git a/crates/ruff_index/Cargo.toml b/crates/ruff_index/Cargo.toml index ceaf8315a9609..a96fa738f6452 100644 --- a/crates/ruff_index/Cargo.toml +++ b/crates/ruff_index/Cargo.toml @@ -15,6 +15,7 @@ doctest = false [dependencies] ruff_macros = { workspace = true } +salsa = { workspace = true, optional = true } [dev-dependencies] static_assertions = { workspace = true } diff --git a/crates/ruff_index/src/vec.rs b/crates/ruff_index/src/vec.rs index 184cf0ec89922..4e9bc479229af 100644 --- a/crates/ruff_index/src/vec.rs +++ b/crates/ruff_index/src/vec.rs @@ -181,3 +181,16 @@ impl From<[T; N]> for IndexVec { // not the phantom data. #[allow(unsafe_code)] unsafe impl Send for IndexVec where T: Send {} + +#[allow(unsafe_code)] +#[cfg(feature = "salsa")] +unsafe impl salsa::Update for IndexVec +where + T: salsa::Update, +{ + #[allow(unsafe_code)] + unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { + let old_vec: &mut IndexVec = unsafe { &mut *old_pointer }; + salsa::Update::maybe_update(&mut old_vec.raw, new_value.raw) + } +} diff --git a/crates/ruff_python_ast/Cargo.toml b/crates/ruff_python_ast/Cargo.toml index f923773b93638..22f484fdbc0d1 100644 --- a/crates/ruff_python_ast/Cargo.toml +++ b/crates/ruff_python_ast/Cargo.toml @@ -26,6 +26,7 @@ is-macro = { workspace = true } itertools = { workspace = true } memchr = { workspace = true } rustc-hash = { workspace = true } +salsa = { workspace = true, optional = true } schemars = { workspace = true, optional = true } serde = { workspace = true, optional = true } @@ -33,10 +34,10 @@ serde = { workspace = true, optional = true } schemars = ["dep:schemars"] cache = ["dep:ruff_cache", "dep:ruff_macros"] serde = [ - "dep:serde", - "ruff_text_size/serde", - "dep:ruff_cache", - "compact_str/serde", + "dep:serde", + "ruff_text_size/serde", + "dep:ruff_cache", + "compact_str/serde", ] [lints] diff --git a/crates/ruff_python_ast/src/name.rs b/crates/ruff_python_ast/src/name.rs index bfd23d5b0cad8..2cc8843ab2346 100644 --- a/crates/ruff_python_ast/src/name.rs +++ b/crates/ruff_python_ast/src/name.rs @@ -8,6 +8,7 @@ use crate::{nodes, Expr}; #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Default)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[cfg_attr(feature = "cache", derive(ruff_macros::CacheKey))] +#[cfg_attr(feature = "salsa", derive(salsa::Update))] pub struct Name(compact_str::CompactString); impl Name { diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 6f6a74046aabb..0986d1c2d9b05 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -29,7 +29,7 @@ ruff_python_formatter = { path = "../crates/ruff_python_formatter" } ruff_text_size = { path = "../crates/ruff_text_size" } libfuzzer-sys = { git = "https://github.com/rust-fuzz/libfuzzer", default-features = false } -salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "88a1d7774d78f048fbd77d40abca9ebd729fd1f0" } +salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "351d9cf0037be949d17800d0c7b4838e533c2ed6" } similar = { version = "2.5.0" } tracing = { version = "0.1.40" } From df1d4302946678065dcee4b7aa6bff92c489711d Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 11 Feb 2025 11:09:37 +0000 Subject: [PATCH 08/12] [red-knot] Reduce usage of `From` implementations when working with `Symbol`s (#16076) --- crates/red_knot_python_semantic/src/symbol.rs | 13 +- crates/red_knot_python_semantic/src/types.rs | 118 +++++++++--------- .../src/types/infer.rs | 40 +++--- 3 files changed, 94 insertions(+), 77 deletions(-) diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 3b8866eaf1ae8..e7423942651e0 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -1,5 +1,5 @@ use crate::{ - types::{Type, UnionType}, + types::{todo_type, Type, UnionType}, Db, }; @@ -33,6 +33,17 @@ pub(crate) enum Symbol<'db> { } impl<'db> Symbol<'db> { + /// Constructor that creates a `Symbol` with boundness [`Boundness::Bound`]. + pub(crate) fn bound(ty: impl Into>) -> Self { + Symbol::Type(ty.into(), Boundness::Bound) + } + + /// Constructor that creates a [`Symbol`] with a [`crate::types::TodoType`] type + /// and boundness [`Boundness::Bound`]. + pub(crate) fn todo(message: &'static str) -> Self { + Symbol::Type(todo_type!(message), Boundness::Bound) + } + pub(crate) fn is_unbound(&self) -> bool { matches!(self, Symbol::Unbound) } diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 217337be85175..50ed347b9c8e0 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -143,7 +143,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> } // Symbol is possibly undeclared and (possibly) bound Symbol::Type(inferred_ty, boundness) => Symbol::Type( - UnionType::from_elements(db, [inferred_ty, declared_ty].iter().copied()), + UnionType::from_elements(db, [inferred_ty, declared_ty]), boundness, ), } @@ -159,7 +159,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> Err((declared_ty, _)) => { // Intentionally ignore conflicting declared types; that's not our problem, // it's the problem of the module we are importing from. - declared_ty.inner_type().into() + Symbol::bound(declared_ty.inner_type()) } } @@ -187,7 +187,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> && file_to_module(db, scope.file(db)) .is_some_and(|module| module.is_known(KnownModule::Typing)) { - return Symbol::Type(Type::BooleanLiteral(true), Boundness::Bound); + return Symbol::bound(Type::BooleanLiteral(true)); } if name == "platform" && file_to_module(db, scope.file(db)) @@ -195,10 +195,7 @@ fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Symbol<'db> { match Program::get(db).python_platform(db) { crate::PythonPlatform::Identifier(platform) => { - return Symbol::Type( - Type::StringLiteral(StringLiteralType::new(db, platform.as_str())), - Boundness::Bound, - ); + return Symbol::bound(Type::string_literal(db, platform.as_str())); } crate::PythonPlatform::All => { // Fall through to the looked up type @@ -401,9 +398,16 @@ fn symbol_from_bindings<'db>( /// If we look up the declared type of `variable` in the scope of class `C`, we will get /// the type `int`, a "declaredness" of [`Boundness::PossiblyUnbound`], and the information /// that this comes with a [`TypeQualifiers::CLASS_VAR`] type qualifier. +#[derive(Debug)] pub(crate) struct SymbolAndQualifiers<'db>(Symbol<'db>, TypeQualifiers); impl SymbolAndQualifiers<'_> { + /// Constructor that creates a [`SymbolAndQualifiers`] instance with a [`TodoType`] type + /// and no qualifiers. + fn todo(message: &'static str) -> Self { + Self(Symbol::todo(message), TypeQualifiers::empty()) + } + fn is_class_var(&self) -> bool { self.1.contains(TypeQualifiers::CLASS_VAR) } @@ -419,12 +423,6 @@ impl<'db> From> for SymbolAndQualifiers<'db> { } } -impl<'db> From> for SymbolAndQualifiers<'db> { - fn from(ty: Type<'db>) -> Self { - SymbolAndQualifiers(ty.into(), TypeQualifiers::empty()) - } -} - /// The result of looking up a declared type from declarations; see [`symbol_from_declarations`]. type SymbolFromDeclarationsResult<'db> = Result, (TypeAndQualifiers<'db>, Box<[Type<'db>]>)>; @@ -560,6 +558,11 @@ macro_rules! todo_type { $crate::types::TodoType::Message($message), )) }; + ($message:ident) => { + $crate::types::Type::Dynamic($crate::types::DynamicType::Todo( + $crate::types::TodoType::Message($message), + )) + }; } #[cfg(not(debug_assertions))] @@ -570,6 +573,9 @@ macro_rules! todo_type { ($message:literal) => { $crate::types::Type::Dynamic($crate::types::DynamicType::Todo(crate::types::TodoType)) }; + ($message:ident) => { + $crate::types::Type::Dynamic($crate::types::DynamicType::Todo(crate::types::TodoType)) + }; } pub(crate) use todo_type; @@ -1688,17 +1694,17 @@ impl<'db> Type<'db> { #[must_use] pub(crate) fn member(&self, db: &'db dyn Db, name: &str) -> Symbol<'db> { if name == "__class__" { - return self.to_meta_type(db).into(); + return Symbol::bound(self.to_meta_type(db)); } match self { - Type::Dynamic(_) => self.into(), + Type::Dynamic(_) => Symbol::bound(self), - Type::Never => todo_type!("attribute lookup on Never").into(), + Type::Never => Symbol::todo("attribute lookup on Never"), Type::FunctionLiteral(_) => match name { - "__get__" => todo_type!("`__get__` method on functions").into(), - "__call__" => todo_type!("`__call__` method on functions").into(), + "__get__" => Symbol::todo("`__get__` method on functions"), + "__call__" => Symbol::todo("`__call__` method on functions"), _ => KnownClass::FunctionType.to_instance(db).member(db, name), }, @@ -1711,12 +1717,12 @@ impl<'db> Type<'db> { Type::KnownInstance(known_instance) => known_instance.member(db, name), Type::Instance(InstanceType { class }) => match (class.known(db), name) { - (Some(KnownClass::VersionInfo), "major") => { - Type::IntLiteral(Program::get(db).python_version(db).major.into()).into() - } - (Some(KnownClass::VersionInfo), "minor") => { - Type::IntLiteral(Program::get(db).python_version(db).minor.into()).into() - } + (Some(KnownClass::VersionInfo), "major") => Symbol::bound(Type::IntLiteral( + Program::get(db).python_version(db).major.into(), + )), + (Some(KnownClass::VersionInfo), "minor") => Symbol::bound(Type::IntLiteral( + Program::get(db).python_version(db).minor.into(), + )), _ => { let SymbolAndQualifiers(symbol, _) = class.instance_member(db, name); symbol @@ -1762,30 +1768,30 @@ impl<'db> Type<'db> { Type::Intersection(_) => { // TODO perform the get_member on each type in the intersection // TODO return the intersection of those results - todo_type!("Attribute access on `Intersection` types").into() + Symbol::todo("Attribute access on `Intersection` types") } Type::IntLiteral(_) => match name { - "real" | "numerator" => self.into(), + "real" | "numerator" => Symbol::bound(self), // TODO more attributes could probably be usefully special-cased _ => KnownClass::Int.to_instance(db).member(db, name), }, Type::BooleanLiteral(bool_value) => match name { - "real" | "numerator" => Type::IntLiteral(i64::from(*bool_value)).into(), + "real" | "numerator" => Symbol::bound(Type::IntLiteral(i64::from(*bool_value))), _ => KnownClass::Bool.to_instance(db).member(db, name), }, Type::StringLiteral(_) => { // TODO defer to `typing.LiteralString`/`builtins.str` methods // from typeshed's stubs - todo_type!("Attribute access on `StringLiteral` types").into() + Symbol::todo("Attribute access on `StringLiteral` types") } Type::LiteralString => { // TODO defer to `typing.LiteralString`/`builtins.str` methods // from typeshed's stubs - todo_type!("Attribute access on `LiteralString` types").into() + Symbol::todo("Attribute access on `LiteralString` types") } Type::BytesLiteral(_) => KnownClass::Bytes.to_instance(db).member(db, name), @@ -1797,13 +1803,13 @@ impl<'db> Type<'db> { Type::Tuple(_) => { // TODO: implement tuple methods - todo_type!("Attribute access on heterogeneous tuple types").into() + Symbol::todo("Attribute access on heterogeneous tuple types") } Type::AlwaysTruthy | Type::AlwaysFalsy => match name { "__bool__" => { // TODO should be `Callable[[], Literal[True/False]]` - todo_type!("`__bool__` for `AlwaysTruthy`/`AlwaysFalsy` Type variants").into() + Symbol::todo("`__bool__` for `AlwaysTruthy`/`AlwaysFalsy` Type variants") } _ => Type::object(db).member(db, name), }, @@ -2528,18 +2534,6 @@ impl<'db> From<&Type<'db>> for Type<'db> { } } -impl<'db> From> for Symbol<'db> { - fn from(value: Type<'db>) -> Self { - Symbol::Type(value, Boundness::Bound) - } -} - -impl<'db> From<&Type<'db>> for Symbol<'db> { - fn from(value: &Type<'db>) -> Self { - Self::from(*value) - } -} - #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq)] pub enum DynamicType { // An explicitly annotated `typing.Any` @@ -2572,7 +2566,7 @@ impl std::fmt::Display for DynamicType { bitflags! { /// Type qualifiers that appear in an annotation expression. - #[derive(Copy, Clone, Debug, Eq, PartialEq)] + #[derive(Copy, Clone, Debug, Eq, PartialEq, Default)] pub(crate) struct TypeQualifiers: u8 { /// `typing.ClassVar` const CLASS_VAR = 1 << 0; @@ -2599,6 +2593,14 @@ impl<'db> TypeAndQualifiers<'db> { Self { inner, qualifiers } } + /// Constructor that creates a [`TypeAndQualifiers`] instance with type `Unknown` and no qualifiers. + pub(crate) fn unknown() -> Self { + Self { + inner: Type::unknown(), + qualifiers: TypeQualifiers::empty(), + } + } + /// Forget about type qualifiers and only return the inner type. pub(crate) fn inner_type(&self) -> Type<'db> { self.inner @@ -3330,7 +3332,7 @@ impl<'db> KnownInstanceType<'db> { (Self::TypeAliasType(alias), "__name__") => Type::string_literal(db, alias.name(db)), _ => return self.instance_fallback(db).member(db, name), }; - ty.into() + Symbol::bound(ty) } } @@ -3788,8 +3790,7 @@ impl<'db> ModuleLiteralType<'db> { full_submodule_name.extend(&submodule_name); if imported_submodules.contains(&full_submodule_name) { if let Some(submodule) = resolve_module(db, &full_submodule_name) { - let submodule_ty = Type::module_literal(db, importing_file, submodule); - return Symbol::Type(submodule_ty, Boundness::Bound); + return Symbol::bound(Type::module_literal(db, importing_file, submodule)); } } } @@ -4123,7 +4124,7 @@ impl<'db> Class<'db> { pub(crate) fn class_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> { if name == "__mro__" { let tuple_elements = self.iter_mro(db).map(Type::from); - return TupleType::from_elements(db, tuple_elements).into(); + return Symbol::bound(TupleType::from_elements(db, tuple_elements)); } for superclass in self.iter_mro(db) { @@ -4163,7 +4164,9 @@ impl<'db> Class<'db> { for superclass in self.iter_mro(db) { match superclass { ClassBase::Dynamic(_) => { - return todo_type!("instance attribute on class with dynamic base").into(); + return SymbolAndQualifiers::todo( + "instance attribute on class with dynamic base", + ); } ClassBase::Class(class) => { if let member @ SymbolAndQualifiers(Symbol::Type(_, _), _) = @@ -4213,7 +4216,7 @@ impl<'db> Class<'db> { .and_then(|assignments| assignments.get(name)) else { if inferred_type_from_class_body.is_some() { - return union_of_inferred_types.build().into(); + return Symbol::bound(union_of_inferred_types.build()); } return Symbol::Unbound; }; @@ -4230,7 +4233,7 @@ impl<'db> Class<'db> { let annotation_ty = infer_expression_type(db, *annotation); // TODO: check if there are conflicting declarations - return annotation_ty.into(); + return Symbol::bound(annotation_ty); } AttributeAssignment::Unannotated { value } => { // We found an un-annotated attribute assignment of the form: @@ -4270,7 +4273,7 @@ impl<'db> Class<'db> { } } - union_of_inferred_types.build().into() + Symbol::bound(union_of_inferred_types.build()) } /// A helper function for `instance_member` that looks up the `name` attribute only on @@ -4299,12 +4302,12 @@ impl<'db> Class<'db> { // just a temporary heuristic to provide a broad categorization into properties // and non-property methods. if function.has_decorator(db, KnownClass::Property.to_class_literal(db)) { - todo_type!("@property").into() + SymbolAndQualifiers::todo("@property") } else { - todo_type!("bound method").into() + SymbolAndQualifiers::todo("bound method") } } else { - SymbolAndQualifiers(Symbol::Type(declared_ty, Boundness::Bound), qualifiers) + SymbolAndQualifiers(Symbol::bound(declared_ty), qualifiers) } } Ok(SymbolAndQualifiers(Symbol::Unbound, _)) => { @@ -4319,7 +4322,10 @@ impl<'db> Class<'db> { } Err((declared_ty, _conflicting_declarations)) => { // There are conflicting declarations for this attribute in the class body. - SymbolAndQualifiers(declared_ty.inner_type().into(), declared_ty.qualifiers()) + SymbolAndQualifiers( + Symbol::bound(declared_ty.inner_type()), + declared_ty.qualifiers(), + ) } } } else { diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 8fe12e1cee41b..9a5379b6393fc 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -116,7 +116,9 @@ fn infer_definition_types_cycle_recovery<'db>( let mut inference = TypeInference::empty(input.scope(db)); let category = input.category(db); if category.is_declaration() { - inference.declarations.insert(input, Type::unknown().into()); + inference + .declarations + .insert(input, TypeAndQualifiers::unknown()); } if category.is_binding() { inference.bindings.insert(input, Type::unknown()); @@ -919,7 +921,7 @@ impl<'db> TypeInferenceBuilder<'db> { inferred_ty.display(self.db()) ), ); - Type::unknown().into() + TypeAndQualifiers::unknown() }; self.types.declarations.insert(declaration, ty); } @@ -3439,19 +3441,17 @@ impl<'db> TypeInferenceBuilder<'db> { let value_ty = self.infer_expression(value); match value_ty.member(self.db(), &attr.id) { - Symbol::Type(member_ty, boundness) => { - if boundness == Boundness::PossiblyUnbound { - self.context.report_lint( - &POSSIBLY_UNBOUND_ATTRIBUTE, - attribute.into(), - format_args!( - "Attribute `{}` on type `{}` is possibly unbound", - attr.id, - value_ty.display(self.db()), - ), - ); - } - + Symbol::Type(member_ty, Boundness::Bound) => member_ty, + Symbol::Type(member_ty, Boundness::PossiblyUnbound) => { + self.context.report_lint( + &POSSIBLY_UNBOUND_ATTRIBUTE, + attribute.into(), + format_args!( + "Attribute `{}` on type `{}` is possibly unbound", + attr.id, + value_ty.display(self.db()), + ), + ); member_ty } Symbol::Unbound => { @@ -4845,7 +4845,7 @@ impl<'db> TypeInferenceBuilder<'db> { bytes.into(), format_args!("Type expressions cannot use bytes literal"), ); - Type::unknown().into() + TypeAndQualifiers::unknown() } ast::Expr::FString(fstring) => { @@ -4855,7 +4855,7 @@ impl<'db> TypeInferenceBuilder<'db> { format_args!("Type expressions cannot use f-strings"), ); self.infer_fstring_expression(fstring); - Type::unknown().into() + TypeAndQualifiers::unknown() } ast::Expr::Name(name) => match name.ctx { @@ -4876,7 +4876,7 @@ impl<'db> TypeInferenceBuilder<'db> { .into(), } } - ast::ExprContext::Invalid => Type::unknown().into(), + ast::ExprContext::Invalid => TypeAndQualifiers::unknown(), ast::ExprContext::Store | ast::ExprContext::Del => todo_type!().into(), }, @@ -4914,7 +4914,7 @@ impl<'db> TypeInferenceBuilder<'db> { inner_annotation_ty } else { self.infer_type_expression(slice); - Type::unknown().into() + TypeAndQualifiers::unknown() } } else { report_invalid_arguments_to_annotated( @@ -4983,7 +4983,7 @@ impl<'db> TypeInferenceBuilder<'db> { DeferredExpressionState::InStringAnnotation, ) } - None => Type::unknown().into(), + None => TypeAndQualifiers::unknown(), } } } From 7b487d853a81fee19a3630ba9a4b945852a4e9d5 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Tue, 11 Feb 2025 12:05:29 -0500 Subject: [PATCH 09/12] [`pydocstyle`] Handle arguments with the same names as sections (`D417`) (#16011) ## Summary Fixes #16007. The logic from the last fix for this (#9427) was sufficient, it just wasn't being applied because `Attributes` sections aren't expected to have nested sections. I just deleted the outer conditional, which should hopefully fix this for all section types. ## Test Plan New regression test, plus the existing D417 tests. --- .../test/fixtures/pydocstyle/D417.py | 43 ++++- crates/ruff_linter/src/docstrings/sections.rs | 149 +++++++++--------- ...rules__pydocstyle__tests__d417_google.snap | 9 ++ ...ts__d417_google_ignore_var_parameters.snap | 10 +- ...__pydocstyle__tests__d417_unspecified.snap | 9 ++ ...417_unspecified_ignore_var_parameters.snap | 9 ++ 6 files changed, 151 insertions(+), 78 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py b/crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py index d3f5910f3533b..7c5a1f538c856 100644 --- a/crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py +++ b/crates/ruff_linter/resources/test/fixtures/pydocstyle/D417.py @@ -176,4 +176,45 @@ def f(x, *args, **kwargs): x: the value *args: var-arguments """ - return x \ No newline at end of file + return x + + +# regression test for https://github.com/astral-sh/ruff/issues/16007. +# attributes is a section name without subsections, so it was failing the +# previous workaround for Args: args: sections +def send(payload: str, attributes: dict[str, Any]) -> None: + """ + Send a message. + + Args: + payload: + The message payload. + + attributes: + Additional attributes to be sent alongside the message. + """ + + +# undocumented argument with the same name as a section +def should_fail(payload, Args): + """ + Send a message. + + Args: + payload: + The message payload. + """ + + +# documented argument with the same name as a section +def should_not_fail(payload, Args): + """ + Send a message. + + Args: + payload: + The message payload. + + Args: + The other arguments. + """ diff --git a/crates/ruff_linter/src/docstrings/sections.rs b/crates/ruff_linter/src/docstrings/sections.rs index bed29daedb13d..edfc8bb7dbe5e 100644 --- a/crates/ruff_linter/src/docstrings/sections.rs +++ b/crates/ruff_linter/src/docstrings/sections.rs @@ -2,6 +2,7 @@ use std::fmt::{Debug, Formatter}; use std::iter::FusedIterator; use ruff_python_ast::docstrings::{leading_space, leading_words}; +use ruff_python_semantic::Definition; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use strum_macros::EnumIter; @@ -130,34 +131,6 @@ impl SectionKind { Self::Yields => "Yields", } } - - /// Returns `true` if a section can contain subsections, as in: - /// ```python - /// Yields - /// ------ - /// int - /// Description of the anonymous integer return value. - /// ``` - /// - /// For NumPy, see: - /// - /// For Google, see: - pub(crate) fn has_subsections(self) -> bool { - matches!( - self, - Self::Args - | Self::Arguments - | Self::OtherArgs - | Self::OtherParameters - | Self::OtherParams - | Self::Parameters - | Self::Raises - | Self::Returns - | Self::SeeAlso - | Self::Warns - | Self::Yields - ) - } } pub(crate) struct SectionContexts<'a> { @@ -195,6 +168,7 @@ impl<'a> SectionContexts<'a> { last.as_ref(), previous_line.as_ref(), lines.peek(), + docstring.definition, ) { if let Some(mut last) = last.take() { last.range = TextRange::new(last.start(), line.start()); @@ -444,6 +418,7 @@ fn suspected_as_section(line: &str, style: SectionStyle) -> Option } /// Check if the suspected context is really a section header. +#[allow(clippy::too_many_arguments)] fn is_docstring_section( line: &Line, indent_size: TextSize, @@ -452,7 +427,9 @@ fn is_docstring_section( previous_section: Option<&SectionContextData>, previous_line: Option<&Line>, next_line: Option<&Line>, + definition: &Definition<'_>, ) -> bool { + // for function definitions, track the known argument names for more accurate section detection. // Determine whether the current line looks like a section header, e.g., "Args:". let section_name_suffix = line[usize::from(indent_size + section_name_size)..].trim(); let this_looks_like_a_section_name = @@ -509,60 +486,80 @@ fn is_docstring_section( // ``` // However, if the header is an _exact_ match (like `Returns:`, as opposed to `returns:`), then // continue to treat it as a section header. - if section_kind.has_subsections() { - if let Some(previous_section) = previous_section { - let verbatim = &line[TextRange::at(indent_size, section_name_size)]; - - // If the section is more deeply indented, assume it's a subsection, as in: - // ```python - // def func(args: tuple[int]): - // """Toggle the gizmo. - // - // Args: - // args: The arguments to the function. - // """ - // ``` - if previous_section.indent_size < indent_size { - if section_kind.as_str() != verbatim { - return false; - } + if let Some(previous_section) = previous_section { + let verbatim = &line[TextRange::at(indent_size, section_name_size)]; + + // If the section is more deeply indented, assume it's a subsection, as in: + // ```python + // def func(args: tuple[int]): + // """Toggle the gizmo. + // + // Args: + // args: The arguments to the function. + // """ + // ``` + // As noted above, an exact match for a section name (like the inner `Args:` below) is + // treated as a section header, unless the enclosing `Definition` is a function and contains + // a parameter with the same name, as in: + // ```python + // def func(Args: tuple[int]): + // """Toggle the gizmo. + // + // Args: + // Args: The arguments to the function. + // """ + // ``` + if previous_section.indent_size < indent_size { + let section_name = section_kind.as_str(); + if section_name != verbatim || has_parameter(definition, section_name) { + return false; } + } - // If the section has a preceding empty line, assume it's _not_ a subsection, as in: - // ```python - // def func(args: tuple[int]): - // """Toggle the gizmo. - // - // Args: - // args: The arguments to the function. - // - // returns: - // The return value of the function. - // """ - // ``` - if previous_line.is_some_and(|line| line.trim().is_empty()) { - return true; - } + // If the section has a preceding empty line, assume it's _not_ a subsection, as in: + // ```python + // def func(args: tuple[int]): + // """Toggle the gizmo. + // + // Args: + // args: The arguments to the function. + // + // returns: + // The return value of the function. + // """ + // ``` + if previous_line.is_some_and(|line| line.trim().is_empty()) { + return true; + } - // If the section isn't underlined, and isn't title-cased, assume it's a subsection, - // as in: - // ```python - // def func(parameters: tuple[int]): - // """Toggle the gizmo. - // - // Parameters: - // ----- - // parameters: - // The arguments to the function. - // """ - // ``` - if !next_line_is_underline && verbatim.chars().next().is_some_and(char::is_lowercase) { - if section_kind.as_str() != verbatim { - return false; - } + // If the section isn't underlined, and isn't title-cased, assume it's a subsection, + // as in: + // ```python + // def func(parameters: tuple[int]): + // """Toggle the gizmo. + // + // Parameters: + // ----- + // parameters: + // The arguments to the function. + // """ + // ``` + if !next_line_is_underline && verbatim.chars().next().is_some_and(char::is_lowercase) { + if section_kind.as_str() != verbatim { + return false; } } } true } + +/// Returns whether or not `definition` is a function definition and contains a parameter with the +/// same name as `section_name`. +fn has_parameter(definition: &Definition, section_name: &str) -> bool { + definition.as_function_def().is_some_and(|func| { + func.parameters + .iter() + .any(|param| param.name() == section_name) + }) +} diff --git a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google.snap b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google.snap index 4d728dac52133..f99e5d73fb9ca 100644 --- a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google.snap +++ b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google.snap @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw | ^ D417 173 | """Do something. | + +D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args` + | +198 | # undocumented argument with the same name as a section +199 | def should_fail(payload, Args): + | ^^^^^^^^^^^ D417 +200 | """ +201 | Send a message. + | diff --git a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google_ignore_var_parameters.snap b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google_ignore_var_parameters.snap index fd0c4d5dd0a42..d09b599a0995e 100644 --- a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google_ignore_var_parameters.snap +++ b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_google_ignore_var_parameters.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/pydocstyle/mod.rs -snapshot_kind: text --- D417.py:1:5: D417 Missing argument descriptions in the docstring for `f`: `y`, `z` | @@ -65,3 +64,12 @@ D417.py:155:5: D417 Missing argument description in the docstring for `select_da 156 | query: str, 157 | args: tuple, | + +D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args` + | +198 | # undocumented argument with the same name as a section +199 | def should_fail(payload, Args): + | ^^^^^^^^^^^ D417 +200 | """ +201 | Send a message. + | diff --git a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified.snap b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified.snap index 4d728dac52133..f99e5d73fb9ca 100644 --- a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified.snap +++ b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified.snap @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw | ^ D417 173 | """Do something. | + +D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args` + | +198 | # undocumented argument with the same name as a section +199 | def should_fail(payload, Args): + | ^^^^^^^^^^^ D417 +200 | """ +201 | Send a message. + | diff --git a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified_ignore_var_parameters.snap b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified_ignore_var_parameters.snap index 4d728dac52133..f99e5d73fb9ca 100644 --- a/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified_ignore_var_parameters.snap +++ b/crates/ruff_linter/src/rules/pydocstyle/snapshots/ruff_linter__rules__pydocstyle__tests__d417_unspecified_ignore_var_parameters.snap @@ -80,3 +80,12 @@ D417.py:172:5: D417 Missing argument description in the docstring for `f`: `**kw | ^ D417 173 | """Do something. | + +D417.py:199:5: D417 Missing argument description in the docstring for `should_fail`: `Args` + | +198 | # undocumented argument with the same name as a section +199 | def should_fail(payload, Args): + | ^^^^^^^^^^^ D417 +200 | """ +201 | Send a message. + | From ce31c2693bf35592c13589f4b61f2f68ed1834fd Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 11 Feb 2025 18:38:41 +0000 Subject: [PATCH 10/12] Fix release build warning about unused todo type message (#16102) --- crates/red_knot_python_semantic/src/symbol.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index e7423942651e0..354c271f6a409 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -40,6 +40,7 @@ impl<'db> Symbol<'db> { /// Constructor that creates a [`Symbol`] with a [`crate::types::TodoType`] type /// and boundness [`Boundness::Bound`]. + #[allow(unused_variables)] pub(crate) fn todo(message: &'static str) -> Self { Symbol::Type(todo_type!(message), Boundness::Bound) } From 9c179314ed4408e2b1ce802f36fa4941aa43d651 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 11 Feb 2025 18:55:50 +0000 Subject: [PATCH 11/12] Remove `Hash` and `Eq` from `AstNodeRef` for types not implementing `Eq` or `Hash` (#16100) ## Summary This is a follow up to https://github.com/astral-sh/ruff/pull/15763#discussion_r1949681336 It reverts the change to using ptr equality for `AstNodeRef`s, which in turn removes the `Eq`, `PartialEq`, and `Hash` implementations for `AstNodeRef`s parametrized with AST nodes. Cheap comparisons shouldn't be needed because the node field is generally marked as `[#tracked]` and `#[no_eq]` and removing the implementations even enforces that those attributes are set on all `AstNodeRef` fields (which is good). The only downside this has is that we technically wouldn't have to mark the `Unpack::target` as `#[tracked]` because the `target` field is accessed in every query accepting `Unpack` as an argument. Overall, enforcing the use of `#[tracked]` seems like a good trade off, espacially considering that it's very likely that we'd probably forget to mark the `Unpack::target` field as tracked if we add a new `Unpack` query that doesn't access the target. ## Test Plan `cargo test` --- .../src/ast_node_ref.rs | 47 ++++++++++++++----- .../src/semantic_index/definition.rs | 16 +++---- crates/red_knot_python_semantic/src/unpack.rs | 1 + 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/crates/red_knot_python_semantic/src/ast_node_ref.rs b/crates/red_knot_python_semantic/src/ast_node_ref.rs index 667b8f6363ecc..ad4b03e9afe3d 100644 --- a/crates/red_knot_python_semantic/src/ast_node_ref.rs +++ b/crates/red_knot_python_semantic/src/ast_node_ref.rs @@ -31,17 +31,13 @@ use ruff_db::parsed::ParsedModule; /// This means that changes to expressions in other scopes don't invalidate the expression's id, giving /// us some form of scope-stable identity for expressions. Only queries accessing the node field /// run on every AST change. All other queries only run when the expression's identity changes. -/// -/// The one exception to this is if it is known that all queries tacking the tracked struct -/// as argument or returning it as part of their result are known to access the node field. -/// Marking the field tracked is then unnecessary. #[derive(Clone)] pub struct AstNodeRef { /// Owned reference to the node's [`ParsedModule`]. /// /// The node's reference is guaranteed to remain valid as long as it's enclosing /// [`ParsedModule`] is alive. - _parsed: ParsedModule, + parsed: ParsedModule, /// Pointer to the referenced node. node: std::ptr::NonNull, @@ -59,7 +55,7 @@ impl AstNodeRef { /// the invariant `node belongs to parsed` is upheld. pub(super) unsafe fn new(parsed: ParsedModule, node: &T) -> Self { Self { - _parsed: parsed, + parsed, node: std::ptr::NonNull::from(node), } } @@ -89,17 +85,44 @@ where } } -impl PartialEq for AstNodeRef { +impl PartialEq for AstNodeRef +where + T: PartialEq, +{ fn eq(&self, other: &Self) -> bool { - self.node.eq(&other.node) + if self.parsed == other.parsed { + // Comparing the pointer addresses is sufficient to determine equality + // if the parsed are the same. + self.node.eq(&other.node) + } else { + // Otherwise perform a deep comparison. + self.node().eq(other.node()) + } } } -impl Eq for AstNodeRef {} +impl Eq for AstNodeRef where T: Eq {} -impl Hash for AstNodeRef { +impl Hash for AstNodeRef +where + T: Hash, +{ fn hash(&self, state: &mut H) { - self.node.hash(state); + self.node().hash(state); + } +} + +#[allow(unsafe_code)] +unsafe impl salsa::Update for AstNodeRef { + unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { + let old_ref = &mut (*old_pointer); + + if old_ref.parsed == new_value.parsed && old_ref.node.eq(&new_value.node) { + false + } else { + *old_ref = new_value; + true + } } } @@ -133,7 +156,7 @@ mod tests { let stmt_cloned = &cloned.syntax().body[0]; let cloned_node = unsafe { AstNodeRef::new(cloned.clone(), stmt_cloned) }; - assert_ne!(node1, cloned_node); + assert_eq!(node1, cloned_node); let other_raw = parse_unchecked_source("2 + 2", PySourceType::Python); let other = ParsedModule::new(other_raw); diff --git a/crates/red_knot_python_semantic/src/semantic_index/definition.rs b/crates/red_knot_python_semantic/src/semantic_index/definition.rs index 830ad0ffd4b19..adc13675603ec 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/definition.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/definition.rs @@ -439,7 +439,7 @@ impl DefinitionCategory { /// [`DefinitionKind`] fields in salsa tracked structs should be tracked (attributed with `#[tracked]`) /// because the kind is a thin wrapper around [`AstNodeRef`]. See the [`AstNodeRef`] documentation /// for an in-depth explanation of why this is necessary. -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub enum DefinitionKind<'db> { Import(AstNodeRef), ImportFrom(ImportFromDefinitionKind), @@ -559,7 +559,7 @@ impl<'db> From>> for TargetKind<'db> { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] #[allow(dead_code)] pub struct MatchPatternDefinitionKind { pattern: AstNodeRef, @@ -577,7 +577,7 @@ impl MatchPatternDefinitionKind { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct ComprehensionDefinitionKind { iterable: AstNodeRef, target: AstNodeRef, @@ -603,7 +603,7 @@ impl ComprehensionDefinitionKind { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct ImportFromDefinitionKind { node: AstNodeRef, alias_index: usize, @@ -619,7 +619,7 @@ impl ImportFromDefinitionKind { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct AssignmentDefinitionKind<'db> { target: TargetKind<'db>, value: AstNodeRef, @@ -645,7 +645,7 @@ impl<'db> AssignmentDefinitionKind<'db> { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct WithItemDefinitionKind { node: AstNodeRef, target: AstNodeRef, @@ -666,7 +666,7 @@ impl WithItemDefinitionKind { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct ForStmtDefinitionKind<'db> { target: TargetKind<'db>, iterable: AstNodeRef, @@ -697,7 +697,7 @@ impl<'db> ForStmtDefinitionKind<'db> { } } -#[derive(Clone, Debug, Hash)] +#[derive(Clone, Debug)] pub struct ExceptHandlerDefinitionKind { handler: AstNodeRef, is_star: bool, diff --git a/crates/red_knot_python_semantic/src/unpack.rs b/crates/red_knot_python_semantic/src/unpack.rs index 05a396d4dc062..ad6eac413d01c 100644 --- a/crates/red_knot_python_semantic/src/unpack.rs +++ b/crates/red_knot_python_semantic/src/unpack.rs @@ -36,6 +36,7 @@ pub(crate) struct Unpack<'db> { /// expression is `(a, b)`. #[no_eq] #[return_ref] + #[tracked] pub(crate) target: AstNodeRef, /// The ingredient representing the value expression of the unpacking. For example, in From 6e34f74c164a646f07f3fe9bed9492c3557a95d7 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Tue, 11 Feb 2025 14:55:12 -0500 Subject: [PATCH 12/12] add diagnostic `Span` (couples `File` and `TextRange`) (#16101) This essentially makes it impossible to construct a `Diagnostic` that has a `TextRange` but no `File`. This is meant to be a precursor to multi-span support. (Note that I consider this more of a prototyping-change and not necessarily what this is going to look like longer term.) Reviewers can probably review this PR as one big diff instead of commit-by-commit. --- crates/red_knot_project/src/lib.rs | 19 +-- .../red_knot_project/src/metadata/options.rs | 39 +++--- .../src/types/diagnostic.rs | 10 +- .../src/server/api/requests/diagnostic.rs | 10 +- crates/red_knot_test/src/diagnostic.rs | 13 +- crates/red_knot_test/src/matcher.rs | 13 +- crates/ruff_benchmark/benches/red_knot.rs | 10 +- crates/ruff_db/src/diagnostic.rs | 115 ++++++++++-------- 8 files changed, 115 insertions(+), 114 deletions(-) diff --git a/crates/red_knot_project/src/lib.rs b/crates/red_knot_project/src/lib.rs index 3d8ee4ae4ce17..2126f36a841e9 100644 --- a/crates/red_knot_project/src/lib.rs +++ b/crates/red_knot_project/src/lib.rs @@ -8,14 +8,13 @@ pub use metadata::{ProjectDiscoveryError, ProjectMetadata}; use red_knot_python_semantic::lint::{LintRegistry, LintRegistryBuilder, RuleSelection}; use red_knot_python_semantic::register_lints; use red_knot_python_semantic::types::check_types; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, ParseDiagnostic, Severity}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, ParseDiagnostic, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::parsed::parsed_module; use ruff_db::source::{source_text, SourceTextError}; use ruff_db::system::walk_directory::WalkState; use ruff_db::system::{FileType, SystemPath}; use ruff_python_ast::PySourceType; -use ruff_text_size::TextRange; use rustc_hash::{FxBuildHasher, FxHashSet}; use salsa::Durability; use salsa::Setter; @@ -345,7 +344,13 @@ fn check_file_impl(db: &dyn Db, file: File) -> Vec> { boxed })); - diagnostics.sort_unstable_by_key(|diagnostic| diagnostic.range().unwrap_or_default().start()); + diagnostics.sort_unstable_by_key(|diagnostic| { + diagnostic + .span() + .and_then(|span| span.range()) + .unwrap_or_default() + .start() + }); diagnostics } @@ -458,12 +463,8 @@ impl Diagnostic for IOErrorDiagnostic { self.error.to_string().into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - None + fn span(&self) -> Option { + Some(Span::from(self.file)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index 401dcf1233345..09a167ca98ae0 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -4,11 +4,10 @@ use red_knot_python_semantic::lint::{GetLintError, Level, LintSource, RuleSelect use red_knot_python_semantic::{ ProgramSettings, PythonPlatform, PythonVersion, SearchPathSettings, SitePackages, }; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; -use ruff_db::files::{system_path_to_file, File}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; +use ruff_db::files::system_path_to_file; use ruff_db::system::{System, SystemPath}; use ruff_macros::Combine; -use ruff_text_size::TextRange; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use std::borrow::Cow; @@ -189,7 +188,14 @@ impl Options { ), }; - diagnostics.push(diagnostic.with_file(file).with_range(rule_name.range())); + let span = file.map(Span::from).map(|span| { + if let Some(range) = rule_name.range() { + span.with_range(range) + } else { + span + } + }); + diagnostics.push(diagnostic.with_span(span)); } } } @@ -348,8 +354,7 @@ pub struct OptionDiagnostic { id: DiagnosticId, message: String, severity: Severity, - file: Option, - range: Option, + span: Option, } impl OptionDiagnostic { @@ -358,21 +363,13 @@ impl OptionDiagnostic { id, message, severity, - file: None, - range: None, + span: None, } } #[must_use] - fn with_file(mut self, file: Option) -> Self { - self.file = file; - self - } - - #[must_use] - fn with_range(mut self, range: Option) -> Self { - self.range = range; - self + fn with_span(self, span: Option) -> Self { + OptionDiagnostic { span, ..self } } } @@ -385,12 +382,8 @@ impl Diagnostic for OptionDiagnostic { Cow::Borrowed(&self.message) } - fn file(&self) -> Option { - self.file - } - - fn range(&self) -> Option { - self.range + fn span(&self) -> Option { + self.span.clone() } fn severity(&self) -> Severity { diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 99d07dd9f973a..2f08b84f88bc0 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -8,7 +8,7 @@ use crate::types::string_annotation::{ }; use crate::types::{ClassLiteralType, KnownInstanceType, Type}; use crate::{declare_lint, Db}; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; use ruff_db::files::File; use ruff_python_ast::{self as ast, AnyNodeRef}; use ruff_text_size::{Ranged, TextRange}; @@ -802,12 +802,8 @@ impl Diagnostic for TypeCheckDiagnostic { TypeCheckDiagnostic::message(self).into() } - fn file(&self) -> Option { - Some(TypeCheckDiagnostic::file(self)) - } - - fn range(&self) -> Option { - Some(Ranged::range(self)) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_server/src/server/api/requests/diagnostic.rs b/crates/red_knot_server/src/server/api/requests/diagnostic.rs index 968c05abe65e0..ba788817ec4c0 100644 --- a/crates/red_knot_server/src/server/api/requests/diagnostic.rs +++ b/crates/red_knot_server/src/server/api/requests/diagnostic.rs @@ -75,11 +75,13 @@ fn to_lsp_diagnostic( diagnostic: &dyn ruff_db::diagnostic::Diagnostic, encoding: crate::PositionEncoding, ) -> Diagnostic { - let range = if let (Some(file), Some(range)) = (diagnostic.file(), diagnostic.range()) { - let index = line_index(db.upcast(), file); - let source = source_text(db.upcast(), file); + let range = if let Some(span) = diagnostic.span() { + let index = line_index(db.upcast(), span.file()); + let source = source_text(db.upcast(), span.file()); - range.to_range(&source, &index, encoding) + span.range() + .map(|range| range.to_range(&source, &index, encoding)) + .unwrap_or_default() } else { Range::default() }; diff --git a/crates/red_knot_test/src/diagnostic.rs b/crates/red_knot_test/src/diagnostic.rs index bb5a099b50ef1..56ee51223cc8b 100644 --- a/crates/red_knot_test/src/diagnostic.rs +++ b/crates/red_knot_test/src/diagnostic.rs @@ -26,7 +26,8 @@ where .into_iter() .map(|diagnostic| DiagnosticWithLine { line_number: diagnostic - .range() + .span() + .and_then(|span| span.range()) .map_or(OneIndexed::from_zero_indexed(0), |range| { line_index.line_index(range.start()) }), @@ -144,7 +145,7 @@ struct DiagnosticWithLine { mod tests { use crate::db::Db; use crate::diagnostic::Diagnostic; - use ruff_db::diagnostic::{DiagnosticId, LintName, Severity}; + use ruff_db::diagnostic::{DiagnosticId, LintName, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::source::line_index; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; @@ -198,12 +199,8 @@ mod tests { "dummy".into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.range) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_test/src/matcher.rs b/crates/red_knot_test/src/matcher.rs index e7add5c50adb2..d350ec7c61c3e 100644 --- a/crates/red_knot_test/src/matcher.rs +++ b/crates/red_knot_test/src/matcher.rs @@ -257,7 +257,8 @@ impl Matcher { fn column(&self, diagnostic: &T) -> OneIndexed { diagnostic - .range() + .span() + .and_then(|span| span.range()) .map(|range| { self.line_index .source_location(range.start(), &self.source) @@ -334,7 +335,7 @@ impl Matcher { #[cfg(test)] mod tests { use super::FailuresByLine; - use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; + use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; use ruff_python_trivia::textwrap::dedent; @@ -385,12 +386,8 @@ mod tests { self.message.into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.range) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 5c6362dc0ac0c..87366b04dec1b 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -229,8 +229,14 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Box]) { .map(|diagnostic| { ( diagnostic.id(), - diagnostic.file().map(|file| file.path(db).as_str()), - diagnostic.range().map(Range::::from), + diagnostic + .span() + .map(|span| span.file()) + .map(|file| file.path(db).as_str()), + diagnostic + .span() + .and_then(|span| span.range()) + .map(Range::::from), diagnostic.message(), diagnostic.severity(), ) diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs index 4b3be76ab9f14..e342014b3f97e 100644 --- a/crates/ruff_db/src/diagnostic.rs +++ b/crates/ruff_db/src/diagnostic.rs @@ -164,19 +164,11 @@ pub trait Diagnostic: Send + Sync + std::fmt::Debug { fn message(&self) -> Cow; - /// The file this diagnostic is associated with. - /// - /// File can be `None` for diagnostics that don't originate from a file. - /// For example: - /// * A diagnostic indicating that a directory couldn't be read. - /// * A diagnostic related to a CLI argument - fn file(&self) -> Option; - - /// The primary range of the diagnostic in `file`. + /// The primary span of the diagnostic. /// /// The range can be `None` if the diagnostic doesn't have a file /// or it applies to the entire file (e.g. the file should be executable but isn't). - fn range(&self) -> Option; + fn span(&self) -> Option; fn severity(&self) -> Severity; @@ -191,6 +183,47 @@ pub trait Diagnostic: Send + Sync + std::fmt::Debug { } } +/// A span represents the source of a diagnostic. +/// +/// It consists of a `File` and an optional range into that file. When the +/// range isn't present, it semantically implies that the diagnostic refers to +/// the entire file. For example, when the file should be executable but isn't. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Span { + file: File, + range: Option, +} + +impl Span { + /// Returns the `File` attached to this `Span`. + pub fn file(&self) -> File { + self.file + } + + /// Returns the range, if available, attached to this `Span`. + /// + /// When there is no range, it is convention to assume that this `Span` + /// refers to the corresponding `File` as a whole. In some cases, consumers + /// of this API may use the range `0..0` to represent this case. + pub fn range(&self) -> Option { + self.range + } + + /// Returns a new `Span` with the given `range` attached to it. + pub fn with_range(self, range: TextRange) -> Span { + Span { + range: Some(range), + ..self + } + } +} + +impl From for Span { + fn from(file: File) -> Span { + Span { file, range: None } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] pub enum Severity { Info, @@ -236,8 +269,8 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { let rendered = renderer.render(message); writeln!(f, "{rendered}") }; - match (self.diagnostic.file(), self.diagnostic.range()) { - (None, _) => { + match self.diagnostic.span() { + None => { // NOTE: This is pretty sub-optimal. It doesn't render well. We // really want a snippet, but without a `File`, we can't really // render anything. It looks like this case currently happens @@ -248,20 +281,20 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { let msg = format!("{}: {}", self.diagnostic.id(), self.diagnostic.message()); render(f, level.title(&msg)) } - (Some(file), range) => { - let path = file.path(self.db).to_string(); - let source = source_text(self.db, file); + Some(span) => { + let path = span.file.path(self.db).to_string(); + let source = source_text(self.db, span.file); let title = self.diagnostic.id().to_string(); let message = self.diagnostic.message(); - let Some(range) = range else { + let Some(range) = span.range else { let snippet = Snippet::source(source.as_str()).origin(&path).line_start(1); return render(f, level.title(&title).snippet(snippet)); }; // The bits below are a simplified copy from // `crates/ruff_linter/src/message/text.rs`. - let index = line_index(self.db, file); + let index = line_index(self.db, span.file); let source_code = SourceCode::new(source.as_str(), &index); let content_start_index = source_code.line_index(range.start()); @@ -315,12 +348,8 @@ where (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -340,12 +369,8 @@ where (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -362,12 +387,8 @@ impl Diagnostic for Box { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -384,12 +405,8 @@ impl Diagnostic for &'_ dyn Diagnostic { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -406,12 +423,8 @@ impl Diagnostic for std::sync::Arc { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -440,12 +453,8 @@ impl Diagnostic for ParseDiagnostic { self.error.error.to_string().into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.error.location) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.error.location)) } fn severity(&self) -> Severity {