add async121 control-flow-in-taskgroup #282
Conversation
docs/rules.rst
Outdated
| @@ -83,6 +83,9 @@ _`ASYNC120` : await-in-except | |||
| This will not trigger when :ref:`ASYNC102 <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. | |||
|
|
|||
| _`ASYNC121`: control-flow-in-taskgroup | |||
| `return`, `continue`, and `break` inside a :ref:`taskgroup_nursery` can lead to counterintuitive behaviour. Refactor the code to instead cancel the :ref:`cancel_scope` and place the statement outside of the TaskGroup/Nursery block. See `trio#1493 <https://github.com/python-trio/trio/issues/1493>`. | |||
There was a problem hiding this comment.
| `return`, `continue`, and `break` inside a :ref:`taskgroup_nursery` can lead to counterintuitive behaviour. Refactor the code to instead cancel the :ref:`cancel_scope` and place the statement outside of the TaskGroup/Nursery block. See `trio#1493 <https://github.com/python-trio/trio/issues/1493>`. | |
| `return`, `continue`, and `break` inside a :ref:`TaskGroup or Nursery <taskgroup_nursery>` can lead to counterintuitive behaviour. Refactor the code to instead cancel the enclosing :ref:`cancel scope <cancel_scope>` and place the control flow statement outside of the TaskGroup/Nursery block. See `Trio issue 1493 <https://github.com/python-trio/trio/issues/1493>`__. |
Suggested reword for clarity
There was a problem hiding this comment.
:ref:`taskgroup_nursery`
is equivalent to
:ref:`TaskGroup / Nursery <taskgroup_nursery>`
Sphinx takes the name of the header being linked to and inserts it, see https://flake8-async--282.org.readthedocs.build/en/282/rules.html#async121
Will incorporate the other suggestions though, thanks :)
There was a problem hiding this comment.
Also not 100% on "enclosing", since often you might just have the cancelscope that is internal to the taskgroup/nursery. Pushing another suggestion
| for item in node.items: | ||
| if get_matching_call(item.context_expr, "open_nursery", base="trio"): | ||
| self.unsafe_stack.append("nursery") | ||
| elif get_matching_call( |
There was a problem hiding this comment.
Perhaps also detect asyncio.TaskGroup? It has the same hazard.
There was a problem hiding this comment.
does it? I tried to repro any of the problems with asyncio, but didn't manage to #261 (comment)
There was a problem hiding this comment.
I left a comment there about a potential hazard that does exist with asyncio. I agree it has fewer due to the lack of user-controllable cancel scopes.
There was a problem hiding this comment.
added support for asyncio.TaskGroup 👍
… tests to be more thorough with state management.
tests/eval_files/async121.py
Outdated
| async def foo_while_continue_unsafe(): | ||
| while True: | ||
| async with trio.open_nursery(): | ||
| if condition(): | ||
| continue # ASYNC121: 16, "continue", "nursery" | ||
| continue # safe |
There was a problem hiding this comment.
What happens if we have a loop, containing a condition, containing continue or break? Let's test that?
There was a problem hiding this comment.
The if statements are solely for avoiding mypy unreachable-statement, and should have zero effect on the visitor logic. But think I added what you requested?
| @@ -83,6 +83,9 @@ _`ASYNC120` : await-in-except | |||
| This will not trigger when :ref:`ASYNC102 <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. | |||
|
|
|||
| _`ASYNC121`: control-flow-in-taskgroup | |||
| `return`, `continue`, and `break` inside a :ref:`taskgroup_nursery` can lead to counterintuitive behaviour. Refactor the code to instead cancel the :ref:`cancel_scope` inside the TaskGroup/Nursery and place the statement outside of the TaskGroup/Nursery block. In asyncio a user might expect the statement to have an immediate effect, but it will wait for all tasks to finish before having an effect. See `Trio issue #1493 <https://github.com/python-trio/trio/issues/1493>` for further issues specific to trio/anyio. | |||
There was a problem hiding this comment.
hmm, is there an .rst linter which can warn us about forgetting the trailing underscore on a hyperlink?
There was a problem hiding this comment.
After looking through 4 linters I did find one that checked for it 🥳
https://pypi.org/project/sphinx-lint/
it, and others, ofc found a bunch of sketchy stuff. Will open PR that adds them & fixes issues
closes #261
Not sure whether to name the rule
control-flow-in-taskgroup,control-flow-in-nursery, orcontrol-flow-in-taskgroup-or-nursery. All previous nursery/taskgroup-related rules have in fact been about the cancelscope within them, such that none of them had to decide.This made me want to double-check that this rule is exclusive to nursery/taskgroups, and that the rule shouldn't be expanded to also trigger on control flows inside cancelscopes, but I cannot personally come up with anything at least.
The test isn't fully exhaustive with nested loops and stuff, but the visitor is simple enough I think it's unlikely something would sneak through.