From 4d3e48b12b82d27e67e8134de4570122eabdd0b7 Mon Sep 17 00:00:00 2001 From: karpetrosyan Date: Tue, 7 Nov 2023 16:15:05 +0400 Subject: [PATCH 1/4] Add TRIO110 rule --- .../test/fixtures/flake8_trio/TRIO110.py | 13 ++++ .../src/checkers/ast/analyze/statement.rs | 6 ++ crates/ruff_linter/src/codes.rs | 1 + .../ruff_linter/src/rules/flake8_trio/mod.rs | 1 + .../src/rules/flake8_trio/rules/mod.rs | 2 + .../rules/flake8_trio/rules/unneeded_sleep.rs | 74 +++++++++++++++++++ ...lake8_trio__tests__TRIO110_TRIO110.py.snap | 26 +++++++ ruff.schema.json | 1 + 8 files changed, 124 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py create mode 100644 crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs create mode 100644 crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py b/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py new file mode 100644 index 0000000000000..d6d7d96b948df --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py @@ -0,0 +1,13 @@ +import trio + +async def foo(): + while True: + await trio.sleep(10) + +async def foo(): + while True: + await trio.sleep_until(10) + +async def foo(): + while True: + trio.sleep(10) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 51863307ea29d..f55c7402661c3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1216,6 +1216,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::TryExceptInLoop) { perflint::rules::try_except_in_loop(checker, body); } + if checker.enabled(Rule::TryExceptInLoop) { + perflint::rules::try_except_in_loop(checker, body); + } + if checker.enabled(Rule::TrioUnneededSleep) { + flake8_trio::rules::unneeded_sleep(checker, stmt, body); + } } Stmt::For( for_stmt @ ast::StmtFor { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 320b62020d4b3..b0ebd06d97b55 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -293,6 +293,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { // flake8-trio (Flake8Trio, "100") => (RuleGroup::Preview, rules::flake8_trio::rules::TrioTimeoutWithoutAwait), (Flake8Trio, "105") => (RuleGroup::Preview, rules::flake8_trio::rules::TrioSyncCall), + (Flake8Trio, "110") => (RuleGroup::Preview, rules::flake8_trio::rules::TrioUnneededSleep), (Flake8Trio, "115") => (RuleGroup::Preview, rules::flake8_trio::rules::TrioZeroSleepCall), // flake8-builtins diff --git a/crates/ruff_linter/src/rules/flake8_trio/mod.rs b/crates/ruff_linter/src/rules/flake8_trio/mod.rs index f68d797581856..20724ac7e6c9c 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/mod.rs @@ -16,6 +16,7 @@ mod tests { #[test_case(Rule::TrioTimeoutWithoutAwait, Path::new("TRIO100.py"))] #[test_case(Rule::TrioSyncCall, Path::new("TRIO105.py"))] + #[test_case(Rule::TrioUnneededSleep, Path::new("TRIO110.py"))] #[test_case(Rule::TrioZeroSleepCall, Path::new("TRIO115.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/mod.rs index ed3806b3f241f..267b80231f8cc 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/mod.rs @@ -1,7 +1,9 @@ pub(crate) use sync_call::*; pub(crate) use timeout_without_await::*; +pub(crate) use unneeded_sleep::*; pub(crate) use zero_sleep_call::*; mod sync_call; mod timeout_without_await; +mod unneeded_sleep; mod zero_sleep_call; diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs new file mode 100644 index 0000000000000..d2038d70de1d2 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs @@ -0,0 +1,74 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{Expr, ExprAwait, ExprCall, Stmt, StmtExpr}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for trio functions that should contain await but don't. +/// +/// ## Why is this bad? +/// Some trio context managers, such as `trio.fail_after` and +/// `trio.move_on_after`, have no effect unless they contain an `await` +/// statement. The use of such functions without an `await` statement is +/// likely a mistake. +/// +/// ## Example +/// ```python +/// async def func(): +/// with trio.move_on_after(2): +/// do_something() +/// ``` +/// +/// Use instead: +/// ```python +/// async def func(): +/// with trio.move_on_after(2): +/// do_something() +/// await awaitable() +/// ``` +#[violation] +pub struct TrioUnneededSleep; + +impl Violation for TrioUnneededSleep { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use event instead of `while : await trio.sleep()`") + } +} + +pub(crate) fn unneeded_sleep(checker: &mut Checker, stmt: &Stmt, body: &[Stmt]) { + if body.len() != 1 { + return; + } + + let awaitable = { + if let Stmt::Expr(StmtExpr { range: _, value }) = &body[0] { + if let Expr::Await(ExprAwait { range: _, value }) = value.as_ref() { + Some(value.as_ref()) + } else { + None + } + } else { + None + } + }; + + if let Some(Expr::Call(ExprCall { + range: _, + func, + arguments: _, + })) = awaitable + { + if checker + .semantic() + .resolve_call_path(func.as_ref()) + .is_some_and(|path| matches!(path.as_slice(), ["trio", "sleep" | "sleep_until"])) + { + checker + .diagnostics + .push(Diagnostic::new(TrioUnneededSleep, stmt.range())); + } + } +} diff --git a/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap b/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap new file mode 100644 index 0000000000000..315ada3f84c9a --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap @@ -0,0 +1,26 @@ +--- +source: crates/ruff_linter/src/rules/flake8_trio/mod.rs +--- +TRIO110.py:4:5: TRIO110 Use event instead of `while : await trio.sleep()` + | +3 | async def foo(): +4 | while True: + | _____^ +5 | | await trio.sleep(10) + | |____________________________^ TRIO110 +6 | +7 | async def foo(): + | + +TRIO110.py:8:5: TRIO110 Use event instead of `while : await trio.sleep()` + | + 7 | async def foo(): + 8 | while True: + | _____^ + 9 | | await trio.sleep_until(10) + | |__________________________________^ TRIO110 +10 | +11 | async def foo(): + | + + diff --git a/ruff.schema.json b/ruff.schema.json index c77e480c46860..3221ae59daa8a 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3475,6 +3475,7 @@ "TRIO100", "TRIO105", "TRIO11", + "TRIO110", "TRIO115", "TRY", "TRY0", From 9683375e883602414ff42dd44f9979a316ced570 Mon Sep 17 00:00:00 2001 From: karpetrosyan Date: Tue, 7 Nov 2023 16:46:29 +0400 Subject: [PATCH 2/4] Fix doc --- .../rules/flake8_trio/rules/unneeded_sleep.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs index d2038d70de1d2..828dd4404ed4b 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs @@ -6,27 +6,27 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for trio functions that should contain await but don't. +/// Checks for the while loop, which waits for the event. /// /// ## Why is this bad? -/// Some trio context managers, such as `trio.fail_after` and -/// `trio.move_on_after`, have no effect unless they contain an `await` -/// statement. The use of such functions without an `await` statement is -/// likely a mistake. +/// Instead of sleeping in a loop waiting for a condition to be true, +/// it's preferable to use a trio. Event. /// /// ## Example /// ```python +/// DONE = False +/// /// async def func(): -/// with trio.move_on_after(2): -/// do_something() +/// while not DONE: +/// await trio.sleep(1) /// ``` /// /// Use instead: /// ```python +/// DONE = trio.Event() +/// /// async def func(): -/// with trio.move_on_after(2): -/// do_something() -/// await awaitable() +/// await DONE.wait() /// ``` #[violation] pub struct TrioUnneededSleep; From ebfcfb0bb174c59dfa0f26fff4697e2c1dda59c5 Mon Sep 17 00:00:00 2001 From: Kar Petrosyan <92274156+karpetrosyan@users.noreply.github.com> Date: Tue, 7 Nov 2023 16:47:52 +0400 Subject: [PATCH 3/4] Update crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs --- .../ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs index 828dd4404ed4b..e3bad1068434a 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs @@ -10,7 +10,7 @@ use crate::checkers::ast::Checker; /// /// ## Why is this bad? /// Instead of sleeping in a loop waiting for a condition to be true, -/// it's preferable to use a trio. Event. +/// it's preferable to use a `trio.Event`. /// /// ## Example /// ```python From cdd9910a9162f458c4889abd8bcc1de6412bf1a1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 7 Nov 2023 16:04:28 -0500 Subject: [PATCH 4/4] Tweaks --- .../test/fixtures/flake8_trio/TRIO110.py | 9 ++- .../src/checkers/ast/analyze/statement.rs | 7 +-- .../rules/flake8_trio/rules/unneeded_sleep.rs | 60 +++++++++---------- ...lake8_trio__tests__TRIO110_TRIO110.py.snap | 20 +++---- 4 files changed, 43 insertions(+), 53 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py b/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py index d6d7d96b948df..b0f3abed4ab90 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_trio/TRIO110.py @@ -1,13 +1,16 @@ import trio -async def foo(): + +async def func(): while True: await trio.sleep(10) -async def foo(): + +async def func(): while True: await trio.sleep_until(10) -async def foo(): + +async def func(): while True: trio.sleep(10) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index f55c7402661c3..6cf80b7f870dd 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1206,7 +1206,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_trio::rules::timeout_without_await(checker, with_stmt, items); } } - Stmt::While(ast::StmtWhile { body, orelse, .. }) => { + Stmt::While(while_stmt @ ast::StmtWhile { body, orelse, .. }) => { if checker.enabled(Rule::FunctionUsesLoopVariable) { flake8_bugbear::rules::function_uses_loop_variable(checker, &Node::Stmt(stmt)); } @@ -1216,11 +1216,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::TryExceptInLoop) { perflint::rules::try_except_in_loop(checker, body); } - if checker.enabled(Rule::TryExceptInLoop) { - perflint::rules::try_except_in_loop(checker, body); - } if checker.enabled(Rule::TrioUnneededSleep) { - flake8_trio::rules::unneeded_sleep(checker, stmt, body); + flake8_trio::rules::unneeded_sleep(checker, while_stmt); } } Stmt::For( diff --git a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs index e3bad1068434a..d7387f5835d96 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs +++ b/crates/ruff_linter/src/rules/flake8_trio/rules/unneeded_sleep.rs @@ -1,21 +1,22 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Expr, ExprAwait, ExprCall, Stmt, StmtExpr}; +use ruff_python_ast::{self as ast, Expr, Stmt}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for the while loop, which waits for the event. +/// Checks for the use of `trio.sleep` in a `while` loop. /// /// ## Why is this bad? -/// Instead of sleeping in a loop waiting for a condition to be true, -/// it's preferable to use a `trio.Event`. +/// Instead of sleeping in a `while` loop, and waiting for a condition +/// to become true, it's preferable to `wait()` on a `trio.Event`. /// /// ## Example /// ```python /// DONE = False /// +/// /// async def func(): /// while not DONE: /// await trio.sleep(1) @@ -25,6 +26,7 @@ use crate::checkers::ast::Checker; /// ```python /// DONE = trio.Event() /// +/// /// async def func(): /// await DONE.wait() /// ``` @@ -34,41 +36,33 @@ pub struct TrioUnneededSleep; impl Violation for TrioUnneededSleep { #[derive_message_formats] fn message(&self) -> String { - format!("Use event instead of `while : await trio.sleep()`") + format!("Use `trio.Event` instead of awaiting `trio.sleep` in a `while` loop") } } -pub(crate) fn unneeded_sleep(checker: &mut Checker, stmt: &Stmt, body: &[Stmt]) { - if body.len() != 1 { +/// TRIO110 +pub(crate) fn unneeded_sleep(checker: &mut Checker, while_stmt: &ast::StmtWhile) { + // The body should be a single `await` call. + let [stmt] = while_stmt.body.as_slice() else { + return; + }; + let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { + return; + }; + let Expr::Await(ast::ExprAwait { value, .. }) = value.as_ref() else { + return; + }; + let Expr::Call(ast::ExprCall { func, .. }) = value.as_ref() else { return; - } - - let awaitable = { - if let Stmt::Expr(StmtExpr { range: _, value }) = &body[0] { - if let Expr::Await(ExprAwait { range: _, value }) = value.as_ref() { - Some(value.as_ref()) - } else { - None - } - } else { - None - } }; - if let Some(Expr::Call(ExprCall { - range: _, - func, - arguments: _, - })) = awaitable + if checker + .semantic() + .resolve_call_path(func.as_ref()) + .is_some_and(|path| matches!(path.as_slice(), ["trio", "sleep" | "sleep_until"])) { - if checker - .semantic() - .resolve_call_path(func.as_ref()) - .is_some_and(|path| matches!(path.as_slice(), ["trio", "sleep" | "sleep_until"])) - { - checker - .diagnostics - .push(Diagnostic::new(TrioUnneededSleep, stmt.range())); - } + checker + .diagnostics + .push(Diagnostic::new(TrioUnneededSleep, while_stmt.range())); } } diff --git a/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap b/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap index 315ada3f84c9a..d95be0a65d321 100644 --- a/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap +++ b/crates/ruff_linter/src/rules/flake8_trio/snapshots/ruff_linter__rules__flake8_trio__tests__TRIO110_TRIO110.py.snap @@ -1,26 +1,22 @@ --- source: crates/ruff_linter/src/rules/flake8_trio/mod.rs --- -TRIO110.py:4:5: TRIO110 Use event instead of `while : await trio.sleep()` +TRIO110.py:5:5: TRIO110 Use `trio.Event` instead of awaiting `trio.sleep` in a `while` loop | -3 | async def foo(): -4 | while True: +4 | async def func(): +5 | while True: | _____^ -5 | | await trio.sleep(10) +6 | | await trio.sleep(10) | |____________________________^ TRIO110 -6 | -7 | async def foo(): | -TRIO110.py:8:5: TRIO110 Use event instead of `while : await trio.sleep()` +TRIO110.py:10:5: TRIO110 Use `trio.Event` instead of awaiting `trio.sleep` in a `while` loop | - 7 | async def foo(): - 8 | while True: + 9 | async def func(): +10 | while True: | _____^ - 9 | | await trio.sleep_until(10) +11 | | await trio.sleep_until(10) | |__________________________________^ TRIO110 -10 | -11 | async def foo(): |