diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/useless_with_lock.py b/crates/ruff_linter/resources/test/fixtures/pylint/useless_with_lock.py new file mode 100644 index 0000000000000..e7f302a39dc44 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/useless_with_lock.py @@ -0,0 +1,56 @@ +import threading +from threading import Lock, RLock, Condition, Semaphore, BoundedSemaphore + + +with threading.Lock(): # [useless-with-lock] + ... + +with Lock(): # [useless-with-lock] + ... + +with threading.Lock() as this_shouldnt_matter: # [useless-with-lock] + ... + +with threading.RLock(): # [useless-with-lock] + ... + +with RLock(): # [useless-with-lock] + ... + +with threading.Condition(): # [useless-with-lock] + ... + +with Condition(): # [useless-with-lock] + ... + +with threading.Semaphore(): # [useless-with-lock] + ... + +with Semaphore(): # [useless-with-lock] + ... + +with threading.BoundedSemaphore(): # [useless-with-lock] + ... + +with BoundedSemaphore(): # [useless-with-lock] + ... + +lock = threading.Lock() +with lock: # this is ok + ... + +rlock = threading.RLock() +with rlock: # this is ok + ... + +cond = threading.Condition() +with cond: # this is ok + ... + +sem = threading.Semaphore() +with sem: # this is ok + ... + +b_sem = threading.BoundedSemaphore() +with b_sem: # this is ok + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 29e5a72173440..dca7ded7fad19 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1186,6 +1186,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ReadWholeFile) { refurb::rules::read_whole_file(checker, with_stmt); } + if checker.enabled(Rule::UselessWithLock) { + pylint::rules::useless_with_lock(checker, with_stmt); + } } Stmt::While(ast::StmtWhile { body, orelse, .. }) => { if checker.enabled(Rule::FunctionUsesLoopVariable) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 249cddfa4276a..6e37584302f76 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -277,6 +277,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W1514") => (RuleGroup::Preview, rules::pylint::rules::UnspecifiedEncoding), #[allow(deprecated)] (Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash), + (Pylint, "W2101") => (RuleGroup::Preview, rules::pylint::rules::UselessWithLock), (Pylint, "R0904") => (RuleGroup::Preview, rules::pylint::rules::TooManyPublicMethods), (Pylint, "W2901") => (RuleGroup::Stable, rules::pylint::rules::RedefinedLoopName), #[allow(deprecated)] diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 2a80dcd63df7d..0bd6bf1962c6c 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -115,6 +115,7 @@ mod tests { #[test_case(Rule::UselessElseOnLoop, Path::new("useless_else_on_loop.py"))] #[test_case(Rule::UselessImportAlias, Path::new("import_aliasing.py"))] #[test_case(Rule::UselessReturn, Path::new("useless_return.py"))] + #[test_case(Rule::UselessWithLock, Path::new("useless_with_lock.py"))] #[test_case( Rule::YieldFromInAsyncFunction, Path::new("yield_from_in_async_function.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 5ac1858a41f7e..6c259c16ba8ac 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -64,6 +64,7 @@ pub(crate) use unspecified_encoding::*; pub(crate) use useless_else_on_loop::*; pub(crate) use useless_import_alias::*; pub(crate) use useless_return::*; +pub(crate) use useless_with_lock::*; pub(crate) use yield_from_in_async_function::*; pub(crate) use yield_in_init::*; @@ -133,5 +134,6 @@ mod unspecified_encoding; mod useless_else_on_loop; mod useless_import_alias; mod useless_return; +mod useless_with_lock; mod yield_from_in_async_function; mod yield_in_init; diff --git a/crates/ruff_linter/src/rules/pylint/rules/useless_with_lock.rs b/crates/ruff_linter/src/rules/pylint/rules/useless_with_lock.rs new file mode 100644 index 0000000000000..49bc38d1e87af --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/useless_with_lock.rs @@ -0,0 +1,86 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for direct uses of lock objects in `with` statements. +/// +/// ## Why is this bad? +/// Creating a lock (via `threading.Lock` or similar) in a `with` statement +/// has no effect, as locks are only relevant when shared between threads. +/// +/// Instead, assign the lock to a variable outside the `with` statement, +/// and share that variable between threads. +/// +/// ## Example +/// ```python +/// import threading +/// +/// counter = 0 +/// +/// +/// def increment(): +/// global counter +/// +/// with threading.Lock(): +/// counter += 1 +/// ``` +/// +/// Use instead: +/// ```python +/// import threading +/// +/// counter = 0 +/// lock = threading.Lock() +/// +/// +/// def increment(): +/// global counter +/// +/// with lock: +/// counter += 1 +/// ``` +/// +/// ## References +/// - [Python documentation: `Lock Objects`](https://docs.python.org/3/library/threading.html#lock-objects) +#[violation] +pub struct UselessWithLock; + +impl Violation for UselessWithLock { + #[derive_message_formats] + fn message(&self) -> String { + format!("Threading lock directly created in `with` statement has no effect") + } +} + +/// PLW2101 +pub(crate) fn useless_with_lock(checker: &mut Checker, with: &ast::StmtWith) { + for item in &with.items { + let Some(call) = item.context_expr.as_call_expr() else { + continue; + }; + + if !checker + .semantic() + .resolve_call_path(call.func.as_ref()) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + [ + "threading", + "Lock" | "RLock" | "Condition" | "Semaphore" | "BoundedSemaphore" + ] + ) + }) + { + return; + } + + checker + .diagnostics + .push(Diagnostic::new(UselessWithLock, call.range())); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW2101_useless_with_lock.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW2101_useless_with_lock.py.snap new file mode 100644 index 0000000000000..0715c3e5d83d9 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW2101_useless_with_lock.py.snap @@ -0,0 +1,101 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +useless_with_lock.py:5:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +5 | with threading.Lock(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^ PLW2101 +6 | ... + | + +useless_with_lock.py:8:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +6 | ... +7 | +8 | with Lock(): # [useless-with-lock] + | ^^^^^^ PLW2101 +9 | ... + | + +useless_with_lock.py:11:6: PLW2101 Threading lock directly created in `with` statement has no effect + | + 9 | ... +10 | +11 | with threading.Lock() as this_shouldnt_matter: # [useless-with-lock] + | ^^^^^^^^^^^^^^^^ PLW2101 +12 | ... + | + +useless_with_lock.py:14:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +12 | ... +13 | +14 | with threading.RLock(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^^ PLW2101 +15 | ... + | + +useless_with_lock.py:17:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +15 | ... +16 | +17 | with RLock(): # [useless-with-lock] + | ^^^^^^^ PLW2101 +18 | ... + | + +useless_with_lock.py:20:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +18 | ... +19 | +20 | with threading.Condition(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^^^^^^ PLW2101 +21 | ... + | + +useless_with_lock.py:23:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +21 | ... +22 | +23 | with Condition(): # [useless-with-lock] + | ^^^^^^^^^^^ PLW2101 +24 | ... + | + +useless_with_lock.py:26:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +24 | ... +25 | +26 | with threading.Semaphore(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^^^^^^ PLW2101 +27 | ... + | + +useless_with_lock.py:29:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +27 | ... +28 | +29 | with Semaphore(): # [useless-with-lock] + | ^^^^^^^^^^^ PLW2101 +30 | ... + | + +useless_with_lock.py:32:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +30 | ... +31 | +32 | with threading.BoundedSemaphore(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW2101 +33 | ... + | + +useless_with_lock.py:35:6: PLW2101 Threading lock directly created in `with` statement has no effect + | +33 | ... +34 | +35 | with BoundedSemaphore(): # [useless-with-lock] + | ^^^^^^^^^^^^^^^^^^ PLW2101 +36 | ... + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 82500eadbda0a..27ea51e4ceaab 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3125,6 +3125,9 @@ "PLW164", "PLW1641", "PLW2", + "PLW21", + "PLW210", + "PLW2101", "PLW29", "PLW290", "PLW2901",