diff --git a/Cargo.lock b/Cargo.lock index c66d588320f19..105f961029ea2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -413,7 +413,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -693,7 +693,7 @@ dependencies = [ "proc-macro2", "quote", "strsim 0.10.0", - "syn", + "syn 2.0.90", ] [[package]] @@ -704,7 +704,7 @@ checksum = "a668eda54683121533a393014d8692171709ff57a7d61f187b6e782719f8933f" dependencies = [ "darling_core", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -774,7 +774,7 @@ dependencies = [ "glob", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -826,7 +826,7 @@ checksum = "97369cbbc041bc366949bc74d34658d6cda5621039731c6310521892a3a20ae0" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -1246,7 +1246,7 @@ checksum = "1ec89e9337638ecdc08744df490b221a7399bf8d164eb52a665454e60e075ad6" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -1420,7 +1420,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -1547,7 +1547,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2ae40017ac09cd2c6a53504cb3c871c7f2b41466eac5bc66ba63f39073b467b" dependencies = [ "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -2013,7 +2013,7 @@ dependencies = [ "pest_meta", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -2173,6 +2173,26 @@ dependencies = [ "memchr", ] +[[package]] +name = "quickcheck" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "588f6378e4dd99458b60ec275b4477add41ce4fa9f64dcba6f15adccb19b50d6" +dependencies = [ + "rand", +] + +[[package]] +name = "quickcheck_macros" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b22a693222d716a9587786f37ac3f6b4faedb5b80c23914e7303ff5a1d8016e9" +dependencies = [ + "proc-macro2", + "quote", + "syn 1.0.109", +] + [[package]] name = "quote" version = "1.0.37" @@ -2273,6 +2293,8 @@ dependencies = [ "itertools 0.13.0", "memchr", "ordermap", + "quickcheck", + "quickcheck_macros", "red_knot_test", "red_knot_vendored", "ruff_db", @@ -2777,7 +2799,7 @@ dependencies = [ "proc-macro2", "quote", "ruff_python_trivia", - "syn", + "syn 2.0.90", ] [[package]] @@ -3197,7 +3219,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn", + "syn 2.0.90", "synstructure", ] @@ -3231,7 +3253,7 @@ dependencies = [ "proc-macro2", "quote", "serde_derive_internals", - "syn", + "syn 2.0.90", ] [[package]] @@ -3280,7 +3302,7 @@ checksum = "ad1e866f866923f252f05c889987993144fb74e722403468a4ebd70c3cd756c0" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3291,7 +3313,7 @@ checksum = "330f01ce65a3a5fe59a60c82f3c9a024b573b8a6e875bd233fe5f934e71d54e3" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3314,7 +3336,7 @@ checksum = "6c64451ba24fc7a6a2d60fc75dd9c83c90903b19028d4eff35e88fc1e86564e9" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3355,7 +3377,7 @@ dependencies = [ "darling", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3463,7 +3485,7 @@ dependencies = [ "proc-macro2", "quote", "rustversion", - "syn", + "syn 2.0.90", ] [[package]] @@ -3472,6 +3494,17 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "81cdd64d312baedb58e21336b31bc043b77e01cc99033ce76ef539f78e965ebc" +[[package]] +name = "syn" +version = "1.0.109" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + [[package]] name = "syn" version = "2.0.90" @@ -3491,7 +3524,7 @@ checksum = "c8af7666ab7b6390ab78131fb5b0fce11d6b7a6951602017c35fa82800708971" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3554,7 +3587,7 @@ dependencies = [ "cfg-if", "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3565,7 +3598,7 @@ checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", "test-case-core", ] @@ -3595,7 +3628,7 @@ checksum = "b607164372e89797d78b8e23a6d67d5d1038c1c65efd52e1389ef8b77caba2a6" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3606,7 +3639,7 @@ checksum = "f077553d607adc1caf65430528a576c757a71ed73944b66ebb58ef2bbd243568" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -3728,7 +3761,7 @@ checksum = "395ae124c09f9e6918a2310af6038fba074bcf474ac352496d5910dd59a2226d" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -4001,7 +4034,7 @@ checksum = "6b91f57fe13a38d0ce9e28a03463d8d3c2468ed03d75375110ec71d93b449a08" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -4096,7 +4129,7 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "syn", + "syn 2.0.90", "wasm-bindgen-shared", ] @@ -4131,7 +4164,7 @@ checksum = "98c9ae5a76e46f4deecd0f0255cc223cfa18dc9b261213b8aa0c7b36f61b3f1d" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -4165,7 +4198,7 @@ checksum = "222ebde6ea87fbfa6bdd2e9f1fd8a91d60aee5db68792632176c4e16a74fc7d8" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -4468,7 +4501,7 @@ checksum = "28cc31741b18cb6f1d5ff12f5b7523e3d6eb0852bbbad19d73905511d9849b95" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", "synstructure", ] @@ -4489,7 +4522,7 @@ checksum = "9ce1b18ccd8e73a9321186f97e46f9f04b778851177567b1975109d26a08d2a6" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] @@ -4509,7 +4542,7 @@ checksum = "0ea7b4a3637ea8669cedf0f1fd5c286a17f3de97b8dd5a70a6c167a1730e63a5" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", "synstructure", ] @@ -4538,7 +4571,7 @@ checksum = "6eafa6dfb17584ea3e2bd6e76e0cc15ad7af12b09abdd1ca55961bed9b1063c6" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.90", ] [[package]] diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index f063bcf3d5886..aeda6c5492df3 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -1,25 +1,23 @@ #![allow(clippy::disallowed_names)] use std::io::Write; -use std::time::Duration; +use std::time::{Duration, Instant}; use anyhow::{anyhow, Context}; use red_knot_python_semantic::{resolve_module, ModuleName, Program, PythonVersion, SitePackages}; use red_knot_workspace::db::{Db, RootDatabase}; -use red_knot_workspace::watch; -use red_knot_workspace::watch::{directory_watcher, WorkspaceWatcher}; +use red_knot_workspace::watch::{directory_watcher, ChangeEvent, WorkspaceWatcher}; use red_knot_workspace::workspace::settings::{Configuration, SearchPathConfiguration}; use red_knot_workspace::workspace::WorkspaceMetadata; 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::testing::{setup_logging, setup_logging_with_filter}; use ruff_db::Upcast; struct TestCase { db: RootDatabase, watcher: Option, - changes_receiver: crossbeam::channel::Receiver>, + changes_receiver: crossbeam::channel::Receiver>, /// The temporary directory that contains the test files. /// We need to hold on to it in the test case or the temp files get deleted. _temp_dir: tempfile::TempDir, @@ -40,12 +38,36 @@ impl TestCase { &self.db } - fn stop_watch(&mut self) -> Vec { - self.try_stop_watch(Duration::from_secs(10)) - .expect("Expected watch changes but observed none") + #[track_caller] + fn stop_watch(&mut self, matcher: M) -> Vec + where + M: MatchEvent, + { + // track_caller is unstable for lambdas -> That's why this is a fn + #[track_caller] + fn panic_with_formatted_events(events: Vec) -> Vec { + panic!( + "Didn't observe expected change:\n{}", + events + .into_iter() + .map(|event| format!(" - {event:?}")) + .collect::>() + .join("\n") + ) + } + + self.try_stop_watch(matcher, Duration::from_secs(10)) + .unwrap_or_else(panic_with_formatted_events) } - fn try_stop_watch(&mut self, timeout: Duration) -> Option> { + fn try_stop_watch( + &mut self, + mut matcher: M, + timeout: Duration, + ) -> Result, Vec> + where + M: MatchEvent, + { tracing::debug!("Try stopping watch with timeout {:?}", timeout); let watcher = self @@ -53,36 +75,50 @@ impl TestCase { .take() .expect("Cannot call `stop_watch` more than once"); - let mut all_events = self - .changes_receiver - .recv_timeout(timeout) - .unwrap_or_default(); + let start = Instant::now(); + let mut all_events = Vec::new(); + + loop { + let events = self + .changes_receiver + .recv_timeout(Duration::from_millis(100)) + .unwrap_or_default(); + + if events + .iter() + .any(|event| matcher.match_event(event) || event.is_rescan()) + { + all_events.extend(events); + break; + } + + all_events.extend(events); + + if start.elapsed() > timeout { + return Err(all_events); + } + } watcher.flush(); tracing::debug!("Flushed file watcher"); watcher.stop(); tracing::debug!("Stopping file watcher"); + // Consume remaining events for event in &self.changes_receiver { all_events.extend(event); } - if all_events.is_empty() { - return None; - } - - Some(all_events) + Ok(all_events) } - fn take_watch_changes(&self) -> Vec { + fn take_watch_changes(&self) -> Vec { self.try_take_watch_changes(Duration::from_secs(10)) .expect("Expected watch changes but observed none") } - fn try_take_watch_changes(&self, timeout: Duration) -> Option> { - let Some(watcher) = &self.watcher else { - return None; - }; + fn try_take_watch_changes(&self, timeout: Duration) -> Option> { + let watcher = self.watcher.as_ref()?; let mut all_events = self .changes_receiver @@ -104,7 +140,7 @@ impl TestCase { Some(all_events) } - fn apply_changes(&mut self, changes: Vec) { + fn apply_changes(&mut self, changes: Vec) { self.db.apply_changes(changes, Some(&self.configuration)); } @@ -140,6 +176,23 @@ impl TestCase { } } +trait MatchEvent { + fn match_event(&mut self, event: &ChangeEvent) -> bool; +} + +fn event_for_file(name: &str) -> impl MatchEvent + '_ { + |event: &ChangeEvent| event.file_name() == Some(name) +} + +impl MatchEvent for F +where + F: FnMut(&ChangeEvent) -> bool, +{ + fn match_event(&mut self, event: &ChangeEvent) -> bool { + (*self)(event) + } +} + trait SetupFiles { fn setup(self, root_path: &SystemPath, workspace_path: &SystemPath) -> anyhow::Result<()>; } @@ -315,7 +368,7 @@ fn new_file() -> anyhow::Result<()> { std::fs::write(foo_path.as_std_path(), "print('Hello')")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -338,7 +391,7 @@ fn new_ignored_file() -> anyhow::Result<()> { std::fs::write(foo_path.as_std_path(), "print('Hello')")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -360,7 +413,7 @@ fn changed_file() -> anyhow::Result<()> { update_file(&foo_path, "print('Version 2')")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); assert!(!changes.is_empty()); @@ -385,7 +438,7 @@ fn deleted_file() -> anyhow::Result<()> { std::fs::remove_file(foo_path.as_std_path())?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -417,7 +470,7 @@ fn move_file_to_trash() -> anyhow::Result<()> { trash_path.join("foo.py").as_std_path(), )?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -449,7 +502,7 @@ fn move_file_to_workspace() -> anyhow::Result<()> { std::fs::rename(foo_path.as_std_path(), foo_in_workspace_path.as_std_path())?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -477,7 +530,7 @@ fn rename_file() -> anyhow::Result<()> { std::fs::rename(foo_path.as_std_path(), bar_path.as_std_path())?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("bar.py")); case.apply_changes(changes); @@ -521,7 +574,7 @@ fn directory_moved_to_workspace() -> anyhow::Result<()> { std::fs::rename(sub_original_path.as_std_path(), sub_new_path.as_std_path()) .with_context(|| "Failed to move sub directory")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("sub")); case.apply_changes(changes); @@ -580,7 +633,7 @@ fn directory_moved_to_trash() -> anyhow::Result<()> { std::fs::rename(sub_path.as_std_path(), trashed_sub.as_std_path()) .with_context(|| "Failed to move the sub directory to the trash")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("sub")); case.apply_changes(changes); @@ -604,8 +657,6 @@ fn directory_moved_to_trash() -> anyhow::Result<()> { #[test] fn directory_renamed() -> anyhow::Result<()> { - let _tracing = setup_logging_with_filter("file_watching=TRACE,red_knot=TRACE"); - let mut case = setup([ ("bar.py", "import sub.a"), ("sub/__init__.py", ""), @@ -644,11 +695,8 @@ fn directory_renamed() -> anyhow::Result<()> { std::fs::rename(sub_path.as_std_path(), foo_baz.as_std_path()) .with_context(|| "Failed to move the sub directory")?; - let changes = case.stop_watch(); - - for event in &changes { - tracing::debug!("Event: {:?}", event); - } + // Linux and windows only emit an event for the newly created root directory, but not for every new component. + let changes = case.stop_watch(event_for_file("sub")); case.apply_changes(changes); @@ -721,7 +769,7 @@ fn directory_deleted() -> anyhow::Result<()> { std::fs::remove_dir_all(sub_path.as_std_path()) .with_context(|| "Failed to remove the sub directory")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("sub")); case.apply_changes(changes); @@ -758,7 +806,7 @@ fn search_path() -> anyhow::Result<()> { std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("a.py")); case.apply_changes(changes); @@ -789,7 +837,7 @@ fn add_search_path() -> anyhow::Result<()> { std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("a.py")); case.apply_changes(changes); @@ -818,9 +866,9 @@ fn remove_search_path() -> anyhow::Result<()> { std::fs::write(site_packages.join("a.py").as_std_path(), "class A: ...")?; - let changes = case.try_stop_watch(Duration::from_millis(100)); + let changes = case.try_stop_watch(|_: &ChangeEvent| true, Duration::from_millis(100)); - assert_eq!(changes, None); + assert_eq!(changes, Err(vec![])); Ok(()) } @@ -858,7 +906,7 @@ fn changed_versions_file() -> anyhow::Result<()> { "os: 3.0-", )?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("VERSIONS")); case.apply_changes(changes); @@ -911,7 +959,7 @@ fn hard_links_in_workspace() -> anyhow::Result<()> { // Write to the hard link target. update_file(foo_path, "print('Version 2')").context("Failed to update foo.py")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -982,7 +1030,7 @@ fn hard_links_to_target_outside_workspace() -> anyhow::Result<()> { // Write to the hard link target. update_file(foo_path, "print('Version 2')").context("Failed to update foo.py")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(ChangeEvent::is_changed); case.apply_changes(changes); @@ -1021,7 +1069,7 @@ mod unix { ) .with_context(|| "Failed to set file permissions.")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("foo.py")); case.apply_changes(changes); @@ -1119,7 +1167,7 @@ mod unix { update_file(baz_workspace, "def baz(): print('Version 3')") .context("Failed to update bar/baz.py")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("baz.py")); case.apply_changes(changes); @@ -1190,7 +1238,7 @@ mod unix { update_file(&patched_bar_baz, "def baz(): print('Version 2')") .context("Failed to update bar/baz.py")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("baz.py")); case.apply_changes(changes); @@ -1298,7 +1346,7 @@ mod unix { update_file(&baz_original, "def baz(): print('Version 2')") .context("Failed to update bar/baz.py")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("baz.py")); case.apply_changes(changes); @@ -1352,7 +1400,7 @@ fn nested_packages_delete_root() -> anyhow::Result<()> { std::fs::remove_file(case.workspace_path("pyproject.toml").as_std_path())?; - let changes = case.stop_watch(); + let changes = case.stop_watch(ChangeEvent::is_deleted); case.apply_changes(changes); @@ -1364,7 +1412,6 @@ fn nested_packages_delete_root() -> anyhow::Result<()> { #[test] fn added_package() -> anyhow::Result<()> { - let _ = setup_logging(); let mut case = setup([ ( "pyproject.toml", @@ -1406,7 +1453,7 @@ fn added_package() -> anyhow::Result<()> { ) .context("failed to write pyproject.toml for package b")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(event_for_file("pyproject.toml")); case.apply_changes(changes); @@ -1449,7 +1496,7 @@ fn removed_package() -> anyhow::Result<()> { std::fs::remove_dir_all(case.workspace_path("packages/b").as_std_path()) .context("failed to remove package 'b'")?; - let changes = case.stop_watch(); + let changes = case.stop_watch(ChangeEvent::is_deleted); case.apply_changes(changes); diff --git a/crates/red_knot_python_semantic/Cargo.toml b/crates/red_knot_python_semantic/Cargo.toml index af735f99e15b9..981b72c169bf7 100644 --- a/crates/red_knot_python_semantic/Cargo.toml +++ b/crates/red_knot_python_semantic/Cargo.toml @@ -49,6 +49,8 @@ anyhow = { workspace = true } dir-test = { workspace = true } insta = { workspace = true } tempfile = { workspace = true } +quickcheck = { version = "1.0.3", default-features = false } +quickcheck_macros = { version = "1.0.0" } [lints] workspace = true diff --git a/crates/red_knot_python_semantic/resources/mdtest/annotations/literal_string.md b/crates/red_knot_python_semantic/resources/mdtest/annotations/literal_string.md new file mode 100644 index 0000000000000..518474f9d507c --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/annotations/literal_string.md @@ -0,0 +1,128 @@ +# `LiteralString` + +`LiteralString` represents a string that is either defined directly within the source code or is +made up of such components. + +Parts of the testcases defined here were adapted from [the specification's examples][1]. + +## Usages + +### Valid places + +It can be used anywhere a type is accepted: + +```py +from typing import LiteralString + +x: LiteralString + +def f(): + reveal_type(x) # revealed: LiteralString +``` + +### Within `Literal` + +`LiteralString` cannot be used within `Literal`: + +```py +from typing import Literal, LiteralString + +bad_union: Literal["hello", LiteralString] # error: [invalid-literal-parameter] +bad_nesting: Literal[LiteralString] # error: [invalid-literal-parameter] +``` + +### Parametrized + +`LiteralString` cannot be parametrized. + +```py +from typing import LiteralString + +a: LiteralString[str] # error: [invalid-type-parameter] +b: LiteralString["foo"] # error: [invalid-type-parameter] +``` + +### As a base class + +Subclassing `LiteralString` leads to a runtime error. + +```py +from typing import LiteralString + +class C(LiteralString): ... # error: [invalid-base] +``` + +## Inference + +### Common operations + +```py +foo: LiteralString = "foo" +reveal_type(foo) # revealed: Literal["foo"] + +bar: LiteralString = "bar" +reveal_type(foo + bar) # revealed: Literal["foobar"] + +baz: LiteralString = "baz" +baz += foo +reveal_type(baz) # revealed: Literal["bazfoo"] + +qux = (foo, bar) +reveal_type(qux) # revealed: tuple[Literal["foo"], Literal["bar"]] + +# TODO: Infer "LiteralString" +reveal_type(foo.join(qux)) # revealed: @Todo(call todo) + +template: LiteralString = "{}, {}" +reveal_type(template) # revealed: Literal["{}, {}"] +# TODO: Infer `LiteralString` +reveal_type(template.format(foo, bar)) # revealed: @Todo(call todo) +``` + +### Assignability + +`Literal[""]` is assignable to `LiteralString`, and `LiteralString` is assignable to `str`, but not +vice versa. + +```py +def coinflip() -> bool: + return True + +foo_1: Literal["foo"] = "foo" +bar_1: LiteralString = foo_1 # fine + +foo_2 = "foo" if coinflip() else "bar" +reveal_type(foo_2) # revealed: Literal["foo", "bar"] +bar_2: LiteralString = foo_2 # fine + +foo_3: LiteralString = "foo" * 1_000_000_000 +bar_3: str = foo_2 # fine + +baz_1: str = str() +qux_1: LiteralString = baz_1 # error: [invalid-assignment] + +baz_2: LiteralString = "baz" * 1_000_000_000 +qux_2: Literal["qux"] = baz_2 # error: [invalid-assignment] + +baz_3 = "foo" if coinflip() else 1 +reveal_type(baz_3) # revealed: Literal["foo"] | Literal[1] +qux_3: LiteralString = baz_3 # error: [invalid-assignment] +``` + +### Narrowing + +```py +lorem: LiteralString = "lorem" * 1_000_000_000 + +reveal_type(lorem) # revealed: LiteralString + +if lorem == "ipsum": + reveal_type(lorem) # revealed: Literal["ipsum"] + +reveal_type(lorem) # revealed: LiteralString + +if "" < lorem == "ipsum": + reveal_type(lorem) # revealed: Literal["ipsum"] +``` + +[1]: https://typing.readthedocs.io/en/latest/spec/literal.html#literalstring diff --git a/crates/red_knot_python_semantic/resources/mdtest/narrow/bool-call.md b/crates/red_knot_python_semantic/resources/mdtest/narrow/bool-call.md new file mode 100644 index 0000000000000..d7ae47b4fdd07 --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/narrow/bool-call.md @@ -0,0 +1,32 @@ +## Narrowing for `bool(..)` checks + +```py +def flag() -> bool: ... + +x = 1 if flag() else None + +# valid invocation, positive +reveal_type(x) # revealed: Literal[1] | None +if bool(x is not None): + reveal_type(x) # revealed: Literal[1] + +# valid invocation, negative +reveal_type(x) # revealed: Literal[1] | None +if not bool(x is not None): + reveal_type(x) # revealed: None + +# no args/narrowing +reveal_type(x) # revealed: Literal[1] | None +if not bool(): + reveal_type(x) # revealed: Literal[1] | None + +# invalid invocation, too many positional args +reveal_type(x) # revealed: Literal[1] | None +if bool(x is not None, 5): # TODO diagnostic + reveal_type(x) # revealed: Literal[1] | None + +# invalid invocation, too many kwargs +reveal_type(x) # revealed: Literal[1] | None +if bool(x is not None, y=5): # TODO diagnostic + reveal_type(x) # revealed: Literal[1] | None +``` diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index bdae6ecab3530..adec8fc70099e 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -40,6 +40,9 @@ mod signatures; mod string_annotation; mod unpacker; +#[cfg(test)] +mod property_tests; + #[salsa::tracked(return_ref)] pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { let _span = tracing::trace_span!("check_types", file=?file.path(db)).entered(); @@ -574,8 +577,11 @@ impl<'db> Type<'db> { Self::BytesLiteral(BytesLiteralType::new(db, bytes)) } - pub fn tuple(db: &'db dyn Db, elements: &[Type<'db>]) -> Self { - Self::Tuple(TupleType::new(db, elements)) + pub fn tuple>>( + db: &'db dyn Db, + elements: impl IntoIterator, + ) -> Self { + TupleType::from_elements(db, elements) } #[must_use] @@ -719,9 +725,6 @@ impl<'db> Type<'db> { (Type::KnownInstance(left), right) => { left.instance_fallback(db).is_subtype_of(db, right) } - (left, Type::KnownInstance(right)) => { - left.is_subtype_of(db, right.instance_fallback(db)) - } (Type::Instance(left), Type::Instance(right)) => left.is_instance_of(db, right.class), // TODO _ => false, @@ -1403,7 +1406,7 @@ impl<'db> Type<'db> { // `Any` is callable, and its return type is also `Any`. Type::Any => CallOutcome::callable(Type::Any), - Type::Todo(_) => CallOutcome::callable(todo_type!()), + Type::Todo(_) => CallOutcome::callable(todo_type!("call todo")), Type::Unknown => CallOutcome::callable(Type::Unknown), @@ -1556,6 +1559,7 @@ impl<'db> Type<'db> { Type::KnownInstance(KnownInstanceType::Never | KnownInstanceType::NoReturn) => { Type::Never } + Type::KnownInstance(KnownInstanceType::LiteralString) => Type::LiteralString, Type::KnownInstance(KnownInstanceType::Any) => Type::Any, _ => todo_type!(), } @@ -1889,6 +1893,8 @@ impl<'db> KnownClass { pub enum KnownInstanceType<'db> { /// The symbol `typing.Literal` (which can also be found as `typing_extensions.Literal`) Literal, + /// The symbol `typing.LiteralString` (which can also be found as `typing_extensions.LiteralString`) + LiteralString, /// The symbol `typing.Optional` (which can also be found as `typing_extensions.Optional`) Optional, /// The symbol `typing.Union` (which can also be found as `typing_extensions.Union`) @@ -1910,6 +1916,7 @@ impl<'db> KnownInstanceType<'db> { pub const fn as_str(self) -> &'static str { match self { Self::Literal => "Literal", + Self::LiteralString => "LiteralString", Self::Optional => "Optional", Self::Union => "Union", Self::TypeVar(_) => "TypeVar", @@ -1924,6 +1931,7 @@ impl<'db> KnownInstanceType<'db> { pub const fn bool(self) -> Truthiness { match self { Self::Literal + | Self::LiteralString | Self::Optional | Self::TypeVar(_) | Self::Union @@ -1938,6 +1946,7 @@ impl<'db> KnownInstanceType<'db> { pub fn repr(self, db: &'db dyn Db) -> &'db str { match self { Self::Literal => "typing.Literal", + Self::LiteralString => "typing.LiteralString", Self::Optional => "typing.Optional", Self::Union => "typing.Union", Self::NoReturn => "typing.NoReturn", @@ -1952,6 +1961,7 @@ impl<'db> KnownInstanceType<'db> { pub const fn class(self) -> KnownClass { match self { Self::Literal => KnownClass::SpecialForm, + Self::LiteralString => KnownClass::SpecialForm, Self::Optional => KnownClass::SpecialForm, Self::Union => KnownClass::SpecialForm, Self::NoReturn => KnownClass::SpecialForm, @@ -1978,6 +1988,7 @@ impl<'db> KnownInstanceType<'db> { match (module.name().as_str(), instance_name) { ("typing", "Any") => Some(Self::Any), ("typing" | "typing_extensions", "Literal") => Some(Self::Literal), + ("typing" | "typing_extensions", "LiteralString") => Some(Self::LiteralString), ("typing" | "typing_extensions", "Optional") => Some(Self::Optional), ("typing" | "typing_extensions", "Union") => Some(Self::Union), ("typing" | "typing_extensions", "NoReturn") => Some(Self::NoReturn), @@ -3019,6 +3030,23 @@ pub struct TupleType<'db> { } impl<'db> TupleType<'db> { + pub fn from_elements>>( + db: &'db dyn Db, + types: impl IntoIterator, + ) -> Type<'db> { + let mut elements = vec![]; + + for ty in types { + let ty = ty.into(); + if ty.is_never() { + return Type::Never; + } + elements.push(ty); + } + + Type::Tuple(Self::new(db, elements.into_boxed_slice())) + } + pub fn get(&self, db: &'db dyn Db, index: usize) -> Option> { self.elements(db).get(index).copied() } @@ -3070,8 +3098,8 @@ pub(crate) mod tests { /// A test representation of a type that can be transformed unambiguously into a real Type, /// given a db. - #[derive(Debug, Clone)] - enum Ty { + #[derive(Debug, Clone, PartialEq)] + pub(crate) enum Ty { Never, Unknown, None, @@ -3095,7 +3123,7 @@ pub(crate) mod tests { } impl Ty { - fn into_type(self, db: &TestDb) -> Type<'_> { + pub(crate) fn into_type(self, db: &TestDb) -> Type<'_> { match self { Ty::Never => Type::Never, Ty::Unknown => Type::Unknown, @@ -3126,13 +3154,21 @@ pub(crate) mod tests { builder.build() } Ty::Tuple(tys) => { - let elements: Vec = tys.into_iter().map(|ty| ty.into_type(db)).collect(); - Type::tuple(db, &elements) + let elements = tys.into_iter().map(|ty| ty.into_type(db)); + Type::tuple(db, elements) } } } } + #[test_case(Ty::Tuple(vec![Ty::Never]))] + #[test_case(Ty::Tuple(vec![Ty::BuiltinInstance("str"), Ty::Never, Ty::BuiltinInstance("int")]))] + #[test_case(Ty::Tuple(vec![Ty::Tuple(vec![Ty::Never])]))] + fn tuple_containing_never_simplifies_to_never(ty: Ty) { + let db = setup_db(); + assert_eq!(ty.into_type(&db), Type::Never); + } + #[test_case(Ty::BuiltinInstance("str"), Ty::BuiltinInstance("object"))] #[test_case(Ty::BuiltinInstance("int"), Ty::BuiltinInstance("object"))] #[test_case(Ty::Unknown, Ty::IntLiteral(1))] @@ -3246,6 +3282,7 @@ pub(crate) mod tests { #[test_case(Ty::IntLiteral(1), Ty::Intersection{pos: vec![Ty::BuiltinInstance("int")], neg: vec![Ty::IntLiteral(1)]})] #[test_case(Ty::BuiltinClassLiteral("int"), Ty::BuiltinClassLiteral("object"))] #[test_case(Ty::BuiltinInstance("int"), Ty::BuiltinClassLiteral("int"))] + #[test_case(Ty::TypingInstance("_SpecialForm"), Ty::TypingLiteral)] fn is_not_subtype_of(from: Ty, to: Ty) { let db = setup_db(); assert!(!from.into_type(&db).is_subtype_of(&db, to.into_type(&db))); diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 0254573a2b667..0f792bc423748 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4544,7 +4544,7 @@ impl<'db> TypeInferenceBuilder<'db> { if element_could_alter_type_of_whole_tuple(single_element, single_element_ty) { todo_type!() } else { - Type::tuple(self.db, &[single_element_ty]) + Type::tuple(self.db, [single_element_ty]) } } } @@ -4649,6 +4649,17 @@ impl<'db> TypeInferenceBuilder<'db> { ); Type::Unknown } + KnownInstanceType::LiteralString => { + self.diagnostics.add( + subscript.into(), + "invalid-type-parameter", + format_args!( + "Type `{}` expected no type parameter. Did you mean to use `Literal[...]` instead?", + known_instance.repr(self.db) + ), + ); + Type::Unknown + } KnownInstanceType::Any => Type::Any, } } diff --git a/crates/red_knot_python_semantic/src/types/mro.rs b/crates/red_knot_python_semantic/src/types/mro.rs index 83873d23d9242..d7f4cc34bdc72 100644 --- a/crates/red_knot_python_semantic/src/types/mro.rs +++ b/crates/red_knot_python_semantic/src/types/mro.rs @@ -374,6 +374,7 @@ impl<'db> ClassBase<'db> { KnownInstanceType::TypeVar(_) | KnownInstanceType::TypeAliasType(_) | KnownInstanceType::Literal + | KnownInstanceType::LiteralString | KnownInstanceType::Union | KnownInstanceType::NoReturn | KnownInstanceType::Never diff --git a/crates/red_knot_python_semantic/src/types/narrow.rs b/crates/red_knot_python_semantic/src/types/narrow.rs index 93e2f5afa357b..69513ccfba4c1 100644 --- a/crates/red_knot_python_semantic/src/types/narrow.rs +++ b/crates/red_knot_python_semantic/src/types/narrow.rs @@ -294,8 +294,15 @@ impl<'db> NarrowingConstraintsBuilder<'db> { .chain(comparators) .tuple_windows::<(&ruff_python_ast::Expr, &ruff_python_ast::Expr)>(); let mut constraints = NarrowingConstraints::default(); + + let mut last_rhs_ty: Option = None; + for (op, (left, right)) in std::iter::zip(&**ops, comparator_tuples) { + let lhs_ty = last_rhs_ty.unwrap_or_else(|| { + inference.expression_ty(left.scoped_expression_id(self.db, scope)) + }); let rhs_ty = inference.expression_ty(right.scoped_expression_id(self.db, scope)); + last_rhs_ty = Some(rhs_ty); match left { ast::Expr::Name(ast::ExprName { @@ -330,6 +337,9 @@ impl<'db> NarrowingConstraintsBuilder<'db> { constraints.insert(symbol, ty); } } + ast::CmpOp::Eq if lhs_ty.is_literal_string() => { + constraints.insert(symbol, rhs_ty); + } _ => { // TODO other comparison types } @@ -388,46 +398,58 @@ impl<'db> NarrowingConstraintsBuilder<'db> { let scope = self.scope(); let inference = infer_expression_types(self.db, expression); + let callable_ty = + inference.expression_ty(expr_call.func.scoped_expression_id(self.db, scope)); + // TODO: add support for PEP 604 union types on the right hand side of `isinstance` // and `issubclass`, for example `isinstance(x, str | (int | float))`. - match inference - .expression_ty(expr_call.func.scoped_expression_id(self.db, scope)) - .into_function_literal() - .and_then(|f| f.known(self.db)) - .and_then(KnownFunction::constraint_function) - { - Some(function) if expr_call.arguments.keywords.is_empty() => { - if let [ast::Expr::Name(ast::ExprName { id, .. }), class_info] = + match callable_ty { + Type::FunctionLiteral(function_type) if expr_call.arguments.keywords.is_empty() => { + let function = function_type + .known(self.db) + .and_then(KnownFunction::constraint_function)?; + + let [ast::Expr::Name(ast::ExprName { id, .. }), class_info] = &*expr_call.arguments.args - { - let symbol = self.symbols().symbol_id_by_name(id).unwrap(); + else { + return None; + }; - let class_info_ty = - inference.expression_ty(class_info.scoped_expression_id(self.db, scope)); + let symbol = self.symbols().symbol_id_by_name(id).unwrap(); - let to_constraint = match function { - KnownConstraintFunction::IsInstance => { - |class_literal: ClassLiteralType<'db>| { - Type::instance(class_literal.class) - } - } - KnownConstraintFunction::IsSubclass => { - |class_literal: ClassLiteralType<'db>| { - Type::subclass_of(class_literal.class) - } - } - }; + let class_info_ty = + inference.expression_ty(class_info.scoped_expression_id(self.db, scope)); - generate_classinfo_constraint(self.db, &class_info_ty, to_constraint).map( - |constraint| { - let mut constraints = NarrowingConstraints::default(); - constraints.insert(symbol, constraint.negate_if(self.db, !is_positive)); - constraints - }, - ) - } else { - None - } + let to_constraint = match function { + KnownConstraintFunction::IsInstance => { + |class_literal: ClassLiteralType<'db>| Type::instance(class_literal.class) + } + KnownConstraintFunction::IsSubclass => { + |class_literal: ClassLiteralType<'db>| { + Type::subclass_of(class_literal.class) + } + } + }; + + generate_classinfo_constraint(self.db, &class_info_ty, to_constraint).map( + |constraint| { + let mut constraints = NarrowingConstraints::default(); + constraints.insert(symbol, constraint.negate_if(self.db, !is_positive)); + constraints + }, + ) + } + // for the expression `bool(E)`, we further narrow the type based on `E` + Type::ClassLiteral(class_type) + if expr_call.arguments.args.len() == 1 + && expr_call.arguments.keywords.is_empty() + && class_type.class.is_known(self.db, KnownClass::Bool) => + { + self.evaluate_expression_node_constraint( + &expr_call.arguments.args[0], + expression, + is_positive, + ) } _ => None, } diff --git a/crates/red_knot_python_semantic/src/types/property_tests.rs b/crates/red_knot_python_semantic/src/types/property_tests.rs new file mode 100644 index 0000000000000..8dd8c7ae6fccd --- /dev/null +++ b/crates/red_knot_python_semantic/src/types/property_tests.rs @@ -0,0 +1,237 @@ +//! This module contains quickcheck-based property tests for `Type`s. +//! +//! These tests are disabled by default, as they are non-deterministic and slow. You can +//! run them explicitly using: +//! +//! ```sh +//! cargo test -p red_knot_python_semantic -- --ignored types::property_tests::stable +//! ``` +//! +//! The number of tests (default: 100) can be controlled by setting the `QUICKCHECK_TESTS` +//! environment variable. For example: +//! +//! ```sh +//! QUICKCHECK_TESTS=10000 cargo test … +//! ``` +//! +//! If you want to run these tests for a longer period of time, it's advisable to run them +//! in release mode. As some tests are slower than others, it's advisable to run them in a +//! loop until they fail: +//! +//! ```sh +//! export QUICKCHECK_TESTS=100000 +//! while cargo test --release -p red_knot_python_semantic -- \ +//! --ignored types::property_tests::stable; do :; done +//! ``` + +use std::sync::{Arc, Mutex, MutexGuard, OnceLock}; + +use super::tests::{setup_db, Ty}; +use crate::db::tests::TestDb; +use crate::types::KnownClass; +use quickcheck::{Arbitrary, Gen}; + +fn arbitrary_core_type(g: &mut Gen) -> Ty { + // We could select a random integer here, but this would make it much less + // likely to explore interesting edge cases: + let int_lit = Ty::IntLiteral(*g.choose(&[-2, -1, 0, 1, 2]).unwrap()); + let bool_lit = Ty::BooleanLiteral(bool::arbitrary(g)); + g.choose(&[ + Ty::Never, + Ty::Unknown, + Ty::None, + Ty::Any, + int_lit, + bool_lit, + Ty::StringLiteral(""), + Ty::StringLiteral("a"), + Ty::LiteralString, + Ty::BytesLiteral(""), + Ty::BytesLiteral("\x00"), + Ty::KnownClassInstance(KnownClass::Object), + Ty::KnownClassInstance(KnownClass::Str), + Ty::KnownClassInstance(KnownClass::Int), + Ty::KnownClassInstance(KnownClass::Bool), + Ty::KnownClassInstance(KnownClass::List), + Ty::KnownClassInstance(KnownClass::Tuple), + Ty::KnownClassInstance(KnownClass::FunctionType), + Ty::KnownClassInstance(KnownClass::SpecialForm), + Ty::KnownClassInstance(KnownClass::TypeVar), + Ty::KnownClassInstance(KnownClass::TypeAliasType), + Ty::KnownClassInstance(KnownClass::NoDefaultType), + Ty::TypingLiteral, + Ty::BuiltinClassLiteral("str"), + Ty::BuiltinClassLiteral("int"), + Ty::BuiltinClassLiteral("bool"), + Ty::BuiltinClassLiteral("object"), + ]) + .unwrap() + .clone() +} + +/// Constructs an arbitrary type. +/// +/// The `size` parameter controls the depth of the type tree. For example, +/// a simple type like `int` has a size of 0, `Union[int, str]` has a size +/// of 1, `tuple[int, Union[str, bytes]]` has a size of 2, etc. +fn arbitrary_type(g: &mut Gen, size: u32) -> Ty { + if size == 0 { + arbitrary_core_type(g) + } else { + match u32::arbitrary(g) % 4 { + 0 => arbitrary_core_type(g), + 1 => Ty::Union( + (0..*g.choose(&[2, 3]).unwrap()) + .map(|_| arbitrary_type(g, size - 1)) + .collect(), + ), + 2 => Ty::Tuple( + (0..*g.choose(&[0, 1, 2]).unwrap()) + .map(|_| arbitrary_type(g, size - 1)) + .collect(), + ), + 3 => Ty::Intersection { + pos: (0..*g.choose(&[0, 1, 2]).unwrap()) + .map(|_| arbitrary_type(g, size - 1)) + .collect(), + neg: (0..*g.choose(&[0, 1, 2]).unwrap()) + .map(|_| arbitrary_type(g, size - 1)) + .collect(), + }, + _ => unreachable!(), + } + } +} + +impl Arbitrary for Ty { + fn arbitrary(g: &mut Gen) -> Ty { + const MAX_SIZE: u32 = 2; + arbitrary_type(g, MAX_SIZE) + } + + fn shrink(&self) -> Box> { + // This is incredibly naive. We can do much better here by + // trying various subsets of the elements in unions, tuples, + // and intersections. For now, we only try to shrink by + // reducing unions/tuples/intersections to a single element. + match self.clone() { + Ty::Union(types) => Box::new(types.into_iter()), + Ty::Tuple(types) => Box::new(types.into_iter()), + Ty::Intersection { pos, neg } => Box::new(pos.into_iter().chain(neg)), + _ => Box::new(std::iter::empty()), + } + } +} + +static CACHED_DB: OnceLock>> = OnceLock::new(); + +fn get_cached_db() -> MutexGuard<'static, TestDb> { + let db = CACHED_DB.get_or_init(|| Arc::new(Mutex::new(setup_db()))); + db.lock().unwrap() +} + +/// A macro to define a property test for types. +/// +/// The `$test_name` identifier specifies the name of the test function. The `$db` identifier +/// is used to refer to the salsa database in the property to be tested. The actual property is +/// specified using the syntax: +/// +/// forall types t1, t2, ..., tn . ` +/// +/// where `t1`, `t2`, ..., `tn` are identifiers that represent arbitrary types, and `` +/// is an expression using these identifiers. +/// +macro_rules! type_property_test { + ($test_name:ident, $db:ident, forall types $($types:ident),+ . $property:expr) => { + #[quickcheck_macros::quickcheck] + #[ignore] + fn $test_name($($types: crate::types::tests::Ty),+) -> bool { + let db_cached = super::get_cached_db(); + let $db = &*db_cached; + $(let $types = $types.into_type($db);)+ + + $property + } + }; + // A property test with a logical implication. + ($name:ident, $db:ident, forall types $($types:ident),+ . $premise:expr => $conclusion:expr) => { + type_property_test!($name, $db, forall types $($types),+ . !($premise) || ($conclusion)); + }; +} + +mod stable { + // `T` is equivalent to itself. + type_property_test!( + equivalent_to_is_reflexive, db, + forall types t. t.is_equivalent_to(db, t) + ); + + // `T` is a subtype of itself. + type_property_test!( + subtype_of_is_reflexive, db, + forall types t. t.is_subtype_of(db, t) + ); + + // `S <: T` and `T <: U` implies that `S <: U`. + type_property_test!( + subtype_of_is_transitive, db, + forall types s, t, u. s.is_subtype_of(db, t) && t.is_subtype_of(db, u) => s.is_subtype_of(db, u) + ); + + // `T` is not disjoint from itself, unless `T` is `Never`. + type_property_test!( + disjoint_from_is_irreflexive, db, + forall types t. t.is_disjoint_from(db, t) => t.is_never() + ); + + // `S` is disjoint from `T` implies that `T` is disjoint from `S`. + type_property_test!( + disjoint_from_is_symmetric, db, + forall types s, t. s.is_disjoint_from(db, t) == t.is_disjoint_from(db, s) + ); + + // `S <: T` implies that `S` is not disjoint from `T`, unless `S` is `Never`. + type_property_test!( + subtype_of_implies_not_disjoint_from, db, + forall types s, t. s.is_subtype_of(db, t) => !s.is_disjoint_from(db, t) || s.is_never() + ); + + // `T` can be assigned to itself. + type_property_test!( + assignable_to_is_reflexive, db, + forall types t. t.is_assignable_to(db, t) + ); + + // `S <: T` implies that `S` can be assigned to `T`. + type_property_test!( + subtype_of_implies_assignable_to, db, + forall types s, t. s.is_subtype_of(db, t) => s.is_assignable_to(db, t) + ); + + // If `T` is a singleton, it is also single-valued. + type_property_test!( + singleton_implies_single_valued, db, + forall types t. t.is_singleton(db) => t.is_single_valued(db) + ); +} + +/// This module contains property tests that currently lead to many false positives. +/// +/// The reason for this is our insufficient understanding of equivalence of types. For +/// example, we currently consider `int | str` and `str | int` to be different types. +/// Similar issues exist for intersection types. Once this is resolved, we can move these +/// tests to the `stable` section. In the meantime, it can still be useful to run these +/// tests (using [`types::property_tests::flaky`]), to see if there are any new obvious bugs. +mod flaky { + // `S <: T` and `T <: S` implies that `S` is equivalent to `T`. + type_property_test!( + subtype_of_is_antisymmetric, db, + forall types s, t. s.is_subtype_of(db, t) && t.is_subtype_of(db, s) => s.is_equivalent_to(db, t) + ); + + // Negating `T` twice is equivalent to `T`. + type_property_test!( + double_negation_is_identity, db, + forall types t. t.negate(db).negate(db).is_equivalent_to(db, t) + ); +} diff --git a/crates/red_knot_python_semantic/src/types/unpacker.rs b/crates/red_knot_python_semantic/src/types/unpacker.rs index 2e8c39a9b23c0..dc96fac252af5 100644 --- a/crates/red_knot_python_semantic/src/types/unpacker.rs +++ b/crates/red_knot_python_semantic/src/types/unpacker.rs @@ -95,7 +95,7 @@ impl<'db> Unpacker<'db> { // there would be a cost and it's not clear that it's worth it. let value_ty = Type::tuple( self.db, - &vec![Type::LiteralString; string_literal_ty.len(self.db)], + std::iter::repeat(Type::LiteralString).take(string_literal_ty.len(self.db)), ); self.unpack(target, value_ty, scope); } diff --git a/crates/red_knot_workspace/src/watch.rs b/crates/red_knot_workspace/src/watch.rs index a0d120882206b..65e01491655e0 100644 --- a/crates/red_knot_workspace/src/watch.rs +++ b/crates/red_knot_workspace/src/watch.rs @@ -81,6 +81,22 @@ impl ChangeEvent { _ => None, } } + + pub const fn is_rescan(&self) -> bool { + matches!(self, ChangeEvent::Rescan) + } + + pub const fn is_created(&self) -> bool { + matches!(self, ChangeEvent::Created { .. }) + } + + pub const fn is_changed(&self) -> bool { + matches!(self, ChangeEvent::Changed { .. }) + } + + pub const fn is_deleted(&self) -> bool { + matches!(self, ChangeEvent::Deleted { .. }) + } } /// Classification of an event that creates a new path. diff --git a/crates/red_knot_workspace/tests/check.rs b/crates/red_knot_workspace/tests/check.rs index f3be665151413..733bf2efdd0d3 100644 --- a/crates/red_knot_workspace/tests/check.rs +++ b/crates/red_knot_workspace/tests/check.rs @@ -42,7 +42,6 @@ fn parser_no_panic() -> anyhow::Result<()> { } #[test] -#[ignore = "Enable running once there are fewer failures"] fn linter_af_no_panic() -> anyhow::Result<()> { let workspace_root = get_workspace_root()?; run_corpus_tests(&format!( @@ -51,7 +50,6 @@ fn linter_af_no_panic() -> anyhow::Result<()> { } #[test] -#[ignore = "Enable running once there are fewer failures"] fn linter_gz_no_panic() -> anyhow::Result<()> { let workspace_root = get_workspace_root()?; run_corpus_tests(&format!( @@ -272,6 +270,7 @@ const KNOWN_FAILURES: &[(&str, bool, bool)] = &[ ("crates/ruff_linter/resources/test/fixtures/pyupgrade/UP039.py", true, false), // related to circular references in type aliases (salsa cycle panic): ("crates/ruff_python_parser/resources/inline/err/type_alias_invalid_value_expr.py", true, true), + ("crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008.py", true, true), // related to circular references in f-string annotations (invalid syntax) ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_15.py", true, true), ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_14.py", false, true), diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index 8c7ae672d8002..31153d506dab2 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -2003,6 +2003,42 @@ fn flake8_import_convention_invalid_aliases_config_alias_name() -> Result<()> { Ok(()) } +#[test] +fn flake8_import_convention_invalid_aliases_config_extend_alias_name() -> Result<()> { + let tempdir = TempDir::new()?; + let ruff_toml = tempdir.path().join("ruff.toml"); + fs::write( + &ruff_toml, + r#" +[lint.flake8-import-conventions.extend-aliases] +"module.name" = "__debug__" +"#, + )?; + + insta::with_settings!({ + filters => vec![(tempdir_filter(&tempdir).as_str(), "[TMP]/")] + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .arg("--config") + .arg(&ruff_toml) + , @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + ruff failed + Cause: Failed to parse [TMP]/ruff.toml + Cause: TOML parse error at line 2, column 2 + | + 2 | [lint.flake8-import-conventions.extend-aliases] + | ^^^^ + invalid value: string "__debug__", expected an assignable Python identifier + "###);}); + Ok(()) +} + #[test] fn flake8_import_convention_invalid_aliases_config_module_name() -> Result<()> { let tempdir = TempDir::new()?; diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py index facb2a373568f..936bfdae3d8de 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB157.py @@ -42,3 +42,17 @@ Decimal("_") # Ok Decimal(" ") # Ok Decimal("10000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000") # Ok + +# Non-finite variants +# https://github.com/astral-sh/ruff/issues/14587 +Decimal(float(" nan ")) # Decimal(" nan ") +Decimal(float(" +nan ")) # Decimal(" +nan ") +# In this one case, " -nan ", the fix has to be +# `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +Decimal(float(" -nan ")) # Decimal(" nan ") +Decimal(float(" inf ")) # Decimal(" inf ") +Decimal(float(" +inf ")) # Decimal(" +inf ") +Decimal(float(" -inf ")) # Decimal(" -inf ") +Decimal(float(" infinity ")) # Decimal(" infinity ") +Decimal(float(" +infinity ")) # Decimal(" +infinity ") +Decimal(float(" -infinity ")) # Decimal(" -infinity ") diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py new file mode 100644 index 0000000000000..4f37b1d7fac4e --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -0,0 +1,131 @@ +# Correct + +for _ in range(5): + pass + +_valid_type = int + +_valid_var_1: _valid_type + +_valid_var_1 = 1 + +_valid_var_2 = 2 + +_valid_var_3 = _valid_var_1 + _valid_var_2 + +def _valid_fun(): + pass + +_valid_fun() + +def fun(arg): + _valid_unused_var = arg + pass + +_x = "global" + +def fun(): + global _x + return _x + +def fun(): + __dunder__ = "dunder variable" + return __dunder__ + +def fun(): + global _x + _x = "reassigned global" + return _x + +class _ValidClass: + pass + +_ValidClass() + +class ClassOk: + _valid_private_cls_attr = 1 + + print(_valid_private_cls_attr) + + def __init__(self): + self._valid_private_ins_attr = 2 + print(self._valid_private_ins_attr) + + def _valid_method(self): + return self._valid_private_ins_attr + + def method(arg): + _valid_unused_var = arg + return + +def fun(x): + _ = 1 + __ = 2 + ___ = 3 + if x == 1: + return _ + if x == 2: + return __ + if x == 3: + return ___ + return x + +# Incorrect + +class Class_: + def fun(self): + _var = "method variable" # [RUF052] + return _var + +def fun(_var): # [RUF052] + return _var + +def fun(): + _list = "built-in" # [RUF052] + return _list + +x = "global" + +def fun(): + global x + _x = "shadows global" # [RUF052] + return _x + +def foo(): + x = "outer" + def bar(): + nonlocal x + _x = "shadows nonlocal" # [RUF052] + return _x + bar() + return x + +def fun(): + x = "local" + _x = "shadows local" # [RUF052] + return _x + + +GLOBAL_1 = "global 1" +GLOBAL_1_ = "global 1 with trailing underscore" + +def unfixables(): + _GLOBAL_1 = "foo" + # unfixable because the rename would shadow a global variable + print(_GLOBAL_1) # [RUF052] + + local = "local" + local_ = "local2" + + # unfixable because the rename would shadow a local variable + _local = "local3" # [RUF052] + print(_local) + + def nested(): + _GLOBAL_1 = "foo" + # unfixable because the rename would shadow a global variable + print(_GLOBAL_1) # [RUF052] + + # unfixable because the rename would shadow a variable from the outer function + _local = "local4" + print(_local) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF055.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF055_0.py similarity index 77% rename from crates/ruff_linter/resources/test/fixtures/ruff/RUF055.py rename to crates/ruff_linter/resources/test/fixtures/ruff/RUF055_0.py index 7dd067d872e63..0d664c5232ad5 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF055.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF055_0.py @@ -74,3 +74,21 @@ def dashrepl(matchobj): "", s, # string ) + +# A diagnostic should not be emitted for `sub` replacements with backreferences or +# most other ASCII escapes +re.sub(r"a", r"\g<0>\g<0>\g<0>", "a") +re.sub(r"a", r"\1", "a") +re.sub(r"a", r"\s", "a") + +# Escapes like \n are "processed": +# `re.sub(r"a", r"\n", some_string)` is fixed to `some_string.replace("a", "\n")` +# *not* `some_string.replace("a", "\\n")`. +# We currently emit diagnostics for some of these without fixing them. +re.sub(r"a", "\n", "a") +re.sub(r"a", r"\n", "a") +re.sub(r"a", "\a", "a") +re.sub(r"a", r"\a", "a") + +re.sub(r"a", "\?", "a") +re.sub(r"a", r"\?", "a") diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF055_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF055_1.py new file mode 100644 index 0000000000000..89e177d48098e --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF055_1.py @@ -0,0 +1,17 @@ +"""Test that RUF055 can follow a single str assignment for both the pattern and +the replacement argument to re.sub +""" + +import re + +pat1 = "needle" + +re.sub(pat1, "", haystack) + +# aliases are not followed, so this one should not trigger the rule +if pat4 := pat1: + re.sub(pat4, "", haystack) + +# also works for the `repl` argument in sub +repl = "new" +re.sub(r"abc", repl, haystack) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index ed317802f4d35..a871f307e3bed 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -10,6 +10,7 @@ use crate::rules::{ /// Run lint rules over the [`Binding`]s. pub(crate) fn bindings(checker: &mut Checker) { if !checker.any_enabled(&[ + Rule::AssignmentInAssert, Rule::InvalidAllFormat, Rule::InvalidAllObject, Rule::NonAsciiName, @@ -18,7 +19,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::UnsortedDunderSlots, Rule::UnusedVariable, Rule::UnquotedTypeAlias, - Rule::AssignmentInAssert, + Rule::UsedDummyVariable, ]) { return; } @@ -88,6 +89,11 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::UsedDummyVariable) { + if let Some(diagnostic) = ruff::rules::used_dummy_variable(checker, binding) { + checker.diagnostics.push(diagnostic); + } + } if checker.enabled(Rule::AssignmentInAssert) { if let Some(diagnostic) = ruff::rules::assignment_in_assert(checker, binding) { checker.diagnostics.push(diagnostic); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index dd9426193701c..d35bf82bbde2e 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -984,6 +984,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument), (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral), (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), + (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable), (Ruff, "055") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryRegularExpression), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/glob_rule.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/glob_rule.rs index 487cc1c294c5a..be4fb8988e361 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/glob_rule.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/glob_rule.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, ViolationMetadata}; /// ## What it does -/// Checks for the use of `glob` and `iglob`. +/// Checks for the use of `glob.glob()` and `glob.iglob()`. /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to @@ -12,35 +12,43 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// improve readability over their low-level counterparts (e.g., /// `glob.glob()`). /// -/// Note that `glob.glob` and `Path.glob` are not exact equivalents: +/// Note that `glob.glob()` and `Path.glob()` are not exact equivalents: /// -/// | | `glob` | `Path.glob` | -/// |-------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------| -/// | Hidden files | Excludes hidden files by default. From Python 3.11 onwards, the `include_hidden` keyword can be used to include hidden directories. | Includes hidden files by default. | -/// | Iterator | `iglob` returns an iterator. Under the hood, `glob` simply converts the iterator to a list. | `Path.glob` returns an iterator. | -/// | Working directory | `glob` takes a `root_dir` keyword to set the current working directory. | `Path.rglob` can be used to return the relative path. | -/// | Globstar (`**`) | `glob` requires the `recursive` flag to be set to `True` for the `**` pattern to match any files and zero or more directories, subdirectories, and symbolic links. | The `**` pattern in `Path.glob` means "this directory and all subdirectories, recursively". In other words, it enables recursive globbing. | +/// | | `glob`-module functions | `Path.glob()` | +/// |-------------------|------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------| +/// | Hidden files | Hidden files are excluded by default. On Python 3.11+, the `include_hidden` keyword can be used to include hidden directories. | Includes hidden files by default. | +/// | Eagerness | `glob.iglob()` returns a lazy iterator. Under the hood, `glob.glob()` simply converts the iterator to a list. | `Path.glob()` returns a lazy iterator. | +/// | Working directory | `glob.glob()` and `glob.iglob()` take a `root_dir` keyword to set the current working directory. | `Path.rglob()` can be used to return the relative path. | +/// | Globstar (`**`) | The `recursive` flag must be set to `True` for the `**` pattern to match any files and zero or more directories, subdirectories, and symbolic links. | The `**` pattern in `Path.glob()` means "this directory and all subdirectories, recursively". In other words, it enables recursive globbing. | /// /// ## Example /// ```python /// import glob /// import os /// -/// glob.glob(os.path.join(path, "requirements*.txt")) +/// glob.glob(os.path.join("my_path", "requirements*.txt")) /// ``` /// /// Use instead: /// ```python /// from pathlib import Path /// -/// Path(path).glob("requirements*.txt") +/// Path("my_path").glob("requirements*.txt") /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.glob`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.glob) /// - [Python documentation: `Path.rglob`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.rglob) /// - [Python documentation: `glob.glob`](https://docs.python.org/3/library/glob.html#glob.glob) /// - [Python documentation: `glob.iglob`](https://docs.python.org/3/library/glob.html#glob.iglob) +/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/) +/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/) +/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/) #[derive(ViolationMetadata)] pub(crate) struct Glob { pub function: String, diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getatime.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getatime.rs index 5feaf9c08e377..15da2eb4163fd 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getatime.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getatime.rs @@ -6,15 +6,12 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. +/// the lower-level API offered by `os.path`. /// /// When possible, using `Path` object methods such as `Path.stat()` can -/// improve readability over the `os` module's counterparts (e.g., +/// improve readability over the `os.path` module's counterparts (e.g., /// `os.path.getatime()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -29,6 +26,11 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// Path(__file__).stat().st_atime /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) /// - [Python documentation: `os.path.getatime`](https://docs.python.org/3/library/os.path.html#os.path.getatime) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getctime.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getctime.rs index 2ad153208780c..0eae426b35324 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getctime.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getctime.rs @@ -6,15 +6,12 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. +/// the lower-level API offered by `os.path`. /// /// When possible, using `Path` object methods such as `Path.stat()` can -/// improve readability over the `os` module's counterparts (e.g., +/// improve readability over the `os.path` module's counterparts (e.g., /// `os.path.getctime()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -29,6 +26,11 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// Path(__file__).stat().st_ctime /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) /// - [Python documentation: `os.path.getctime`](https://docs.python.org/3/library/os.path.html#os.path.getctime) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getmtime.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getmtime.rs index eef65f31a448f..dbc1f1d50022f 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getmtime.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getmtime.rs @@ -6,15 +6,12 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. +/// the lower-level API offered by `os.path`. /// /// When possible, using `Path` object methods such as `Path.stat()` can -/// improve readability over the `os` module's counterparts (e.g., +/// improve readability over the `os.path` module's counterparts (e.g., /// `os.path.getmtime()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -29,6 +26,11 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// Path(__file__).stat().st_mtime /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) /// - [Python documentation: `os.path.getmtime`](https://docs.python.org/3/library/os.path.html#os.path.getmtime) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getsize.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getsize.rs index d6f9db0718a1b..94b5b793ee306 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getsize.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_path_getsize.rs @@ -6,15 +6,12 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. +/// the lower-level API offered by `os.path`. /// /// When possible, using `Path` object methods such as `Path.stat()` can -/// improve readability over the `os` module's counterparts (e.g., +/// improve readability over the `os.path` module's counterparts (e.g., /// `os.path.getsize()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -29,6 +26,11 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// Path(__file__).stat().st_size /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) /// - [Python documentation: `os.path.getsize`](https://docs.python.org/3/library/os.path.html#os.path.getsize) diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_sep_split.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_sep_split.rs index b4e0390c86fcd..ef226675f1c35 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_sep_split.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/rules/os_sep_split.rs @@ -42,6 +42,16 @@ use crate::checkers::ast::Checker; /// if any(part in blocklist for part in Path("my/file/path").parts): /// ... /// ``` +/// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than working directly with strings, +/// especially on older versions of Python. +/// +/// ## References +/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/) +/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/) +/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/) #[derive(ViolationMetadata)] pub(crate) struct OsSepSplit; diff --git a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs index 12dd0044765b6..28b9aa6bccd0b 100644 --- a/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs +++ b/crates/ruff_linter/src/rules/flake8_use_pathlib/violations.rs @@ -6,13 +6,10 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.resolve()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.resolve()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.abspath()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -27,6 +24,11 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; /// file_path = Path("../path/to/file").resolve() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.resolve`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve) /// - [Python documentation: `os.path.abspath`](https://docs.python.org/3/library/os.path.html#os.path.abspath) @@ -53,9 +55,6 @@ impl Violation for OsPathAbspath { /// methods such as `Path.chmod()` can improve readability over the `os` /// module's counterparts (e.g., `os.chmod()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -70,6 +69,11 @@ impl Violation for OsPathAbspath { /// Path("file.py").chmod(0o444) /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.chmod`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.chmod) /// - [Python documentation: `os.chmod`](https://docs.python.org/3/library/os.html#os.chmod) @@ -96,9 +100,6 @@ impl Violation for OsChmod { /// methods such as `Path.mkdir(parents=True)` can improve readability over the /// `os` module's counterparts (e.g., `os.makedirs()`. /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -113,6 +114,11 @@ impl Violation for OsChmod { /// Path("./nested/directory/").mkdir(parents=True) /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.mkdir`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir) /// - [Python documentation: `os.makedirs`](https://docs.python.org/3/library/os.html#os.makedirs) @@ -139,9 +145,6 @@ impl Violation for OsMakedirs { /// methods such as `Path.mkdir()` can improve readability over the `os` /// module's counterparts (e.g., `os.mkdir()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -156,6 +159,11 @@ impl Violation for OsMakedirs { /// Path("./directory/").mkdir() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.mkdir`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.mkdir) /// - [Python documentation: `os.mkdir`](https://docs.python.org/3/library/os.html#os.mkdir) @@ -182,9 +190,6 @@ impl Violation for OsMkdir { /// methods such as `Path.rename()` can improve readability over the `os` /// module's counterparts (e.g., `os.rename()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -199,6 +204,11 @@ impl Violation for OsMkdir { /// Path("old.py").rename("new.py") /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.rename`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename) /// - [Python documentation: `os.rename`](https://docs.python.org/3/library/os.html#os.rename) @@ -242,6 +252,11 @@ impl Violation for OsRename { /// Path("old.py").replace("new.py") /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.replace`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.replace) /// - [Python documentation: `os.replace`](https://docs.python.org/3/library/os.html#os.replace) @@ -268,9 +283,6 @@ impl Violation for OsReplace { /// methods such as `Path.rmdir()` can improve readability over the `os` /// module's counterparts (e.g., `os.rmdir()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -285,6 +297,11 @@ impl Violation for OsReplace { /// Path("folder/").rmdir() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.rmdir`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.rmdir) /// - [Python documentation: `os.rmdir`](https://docs.python.org/3/library/os.html#os.rmdir) @@ -311,9 +328,6 @@ impl Violation for OsRmdir { /// methods such as `Path.unlink()` can improve readability over the `os` /// module's counterparts (e.g., `os.remove()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -328,6 +342,11 @@ impl Violation for OsRmdir { /// Path("file.py").unlink() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.unlink`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.unlink) /// - [Python documentation: `os.remove`](https://docs.python.org/3/library/os.html#os.remove) @@ -354,9 +373,6 @@ impl Violation for OsRemove { /// methods such as `Path.unlink()` can improve readability over the `os` /// module's counterparts (e.g., `os.unlink()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -371,6 +387,11 @@ impl Violation for OsRemove { /// Path("file.py").unlink() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.unlink`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.unlink) /// - [Python documentation: `os.unlink`](https://docs.python.org/3/library/os.html#os.unlink) @@ -397,9 +418,6 @@ impl Violation for OsUnlink { /// methods such as `Path.cwd()` can improve readability over the `os` /// module's counterparts (e.g., `os.getcwd()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -414,6 +432,11 @@ impl Violation for OsUnlink { /// cwd = Path.cwd() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.cwd`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.cwd) /// - [Python documentation: `os.getcwd`](https://docs.python.org/3/library/os.html#os.getcwd) @@ -437,13 +460,10 @@ impl Violation for OsGetcwd { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.exists()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.exists()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.exists()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -458,6 +478,11 @@ impl Violation for OsGetcwd { /// Path("file.py").exists() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.exists`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.exists) /// - [Python documentation: `os.path.exists`](https://docs.python.org/3/library/os.path.html#os.path.exists) @@ -480,13 +505,10 @@ impl Violation for OsPathExists { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.expanduser()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.expanduser()` can improve readability over the `os.path` /// module's counterparts (e.g., as `os.path.expanduser()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -501,6 +523,11 @@ impl Violation for OsPathExists { /// Path("~/films/Monty Python").expanduser() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.expanduser`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.expanduser) /// - [Python documentation: `os.path.expanduser`](https://docs.python.org/3/library/os.path.html#os.path.expanduser) @@ -523,13 +550,10 @@ impl Violation for OsPathExpanduser { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.is_dir()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.is_dir()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.isdir()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -544,6 +568,11 @@ impl Violation for OsPathExpanduser { /// Path("docs").is_dir() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.is_dir`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.is_dir) /// - [Python documentation: `os.path.isdir`](https://docs.python.org/3/library/os.path.html#os.path.isdir) @@ -566,13 +595,10 @@ impl Violation for OsPathIsdir { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.is_file()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.is_file()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.isfile()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -587,6 +613,11 @@ impl Violation for OsPathIsdir { /// Path("docs").is_file() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.is_file`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.is_file) /// - [Python documentation: `os.path.isfile`](https://docs.python.org/3/library/os.path.html#os.path.isfile) @@ -609,13 +640,10 @@ impl Violation for OsPathIsfile { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.is_symlink()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.is_symlink()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.islink()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -630,6 +658,11 @@ impl Violation for OsPathIsfile { /// Path("docs").is_symlink() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.is_symlink`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.is_symlink) /// - [Python documentation: `os.path.islink`](https://docs.python.org/3/library/os.path.html#os.path.islink) @@ -656,9 +689,6 @@ impl Violation for OsPathIslink { /// methods such as `Path.readlink()` can improve readability over the `os` /// module's counterparts (e.g., `os.readlink()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -673,6 +703,11 @@ impl Violation for OsPathIslink { /// Path(file_name).readlink() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.readlink`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.readline) /// - [Python documentation: `os.readlink`](https://docs.python.org/3/library/os.html#os.readlink) @@ -699,9 +734,6 @@ impl Violation for OsReadlink { /// methods such as `Path.stat()` can improve readability over the `os` /// module's counterparts (e.g., `os.path.stat()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -723,6 +755,11 @@ impl Violation for OsReadlink { /// group_name = file_path.group() /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.stat`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) /// - [Python documentation: `Path.group`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.group) @@ -748,13 +785,10 @@ impl Violation for OsStat { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.is_absolute()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.is_absolute()` can improve readability over the `os.path` /// module's counterparts (e.g., as `os.path.isabs()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -771,6 +805,11 @@ impl Violation for OsStat { /// print("Absolute path!") /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `PurePath.is_absolute`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.is_absolute) /// - [Python documentation: `os.path.isabs`](https://docs.python.org/3/library/os.path.html#os.path.isabs) @@ -793,12 +832,9 @@ impl Violation for OsPathIsabs { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object +/// the lower-level API offered by `os.path`. When possible, using `Path` object /// methods such as `Path.joinpath()` or the `/` operator can improve -/// readability over the `os` module's counterparts (e.g., `os.path.join()`). -/// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. +/// readability over the `os.path` module's counterparts (e.g., `os.path.join()`). /// /// ## Examples /// ```python @@ -814,6 +850,11 @@ impl Violation for OsPathIsabs { /// Path(ROOT_PATH) / "folder" / "file.py" /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `PurePath.joinpath`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.joinpath) /// - [Python documentation: `os.path.join`](https://docs.python.org/3/library/os.path.html#os.path.join) @@ -853,13 +894,10 @@ pub(crate) enum Joiner { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.name` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.name` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.basename()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -874,6 +912,11 @@ pub(crate) enum Joiner { /// Path(__file__).name /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `PurePath.name`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.name) /// - [Python documentation: `os.path.basename`](https://docs.python.org/3/library/os.path.html#os.path.basename) @@ -896,13 +939,10 @@ impl Violation for OsPathBasename { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.parent` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.parent` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.dirname()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -917,6 +957,11 @@ impl Violation for OsPathBasename { /// Path(__file__).parent /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `PurePath.parent`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.parent) /// - [Python documentation: `os.path.dirname`](https://docs.python.org/3/library/os.path.html#os.path.dirname) @@ -939,13 +984,10 @@ impl Violation for OsPathDirname { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object -/// methods such as `Path.samefile()` can improve readability over the `os` +/// the lower-level API offered by `os.path`. When possible, using `Path` object +/// methods such as `Path.samefile()` can improve readability over the `os.path` /// module's counterparts (e.g., `os.path.samefile()`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -960,6 +1002,11 @@ impl Violation for OsPathDirname { /// Path("f1.py").samefile("f2.py") /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.samefile`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.samefile) /// - [Python documentation: `os.path.samefile`](https://docs.python.org/3/library/os.path.html#os.path.samefile) @@ -982,9 +1029,9 @@ impl Violation for OsPathSamefile { /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation, as compared to -/// the lower-level API offered by `os`. When possible, using `Path` object +/// the lower-level API offered by `os.path`. When possible, using `Path` object /// methods such as `Path.suffix` and `Path.stem` can improve readability over -/// the `os` module's counterparts (e.g., `os.path.splitext()`). +/// the `os.path` module's counterparts (e.g., `os.path.splitext()`). /// /// `os.path.splitext()` specifically returns a tuple of the file root and /// extension (e.g., given `splitext('/foo/bar.py')`, `os.path.splitext()` @@ -992,9 +1039,6 @@ impl Violation for OsPathSamefile { /// combination of `Path.suffix` (`".py"`), `Path.stem` (`"bar"`), and /// `Path.parent` (`"foo"`). /// -/// Note that `os` functions may be preferable if performance is a concern, -/// e.g., in hot loops. -/// /// ## Examples /// ```python /// import os @@ -1011,6 +1055,11 @@ impl Violation for OsPathSamefile { /// ext = path.suffix /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.suffix`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.suffix) /// - [Python documentation: `Path.suffixes`](https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.suffixes) @@ -1031,7 +1080,7 @@ impl Violation for OsPathSplitext { } /// ## What it does -/// Checks for uses of the `open` builtin. +/// Checks for uses of the `open()` builtin. /// /// ## Why is this bad? /// `pathlib` offers a high-level API for path manipulation. When possible, @@ -1052,6 +1101,11 @@ impl Violation for OsPathSplitext { /// ... /// ``` /// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than working directly with strings, +/// especially on older versions of Python. +/// /// ## References /// - [Python documentation: `Path.open`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.open) /// - [Python documentation: `open`](https://docs.python.org/3/library/functions.html#open) @@ -1139,6 +1193,19 @@ impl Violation for PyPath { /// if (p / "file").exists(): /// ... /// ``` +/// +/// ## Known issues +/// While using `pathlib` can improve the readability and type safety of your code, +/// it can be less performant than the lower-level alternatives that work directly with strings, +/// especially on older versions of Python. +/// +/// ## References +/// - [Python documentation: `Path.iterdir`](https://docs.python.org/3/library/pathlib.html#pathlib.Path.iterdir) +/// - [Python documentation: `os.listdir`](https://docs.python.org/3/library/os.html#os.listdir) +/// - [PEP 428 – The pathlib module – object-oriented filesystem paths](https://peps.python.org/pep-0428/) +/// - [Correspondence between `os` and `pathlib`](https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module) +/// - [Why you should be using pathlib](https://treyhunner.com/2018/12/why-you-should-be-using-pathlib/) +/// - [No really, pathlib is great](https://treyhunner.com/2019/01/no-really-pathlib-is-great/) #[derive(ViolationMetadata)] pub(crate) struct OsListdir; diff --git a/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs index d13ed51d08b16..bb1ff0244650e 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/verbose_decimal_constructor.rs @@ -152,14 +152,34 @@ pub(crate) fn verbose_decimal_constructor(checker: &mut Checker, call: &ast::Exp let Some(float) = float.as_string_literal_expr() else { return; }; - if !matches!( - float.value.to_str().to_lowercase().as_str(), - "inf" | "-inf" | "infinity" | "-infinity" | "nan" - ) { + + let trimmed = float.value.to_str().trim(); + let mut matches_non_finite_keyword = false; + for non_finite_keyword in [ + "inf", + "+inf", + "-inf", + "infinity", + "+infinity", + "-infinity", + "nan", + "+nan", + "-nan", + ] { + if trimmed.eq_ignore_ascii_case(non_finite_keyword) { + matches_non_finite_keyword = true; + break; + } + } + if !matches_non_finite_keyword { return; } - let replacement = checker.locator().slice(float).to_string(); + let mut replacement = checker.locator().slice(float).to_string(); + // `Decimal(float("-nan")) == Decimal("nan")` + if trimmed.eq_ignore_ascii_case("-nan") { + replacement.remove(replacement.find('-').unwrap()); + } let mut diagnostic = Diagnostic::new( VerboseDecimalConstructor { replacement: replacement.clone(), diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap index cdf82865ff52c..967e552a1b2c8 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB157_FURB157.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/refurb/mod.rs -snapshot_kind: text --- FURB157.py:5:9: FURB157 [*] Verbose expression in `Decimal` constructor | @@ -207,3 +206,183 @@ FURB157.py:24:9: FURB157 [*] Verbose expression in `Decimal` constructor 25 25 | 26 26 | # Ok 27 27 | Decimal("2e-4") + +FURB157.py:48:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +46 | # Non-finite variants +47 | # https://github.com/astral-sh/ruff/issues/14587 +48 | Decimal(float(" nan ")) # Decimal(" nan ") + | ^^^^^^^^^^^^^^ FURB157 +49 | Decimal(float(" +nan ")) # Decimal(" +nan ") +50 | # In this one case, " -nan ", the fix has to be + | + = help: Replace with `" nan "` + +ℹ Safe fix +45 45 | +46 46 | # Non-finite variants +47 47 | # https://github.com/astral-sh/ruff/issues/14587 +48 |-Decimal(float(" nan ")) # Decimal(" nan ") + 48 |+Decimal(" nan ") # Decimal(" nan ") +49 49 | Decimal(float(" +nan ")) # Decimal(" +nan ") +50 50 | # In this one case, " -nan ", the fix has to be +51 51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` + +FURB157.py:49:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +47 | # https://github.com/astral-sh/ruff/issues/14587 +48 | Decimal(float(" nan ")) # Decimal(" nan ") +49 | Decimal(float(" +nan ")) # Decimal(" +nan ") + | ^^^^^^^^^^^^^^^ FURB157 +50 | # In this one case, " -nan ", the fix has to be +51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` + | + = help: Replace with `" +nan "` + +ℹ Safe fix +46 46 | # Non-finite variants +47 47 | # https://github.com/astral-sh/ruff/issues/14587 +48 48 | Decimal(float(" nan ")) # Decimal(" nan ") +49 |-Decimal(float(" +nan ")) # Decimal(" +nan ") + 49 |+Decimal(" +nan ") # Decimal(" +nan ") +50 50 | # In this one case, " -nan ", the fix has to be +51 51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 52 | Decimal(float(" -nan ")) # Decimal(" nan ") + +FURB157.py:52:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +50 | # In this one case, " -nan ", the fix has to be +51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 | Decimal(float(" -nan ")) # Decimal(" nan ") + | ^^^^^^^^^^^^^^^ FURB157 +53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 | Decimal(float(" +inf ")) # Decimal(" +inf ") + | + = help: Replace with `" nan "` + +ℹ Safe fix +49 49 | Decimal(float(" +nan ")) # Decimal(" +nan ") +50 50 | # In this one case, " -nan ", the fix has to be +51 51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 |-Decimal(float(" -nan ")) # Decimal(" nan ") + 52 |+Decimal(" nan ") # Decimal(" nan ") +53 53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") + +FURB157.py:53:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 | Decimal(float(" -nan ")) # Decimal(" nan ") +53 | Decimal(float(" inf ")) # Decimal(" inf ") + | ^^^^^^^^^^^^^^ FURB157 +54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 | Decimal(float(" -inf ")) # Decimal(" -inf ") + | + = help: Replace with `" inf "` + +ℹ Safe fix +50 50 | # In this one case, " -nan ", the fix has to be +51 51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 52 | Decimal(float(" -nan ")) # Decimal(" nan ") +53 |-Decimal(float(" inf ")) # Decimal(" inf ") + 53 |+Decimal(" inf ") # Decimal(" inf ") +54 54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 56 | Decimal(float(" infinity ")) # Decimal(" infinity ") + +FURB157.py:54:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +52 | Decimal(float(" -nan ")) # Decimal(" nan ") +53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 | Decimal(float(" +inf ")) # Decimal(" +inf ") + | ^^^^^^^^^^^^^^^ FURB157 +55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 | Decimal(float(" infinity ")) # Decimal(" infinity ") + | + = help: Replace with `" +inf "` + +ℹ Safe fix +51 51 | # `Decimal(" nan ")`` because `Decimal("-nan") != Decimal(float("-nan"))` +52 52 | Decimal(float(" -nan ")) # Decimal(" nan ") +53 53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 |-Decimal(float(" +inf ")) # Decimal(" +inf ") + 54 |+Decimal(" +inf ") # Decimal(" +inf ") +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") + +FURB157.py:55:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 | Decimal(float(" -inf ")) # Decimal(" -inf ") + | ^^^^^^^^^^^^^^^ FURB157 +56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") + | + = help: Replace with `" -inf "` + +ℹ Safe fix +52 52 | Decimal(float(" -nan ")) # Decimal(" nan ") +53 53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 |-Decimal(float(" -inf ")) # Decimal(" -inf ") + 55 |+Decimal(" -inf ") # Decimal(" -inf ") +56 56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") +58 58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + +FURB157.py:56:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 | Decimal(float(" infinity ")) # Decimal(" infinity ") + | ^^^^^^^^^^^^^^^^^^^ FURB157 +57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") +58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + | + = help: Replace with `" infinity "` + +ℹ Safe fix +53 53 | Decimal(float(" inf ")) # Decimal(" inf ") +54 54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 |-Decimal(float(" infinity ")) # Decimal(" infinity ") + 56 |+Decimal(" infinity ") # Decimal(" infinity ") +57 57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") +58 58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + +FURB157.py:57:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") + | ^^^^^^^^^^^^^^^^^^^^ FURB157 +58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + | + = help: Replace with `" +infinity "` + +ℹ Safe fix +54 54 | Decimal(float(" +inf ")) # Decimal(" +inf ") +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 |-Decimal(float(" +infinity ")) # Decimal(" +infinity ") + 57 |+Decimal(" +infinity ") # Decimal(" +infinity ") +58 58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + +FURB157.py:58:9: FURB157 [*] Verbose expression in `Decimal` constructor + | +56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") +58 | Decimal(float(" -infinity ")) # Decimal(" -infinity ") + | ^^^^^^^^^^^^^^^^^^^^ FURB157 + | + = help: Replace with `" -infinity "` + +ℹ Safe fix +55 55 | Decimal(float(" -inf ")) # Decimal(" -inf ") +56 56 | Decimal(float(" infinity ")) # Decimal(" infinity ") +57 57 | Decimal(float(" +infinity ")) # Decimal(" +infinity ") +58 |-Decimal(float(" -infinity ")) # Decimal(" -infinity ") + 58 |+Decimal(" -infinity ") # Decimal(" -infinity ") diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 870cd5f3eb53a..14d6f3e121a35 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -10,6 +10,7 @@ mod tests { use std::path::Path; use anyhow::Result; + use regex::Regex; use rustc_hash::FxHashSet; use test_case::test_case; @@ -70,6 +71,7 @@ mod tests { #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))] + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( @@ -409,7 +411,8 @@ mod tests { #[test_case(Rule::MapIntVersionParsing, Path::new("RUF048_1.py"))] #[test_case(Rule::UnrawRePattern, Path::new("RUF039.py"))] #[test_case(Rule::UnrawRePattern, Path::new("RUF039_concat.py"))] - #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055.py"))] + #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_0.py"))] + #[test_case(Rule::UnnecessaryRegularExpression, Path::new("RUF055_1.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", @@ -449,4 +452,32 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"), r"^_+", 1)] + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"), r"", 2)] + fn custom_regexp_preset( + rule_code: Rule, + path: &Path, + regex_pattern: &str, + id: u8, + ) -> Result<()> { + // Compile the regex from the pattern string + let regex = Regex::new(regex_pattern).unwrap(); + + let snapshot = format!( + "custom_dummy_var_regexp_preset__{}_{}_{}", + rule_code.noqa_code(), + path.to_string_lossy(), + id, + ); + let diagnostics = test_path( + Path::new("ruff").join(path).as_path(), + &settings::LinterSettings { + dummy_variable_rgx: regex, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index a53994e3c928e..69f92b8da2bab 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -38,6 +38,7 @@ pub(crate) use unraw_re_pattern::*; pub(crate) use unsafe_markup_use::*; pub(crate) use unused_async::*; pub(crate) use unused_noqa::*; +pub(crate) use used_dummy_variable::*; pub(crate) use useless_if_else::*; pub(crate) use zip_instead_of_pairwise::*; @@ -85,6 +86,7 @@ mod unraw_re_pattern; mod unsafe_markup_use; mod unused_async; mod unused_noqa; +mod used_dummy_variable; mod useless_if_else; mod zip_instead_of_pairwise; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs index 94769dab8af1f..f88afb8863137 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_regular_expression.rs @@ -1,8 +1,11 @@ -use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix}; +use itertools::Itertools; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::{ - Arguments, CmpOp, Expr, ExprAttribute, ExprCall, ExprCompare, ExprContext, Identifier, + Arguments, CmpOp, Expr, ExprAttribute, ExprCall, ExprCompare, ExprContext, ExprStringLiteral, + Identifier, }; +use ruff_python_semantic::analyze::typing::find_binding_value; use ruff_python_semantic::{Modules, SemanticModel}; use ruff_text_size::TextRange; @@ -53,17 +56,19 @@ use crate::checkers::ast::Checker; /// - [Python Regular Expression HOWTO: Common Problems - Use String Methods](https://docs.python.org/3/howto/regex.html#use-string-methods) #[derive(ViolationMetadata)] pub(crate) struct UnnecessaryRegularExpression { - replacement: String, + replacement: Option, } -impl AlwaysFixableViolation for UnnecessaryRegularExpression { +impl Violation for UnnecessaryRegularExpression { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { "Plain string pattern passed to `re` function".to_string() } - fn fix_title(&self) -> String { - format!("Replace with `{}`", self.replacement) + fn fix_title(&self) -> Option { + Some(format!("Replace with `{}`", self.replacement.as_ref()?)) } } @@ -90,8 +95,8 @@ pub(crate) fn unnecessary_regular_expression(checker: &mut Checker, call: &ExprC return; }; - // For now, restrict this rule to string literals - let Some(string_lit) = re_func.pattern.as_string_literal_expr() else { + // For now, restrict this rule to string literals and variables that can be resolved to literals + let Some(string_lit) = resolve_string_literal(re_func.pattern, semantic) else { return; }; @@ -110,33 +115,36 @@ pub(crate) fn unnecessary_regular_expression(checker: &mut Checker, call: &ExprC // we can proceed with the str method replacement let new_expr = re_func.replacement(); - let repl = checker.generator().expr(&new_expr); - let diagnostic = Diagnostic::new( + let repl = new_expr.map(|expr| checker.generator().expr(&expr)); + let mut diagnostic = Diagnostic::new( UnnecessaryRegularExpression { replacement: repl.clone(), }, call.range, ); - let fix = Fix::applicable_edit( - Edit::range_replacement(repl, call.range), - if checker - .comment_ranges() - .has_comments(call, checker.source()) - { - Applicability::Unsafe - } else { - Applicability::Safe - }, - ); + if let Some(repl) = repl { + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement(repl, call.range), + if checker + .comment_ranges() + .has_comments(call, checker.source()) + { + Applicability::Unsafe + } else { + Applicability::Safe + }, + )); + } - checker.diagnostics.push(diagnostic.with_fix(fix)); + checker.diagnostics.push(diagnostic); } /// The `re` functions supported by this rule. #[derive(Debug)] enum ReFuncKind<'a> { - Sub { repl: &'a Expr }, + // Only `Some` if it's a fixable `re.sub()` call + Sub { repl: Option<&'a Expr> }, Match, Search, Fullmatch, @@ -152,7 +160,7 @@ struct ReFunc<'a> { impl<'a> ReFunc<'a> { fn from_call_expr( - semantic: &SemanticModel, + semantic: &'a SemanticModel, call: &'a ExprCall, func_name: &str, ) -> Option { @@ -173,11 +181,32 @@ impl<'a> ReFunc<'a> { // version ("sub", 3) => { let repl = call.arguments.find_argument("repl", 1)?; - if !repl.is_string_literal_expr() { - return None; + let lit = resolve_string_literal(repl, semantic)?; + let mut fixable = true; + for (c, next) in lit.value.chars().tuple_windows() { + // `\0` (or any other ASCII digit) and `\g` have special meaning in `repl` strings. + // Meanwhile, nearly all other escapes of ASCII letters in a `repl` string causes + // `re.PatternError` to be raised at runtime. + // + // If we see that the escaped character is an alphanumeric ASCII character, + // we should only emit a diagnostic suggesting to replace the `re.sub()` call with + // `str.replace`if we can detect that the escaped character is one that is both + // valid in a `repl` string *and* does not have any special meaning in a REPL string. + // + // It's out of scope for this rule to change invalid `re.sub()` calls into something + // that would not raise an exception at runtime. They should be left as-is. + if c == '\\' && next.is_ascii_alphanumeric() { + if "abfnrtv".contains(next) { + fixable = false; + } else { + return None; + } + } } Some(ReFunc { - kind: ReFuncKind::Sub { repl }, + kind: ReFuncKind::Sub { + repl: fixable.then_some(repl), + }, pattern: call.arguments.find_argument("pattern", 0)?, string: call.arguments.find_argument("string", 2)?, }) @@ -201,20 +230,20 @@ impl<'a> ReFunc<'a> { } } - fn replacement(&self) -> Expr { + fn replacement(&self) -> Option { match self.kind { // string.replace(pattern, repl) - ReFuncKind::Sub { repl } => { - self.method_expr("replace", vec![self.pattern.clone(), repl.clone()]) - } + ReFuncKind::Sub { repl } => repl + .cloned() + .map(|repl| self.method_expr("replace", vec![self.pattern.clone(), repl])), // string.startswith(pattern) - ReFuncKind::Match => self.method_expr("startswith", vec![self.pattern.clone()]), + ReFuncKind::Match => Some(self.method_expr("startswith", vec![self.pattern.clone()])), // pattern in string - ReFuncKind::Search => self.compare_expr(CmpOp::In), + ReFuncKind::Search => Some(self.compare_expr(CmpOp::In)), // string == pattern - ReFuncKind::Fullmatch => self.compare_expr(CmpOp::Eq), + ReFuncKind::Fullmatch => Some(self.compare_expr(CmpOp::Eq)), // string.split(pattern) - ReFuncKind::Split => self.method_expr("split", vec![self.pattern.clone()]), + ReFuncKind::Split => Some(self.method_expr("split", vec![self.pattern.clone()])), } } @@ -248,3 +277,23 @@ impl<'a> ReFunc<'a> { }) } } + +/// Try to resolve `name` to an [`ExprStringLiteral`] in `semantic`. +fn resolve_string_literal<'a>( + name: &'a Expr, + semantic: &'a SemanticModel, +) -> Option<&'a ExprStringLiteral> { + if name.is_string_literal_expr() { + return name.as_string_literal_expr(); + } + + if let Some(name_expr) = name.as_name_expr() { + let binding = semantic.binding(semantic.only_binding(name_expr)?); + let value = find_binding_value(binding, semantic)?; + if value.is_string_literal_expr() { + return value.as_string_literal_expr(); + } + } + + None +} diff --git a/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs new file mode 100644 index 0000000000000..3be6928f8af49 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs @@ -0,0 +1,223 @@ +use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::helpers::is_dunder; +use ruff_python_semantic::{Binding, BindingKind, ScopeId}; +use ruff_python_stdlib::{ + builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, +}; +use ruff_text_size::Ranged; + +use crate::{checkers::ast::Checker, renamer::Renamer}; + +/// ## What it does +/// Checks for "dummy variables" (variables that are named as if to indicate they are unused) +/// that are in fact used. +/// +/// By default, "dummy variables" are any variables with names that start with leading +/// underscores. However, this is customisable using the [`lint.dummy-variable-rgx`] setting). +/// +/// Dunder variables are ignored by this rule, as are variables named `_`. +/// +/// ## Why is this bad? +/// Marking a variable with a leading underscore conveys that it is intentionally unused within the function or method. +/// When these variables are later referenced in the code, it causes confusion and potential misunderstandings about +/// the code's intention. If a variable marked as "unused" is subsequently used, it suggests that either the variable +/// could be given a clearer name, or that the code is accidentally making use of the wrong variable. +/// +/// Sometimes leading underscores are used to avoid variables shadowing other variables, Python builtins, or Python +/// keywords. However, [PEP 8] recommends to use trailing underscores for this rather than leading underscores. +/// +/// ## Example +/// ```python +/// def function(): +/// _variable = 3 +/// return _variable + 1 +/// ``` +/// +/// Use instead: +/// ```python +/// def function(): +/// variable = 3 +/// return variable + 1 +/// ``` +/// +/// ## Fix availability +/// The rule's fix is only available for variables that start with leading underscores. +/// It will also only be available if the "obvious" new name for the variable +/// would not shadow any other known variables already accessible from the scope +/// in which the variable is defined. +/// +/// ## Options +/// - [`lint.dummy-variable-rgx`] +/// +/// [PEP 8]: https://peps.python.org/pep-0008/ +#[derive(ViolationMetadata)] +pub(crate) struct UsedDummyVariable { + name: String, + shadowed_kind: Option, +} + +impl Violation for UsedDummyVariable { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("Local dummy variable `{}` is accessed", self.name) + } + + fn fix_title(&self) -> Option { + self.shadowed_kind.map(|kind| match kind { + ShadowedKind::BuiltIn => { + "Prefer using trailing underscores to avoid shadowing a built-in".to_string() + } + ShadowedKind::Keyword => { + "Prefer using trailing underscores to avoid shadowing a keyword".to_string() + } + ShadowedKind::Some => { + "Prefer using trailing underscores to avoid shadowing a variable".to_string() + } + ShadowedKind::None => "Remove leading underscores".to_string(), + }) + } +} + +/// RUF052 +pub(crate) fn used_dummy_variable(checker: &Checker, binding: &Binding) -> Option { + let name = binding.name(checker.source()); + + // Ignore `_` and dunder variables + if name == "_" || is_dunder(name) { + return None; + } + // only used variables + if binding.is_unused() { + return None; + } + // Only variables defined via function arguments or assignments. + if !matches!( + binding.kind, + BindingKind::Argument | BindingKind::Assignment + ) { + return None; + } + // This excludes `global` and `nonlocal` variables. + if binding.is_global() || binding.is_nonlocal() { + return None; + } + + let semantic = checker.semantic(); + + // Only variables defined in function scopes + let scope = &semantic.scopes[binding.scope]; + if !scope.kind.is_function() { + return None; + } + if !checker.settings.dummy_variable_rgx.is_match(name) { + return None; + } + + let shadowed_kind = try_shadowed_kind(name, checker, binding.scope); + + let mut diagnostic = Diagnostic::new( + UsedDummyVariable { + name: name.to_string(), + shadowed_kind, + }, + binding.range(), + ); + + // If fix available + if let Some(shadowed_kind) = shadowed_kind { + // Get the possible fix based on the scope + if let Some(fix) = get_possible_fix(name, shadowed_kind, binding.scope, checker) { + diagnostic.try_set_fix(|| { + Renamer::rename(name, &fix, scope, semantic, checker.stylist()) + .map(|(edit, rest)| Fix::safe_edits(edit, rest)) + }); + } + } + + Some(diagnostic) +} + +/// Enumeration of various ways in which a binding can shadow other variables +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum ShadowedKind { + /// The variable shadows a global, nonlocal or local symbol + Some, + /// The variable shadows a builtin symbol + BuiltIn, + /// The variable shadows a keyword + Keyword, + /// The variable does not shadow any other symbols + None, +} + +/// Suggests a potential alternative name to resolve a shadowing conflict. +fn get_possible_fix( + name: &str, + kind: ShadowedKind, + scope_id: ScopeId, + checker: &Checker, +) -> Option { + // Remove leading underscores for processing + let trimmed_name = name.trim_start_matches('_'); + + // Construct the potential fix name based on ShadowedKind + let fix_name = match kind { + ShadowedKind::Some | ShadowedKind::BuiltIn | ShadowedKind::Keyword => { + format!("{trimmed_name}_") // Append an underscore + } + ShadowedKind::None => trimmed_name.to_string(), + }; + + // Check if the fix name is again dummy identifier + if checker.settings.dummy_variable_rgx.is_match(&fix_name) { + return None; + } + + // Ensure the fix name is not already taken in the scope or enclosing scopes + if !checker + .semantic() + .is_available_in_scope(&fix_name, scope_id) + { + return None; + } + + // Check if the fix name is a valid identifier + is_identifier(&fix_name).then_some(fix_name) +} + +/// Determines the kind of shadowing or conflict for a given variable name. +fn try_shadowed_kind(name: &str, checker: &Checker, scope_id: ScopeId) -> Option { + // If the name starts with an underscore, we don't consider it + if !name.starts_with('_') { + return None; + } + + // Trim the leading underscores for further checks + let trimmed_name = name.trim_start_matches('_'); + + // Check the kind in order of precedence + if is_keyword(trimmed_name) { + return Some(ShadowedKind::Keyword); + } + + if is_python_builtin( + trimmed_name, + checker.settings.target_version.minor(), + checker.source_type.is_ipynb(), + ) { + return Some(ShadowedKind::BuiltIn); + } + + if !checker + .semantic() + .is_available_in_scope(trimmed_name, scope_id) + { + return Some(ShadowedKind::Some); + } + + // Default to no shadowing + Some(ShadowedKind::None) +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap new file mode 100644 index 0000000000000..f81ceaccac880 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap @@ -0,0 +1,172 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed + | +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +74 74 | +75 75 | class Class_: +76 76 | def fun(self): +77 |- _var = "method variable" # [RUF052] +78 |- return _var + 77 |+ var = "method variable" # [RUF052] + 78 |+ return var +79 79 | +80 80 | def fun(_var): # [RUF052] +81 81 | return _var + +RUF052.py:80:9: RUF052 [*] Local dummy variable `_var` is accessed + | +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +77 77 | _var = "method variable" # [RUF052] +78 78 | return _var +79 79 | +80 |-def fun(_var): # [RUF052] +81 |- return _var + 80 |+def fun(var): # [RUF052] + 81 |+ return var +82 82 | +83 83 | def fun(): +84 84 | _list = "built-in" # [RUF052] + +RUF052.py:84:5: RUF052 [*] Local dummy variable `_list` is accessed + | +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +ℹ Safe fix +81 81 | return _var +82 82 | +83 83 | def fun(): +84 |- _list = "built-in" # [RUF052] +85 |- return _list + 84 |+ list_ = "built-in" # [RUF052] + 85 |+ return list_ +86 86 | +87 87 | x = "global" +88 88 | + +RUF052.py:91:5: RUF052 [*] Local dummy variable `_x` is accessed + | +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +88 88 | +89 89 | def fun(): +90 90 | global x +91 |- _x = "shadows global" # [RUF052] +92 |- return _x + 91 |+ x_ = "shadows global" # [RUF052] + 92 |+ return x_ +93 93 | +94 94 | def foo(): +95 95 | x = "outer" + +RUF052.py:98:5: RUF052 [*] Local dummy variable `_x` is accessed + | + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +95 95 | x = "outer" +96 96 | def bar(): +97 97 | nonlocal x +98 |- _x = "shadows nonlocal" # [RUF052] +99 |- return _x + 98 |+ x_ = "shadows nonlocal" # [RUF052] + 99 |+ return x_ +100 100 | bar() +101 101 | return x +102 102 | + +RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed + | +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +102 102 | +103 103 | def fun(): +104 104 | x = "local" +105 |- _x = "shadows local" # [RUF052] +106 |- return _x + 105 |+ x_ = "shadows local" # [RUF052] + 106 |+ return x_ +107 107 | +108 108 | +109 109 | GLOBAL_1 = "global 1" + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap new file mode 100644 index 0000000000000..f81ceaccac880 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap @@ -0,0 +1,172 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed + | +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +74 74 | +75 75 | class Class_: +76 76 | def fun(self): +77 |- _var = "method variable" # [RUF052] +78 |- return _var + 77 |+ var = "method variable" # [RUF052] + 78 |+ return var +79 79 | +80 80 | def fun(_var): # [RUF052] +81 81 | return _var + +RUF052.py:80:9: RUF052 [*] Local dummy variable `_var` is accessed + | +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +77 77 | _var = "method variable" # [RUF052] +78 78 | return _var +79 79 | +80 |-def fun(_var): # [RUF052] +81 |- return _var + 80 |+def fun(var): # [RUF052] + 81 |+ return var +82 82 | +83 83 | def fun(): +84 84 | _list = "built-in" # [RUF052] + +RUF052.py:84:5: RUF052 [*] Local dummy variable `_list` is accessed + | +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +ℹ Safe fix +81 81 | return _var +82 82 | +83 83 | def fun(): +84 |- _list = "built-in" # [RUF052] +85 |- return _list + 84 |+ list_ = "built-in" # [RUF052] + 85 |+ return list_ +86 86 | +87 87 | x = "global" +88 88 | + +RUF052.py:91:5: RUF052 [*] Local dummy variable `_x` is accessed + | +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +88 88 | +89 89 | def fun(): +90 90 | global x +91 |- _x = "shadows global" # [RUF052] +92 |- return _x + 91 |+ x_ = "shadows global" # [RUF052] + 92 |+ return x_ +93 93 | +94 94 | def foo(): +95 95 | x = "outer" + +RUF052.py:98:5: RUF052 [*] Local dummy variable `_x` is accessed + | + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +95 95 | x = "outer" +96 96 | def bar(): +97 97 | nonlocal x +98 |- _x = "shadows nonlocal" # [RUF052] +99 |- return _x + 98 |+ x_ = "shadows nonlocal" # [RUF052] + 99 |+ return x_ +100 100 | bar() +101 101 | return x +102 102 | + +RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed + | +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +102 102 | +103 103 | def fun(): +104 104 | x = "local" +105 |- _x = "shadows local" # [RUF052] +106 |- return _x + 105 |+ x_ = "shadows local" # [RUF052] + 106 |+ return x_ +107 107 | +108 108 | +109 109 | GLOBAL_1 = "global 1" + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap new file mode 100644 index 0000000000000..b519bae0f8170 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap @@ -0,0 +1,158 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF052.py:21:9: RUF052 Local dummy variable `arg` is accessed + | +19 | _valid_fun() +20 | +21 | def fun(arg): + | ^^^ RUF052 +22 | _valid_unused_var = arg +23 | pass + | + +RUF052.py:50:18: RUF052 Local dummy variable `self` is accessed + | +48 | print(_valid_private_cls_attr) +49 | +50 | def __init__(self): + | ^^^^ RUF052 +51 | self._valid_private_ins_attr = 2 +52 | print(self._valid_private_ins_attr) + | + +RUF052.py:54:23: RUF052 Local dummy variable `self` is accessed + | +52 | print(self._valid_private_ins_attr) +53 | +54 | def _valid_method(self): + | ^^^^ RUF052 +55 | return self._valid_private_ins_attr + | + +RUF052.py:57:16: RUF052 Local dummy variable `arg` is accessed + | +55 | return self._valid_private_ins_attr +56 | +57 | def method(arg): + | ^^^ RUF052 +58 | _valid_unused_var = arg +59 | return + | + +RUF052.py:61:9: RUF052 Local dummy variable `x` is accessed + | +59 | return +60 | +61 | def fun(x): + | ^ RUF052 +62 | _ = 1 +63 | __ = 2 + | + +RUF052.py:77:9: RUF052 Local dummy variable `_var` is accessed + | +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var + | + = help: Remove leading underscores + +RUF052.py:80:9: RUF052 Local dummy variable `_var` is accessed + | +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var + | + = help: Remove leading underscores + +RUF052.py:84:5: RUF052 Local dummy variable `_list` is accessed + | +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +RUF052.py:91:5: RUF052 Local dummy variable `_x` is accessed + | +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:95:3: RUF052 Local dummy variable `x` is accessed + | +94 | def foo(): +95 | x = "outer" + | ^ RUF052 +96 | def bar(): +97 | nonlocal x + | + +RUF052.py:98:5: RUF052 Local dummy variable `_x` is accessed + | + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:105:5: RUF052 Local dummy variable `_x` is accessed + | +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055.py.snap deleted file mode 100644 index 9624f9143627f..0000000000000 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055.py.snap +++ /dev/null @@ -1,129 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text ---- -RUF055.py:6:1: RUF055 [*] Plain string pattern passed to `re` function - | -5 | # this should be replaced with s.replace("abc", "") -6 | re.sub("abc", "", s) - | ^^^^^^^^^^^^^^^^^^^^ RUF055 - | - = help: Replace with `s.replace("abc", "")` - -ℹ Safe fix -3 3 | s = "str" -4 4 | -5 5 | # this should be replaced with s.replace("abc", "") -6 |-re.sub("abc", "", s) - 6 |+s.replace("abc", "") -7 7 | -8 8 | -9 9 | # this example, adapted from https://docs.python.org/3/library/re.html#re.sub, - -RUF055.py:22:4: RUF055 [*] Plain string pattern passed to `re` function - | -20 | # this one should be replaced with s.startswith("abc") because the Match is -21 | # used in an if context for its truth value -22 | if re.match("abc", s): - | ^^^^^^^^^^^^^^^^^^ RUF055 -23 | pass -24 | if m := re.match("abc", s): # this should *not* be replaced - | - = help: Replace with `s.startswith("abc")` - -ℹ Safe fix -19 19 | -20 20 | # this one should be replaced with s.startswith("abc") because the Match is -21 21 | # used in an if context for its truth value -22 |-if re.match("abc", s): - 22 |+if s.startswith("abc"): -23 23 | pass -24 24 | if m := re.match("abc", s): # this should *not* be replaced -25 25 | pass - -RUF055.py:29:4: RUF055 [*] Plain string pattern passed to `re` function - | -28 | # this should be replaced with "abc" in s -29 | if re.search("abc", s): - | ^^^^^^^^^^^^^^^^^^^ RUF055 -30 | pass -31 | re.search("abc", s) # this should not be replaced - | - = help: Replace with `"abc" in s` - -ℹ Safe fix -26 26 | re.match("abc", s) # this should not be replaced because match returns a Match -27 27 | -28 28 | # this should be replaced with "abc" in s -29 |-if re.search("abc", s): - 29 |+if "abc" in s: -30 30 | pass -31 31 | re.search("abc", s) # this should not be replaced -32 32 | - -RUF055.py:34:4: RUF055 [*] Plain string pattern passed to `re` function - | -33 | # this should be replaced with "abc" == s -34 | if re.fullmatch("abc", s): - | ^^^^^^^^^^^^^^^^^^^^^^ RUF055 -35 | pass -36 | re.fullmatch("abc", s) # this should not be replaced - | - = help: Replace with `"abc" == s` - -ℹ Safe fix -31 31 | re.search("abc", s) # this should not be replaced -32 32 | -33 33 | # this should be replaced with "abc" == s -34 |-if re.fullmatch("abc", s): - 34 |+if "abc" == s: -35 35 | pass -36 36 | re.fullmatch("abc", s) # this should not be replaced -37 37 | - -RUF055.py:39:1: RUF055 [*] Plain string pattern passed to `re` function - | -38 | # this should be replaced with s.split("abc") -39 | re.split("abc", s) - | ^^^^^^^^^^^^^^^^^^ RUF055 -40 | -41 | # these currently should not be modified because the patterns contain regex - | - = help: Replace with `s.split("abc")` - -ℹ Safe fix -36 36 | re.fullmatch("abc", s) # this should not be replaced -37 37 | -38 38 | # this should be replaced with s.split("abc") -39 |-re.split("abc", s) - 39 |+s.split("abc") -40 40 | -41 41 | # these currently should not be modified because the patterns contain regex -42 42 | # metacharacters - -RUF055.py:70:1: RUF055 [*] Plain string pattern passed to `re` function - | -69 | # this should trigger an unsafe fix because of the presence of comments -70 | / re.sub( -71 | | # pattern -72 | | "abc", -73 | | # repl -74 | | "", -75 | | s, # string -76 | | ) - | |_^ RUF055 - | - = help: Replace with `s.replace("abc", "")` - -ℹ Unsafe fix -67 67 | re.split("abc", s, maxsplit=2) -68 68 | -69 69 | # this should trigger an unsafe fix because of the presence of comments -70 |-re.sub( -71 |- # pattern -72 |- "abc", -73 |- # repl -74 |- "", -75 |- s, # string -76 |-) - 70 |+s.replace("abc", "") diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_0.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_0.py.snap new file mode 100644 index 0000000000000..f3117d28b0b1c --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_0.py.snap @@ -0,0 +1,227 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF055_0.py:6:1: RUF055 [*] Plain string pattern passed to `re` function + | +5 | # this should be replaced with s.replace("abc", "") +6 | re.sub("abc", "", s) + | ^^^^^^^^^^^^^^^^^^^^ RUF055 + | + = help: Replace with `s.replace("abc", "")` + +ℹ Safe fix +3 3 | s = "str" +4 4 | +5 5 | # this should be replaced with s.replace("abc", "") +6 |-re.sub("abc", "", s) + 6 |+s.replace("abc", "") +7 7 | +8 8 | +9 9 | # this example, adapted from https://docs.python.org/3/library/re.html#re.sub, + +RUF055_0.py:22:4: RUF055 [*] Plain string pattern passed to `re` function + | +20 | # this one should be replaced with s.startswith("abc") because the Match is +21 | # used in an if context for its truth value +22 | if re.match("abc", s): + | ^^^^^^^^^^^^^^^^^^ RUF055 +23 | pass +24 | if m := re.match("abc", s): # this should *not* be replaced + | + = help: Replace with `s.startswith("abc")` + +ℹ Safe fix +19 19 | +20 20 | # this one should be replaced with s.startswith("abc") because the Match is +21 21 | # used in an if context for its truth value +22 |-if re.match("abc", s): + 22 |+if s.startswith("abc"): +23 23 | pass +24 24 | if m := re.match("abc", s): # this should *not* be replaced +25 25 | pass + +RUF055_0.py:29:4: RUF055 [*] Plain string pattern passed to `re` function + | +28 | # this should be replaced with "abc" in s +29 | if re.search("abc", s): + | ^^^^^^^^^^^^^^^^^^^ RUF055 +30 | pass +31 | re.search("abc", s) # this should not be replaced + | + = help: Replace with `"abc" in s` + +ℹ Safe fix +26 26 | re.match("abc", s) # this should not be replaced because match returns a Match +27 27 | +28 28 | # this should be replaced with "abc" in s +29 |-if re.search("abc", s): + 29 |+if "abc" in s: +30 30 | pass +31 31 | re.search("abc", s) # this should not be replaced +32 32 | + +RUF055_0.py:34:4: RUF055 [*] Plain string pattern passed to `re` function + | +33 | # this should be replaced with "abc" == s +34 | if re.fullmatch("abc", s): + | ^^^^^^^^^^^^^^^^^^^^^^ RUF055 +35 | pass +36 | re.fullmatch("abc", s) # this should not be replaced + | + = help: Replace with `"abc" == s` + +ℹ Safe fix +31 31 | re.search("abc", s) # this should not be replaced +32 32 | +33 33 | # this should be replaced with "abc" == s +34 |-if re.fullmatch("abc", s): + 34 |+if "abc" == s: +35 35 | pass +36 36 | re.fullmatch("abc", s) # this should not be replaced +37 37 | + +RUF055_0.py:39:1: RUF055 [*] Plain string pattern passed to `re` function + | +38 | # this should be replaced with s.split("abc") +39 | re.split("abc", s) + | ^^^^^^^^^^^^^^^^^^ RUF055 +40 | +41 | # these currently should not be modified because the patterns contain regex + | + = help: Replace with `s.split("abc")` + +ℹ Safe fix +36 36 | re.fullmatch("abc", s) # this should not be replaced +37 37 | +38 38 | # this should be replaced with s.split("abc") +39 |-re.split("abc", s) + 39 |+s.split("abc") +40 40 | +41 41 | # these currently should not be modified because the patterns contain regex +42 42 | # metacharacters + +RUF055_0.py:70:1: RUF055 [*] Plain string pattern passed to `re` function + | +69 | # this should trigger an unsafe fix because of the presence of comments +70 | / re.sub( +71 | | # pattern +72 | | "abc", +73 | | # repl +74 | | "", +75 | | s, # string +76 | | ) + | |_^ RUF055 +77 | +78 | # A diagnostic should not be emitted for `sub` replacements with backreferences or + | + = help: Replace with `s.replace("abc", "")` + +ℹ Unsafe fix +67 67 | re.split("abc", s, maxsplit=2) +68 68 | +69 69 | # this should trigger an unsafe fix because of the presence of comments +70 |-re.sub( +71 |- # pattern +72 |- "abc", +73 |- # repl +74 |- "", +75 |- s, # string +76 |-) + 70 |+s.replace("abc", "") +77 71 | +78 72 | # A diagnostic should not be emitted for `sub` replacements with backreferences or +79 73 | # most other ASCII escapes + +RUF055_0.py:88:1: RUF055 [*] Plain string pattern passed to `re` function + | +86 | # *not* `some_string.replace("a", "\\n")`. +87 | # We currently emit diagnostics for some of these without fixing them. +88 | re.sub(r"a", "\n", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +89 | re.sub(r"a", r"\n", "a") +90 | re.sub(r"a", "\a", "a") + | + = help: Replace with `"a".replace("a", "\n")` + +ℹ Safe fix +85 85 | # `re.sub(r"a", r"\n", some_string)` is fixed to `some_string.replace("a", "\n")` +86 86 | # *not* `some_string.replace("a", "\\n")`. +87 87 | # We currently emit diagnostics for some of these without fixing them. +88 |-re.sub(r"a", "\n", "a") + 88 |+"a".replace("a", "\n") +89 89 | re.sub(r"a", r"\n", "a") +90 90 | re.sub(r"a", "\a", "a") +91 91 | re.sub(r"a", r"\a", "a") + +RUF055_0.py:89:1: RUF055 Plain string pattern passed to `re` function + | +87 | # We currently emit diagnostics for some of these without fixing them. +88 | re.sub(r"a", "\n", "a") +89 | re.sub(r"a", r"\n", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +90 | re.sub(r"a", "\a", "a") +91 | re.sub(r"a", r"\a", "a") + | + +RUF055_0.py:90:1: RUF055 [*] Plain string pattern passed to `re` function + | +88 | re.sub(r"a", "\n", "a") +89 | re.sub(r"a", r"\n", "a") +90 | re.sub(r"a", "\a", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +91 | re.sub(r"a", r"\a", "a") + | + = help: Replace with `"a".replace("a", "\x07")` + +ℹ Safe fix +87 87 | # We currently emit diagnostics for some of these without fixing them. +88 88 | re.sub(r"a", "\n", "a") +89 89 | re.sub(r"a", r"\n", "a") +90 |-re.sub(r"a", "\a", "a") + 90 |+"a".replace("a", "\x07") +91 91 | re.sub(r"a", r"\a", "a") +92 92 | +93 93 | re.sub(r"a", "\?", "a") + +RUF055_0.py:91:1: RUF055 Plain string pattern passed to `re` function + | +89 | re.sub(r"a", r"\n", "a") +90 | re.sub(r"a", "\a", "a") +91 | re.sub(r"a", r"\a", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +92 | +93 | re.sub(r"a", "\?", "a") + | + +RUF055_0.py:93:1: RUF055 [*] Plain string pattern passed to `re` function + | +91 | re.sub(r"a", r"\a", "a") +92 | +93 | re.sub(r"a", "\?", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +94 | re.sub(r"a", r"\?", "a") + | + = help: Replace with `"a".replace("a", "\\?")` + +ℹ Safe fix +90 90 | re.sub(r"a", "\a", "a") +91 91 | re.sub(r"a", r"\a", "a") +92 92 | +93 |-re.sub(r"a", "\?", "a") + 93 |+"a".replace("a", "\\?") +94 94 | re.sub(r"a", r"\?", "a") + +RUF055_0.py:94:1: RUF055 [*] Plain string pattern passed to `re` function + | +93 | re.sub(r"a", "\?", "a") +94 | re.sub(r"a", r"\?", "a") + | ^^^^^^^^^^^^^^^^^^^^^^^^ RUF055 + | + = help: Replace with `"a".replace("a", "\\?")` + +ℹ Safe fix +91 91 | re.sub(r"a", r"\a", "a") +92 92 | +93 93 | re.sub(r"a", "\?", "a") +94 |-re.sub(r"a", r"\?", "a") + 94 |+"a".replace("a", "\\?") diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_1.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_1.py.snap new file mode 100644 index 0000000000000..c9c05238b731b --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF055_RUF055_1.py.snap @@ -0,0 +1,39 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF055_1.py:9:1: RUF055 [*] Plain string pattern passed to `re` function + | + 7 | pat1 = "needle" + 8 | + 9 | re.sub(pat1, "", haystack) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF055 +10 | +11 | # aliases are not followed, so this one should not trigger the rule + | + = help: Replace with `haystack.replace(pat1, "")` + +ℹ Safe fix +6 6 | +7 7 | pat1 = "needle" +8 8 | +9 |-re.sub(pat1, "", haystack) + 9 |+haystack.replace(pat1, "") +10 10 | +11 11 | # aliases are not followed, so this one should not trigger the rule +12 12 | if pat4 := pat1: + +RUF055_1.py:17:1: RUF055 [*] Plain string pattern passed to `re` function + | +15 | # also works for the `repl` argument in sub +16 | repl = "new" +17 | re.sub(r"abc", repl, haystack) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF055 + | + = help: Replace with `haystack.replace("abc", repl)` + +ℹ Safe fix +14 14 | +15 15 | # also works for the `repl` argument in sub +16 16 | repl = "new" +17 |-re.sub(r"abc", repl, haystack) + 17 |+haystack.replace("abc", repl) diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index b5d7e993ec201..f602c763046fd 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -325,9 +325,15 @@ impl<'a> SemanticModel<'a> { } /// Return `true` if `member` is an "available" symbol, i.e., a symbol that has not been bound - /// in the current scope, or in any containing scope. + /// in the current scope currently being visited, or in any containing scope. pub fn is_available(&self, member: &str) -> bool { - self.lookup_symbol(member) + self.is_available_in_scope(member, self.scope_id) + } + + /// Return `true` if `member` is an "available" symbol in a given scope, i.e., + /// a symbol that has not been bound in that current scope, or in any containing scope. + pub fn is_available_in_scope(&self, member: &str, scope_id: ScopeId) -> bool { + self.lookup_symbol_in_scope(member, scope_id, false) .map(|binding_id| &self.bindings[binding_id]) .map_or(true, |binding| binding.kind.is_builtin()) } @@ -620,10 +626,22 @@ impl<'a> SemanticModel<'a> { } } - /// Lookup a symbol in the current scope. This is a carbon copy of [`Self::resolve_load`], but - /// doesn't add any read references to the resolved symbol. + /// Lookup a symbol in the current scope. pub fn lookup_symbol(&self, symbol: &str) -> Option { - if self.in_forward_reference() { + self.lookup_symbol_in_scope(symbol, self.scope_id, self.in_forward_reference()) + } + + /// Lookup a symbol in a certain scope + /// + /// This is a carbon copy of [`Self::resolve_load`], but + /// doesn't add any read references to the resolved symbol. + pub fn lookup_symbol_in_scope( + &self, + symbol: &str, + scope_id: ScopeId, + in_forward_reference: bool, + ) -> Option { + if in_forward_reference { if let Some(binding_id) = self.scopes.global().get(symbol) { if !self.bindings[binding_id].is_unbound() { return Some(binding_id); @@ -633,7 +651,7 @@ impl<'a> SemanticModel<'a> { let mut seen_function = false; let mut class_variables_visible = true; - for (index, scope_id) in self.scopes.ancestor_ids(self.scope_id).enumerate() { + for (index, scope_id) in self.scopes.ancestor_ids(scope_id).enumerate() { let scope = &self.scopes[scope_id]; if scope.kind.is_class() { if seen_function && matches!(symbol, "__class__") { diff --git a/crates/ruff_python_stdlib/src/keyword.rs b/crates/ruff_python_stdlib/src/keyword.rs index 7f361c0b6988f..6057a0d9cf1dc 100644 --- a/crates/ruff_python_stdlib/src/keyword.rs +++ b/crates/ruff_python_stdlib/src/keyword.rs @@ -1,5 +1,5 @@ // See: https://github.com/python/cpython/blob/9d692841691590c25e6cf5b2250a594d3bf54825/Lib/keyword.py#L18 -pub(crate) fn is_keyword(name: &str) -> bool { +pub fn is_keyword(name: &str) -> bool { matches!( name, "False" diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 7da7f9dd5e19a..0d9fc739c557c 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -1342,7 +1342,7 @@ pub struct Flake8ImportConventionsOptions { "dask.dataframe" = "dd" "# )] - pub extend_aliases: Option>, + pub extend_aliases: Option>, /// A mapping from module to its banned import aliases. #[option( @@ -1415,6 +1415,15 @@ impl<'de> Deserialize<'de> for Alias { D: Deserializer<'de>, { let name = String::deserialize(deserializer)?; + // Assigning to "__debug__" is a SyntaxError + // see the note here: + // https://docs.python.org/3/library/constants.html#debug__ + if &*name == "__debug__" { + return Err(de::Error::invalid_value( + de::Unexpected::Str(&name), + &"an assignable Python identifier", + )); + } if is_identifier(&name) { Ok(Self(name)) } else { @@ -1436,7 +1445,11 @@ impl Flake8ImportConventionsOptions { None => flake8_import_conventions::settings::default_aliases(), }; if let Some(extend_aliases) = self.extend_aliases { - aliases.extend(extend_aliases); + aliases.extend( + extend_aliases + .into_iter() + .map(|(module, alias)| (module.into_string(), alias.into_string())), + ); } flake8_import_conventions::settings::Settings { diff --git a/ruff.schema.json b/ruff.schema.json index 025f8e3011999..72ff32f77bace 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1168,7 +1168,7 @@ "null" ], "additionalProperties": { - "type": "string" + "$ref": "#/definitions/Alias" } } }, @@ -3845,6 +3845,7 @@ "RUF041", "RUF048", "RUF05", + "RUF052", "RUF055", "RUF1", "RUF10",