Skip to content

Commit ea58c7a

Browse files
authored
ASYNC100 now treats start_soon() as a cancel point (#327)
* ASYNC100 now treats start_soon() as a cancel point * oh derp, visitor91x is a CST visitor * start_soon now makes open_nursery a cancel point on exit
1 parent b842265 commit ea58c7a

13 files changed

+349
-33
lines changed

docs/changelog.rst

+5-1
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,13 @@ Changelog
44

55
`CalVer, YY.month.patch <https://calver.org/>`_
66

7+
24.11.4
8+
=======
9+
- :ref:`ASYNC100 <async100>` once again ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`, unless we find a call to ``.start_soon()``.
10+
711
24.11.3
812
=======
9-
- Revert :ref:`ASYNC100 <async100>` ignoring :func:`trio.open_nursery` and :func:`anyio.create_task_group` due to it not viewing `start_soon()` as introducing a :ref:`cancel point <cancel_point>`.
13+
- Revert :ref:`ASYNC100 <async100>` ignoring :func:`trio.open_nursery` and :func:`anyio.create_task_group` due to it not viewing ``.start_soon()`` as introducing a :ref:`cancel point <cancel_point>`.
1014

1115
24.11.2
1216
=======

docs/glossary.rst

+1-1
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ functions defined by Trio will either checkpoint or raise an exception when
9999
iteration, and when exhausting the iterator, and ``async with`` will checkpoint
100100
on at least one of enter/exit.
101101

102-
The one exception is :func:`trio.open_nursery` and :func:`anyio.create_task_group` which are :ref:`schedule points <schedule_point>` but not :ref:`cancel points <cancel_point>`.
102+
The one exception is :func:`trio.open_nursery` and :func:`anyio.create_task_group`. They do not checkpoint on entry, and on exit they insert a :ref:`schedule point <schedule_point>`. However, if sub-tasks are cancelled they will be propagated on exit, so if you're starting tasks you can usually treat the exit as a :ref:`cancel point <cancel_point>`.
103103

104104
asyncio does not place any guarantees on if or when asyncio functions will
105105
checkpoint. This means that enabling and adhering to :ref:`ASYNC91x <ASYNC910>`

docs/rules.rst

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ _`ASYNC100` : cancel-scope-no-checkpoint
1313
A :ref:`timeout_context` does not contain any :ref:`checkpoints <checkpoint>`.
1414
This makes it pointless, as the timeout can only be triggered by a checkpoint.
1515
This check also treats ``yield`` as a checkpoint, since checkpoints can happen in the caller we yield to.
16+
:func:`trio.open_nursery` and :func:`anyio.create_task_group` are excluded, as they are :ref:`schedule points <schedule_point>` but not :ref:`cancel points <cancel_point>` (unless they have tasks started in them).
1617
See :ref:`ASYNC912 <async912>` which will in addition guarantee checkpoints on every code path.
1718

1819
_`ASYNC101` : yield-in-cancel-scope

flake8_async/__init__.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838

3939

4040
# CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
41-
__version__ = "24.11.3"
41+
__version__ = "24.11.4"
4242

4343

4444
# taken from https://github.com/Zac-HD/shed

flake8_async/visitors/visitor91x.py

+73-12
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
flatten_preserving_comments,
2626
fnmatch_qualified_name_cst,
2727
func_has_decorator,
28+
identifier_to_string,
2829
iter_guaranteed_once_cst,
2930
with_has_call,
3031
)
@@ -285,6 +286,7 @@ def __init__(self, *args: Any, **kwargs: Any):
285286
# ASYNC100
286287
self.has_checkpoint_stack: list[bool] = []
287288
self.node_dict: dict[cst.With, list[AttributeCall]] = {}
289+
self.taskgroup_has_start_soon: dict[str, bool] = {}
288290

289291
# --exception-suppress-context-manager
290292
self.suppress_imported_as: list[str] = []
@@ -299,13 +301,31 @@ def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool:
299301
and self.library != ("asyncio",)
300302
)
301303

302-
def checkpoint(self) -> None:
303-
self.uncheckpointed_statements = set()
304+
def checkpoint_cancel_point(self) -> None:
304305
self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack)
306+
# don't need to look for any .start_soon() calls
307+
self.taskgroup_has_start_soon.clear()
308+
309+
def checkpoint_schedule_point(self) -> None:
310+
self.uncheckpointed_statements = set()
311+
312+
def checkpoint(self) -> None:
313+
self.checkpoint_schedule_point()
314+
self.checkpoint_cancel_point()
305315

306316
def checkpoint_statement(self) -> cst.SimpleStatementLine:
307317
return checkpoint_statement(self.library[0])
308318

319+
def visit_Call(self, node: cst.Call) -> None:
320+
# [Nursery/TaskGroup].start_soon introduces a cancel point
321+
if (
322+
isinstance(node.func, cst.Attribute)
323+
and isinstance(node.func.value, cst.Name)
324+
and node.func.attr.value == "start_soon"
325+
and node.func.value.value in self.taskgroup_has_start_soon
326+
):
327+
self.taskgroup_has_start_soon[node.func.value.value] = True
328+
309329
def visit_ImportFrom(self, node: cst.ImportFrom) -> None:
310330
# Semi-crude approach to handle `from contextlib import suppress`.
311331
# It does not handle the identifier being overridden, or assigned
@@ -341,16 +361,21 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool:
341361
"safe_decorator",
342362
"async_function",
343363
"uncheckpointed_statements",
364+
# comp_unknown does not need to be saved
344365
"loop_state",
345366
"try_state",
346367
"has_checkpoint_stack",
347-
"suppress_imported_as",
368+
# node_dict is cleaned up and don't need to be saved
369+
"taskgroup_has_start_soon",
370+
"suppress_imported_as", # a copy is saved, but state is not reset
348371
copy=True,
349372
)
350-
self.uncheckpointed_statements = set()
351-
self.has_checkpoint_stack = []
352373
self.has_yield = self.safe_decorator = False
374+
self.uncheckpointed_statements = set()
353375
self.loop_state = LoopState()
376+
# try_state is reset upon entering try
377+
self.has_checkpoint_stack = []
378+
self.taskgroup_has_start_soon = {}
354379

355380
self.async_function = (
356381
node.asynchronous is not None
@@ -440,8 +465,8 @@ def leave_Return(
440465
):
441466
self.add_statement = self.checkpoint_statement()
442467
# avoid duplicate error messages
443-
self.uncheckpointed_statements = set()
444-
# we don't treat it as a checkpoint for ASYNC100
468+
# but don't see it as a cancel point for ASYNC100
469+
self.checkpoint_schedule_point()
445470

446471
# return original node to avoid problems with identity equality
447472
assert original_node.deep_equals(updated_node)
@@ -491,12 +516,47 @@ def _is_exception_suppressing_context_manager(self, node: cst.With) -> bool:
491516
is not None
492517
)
493518

519+
def _checkpoint_with(self, node: cst.With, entry: bool):
520+
"""Conditionally checkpoints entry/exit of With.
521+
522+
If the `with` only contains calls to open_nursery/create_task_group, it's a
523+
schedule point but not a cancellation point, so we treat it as a checkpoint
524+
for async91x but not for async100.
525+
526+
Saves the name of the taskgroup/nursery if entry is set
527+
"""
528+
if not getattr(node, "asynchronous", None):
529+
return
530+
531+
for item in node.items:
532+
if isinstance(item.item, cst.Call) and identifier_to_string(
533+
item.item.func
534+
) in (
535+
"trio.open_nursery",
536+
"anyio.create_task_group",
537+
):
538+
if item.asname is not None and isinstance(item.asname.name, cst.Name):
539+
# save the nursery/taskgroup to see if it has a `.start_soon`
540+
if entry:
541+
self.taskgroup_has_start_soon[item.asname.name.value] = False
542+
elif self.taskgroup_has_start_soon.pop(
543+
item.asname.name.value, False
544+
):
545+
self.checkpoint()
546+
return
547+
else:
548+
self.checkpoint()
549+
break
550+
else:
551+
# only taskgroup/nursery calls
552+
self.checkpoint_schedule_point()
553+
494554
# Async context managers can reasonably checkpoint on either or both of entry and
495555
# exit. Given that we can't tell which, we assume "both" to avoid raising a
496556
# missing-checkpoint warning when there might in fact be one (i.e. a false alarm).
497557
def visit_With_body(self, node: cst.With):
498-
if getattr(node, "asynchronous", None):
499-
self.checkpoint()
558+
self.save_state(node, "taskgroup_has_start_soon", copy=True)
559+
self._checkpoint_with(node, entry=True)
500560

501561
# if this might suppress exceptions, we cannot treat anything inside it as
502562
# checkpointing.
@@ -548,15 +608,16 @@ def leave_With(self, original_node: cst.With, updated_node: cst.With):
548608
for res in self.node_dict[original_node]:
549609
self.error(res.node, error_code="ASYNC912")
550610

611+
self.node_dict.pop(original_node, None)
612+
551613
# if exception-suppressing, restore all uncheckpointed statements from
552614
# before the `with`.
553615
if self._is_exception_suppressing_context_manager(original_node):
554616
prev_checkpoints = self.uncheckpointed_statements
555617
self.restore_state(original_node)
556618
self.uncheckpointed_statements.update(prev_checkpoints)
557619

558-
if getattr(original_node, "asynchronous", None):
559-
self.checkpoint()
620+
self._checkpoint_with(original_node, entry=False)
560621
return updated_node
561622

562623
# error if no checkpoint since earlier yield or function entry
@@ -569,7 +630,7 @@ def leave_Yield(
569630

570631
# Treat as a checkpoint for ASYNC100, since the context we yield to
571632
# may checkpoint.
572-
self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack)
633+
self.checkpoint_cancel_point()
573634

574635
if self.check_function_exit(original_node) and self.should_autofix(
575636
original_node

tests/autofix_files/async100.py

-6
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,6 @@ async def fn(timeout):
132132
await trio.sleep(1)
133133

134134

135-
async def nursery_no_cancel_point():
136-
with trio.CancelScope(): # should error, but reverted PR
137-
async with anyio.create_task_group():
138-
...
139-
140-
141135
async def dont_crash_on_non_name_or_attr_call():
142136
async with contextlib.asynccontextmanager(agen_fn)():
143137
...

tests/autofix_files/async100_anyio.py

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
# AUTOFIX
2+
# BASE_LIBRARY anyio
3+
# NOTRIO # trio.create_task_group doesn't exist
4+
# ASYNCIO_NO_ERROR
5+
import anyio
6+
7+
8+
async def bar() -> None: ...
9+
10+
11+
async def anyio_cancelscope():
12+
# error: 9, "anyio", "CancelScope"
13+
...
14+
15+
16+
# see async100_trio for more comprehensive tests
17+
async def nursery_no_cancel_point():
18+
# error: 9, "anyio", "CancelScope"
19+
async with anyio.create_task_group():
20+
...
21+
22+
23+
async def nursery_with_start_soon():
24+
with anyio.CancelScope():
25+
async with anyio.create_task_group() as tg:
26+
tg.start_soon(bar)
+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
+++
3+
@@ x,15 x,15 @@
4+
5+
6+
async def anyio_cancelscope():
7+
- with anyio.CancelScope(): # error: 9, "anyio", "CancelScope"
8+
- ...
9+
+ # error: 9, "anyio", "CancelScope"
10+
+ ...
11+
12+
13+
# see async100_trio for more comprehensive tests
14+
async def nursery_no_cancel_point():
15+
- with anyio.CancelScope(): # error: 9, "anyio", "CancelScope"
16+
- async with anyio.create_task_group():
17+
- ...
18+
+ # error: 9, "anyio", "CancelScope"
19+
+ async with anyio.create_task_group():
20+
+ ...
21+
22+
23+
async def nursery_with_start_soon():

tests/autofix_files/async100_trio.py

+69-4
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,75 @@
11
# AUTOFIX
22
# ASYNCIO_NO_ERROR # asyncio.open_nursery doesn't exist
3-
# ANYIO_NO_ERROR # anyio.open_nursery doesn't exist
3+
# NOANYIO # anyio.open_nursery doesn't exist
44
import trio
55

66

77
async def nursery_no_cancel_point():
8-
with trio.CancelScope(): # should error, but reverted PR
9-
async with trio.open_nursery():
10-
...
8+
# error: 9, "trio", "CancelScope"
9+
async with trio.open_nursery():
10+
...
11+
12+
13+
# but it is a cancel point if the nursery contains a call to start_soon()
14+
15+
16+
async def nursery_start_soon():
17+
with trio.CancelScope():
18+
async with trio.open_nursery() as n:
19+
n.start_soon(trio.sleep, 0)
20+
21+
22+
async def nursery_start_soon_misnested():
23+
async with trio.open_nursery() as n:
24+
# error: 13, "trio", "CancelScope"
25+
n.start_soon(trio.sleep, 0)
26+
27+
28+
async def nested_scope():
29+
with trio.CancelScope():
30+
with trio.CancelScope():
31+
async with trio.open_nursery() as n:
32+
n.start_soon(trio.sleep, 0)
33+
34+
35+
async def nested_nursery():
36+
with trio.CancelScope():
37+
async with trio.open_nursery() as n:
38+
async with trio.open_nursery() as n2:
39+
n2.start_soon(trio.sleep, 0)
40+
41+
42+
async def nested_function_call():
43+
44+
# error: 9, "trio", "CancelScope"
45+
async with trio.open_nursery() as n:
46+
47+
def foo():
48+
n.start_soon(trio.sleep, 0)
49+
50+
# a false alarm in case we call foo()... but we can't check if they do
51+
foo()
52+
53+
54+
# insert cancel point on nursery exit, not at the start_soon call
55+
async def cancel_point_on_nursery_exit():
56+
with trio.CancelScope():
57+
async with trio.open_nursery() as n:
58+
# error: 17, "trio", "CancelScope"
59+
n.start_soon(trio.sleep, 0)
60+
61+
62+
# async100 does not consider *redundant* cancel scopes
63+
async def redundant_cancel_scope():
64+
with trio.CancelScope():
65+
with trio.CancelScope():
66+
await trio.lowlevel.checkpoint()
67+
68+
69+
# but if it did then none of these scopes should be marked redundant
70+
# The inner checks task startup, the outer checks task exit
71+
async def nursery_exit_blocks_with_start():
72+
with trio.CancelScope():
73+
async with trio.open_nursery() as n:
74+
with trio.CancelScope():
75+
await n.start(trio.sleep, 0)

0 commit comments

Comments
 (0)