-
-
Notifications
You must be signed in to change notification settings - Fork 6
add async121 control-flow-in-taskgroup #282
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
Changes from 2 commits
2742ad1
459c748
ff22623
402993e
3435f63
ceadc3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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>`. | ||||||
|
`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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not 100% on "enclosing", since often you might just have the cancelscope that is internal to the taskgroup/nursery. Pushing another suggestion
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -350,6 +350,58 @@ def visit_Yield(self, node: ast.Yield): | |
visit_Lambda = visit_AsyncFunctionDef | ||
|
||
|
||
@error_class | ||
class Visitor121(Flake8AsyncVisitor): | ||
error_codes: Mapping[str, str] = { | ||
"ASYNC121": ( | ||
"{0} in a {1} block behaves counterintuitively in several" | ||
" situations. Refactor to have the {0} outside." | ||
) | ||
} | ||
|
||
def __init__(self, *args: Any, **kwargs: Any): | ||
super().__init__(*args, **kwargs) | ||
self.unsafe_stack: list[str] = [] | ||
|
||
def visit_AsyncWith(self, node: ast.AsyncWith): | ||
self.save_state(node, "unsafe_stack", copy=True) | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps also detect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added support for |
||
item.context_expr, "create_task_group", base="anyio" | ||
): | ||
self.unsafe_stack.append("task group") | ||
|
||
def visit_While(self, node: ast.While | ast.For): | ||
self.save_state(node, "unsafe_stack", copy=True) | ||
self.unsafe_stack.append("loop") | ||
|
||
visit_For = visit_While | ||
Zac-HD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def check_loop_flow(self, node: ast.Continue | ast.Break, statement: str) -> None: | ||
# self.unsafe_stack should never be empty, but no reason not to avoid a crash | ||
# for invalid code. | ||
if self.unsafe_stack and self.unsafe_stack[-1] != "loop": | ||
self.error(node, statement, self.unsafe_stack[-1]) | ||
|
||
def visit_Continue(self, node: ast.Continue) -> None: | ||
self.check_loop_flow(node, "continue") | ||
|
||
def visit_Break(self, node: ast.Break) -> None: | ||
self.check_loop_flow(node, "break") | ||
|
||
def visit_Return(self, node: ast.Return) -> None: | ||
for unsafe_cm in "nursery", "task group": | ||
if unsafe_cm in self.unsafe_stack: | ||
self.error(node, "return", unsafe_cm) | ||
|
||
def visit_FunctionDef(self, node: ast.FunctionDef): | ||
self.save_state(node, "unsafe_stack", copy=True) | ||
self.unsafe_stack = [] | ||
|
||
|
||
@error_class_cst | ||
class Visitor300(Flake8AsyncVisitor_cst): | ||
error_codes: Mapping[str, str] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
# ASYNCIO_NO_ERROR # not a problem in asyncio | ||
# ANYIO_NO_ERROR # checked in async121_anyio.py | ||
|
||
import trio | ||
|
||
|
||
async def foo_return(): | ||
async with trio.open_nursery(): | ||
return # ASYNC121: 8, "return", "nursery" | ||
|
||
|
||
async def foo_return_nested(): | ||
async with trio.open_nursery(): | ||
|
||
def bar(): | ||
return # safe | ||
|
||
|
||
# continue | ||
async def foo_while_continue_safe(): | ||
async with trio.open_nursery(): | ||
while True: | ||
continue # safe | ||
|
||
|
||
async def foo_while_continue_unsafe(): | ||
while True: | ||
async with trio.open_nursery(): | ||
continue # ASYNC121: 12, "continue", "nursery" | ||
|
||
|
||
async def foo_for_continue_safe(): | ||
async with trio.open_nursery(): | ||
for _ in range(5): | ||
continue # safe | ||
|
||
|
||
async def foo_for_continue_unsafe(): | ||
for _ in range(5): | ||
async with trio.open_nursery(): | ||
continue # ASYNC121: 12, "continue", "nursery" | ||
|
||
|
||
# break | ||
async def foo_while_break_safe(): | ||
async with trio.open_nursery(): | ||
while True: | ||
break # safe | ||
|
||
|
||
async def foo_while_break_unsafe(): | ||
while True: | ||
async with trio.open_nursery(): | ||
break # ASYNC121: 12, "break", "nursery" | ||
|
||
|
||
async def foo_for_break_safe(): | ||
async with trio.open_nursery(): | ||
for _ in range(5): | ||
break # safe | ||
|
||
|
||
async def foo_for_break_unsafe(): | ||
for _ in range(5): | ||
async with trio.open_nursery(): | ||
break # ASYNC121: 12, "break", "nursery" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
# ASYNCIO_NO_ERROR # not a problem in asyncio | ||
# TRIO_NO_ERROR # checked in async121.py | ||
# BASE_LIBRARY anyio | ||
|
||
import anyio | ||
|
||
|
||
async def foo_return(): | ||
async with anyio.create_task_group(): | ||
return # ASYNC121: 8, "return", "task group" | ||
|
||
|
||
async def foo_return_nested(): | ||
async with anyio.create_task_group(): | ||
|
||
def bar(): | ||
return # safe | ||
|
||
|
||
# continue | ||
async def foo_while_continue_safe(): | ||
async with anyio.create_task_group(): | ||
while True: | ||
continue # safe | ||
|
||
|
||
async def foo_while_continue_unsafe(): | ||
while True: | ||
async with anyio.create_task_group(): | ||
continue # ASYNC121: 12, "continue", "task group" | ||
|
||
|
||
async def foo_for_continue_safe(): | ||
async with anyio.create_task_group(): | ||
for _ in range(5): | ||
continue # safe | ||
|
||
|
||
async def foo_for_continue_unsafe(): | ||
for _ in range(5): | ||
async with anyio.create_task_group(): | ||
continue # ASYNC121: 12, "continue", "task group" | ||
|
||
|
||
# break | ||
async def foo_while_break_safe(): | ||
async with anyio.create_task_group(): | ||
while True: | ||
break # safe | ||
|
||
|
||
async def foo_while_break_unsafe(): | ||
while True: | ||
async with anyio.create_task_group(): | ||
break # ASYNC121: 12, "break", "task group" | ||
|
||
|
||
async def foo_for_break_safe(): | ||
async with anyio.create_task_group(): | ||
for _ in range(5): | ||
break # safe | ||
|
||
|
||
async def foo_for_break_unsafe(): | ||
for _ in range(5): | ||
async with anyio.create_task_group(): | ||
break # ASYNC121: 12, "break", "task group" |
Uh oh!
There was an error while loading. Please reload this page.