From ebdaf5765a3308a0903c47eb6301cc02533c185e Mon Sep 17 00:00:00 2001 From: Evan Kohilas Date: Fri, 24 May 2024 06:25:50 +1000 Subject: [PATCH] [flake8-async] Sleep with >24 hour interval should usually sleep forever (ASYNC116) (#11498) ## Summary Addresses #8451 by implementing rule 116 to add an unsafe fix when sleep is used with a >24 hour interval to instead consider sleeping forever. This rule is added as async instead as I my understanding was that these trio rules would be moved to async anyway. There are a couple of TODOs, which address further extending the rule by adding support for lookups and evaluations, and also supporting `anyio`. --- .../test/fixtures/flake8_async/ASYNC116.py | 57 +++++++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../ruff_linter/src/rules/flake8_async/mod.rs | 1 + .../src/rules/flake8_async/rules/mod.rs | 2 + .../flake8_async/rules/sleep_forever_call.rs | 110 +++++++++++++ ...e8_async__tests__ASYNC116_ASYNC116.py.snap | 145 ++++++++++++++++++ ruff.schema.json | 2 + 8 files changed, 321 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC116.py create mode 100644 crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs create mode 100644 crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC116_ASYNC116.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC116.py b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC116.py new file mode 100644 index 0000000000..2009e31147 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC116.py @@ -0,0 +1,57 @@ +# type: ignore +# ASYNCIO_NO_ERROR - no asyncio.sleep_forever, so check intentionally doesn't trigger. +import math +from math import inf + + +async def import_trio(): + import trio + + # These examples are probably not meant to ever wake up: + await trio.sleep(100000) # error: 116, "async" + + # 'inf literal' overflow trick + await trio.sleep(1e999) # error: 116, "async" + + await trio.sleep(86399) + await trio.sleep(86400) + await trio.sleep(86400.01) # error: 116, "async" + await trio.sleep(86401) # error: 116, "async" + + await trio.sleep(-1) # will raise a runtime error + await trio.sleep(0) # handled by different check + + # these ones _definitely_ never wake up (TODO) + await trio.sleep(float("inf")) + await trio.sleep(math.inf) + await trio.sleep(inf) + + # don't require inf to be in math (TODO) + await trio.sleep(np.inf) + + # don't evaluate expressions (TODO) + one_day = 86401 + await trio.sleep(86400 + 1) + await trio.sleep(60 * 60 * 24 + 1) + await trio.sleep(foo()) + await trio.sleep(one_day) + await trio.sleep(86400 + foo()) + await trio.sleep(86400 + ...) + await trio.sleep("hello") + await trio.sleep(...) + + +def not_async_fun(): + import trio + + # does not require the call to be awaited, nor in an async fun + trio.sleep(86401) # error: 116, "async" + # also checks that we don't break visit_Call + trio.run(trio.sleep(86401)) # error: 116, "async" + + +async def import_from_trio(): + from trio import sleep + + # catch from import + await sleep(86401) # error: 116, "async" diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 8382b766c0..4efcf6cbe9 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -508,6 +508,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::BlockingOsCallInAsyncFunction) { flake8_async::rules::blocking_os_call(checker, call); } + if checker.enabled(Rule::SleepForeverCall) { + flake8_async::rules::sleep_forever_call(checker, call); + } if checker.any_enabled(&[Rule::Print, Rule::PPrint]) { flake8_print::rules::print_call(checker, call); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 05906db3c9..3d490cc961 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -334,6 +334,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Async, "100") => (RuleGroup::Stable, rules::flake8_async::rules::BlockingHttpCallInAsyncFunction), (Flake8Async, "101") => (RuleGroup::Stable, rules::flake8_async::rules::OpenSleepOrSubprocessInAsyncFunction), (Flake8Async, "102") => (RuleGroup::Stable, rules::flake8_async::rules::BlockingOsCallInAsyncFunction), + (Flake8Async, "116") => (RuleGroup::Preview, rules::flake8_async::rules::SleepForeverCall), // flake8-trio (Flake8Trio, "100") => (RuleGroup::Stable, rules::flake8_trio::rules::TrioTimeoutWithoutAwait), diff --git a/crates/ruff_linter/src/rules/flake8_async/mod.rs b/crates/ruff_linter/src/rules/flake8_async/mod.rs index 37a451233e..dfbf3dab1f 100644 --- a/crates/ruff_linter/src/rules/flake8_async/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_async/mod.rs @@ -16,6 +16,7 @@ mod tests { #[test_case(Rule::BlockingHttpCallInAsyncFunction, Path::new("ASYNC100.py"))] #[test_case(Rule::OpenSleepOrSubprocessInAsyncFunction, Path::new("ASYNC101.py"))] #[test_case(Rule::BlockingOsCallInAsyncFunction, Path::new("ASYNC102.py"))] + #[test_case(Rule::SleepForeverCall, Path::new("ASYNC116.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_async/rules/mod.rs index d2c2472c5b..2ae3723f49 100644 --- a/crates/ruff_linter/src/rules/flake8_async/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_async/rules/mod.rs @@ -1,7 +1,9 @@ pub(crate) use blocking_http_call::*; pub(crate) use blocking_os_call::*; pub(crate) use open_sleep_or_subprocess_call::*; +pub(crate) use sleep_forever_call::*; mod blocking_http_call; mod blocking_os_call; mod open_sleep_or_subprocess_call; +mod sleep_forever_call; diff --git a/crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs b/crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs new file mode 100644 index 0000000000..885bda3341 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs @@ -0,0 +1,110 @@ +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{Expr, ExprCall, ExprNumberLiteral, Number}; +use ruff_python_semantic::Modules; +use ruff_text_size::Ranged; + +use crate::{checkers::ast::Checker, importer::ImportRequest}; + +/// ## What it does +/// Checks for uses of `trio.sleep()` with an interval greater than 24 hours. +/// +/// ## Why is this bad? +/// `trio.sleep()` with an interval greater than 24 hours is usually intended +/// to sleep indefinitely. Instead of using a large interval, +/// `trio.sleep_forever()` better conveys the intent. +/// +/// +/// ## Example +/// ```python +/// import trio +/// +/// +/// async def func(): +/// await trio.sleep(86401) +/// ``` +/// +/// Use instead: +/// ```python +/// import trio +/// +/// +/// async def func(): +/// await trio.sleep_forever() +/// ``` +#[violation] +pub struct SleepForeverCall; + +impl Violation for SleepForeverCall { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] + fn message(&self) -> String { + format!("`trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()`") + } + + fn fix_title(&self) -> Option { + Some(format!("Replace with `trio.sleep_forever()`")) + } +} + +/// ASYNC116 +pub(crate) fn sleep_forever_call(checker: &mut Checker, call: &ExprCall) { + if !checker.semantic().seen_module(Modules::TRIO) { + return; + } + + if call.arguments.len() != 1 { + return; + } + + let Some(arg) = call.arguments.find_argument("seconds", 0) else { + return; + }; + + if !checker + .semantic() + .resolve_qualified_name(call.func.as_ref()) + .is_some_and(|qualified_name| matches!(qualified_name.segments(), ["trio", "sleep"])) + { + return; + } + + let Expr::NumberLiteral(ExprNumberLiteral { value, .. }) = arg else { + return; + }; + + // TODO(ekohilas): Replace with Duration::from_days(1).as_secs(); when available. + let one_day_in_secs = 60 * 60 * 24; + match value { + Number::Int(int_value) => { + let Some(int_value) = int_value.as_u64() else { + return; + }; + if int_value <= one_day_in_secs { + return; + } + } + Number::Float(float_value) => + { + #[allow(clippy::cast_precision_loss)] + if *float_value <= one_day_in_secs as f64 { + return; + } + } + Number::Complex { .. } => return, + } + + let mut diagnostic = Diagnostic::new(SleepForeverCall, call.range()); + let replacement_function = "sleep_forever"; + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import_from("trio", replacement_function), + call.func.start(), + checker.semantic(), + )?; + let reference_edit = Edit::range_replacement(binding, call.func.range()); + let arg_edit = Edit::range_replacement("()".to_string(), call.arguments.range()); + Ok(Fix::unsafe_edits(import_edit, [reference_edit, arg_edit])) + }); + checker.diagnostics.push(diagnostic); +} diff --git a/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC116_ASYNC116.py.snap b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC116_ASYNC116.py.snap new file mode 100644 index 0000000000..5250751158 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_async/snapshots/ruff_linter__rules__flake8_async__tests__ASYNC116_ASYNC116.py.snap @@ -0,0 +1,145 @@ +--- +source: crates/ruff_linter/src/rules/flake8_async/mod.rs +--- +ASYNC116.py:11:11: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +10 | # These examples are probably not meant to ever wake up: +11 | await trio.sleep(100000) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^^ ASYNC116 +12 | +13 | # 'inf literal' overflow trick + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +8 8 | import trio +9 9 | +10 10 | # These examples are probably not meant to ever wake up: +11 |- await trio.sleep(100000) # error: 116, "async" + 11 |+ await trio.sleep_forever() # error: 116, "async" +12 12 | +13 13 | # 'inf literal' overflow trick +14 14 | await trio.sleep(1e999) # error: 116, "async" + +ASYNC116.py:14:11: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +13 | # 'inf literal' overflow trick +14 | await trio.sleep(1e999) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^ ASYNC116 +15 | +16 | await trio.sleep(86399) + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +11 11 | await trio.sleep(100000) # error: 116, "async" +12 12 | +13 13 | # 'inf literal' overflow trick +14 |- await trio.sleep(1e999) # error: 116, "async" + 14 |+ await trio.sleep_forever() # error: 116, "async" +15 15 | +16 16 | await trio.sleep(86399) +17 17 | await trio.sleep(86400) + +ASYNC116.py:18:11: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +16 | await trio.sleep(86399) +17 | await trio.sleep(86400) +18 | await trio.sleep(86400.01) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^^^^ ASYNC116 +19 | await trio.sleep(86401) # error: 116, "async" + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +15 15 | +16 16 | await trio.sleep(86399) +17 17 | await trio.sleep(86400) +18 |- await trio.sleep(86400.01) # error: 116, "async" + 18 |+ await trio.sleep_forever() # error: 116, "async" +19 19 | await trio.sleep(86401) # error: 116, "async" +20 20 | +21 21 | await trio.sleep(-1) # will raise a runtime error + +ASYNC116.py:19:11: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +17 | await trio.sleep(86400) +18 | await trio.sleep(86400.01) # error: 116, "async" +19 | await trio.sleep(86401) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^ ASYNC116 +20 | +21 | await trio.sleep(-1) # will raise a runtime error + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +16 16 | await trio.sleep(86399) +17 17 | await trio.sleep(86400) +18 18 | await trio.sleep(86400.01) # error: 116, "async" +19 |- await trio.sleep(86401) # error: 116, "async" + 19 |+ await trio.sleep_forever() # error: 116, "async" +20 20 | +21 21 | await trio.sleep(-1) # will raise a runtime error +22 22 | await trio.sleep(0) # handled by different check + +ASYNC116.py:48:5: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +47 | # does not require the call to be awaited, nor in an async fun +48 | trio.sleep(86401) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^ ASYNC116 +49 | # also checks that we don't break visit_Call +50 | trio.run(trio.sleep(86401)) # error: 116, "async" + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +45 45 | import trio +46 46 | +47 47 | # does not require the call to be awaited, nor in an async fun +48 |- trio.sleep(86401) # error: 116, "async" + 48 |+ trio.sleep_forever() # error: 116, "async" +49 49 | # also checks that we don't break visit_Call +50 50 | trio.run(trio.sleep(86401)) # error: 116, "async" +51 51 | + +ASYNC116.py:50:14: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +48 | trio.sleep(86401) # error: 116, "async" +49 | # also checks that we don't break visit_Call +50 | trio.run(trio.sleep(86401)) # error: 116, "async" + | ^^^^^^^^^^^^^^^^^ ASYNC116 + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +47 47 | # does not require the call to be awaited, nor in an async fun +48 48 | trio.sleep(86401) # error: 116, "async" +49 49 | # also checks that we don't break visit_Call +50 |- trio.run(trio.sleep(86401)) # error: 116, "async" + 50 |+ trio.run(trio.sleep_forever()) # error: 116, "async" +51 51 | +52 52 | +53 53 | async def import_from_trio(): + +ASYNC116.py:57:11: ASYNC116 [*] `trio.sleep()` with >24 hour interval should usually be `trio.sleep_forever()` + | +56 | # catch from import +57 | await sleep(86401) # error: 116, "async" + | ^^^^^^^^^^^^ ASYNC116 + | + = help: Replace with `trio.sleep_forever()` + +ℹ Unsafe fix +2 2 | # ASYNCIO_NO_ERROR - no asyncio.sleep_forever, so check intentionally doesn't trigger. +3 3 | import math +4 4 | from math import inf + 5 |+from trio import sleep_forever +5 6 | +6 7 | +7 8 | async def import_trio(): +-------------------------------------------------------------------------------- +54 55 | from trio import sleep +55 56 | +56 57 | # catch from import +57 |- await sleep(86401) # error: 116, "async" + 58 |+ await sleep_forever() # error: 116, "async" diff --git a/ruff.schema.json b/ruff.schema.json index 9827c7d241..2428468dde 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2685,6 +2685,8 @@ "ASYNC100", "ASYNC101", "ASYNC102", + "ASYNC11", + "ASYNC116", "B", "B0", "B00",