[flake8-async] Sleep with >24 hour interval should usually sleep forever (ASYNC116) #11498
Conversation
| /// ## What it does | ||
| /// Checks for uses of `trio.sleep()` with >24 hour interval. | ||
| /// | ||
| /// ## Why is this bad? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Has a different testing methodology been thought about?
This feels error prone to me, since it can be easy to miss and difficult to know what exactly should pass or fail.
There was a problem hiding this comment.
The snapshot tests have been incredibly useful for us! I'm really happy with them -- we have extensive test coverage and it's really easy to add coverage + review changes once you're used to the format.
Structurally, it could be nice if each snippet were its own file. It could also be nice to somehow encode the expectation in the source itself, though I don't know that a strategy like that it is consistent with snapshotting.
There was a problem hiding this comment.
The snapshot tests have been incredibly useful for us!
Oh yes I agree! They are quite nice 😊 We had them at a past job, but the indirection in combination with the human element of checking caused errors to happen every so often 😞
Structurally, it could be nice if each snippet were its own file.
I was thinking something like an ASYNC116 folder with files for each test case, but I can also image that would be annoying to edit. I assume that something like this could work instead? cargo run -p ruff -- check crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC116/ --no-cache --preview --select ASYNC116
It could also be nice to somehow encode the expectation in the source itself,
Would comments work for this, like in ASYNC116.py?
though I don't know that a strategy like that it is consistent with snapshotting.
What are your thoughts on if there was an extra check that did something like "each file can only have one error" or "an error must appear at the same line as a comment"?
ruff.schema.json
Outdated
There was a problem hiding this comment.
I needed this schema to be explained to me, will add some extra documentation in a new PR to address this for future contributors if that's okay.
| } | ||
| Number::Float(float_value) => | ||
| { | ||
| #[allow(clippy::cast_precision_loss)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Personally think this is ok -- we know that this constant fits in f64.
|
| let mut diagnostic = Diagnostic::new(SleepForeverCall, call.range()); | ||
| let replacement_function = "sleep_forever"; | ||
| diagnostic.try_set_fix(|| { | ||
| let (import_edit, ..) = checker.importer().get_or_import_symbol( |
There was a problem hiding this comment.
This returns a binding that should be used below in lieu of replacement_function.to_string(), since in theory this could end up being trio.sleep_forever rather than just sleep_forever.
crates/ruff_linter/src/rules/flake8_async/rules/sleep_forever_call.rs
Outdated
Show resolved
Hide resolved
| # 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" |
There was a problem hiding this comment.
Mind running ruff format over this to ensure (e.g.) newline consistency?
There was a problem hiding this comment.
done, thoughts on having a ci job to check the format of crates/ruff_linter/resources/test/**.*.py ?
There was a problem hiding this comment.
Unfortunately in some cases we actively don't want to re-format fixtures, since we're often testing unformatted syntax intentionally (or, e.g., strange comment placement). I'd like to enable format on the fixtures, but we'd need to add suppression comments to some of the existing fixtures.
charliermarsh
added
rule
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.