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

add async121 control-flow-in-taskgroup #282

Merged
merged 6 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ Changelog

*[CalVer, YY.month.patch](https://calver.org/)*

24.8.1
jakkdl marked this conversation as resolved.
Show resolved Hide resolved
======
- Add :ref:`ASYNC121 <async121>` control-flow-in-taskgroup

24.8.1
======
- Add config option ``transform-async-generator-decorators``, to list decorators which
Expand Down
3 changes: 3 additions & 0 deletions docs/rules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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>`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`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

Copy link
Member Author

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 :)

Copy link
Member Author

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



Blocking sync calls in async functions
======================================
Expand Down
2 changes: 1 addition & 1 deletion flake8_async/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@


# CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
__version__ = "24.8.1"
__version__ = "24.8.2"


# taken from https://github.com/Zac-HD/shed
Expand Down
52 changes: 52 additions & 0 deletions flake8_async/visitors/visitors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps also detect asyncio.TaskGroup? It has the same hazard.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

added support for asyncio.TaskGroup 👍

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] = {
Expand Down
66 changes: 66 additions & 0 deletions tests/eval_files/async121.py
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"
67 changes: 67 additions & 0 deletions tests/eval_files/async121_anyio.py
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"
3 changes: 3 additions & 0 deletions tests/test_flake8_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,9 @@ def _parse_eval_file(
"ASYNC116",
"ASYNC117",
"ASYNC118",
# opening nurseries & taskgroups can only be done in async context, so ASYNC121
# doesn't check for it
"ASYNC121",
"ASYNC300",
"ASYNC912",
}
Expand Down
Loading