Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-async] Sleep with >24 hour interval should usually sleep forever (ASYNC116) #11498

Merged
merged 14 commits into from
May 23, 2024
Original file line number Diff line number Diff line change
@@ -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"
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,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);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/flake8_async/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_async/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
110 changes: 110 additions & 0 deletions crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs
Original file line number Diff line number Diff line change
@@ -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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing how "no gil" was renamed to "free threading" to remove negative language.

Thoughts on changing "Why is this bad" to something like "How could it be better?" across all rules?

/// `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<String> {
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)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help, as a previous c user this casting doesn't feel right to me, and @AlexWaygood and @carljm had taken a look but couldn't get this to work either.
What is the preferred way of doing this kind of logic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we had discussed a couple alternatives to avoid this lint, e.g. rounding the float value to the next highest integer and then doing an integer comparison. Did that not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, if I remember correctly we had discussed that the problem with rounding to an int was that it could possibly overflow the int and that logic would need to be checked instead.
Thus neither method was ideal and thus we needed another opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally think this is ok -- we know that this constant fits in f64.

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);
}
Original file line number Diff line number Diff line change
@@ -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"
2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading