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

Implement flake8-async rules #8451

Open
12 of 37 tasks
charliermarsh opened this issue Nov 3, 2023 · 13 comments
Open
12 of 37 tasks

Implement flake8-async rules #8451

charliermarsh opened this issue Nov 3, 2023 · 13 comments
Labels
plugin Implementing a known but unsupported plugin

Comments

@charliermarsh
Copy link
Member

charliermarsh commented Nov 3, 2023

General Rules

  • ASYNC100: A with trio.fail_after(...): or with trio.move_on_after(...): context does not contain any await statements. This makes it pointless, as the timeout can only be triggered by a checkpoint.
  • ASYNC101: yield inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling.
  • ASYNC102: It's unsafe to await inside finally: or except BaseException/trio.Cancelled unless you use a shielded cancel scope with a timeout.
  • ASYNC103: except BaseException, except trio.Cancelled or a bare except: with a code path that doesn't re-raise. If you don't want to re-raise BaseException, add a separate handler for trio.Cancelled before.
  • ASYNC104: Cancelled and BaseException must be re-raised - when a user tries to return or raise a different exception.
  • ASYNC105: Calling a trio async function without immediately awaiting it.
  • ASYNC106: trio must be imported with import trio for the linter to work.
  • ASYNC109: Async function definition with a timeout parameter - use trio.[fail/move_on]_[after/at] instead
  • ASYNC110: while <condition>: await trio.sleep() should be replaced by a trio.Event.
  • ASYNC111: Variable, from context manager opened inside nursery, passed to start[_soon] might be invalidly accessed while in use, due to context manager closing before the nursery. This is usually a bug, and nurseries should generally be the inner-most context manager.
  • ASYNC112: Nursery body with only a call to nursery.start[_soon] and not passing itself as a parameter can be replaced with a regular function call.
  • ASYNC113: Using nursery.start_soon in __aenter__ doesn't wait for the task to begin. Consider replacing with nursery.start.
  • ASYNC114: Startable function (i.e. has a task_status keyword parameter) not in --startable-in-context-manager parameter list, please add it so TRIO113 can catch errors when using it.
  • ASYNC115: Replace trio.sleep(0) with the more suggestive trio.lowlevel.checkpoint().
  • ASYNC116: trio.sleep() with >24 hour interval should usually be trio.sleep_forever().
  • ASYNC118: Don't assign the value of anyio.get_cancelled_exc_class() to a variable, since that breaks linter checks and multi-backend programs.
  • ASYNC119: yield in context manager in async generator is unsafe, the cleanup may be delayed until await is no longer allowed. We strongly encourage you to read PEP 533 and use async with aclosing(…), or better yet avoid async generators entirely (see ASYNC900 ) in favor of context managers which return an iterable channel/stream/queue.
  • ASYNC120: Dangerous Checkpoint inside an except block. If this checkpoint is cancelled, the current active exception will be replaced by the Cancelled exception, and cannot be reraised later. This will not trigger when ASYNC102 does, and if you don’t care about losing non-cancelled exceptions you could disable this rule. This is currently not able to detect asyncio shields.

Blocking sync calls in async functions

  • ASYNC200: User-configured error for blocking sync calls in async functions. Does nothing by default, see trio200-blocking-calls for how to configure it.
  • ASYNC210: Sync HTTP call in async function, use httpx.AsyncClient.
  • ASYNC211: Likely sync HTTP call in async function, use httpx.AsyncClient. Looks for urllib3 method calls on pool objects, but only matching on the method signature and not the object.
  • ASYNC212: Blocking sync HTTP call on httpx object, use httpx.AsyncClient.
  • ASYNC220: Sync process call in async function, use await nursery.start(trio.run_process, ...).
  • ASYNC221: Sync process call in async function, use await trio.run_process(...).
  • ASYNC222: Sync os.* call in async function, wrap in await trio.to_thread.run_sync().
  • ASYNC230: Sync IO call in async function, use trio.open_file(...).
  • ASYNC231: Sync IO call in async function, use trio.wrap_file(...).
  • ASYNC232: Blocking sync call on file object, wrap the file object in trio.wrap_file() to get an async file object.
  • ASYNC240: Avoid using os.path in async functions, prefer using trio.Path objects.
  • ASYNC250: Builtin input() should not be called from async function. Wrap in trio.to_thread.run_sync()/anyio.to_thread.run_sync() or asyncio.loop.run_in_executor().
  • ASYNC251: time.sleep() should not be called from async function. Use trio.sleep()/anyio.sleep()/asyncio.sleep().

Asyncio-specific rules

  • ASYNC300: Calling asyncio.create_task() without saving the result. A task that isn’t referenced elsewhere may get garbage collected at any time, even before it’s done. Note that this rule won’t check whether the variable the result is saved in is susceptible to being garbage-collected itself. See the asyncio documentation for best practices. You might consider instead using a TaskGroup and calling asyncio.TaskGroup.create_task() to avoid this problem, and gain the advantages of structured concurrency with e.g. better cancellation semantics.

Optional rules disabled by default

  • ASYNC900: Async generator without @asynccontextmanager not allowed.
  • ASYNC910: Exit or return from async function with no guaranteed checkpoint or exception since function definition.
  • ASYNC911: Exit, yield or return from async iterable with no guaranteed checkpoint since possible function entry (yield or function definition) Checkpoints are await, async for, and async with (on one of enter/exit).
  • ASYNC912: A timeout/cancelscope has checkpoints, but they’re not guaranteed to run. Similar to ASYNC100, but it does not warn on trivial cases where there is no checkpoint at all. It instead shares logic with ASYNC910 and ASYNC911 for parsing conditionals and branches.
  • ASYNC913: An indefinite loop (e.g. while True) has no guaranteed checkpoint. This could potentially cause a deadlock.

Resources

charliermarsh pushed a commit that referenced this issue Nov 3, 2023
## Summary

This pull request adds
[flake8-trio](https://github.com/Zac-HD/flake8-trio) support to ruff,
which is a very useful plugin for trio users to avoid very common
mistakes.

Part of #8451.

## Test Plan

Traditional rule testing, as [described in the
documentation](https://docs.astral.sh/ruff/contributing/#rule-testing-fixtures-and-snapshots).
@charliermarsh charliermarsh added the plugin Implementing a known but unsupported plugin label Nov 3, 2023
@qdegraaf
Copy link
Contributor

qdegraaf commented Nov 4, 2023

TRIO118 sounds like it is not needed in Ruff, similar to TRIO106

The TRIO2XX rules are partially covered by the port of flake8_async.

The recommended fixes in flake8-trio are TRIO specific though. How do we want to combine those two?

charliermarsh pushed a commit that referenced this issue Nov 5, 2023
## Summary

Adds `TRIO105` from the [flake8-trio
plugin](https://github.com/Zac-HD/flake8-trio). The `MethodName` logic
mirrors that of `TRIO100` to stay consistent within the plugin.

It is at 95% parity with the exception of upstream also checking for a
slightly more complex scenario where a call to `start()` on a
`trio.Nursery` context should also be immediately awaited. Upstream
plugin appears to just check for anything named `nursery` judging from
[the relevant issue](python-trio/flake8-async#56).

Unsure if we want to do so something similar or, alternatively, if there
is some capability in ruff to check for calls made on this context some
other way

## Test Plan

Added a new fixture, based on [the one from upstream
plugin](https://github.com/Zac-HD/flake8-trio/blob/main/tests/eval_files/trio105.py)

## Issue link

Refers: #8451
charliermarsh pushed a commit that referenced this issue Nov 6, 2023
## Summary

Adds `TRIO115` from the [flake8-trio
plugin](https://github.com/Zac-HD/flake8-trio).

## Test Plan

Added a new fixture, based on [the one from upstream
plugin](https://github.com/Zac-HD/flake8-trio/blob/main/tests/eval_files/trio115.py)

## Issue link

Relates to: #8451
This was referenced Nov 7, 2023
charliermarsh pushed a commit that referenced this issue Nov 7, 2023
## Summary

Adds TRIO110 from the [flake8-trio
plugin](https://github.com/Zac-HD/flake8-trio).
Relates to: #8451
charliermarsh pushed a commit that referenced this issue Nov 7, 2023
## Summary

Adds TRIO109 from the [flake8-trio
plugin](https://github.com/Zac-HD/flake8-trio).
Relates to: #8451
@jakkdl
Copy link

jakkdl commented Nov 10, 2023

whoa, this is super cool!! As the primary author of flake8-trio I'm super honored to be included in ruff ❤️
Feel free to ping me about any questions of functionality or implementation details
@Zac-HD might also have opinions or suggestions about stuff

@jakkdl
Copy link

jakkdl commented Nov 10, 2023

The recommended fixes in flake8-trio are TRIO specific though. How do we want to combine those two?

There's some anyio support as well in flake8-trio and for a while we considered renaming the library. If flake8-trio sees import anyio and no import trio in the code, or --anyio is passed it will suggest autofixes from the anyio library instead of trio.

@jakkdl
Copy link

jakkdl commented Nov 10, 2023

TRIO118 sounds like it is not needed in Ruff, similar to TRIO106

TRIO106 definitely isn't needed by itself, and solely a consequence of implementation at the time. TRIO118 is only partially a weakness in implementation, the part about multi-backend programs still holds though.

The TRIO2XX rules are partially covered by the port of flake8_async.

Yeah there's overlap there, though I gave the implementation in flake8-trio quite a bit more love and the suggestions/error messages are more detailed/specific since it can know what the backend is.

TRIO105 is mostly redundant as of the recently released trio 0.23 which included types into the main package, and type checkers will complain about async functions not being awaited. I suppose it could be useful for somebody not using type-checking, but enabling this check, but I think that's a pretty slim proportion and the rule requires constantly keeping up with the API of anyio/trio.

@Zac-HD
Copy link

Zac-HD commented Nov 12, 2023

🥳 I'm delighted to see this going into ruff!

TRIO105 doesn't seem redundant to me - many people aren't using type annotations (everywhere), and typecheckers also don't offer autofixers the way that ruff can. Those fixers would mostly be unsafe, but still remarkably useful IMO!

@charliermarsh
Copy link
Member Author

Thank you both so much for chiming in here, for all the helpful commentary, and for all your work on flake8-trio (among so much else)!

I already gated one rule behind "require an import trio to be present before firing" so we now have some precedent and infrastructure for that, which will likely be helpful with a few of the other rules and their associated fixes.

@alex007sirois
Copy link

I already gated one rule behind "require an import trio to be present before firing"

Most of the rules for trio would apply for anyio too:

The recommended fixes in flake8-trio are TRIO specific though. How do we want to combine those two?

There's some anyio support as well in flake8-trio and for a while we considered renaming the library. If flake8-trio sees import anyio and no import trio in the code, or --anyio is passed it will suggest autofixes from the anyio library instead of trio.

@jakkdl
Copy link

jakkdl commented Apr 12, 2024

This issue should be renamed and all the rules in the checklist renamed to match upstream merging with flake8-async. See #10303

Also TRIO117 is removed (because trio.MultiError is removed), and there's two new rules ASYNC250 and ASYNC251

charliermarsh pushed a commit that referenced this issue May 23, 2024
…ver (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`.
@MichaReiser MichaReiser changed the title Implement flake8-trio rules Implement flake8-async rules Jun 26, 2024
@MichaReiser
Copy link
Member

I updated the issue to reflect the flake8-trio to flake8-async rename, crossed of the new rules implemented in #10416, and added new rules to the list to match flake8-async's documentation

@Zac-HD
Copy link

Zac-HD commented Aug 15, 2024

Hey all - I just tried moving some code from flake8-async to ruff check for that delicious massive speedup, and ended up back on this issue 🙂

The good news: #10416 also implemented ASYNC251!

The sad-for-me news: lots of other rules still to implement. If you feel inspired but not so inspired as to finish closing out this issue, ASYNC900 and ASYNC200 seem pretty simple to implement, and would be really nice to have...

@Zac-HD
Copy link

Zac-HD commented Sep 18, 2024

Prompted by a conversation elsewhere: if ruff starts to handle cross-file analyses, you could avoid ASYNC114 entirely (ie check whether there's a task_status= argument instead of configuring a list) which would be quite nice 🙂

@charliermarsh
Copy link
Member Author

Also prompted by a conversation elsewhere: I'd love to see implementations for ASYNC900, ASYNC910, and ASYNC911 in particular.

@jakkdl
Copy link

jakkdl commented Oct 22, 2024

New rules in upstream https://flake8-async.readthedocs.io/en/latest/rules.html#async121

  • ASYNC121 control-flow-in-taskgroup
  • ASYNC122 delayed-entry-of-relative-cancelscope
  • ASYNC123 bad-exception-group-flattening
    • this one isn't specific to async, and affects anybody trying to extract and reraise an exception contained inside an exception group. Upstream implementation is fairly crude

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Implementing a known but unsupported plugin
Projects
None yet
Development

No branches or pull requests

6 participants