From b6b9d67fe0d9d44cccc67dd6eb2fddd9c05aa041 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 19 Nov 2024 15:35:51 +0100 Subject: [PATCH 1/3] ASYNC100 now treats start_soon() as a cancel point --- docs/changelog.rst | 6 +- docs/rules.rst | 1 + flake8_async/__init__.py | 2 +- flake8_async/visitors/flake8asyncvisitor.py | 4 +- flake8_async/visitors/visitor91x.py | 63 +++++++++++++++++++-- tests/autofix_files/async100.py | 6 -- tests/autofix_files/async100_anyio.py | 26 +++++++++ tests/autofix_files/async100_anyio.py.diff | 23 ++++++++ tests/autofix_files/async100_trio.py | 49 ++++++++++++++-- tests/autofix_files/async100_trio.py.diff | 33 +++++++++++ tests/eval_files/async100.py | 6 -- tests/eval_files/async100_anyio.py | 26 +++++++++ tests/eval_files/async100_trio.py | 45 ++++++++++++++- 13 files changed, 261 insertions(+), 29 deletions(-) create mode 100644 tests/autofix_files/async100_anyio.py create mode 100644 tests/autofix_files/async100_anyio.py.diff create mode 100644 tests/eval_files/async100_anyio.py diff --git a/docs/changelog.rst b/docs/changelog.rst index c420d15..a2121f7 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,9 +4,13 @@ Changelog `CalVer, YY.month.patch `_ +24.11.4 +======= +- :ref:`ASYNC100 ` once again ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`, but now treats calls to ``.start_soon()`` as introducing a :ref:`cancel point `. + 24.11.3 ======= -- Revert :ref:`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 `. +- Revert :ref:`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 `. 24.11.2 ======= diff --git a/docs/rules.rst b/docs/rules.rst index 41668c1..c5163ca 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -13,6 +13,7 @@ _`ASYNC100` : cancel-scope-no-checkpoint A :ref:`timeout_context` does not contain any :ref:`checkpoints `. This makes it pointless, as the timeout can only be triggered by a checkpoint. This check also treats ``yield`` as a checkpoint, since checkpoints can happen in the caller we yield to. + :func:`trio.open_nursery` and :func:`anyio.create_task_group` are excluded, as they are :ref:`schedule points ` but not :ref:`cancel points ` (unless they contain calls to :meth:`trio.Nursery.start_soon`/:meth:`anyio.abc.TaskGroup.start_soon`). See :ref:`ASYNC912 ` which will in addition guarantee checkpoints on every code path. _`ASYNC101` : yield-in-cancel-scope diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 2710c5e..2e11fe0 100644 --- a/flake8_async/__init__.py +++ b/flake8_async/__init__.py @@ -38,7 +38,7 @@ # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" -__version__ = "24.11.3" +__version__ = "24.11.4" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/flake8asyncvisitor.py b/flake8_async/visitors/flake8asyncvisitor.py index 7bce4a9..9e95992 100644 --- a/flake8_async/visitors/flake8asyncvisitor.py +++ b/flake8_async/visitors/flake8asyncvisitor.py @@ -134,9 +134,7 @@ def set_state(self, attrs: dict[str, Any], copy: bool = False): def save_state(self, node: ast.AST, *attrs: str, copy: bool = False): state = self.get_state(*attrs, copy=copy) if node in self.outer: - # not currently used, and not gonna bother adding dedicated test - # visitors atm - self.outer[node].update(state) # pragma: no cover + self.outer[node].update(state) else: self.outer[node] = state diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 539fcee..5a44993 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -25,6 +25,7 @@ flatten_preserving_comments, fnmatch_qualified_name_cst, func_has_decorator, + identifier_to_string, iter_guaranteed_once_cst, with_has_call, ) @@ -285,6 +286,7 @@ def __init__(self, *args: Any, **kwargs: Any): # ASYNC100 self.has_checkpoint_stack: list[bool] = [] self.node_dict: dict[cst.With, list[AttributeCall]] = {} + self.taskgroups: list[str] = [] # --exception-suppress-context-manager self.suppress_imported_as: list[str] = [] @@ -299,13 +301,28 @@ def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool: and self.library != ("asyncio",) ) + def checkpoint_cancel_point(self) -> None: + self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack) + # don't need to look for any .start_soon() calls + self.taskgroups.clear() + def checkpoint(self) -> None: self.uncheckpointed_statements = set() - self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack) + self.checkpoint_cancel_point() def checkpoint_statement(self) -> cst.SimpleStatementLine: return checkpoint_statement(self.library[0]) + def visit_Call(self, node: cst.Call) -> None: + # [Nursery/TaskGroup].start_soon introduces a cancel point + if ( + isinstance(node.func, cst.Attribute) + and isinstance(node.func.value, cst.Name) + and node.func.attr.value == "start_soon" + and node.func.value.value in self.taskgroups + ): + self.checkpoint_cancel_point() + def visit_ImportFrom(self, node: cst.ImportFrom) -> None: # Semi-crude approach to handle `from contextlib import suppress`. # It does not handle the identifier being overridden, or assigned @@ -341,9 +358,12 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: "safe_decorator", "async_function", "uncheckpointed_statements", + # comp_unknown does not need to be saved "loop_state", "try_state", "has_checkpoint_stack", + # node_dict is cleaned up and don't need to be saved + "taskgroups", "suppress_imported_as", copy=True, ) @@ -491,12 +511,42 @@ def _is_exception_suppressing_context_manager(self, node: cst.With) -> bool: is not None ) + def _checkpoint_with(self, node: cst.With, entry: bool): + """Conditionally checkpoints entry/exit of With. + + If the `with` only contains calls to open_nursery/create_task_group, it's a + schedule point but not a cancellation point, so we treat it as a checkpoint + for async91x but not for async100. + + Saves the name of the taskgroup/nursery if entry is set + """ + if getattr(node, "asynchronous", None): + for item in node.items: + if isinstance(item.item, cst.Call) and identifier_to_string( + item.item.func + ) in ( + "trio.open_nursery", + "anyio.create_task_group", + ): + # save the nursery/taskgroup to see if it has a `.start_soon` + if ( + entry + and item.asname is not None + and isinstance(item.asname.name, cst.Name) + ): + self.taskgroups.append(item.asname.name.value) + else: + self.checkpoint() + break + else: + self.uncheckpointed_statements = set() + # Async context managers can reasonably checkpoint on either or both of entry and # exit. Given that we can't tell which, we assume "both" to avoid raising a # missing-checkpoint warning when there might in fact be one (i.e. a false alarm). def visit_With_body(self, node: cst.With): - if getattr(node, "asynchronous", None): - self.checkpoint() + self.save_state(node, "taskgroups", copy=True) + self._checkpoint_with(node, entry=True) # if this might suppress exceptions, we cannot treat anything inside it as # checkpointing. @@ -548,6 +598,8 @@ def leave_With(self, original_node: cst.With, updated_node: cst.With): for res in self.node_dict[original_node]: self.error(res.node, error_code="ASYNC912") + self.node_dict.pop(original_node, None) + # if exception-suppressing, restore all uncheckpointed statements from # before the `with`. if self._is_exception_suppressing_context_manager(original_node): @@ -555,8 +607,7 @@ def leave_With(self, original_node: cst.With, updated_node: cst.With): self.restore_state(original_node) self.uncheckpointed_statements.update(prev_checkpoints) - if getattr(original_node, "asynchronous", None): - self.checkpoint() + self._checkpoint_with(original_node, entry=False) return updated_node # error if no checkpoint since earlier yield or function entry @@ -569,7 +620,7 @@ def leave_Yield( # Treat as a checkpoint for ASYNC100, since the context we yield to # may checkpoint. - self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack) + self.checkpoint_cancel_point() if self.check_function_exit(original_node) and self.should_autofix( original_node diff --git a/tests/autofix_files/async100.py b/tests/autofix_files/async100.py index 609f034..bfcad5f 100644 --- a/tests/autofix_files/async100.py +++ b/tests/autofix_files/async100.py @@ -132,12 +132,6 @@ async def fn(timeout): await trio.sleep(1) -async def nursery_no_cancel_point(): - with trio.CancelScope(): # should error, but reverted PR - async with anyio.create_task_group(): - ... - - async def dont_crash_on_non_name_or_attr_call(): async with contextlib.asynccontextmanager(agen_fn)(): ... diff --git a/tests/autofix_files/async100_anyio.py b/tests/autofix_files/async100_anyio.py new file mode 100644 index 0000000..80516cf --- /dev/null +++ b/tests/autofix_files/async100_anyio.py @@ -0,0 +1,26 @@ +# AUTOFIX +# BASE_LIBRARY anyio +# NOTRIO # trio.create_task_group doesn't exist +# ASYNCIO_NO_ERROR +import anyio + + +async def bar() -> None: ... + + +async def anyio_cancelscope(): + # error: 9, "anyio", "CancelScope" + ... + + +# see async100_trio for more comprehensive tests +async def nursery_no_cancel_point(): + # error: 9, "anyio", "CancelScope" + async with anyio.create_task_group(): + ... + + +async def nursery_with_start_soon(): + with anyio.CancelScope(): + async with anyio.create_task_group() as tg: + tg.start_soon(bar) diff --git a/tests/autofix_files/async100_anyio.py.diff b/tests/autofix_files/async100_anyio.py.diff new file mode 100644 index 0000000..3e57cd9 --- /dev/null +++ b/tests/autofix_files/async100_anyio.py.diff @@ -0,0 +1,23 @@ +--- ++++ +@@ x,15 x,15 @@ + + + async def anyio_cancelscope(): +- with anyio.CancelScope(): # error: 9, "anyio", "CancelScope" +- ... ++ # error: 9, "anyio", "CancelScope" ++ ... + + + # see async100_trio for more comprehensive tests + async def nursery_no_cancel_point(): +- with anyio.CancelScope(): # error: 9, "anyio", "CancelScope" +- async with anyio.create_task_group(): +- ... ++ # error: 9, "anyio", "CancelScope" ++ async with anyio.create_task_group(): ++ ... + + + async def nursery_with_start_soon(): diff --git a/tests/autofix_files/async100_trio.py b/tests/autofix_files/async100_trio.py index 2595814..3562da0 100644 --- a/tests/autofix_files/async100_trio.py +++ b/tests/autofix_files/async100_trio.py @@ -1,10 +1,51 @@ # AUTOFIX # ASYNCIO_NO_ERROR # asyncio.open_nursery doesn't exist -# ANYIO_NO_ERROR # anyio.open_nursery doesn't exist +# NOANYIO # anyio.open_nursery doesn't exist import trio async def nursery_no_cancel_point(): - with trio.CancelScope(): # should error, but reverted PR - async with trio.open_nursery(): - ... + # error: 9, "trio", "CancelScope" + async with trio.open_nursery(): + ... + + +# but it is a cancel point if the nursery contains a call to start_soon() + + +async def nursery_start_soon(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + n.start_soon(trio.sleep, 0) + + +async def nursery_start_soon_misnested(): + async with trio.open_nursery() as n: + with trio.CancelScope(): + n.start_soon(trio.sleep, 0) + + +async def nested_scope(): + with trio.CancelScope(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + n.start_soon(trio.sleep, 0) + + +async def nested_nursery(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + async with trio.open_nursery() as n2: + n2.start_soon(trio.sleep, 0) + + +async def nested_function_call(): + + # error: 9, "trio", "CancelScope" + async with trio.open_nursery() as n: + + def foo(): + n.start_soon(trio.sleep, 0) + + # a false alarm in case we call foo()... but we can't check if they do + foo() diff --git a/tests/autofix_files/async100_trio.py.diff b/tests/autofix_files/async100_trio.py.diff index e69de29..f8ce6e5 100644 --- a/tests/autofix_files/async100_trio.py.diff +++ b/tests/autofix_files/async100_trio.py.diff @@ -0,0 +1,33 @@ +--- ++++ +@@ x,9 x,9 @@ + + + async def nursery_no_cancel_point(): +- with trio.CancelScope(): # error: 9, "trio", "CancelScope" +- async with trio.open_nursery(): +- ... ++ # error: 9, "trio", "CancelScope" ++ async with trio.open_nursery(): ++ ... + + + # but it is a cancel point if the nursery contains a call to start_soon() +@@ x,11 x,11 @@ + + async def nested_function_call(): + +- with trio.CancelScope(): # error: 9, "trio", "CancelScope" +- async with trio.open_nursery() as n: ++ # error: 9, "trio", "CancelScope" ++ async with trio.open_nursery() as n: + +- def foo(): +- n.start_soon(trio.sleep, 0) ++ def foo(): ++ n.start_soon(trio.sleep, 0) + +- # a false alarm in case we call foo()... but we can't check if they do +- foo() ++ # a false alarm in case we call foo()... but we can't check if they do ++ foo() diff --git a/tests/eval_files/async100.py b/tests/eval_files/async100.py index b7857a5..2c6cf47 100644 --- a/tests/eval_files/async100.py +++ b/tests/eval_files/async100.py @@ -132,12 +132,6 @@ async def fn(timeout): await trio.sleep(1) -async def nursery_no_cancel_point(): - with trio.CancelScope(): # should error, but reverted PR - async with anyio.create_task_group(): - ... - - async def dont_crash_on_non_name_or_attr_call(): async with contextlib.asynccontextmanager(agen_fn)(): ... diff --git a/tests/eval_files/async100_anyio.py b/tests/eval_files/async100_anyio.py new file mode 100644 index 0000000..f274969 --- /dev/null +++ b/tests/eval_files/async100_anyio.py @@ -0,0 +1,26 @@ +# AUTOFIX +# BASE_LIBRARY anyio +# NOTRIO # trio.create_task_group doesn't exist +# ASYNCIO_NO_ERROR +import anyio + + +async def bar() -> None: ... + + +async def anyio_cancelscope(): + with anyio.CancelScope(): # error: 9, "anyio", "CancelScope" + ... + + +# see async100_trio for more comprehensive tests +async def nursery_no_cancel_point(): + with anyio.CancelScope(): # error: 9, "anyio", "CancelScope" + async with anyio.create_task_group(): + ... + + +async def nursery_with_start_soon(): + with anyio.CancelScope(): + async with anyio.create_task_group() as tg: + tg.start_soon(bar) diff --git a/tests/eval_files/async100_trio.py b/tests/eval_files/async100_trio.py index 2595814..d14c8d3 100644 --- a/tests/eval_files/async100_trio.py +++ b/tests/eval_files/async100_trio.py @@ -1,10 +1,51 @@ # AUTOFIX # ASYNCIO_NO_ERROR # asyncio.open_nursery doesn't exist -# ANYIO_NO_ERROR # anyio.open_nursery doesn't exist +# NOANYIO # anyio.open_nursery doesn't exist import trio async def nursery_no_cancel_point(): - with trio.CancelScope(): # should error, but reverted PR + with trio.CancelScope(): # error: 9, "trio", "CancelScope" async with trio.open_nursery(): ... + + +# but it is a cancel point if the nursery contains a call to start_soon() + + +async def nursery_start_soon(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + n.start_soon(trio.sleep, 0) + + +async def nursery_start_soon_misnested(): + async with trio.open_nursery() as n: + with trio.CancelScope(): + n.start_soon(trio.sleep, 0) + + +async def nested_scope(): + with trio.CancelScope(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + n.start_soon(trio.sleep, 0) + + +async def nested_nursery(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + async with trio.open_nursery() as n2: + n2.start_soon(trio.sleep, 0) + + +async def nested_function_call(): + + with trio.CancelScope(): # error: 9, "trio", "CancelScope" + async with trio.open_nursery() as n: + + def foo(): + n.start_soon(trio.sleep, 0) + + # a false alarm in case we call foo()... but we can't check if they do + foo() From f38fbf54606fb00522b82c5f1eb1a045ecfc20bb Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 19 Nov 2024 16:11:36 +0100 Subject: [PATCH 2/3] oh derp, visitor91x is a CST visitor --- flake8_async/visitors/flake8asyncvisitor.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/flake8_async/visitors/flake8asyncvisitor.py b/flake8_async/visitors/flake8asyncvisitor.py index 9e95992..7bce4a9 100644 --- a/flake8_async/visitors/flake8asyncvisitor.py +++ b/flake8_async/visitors/flake8asyncvisitor.py @@ -134,7 +134,9 @@ def set_state(self, attrs: dict[str, Any], copy: bool = False): def save_state(self, node: ast.AST, *attrs: str, copy: bool = False): state = self.get_state(*attrs, copy=copy) if node in self.outer: - self.outer[node].update(state) + # not currently used, and not gonna bother adding dedicated test + # visitors atm + self.outer[node].update(state) # pragma: no cover else: self.outer[node] = state From fc27e2c246ea47681f42f8a664a4e3a9e63ce36f Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 22 Nov 2024 13:20:13 +0100 Subject: [PATCH 3/3] start_soon now makes open_nursery a cancel point on exit --- docs/changelog.rst | 2 +- docs/glossary.rst | 2 +- docs/rules.rst | 2 +- flake8_async/visitors/visitor91x.py | 68 +++++++++++++---------- tests/autofix_files/async100_trio.py | 28 +++++++++- tests/autofix_files/async100_trio.py.diff | 26 ++++++++- tests/eval_files/async100_trio.py | 26 ++++++++- 7 files changed, 118 insertions(+), 36 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index a2121f7..ec35cce 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,7 +6,7 @@ Changelog 24.11.4 ======= -- :ref:`ASYNC100 ` once again ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`, but now treats calls to ``.start_soon()`` as introducing a :ref:`cancel point `. +- :ref:`ASYNC100 ` once again ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`, unless we find a call to ``.start_soon()``. 24.11.3 ======= diff --git a/docs/glossary.rst b/docs/glossary.rst index 2868b95..65544cd 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -99,7 +99,7 @@ functions defined by Trio will either checkpoint or raise an exception when iteration, and when exhausting the iterator, and ``async with`` will checkpoint on at least one of enter/exit. -The one exception is :func:`trio.open_nursery` and :func:`anyio.create_task_group` which are :ref:`schedule points ` but not :ref:`cancel points `. +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 `. 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 `. asyncio does not place any guarantees on if or when asyncio functions will checkpoint. This means that enabling and adhering to :ref:`ASYNC91x ` diff --git a/docs/rules.rst b/docs/rules.rst index c5163ca..648fc24 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -13,7 +13,7 @@ _`ASYNC100` : cancel-scope-no-checkpoint A :ref:`timeout_context` does not contain any :ref:`checkpoints `. This makes it pointless, as the timeout can only be triggered by a checkpoint. This check also treats ``yield`` as a checkpoint, since checkpoints can happen in the caller we yield to. - :func:`trio.open_nursery` and :func:`anyio.create_task_group` are excluded, as they are :ref:`schedule points ` but not :ref:`cancel points ` (unless they contain calls to :meth:`trio.Nursery.start_soon`/:meth:`anyio.abc.TaskGroup.start_soon`). + :func:`trio.open_nursery` and :func:`anyio.create_task_group` are excluded, as they are :ref:`schedule points ` but not :ref:`cancel points ` (unless they have tasks started in them). See :ref:`ASYNC912 ` which will in addition guarantee checkpoints on every code path. _`ASYNC101` : yield-in-cancel-scope diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 5a44993..ec65439 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -286,7 +286,7 @@ def __init__(self, *args: Any, **kwargs: Any): # ASYNC100 self.has_checkpoint_stack: list[bool] = [] self.node_dict: dict[cst.With, list[AttributeCall]] = {} - self.taskgroups: list[str] = [] + self.taskgroup_has_start_soon: dict[str, bool] = {} # --exception-suppress-context-manager self.suppress_imported_as: list[str] = [] @@ -304,10 +304,13 @@ def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool: def checkpoint_cancel_point(self) -> None: self.has_checkpoint_stack = [True] * len(self.has_checkpoint_stack) # don't need to look for any .start_soon() calls - self.taskgroups.clear() + self.taskgroup_has_start_soon.clear() - def checkpoint(self) -> None: + def checkpoint_schedule_point(self) -> None: self.uncheckpointed_statements = set() + + def checkpoint(self) -> None: + self.checkpoint_schedule_point() self.checkpoint_cancel_point() def checkpoint_statement(self) -> cst.SimpleStatementLine: @@ -319,9 +322,9 @@ def visit_Call(self, node: cst.Call) -> None: isinstance(node.func, cst.Attribute) and isinstance(node.func.value, cst.Name) and node.func.attr.value == "start_soon" - and node.func.value.value in self.taskgroups + and node.func.value.value in self.taskgroup_has_start_soon ): - self.checkpoint_cancel_point() + self.taskgroup_has_start_soon[node.func.value.value] = True def visit_ImportFrom(self, node: cst.ImportFrom) -> None: # Semi-crude approach to handle `from contextlib import suppress`. @@ -363,14 +366,16 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: "try_state", "has_checkpoint_stack", # node_dict is cleaned up and don't need to be saved - "taskgroups", - "suppress_imported_as", + "taskgroup_has_start_soon", + "suppress_imported_as", # a copy is saved, but state is not reset copy=True, ) - self.uncheckpointed_statements = set() - self.has_checkpoint_stack = [] self.has_yield = self.safe_decorator = False + self.uncheckpointed_statements = set() self.loop_state = LoopState() + # try_state is reset upon entering try + self.has_checkpoint_stack = [] + self.taskgroup_has_start_soon = {} self.async_function = ( node.asynchronous is not None @@ -460,8 +465,8 @@ def leave_Return( ): self.add_statement = self.checkpoint_statement() # avoid duplicate error messages - self.uncheckpointed_statements = set() - # we don't treat it as a checkpoint for ASYNC100 + # but don't see it as a cancel point for ASYNC100 + self.checkpoint_schedule_point() # return original node to avoid problems with identity equality assert original_node.deep_equals(updated_node) @@ -520,32 +525,37 @@ def _checkpoint_with(self, node: cst.With, entry: bool): Saves the name of the taskgroup/nursery if entry is set """ - if getattr(node, "asynchronous", None): - for item in node.items: - if isinstance(item.item, cst.Call) and identifier_to_string( - item.item.func - ) in ( - "trio.open_nursery", - "anyio.create_task_group", - ): + if not getattr(node, "asynchronous", None): + return + + for item in node.items: + if isinstance(item.item, cst.Call) and identifier_to_string( + item.item.func + ) in ( + "trio.open_nursery", + "anyio.create_task_group", + ): + if item.asname is not None and isinstance(item.asname.name, cst.Name): # save the nursery/taskgroup to see if it has a `.start_soon` - if ( - entry - and item.asname is not None - and isinstance(item.asname.name, cst.Name) + if entry: + self.taskgroup_has_start_soon[item.asname.name.value] = False + elif self.taskgroup_has_start_soon.pop( + item.asname.name.value, False ): - self.taskgroups.append(item.asname.name.value) - else: - self.checkpoint() - break + self.checkpoint() + return else: - self.uncheckpointed_statements = set() + self.checkpoint() + break + else: + # only taskgroup/nursery calls + self.checkpoint_schedule_point() # Async context managers can reasonably checkpoint on either or both of entry and # exit. Given that we can't tell which, we assume "both" to avoid raising a # missing-checkpoint warning when there might in fact be one (i.e. a false alarm). def visit_With_body(self, node: cst.With): - self.save_state(node, "taskgroups", copy=True) + self.save_state(node, "taskgroup_has_start_soon", copy=True) self._checkpoint_with(node, entry=True) # if this might suppress exceptions, we cannot treat anything inside it as diff --git a/tests/autofix_files/async100_trio.py b/tests/autofix_files/async100_trio.py index 3562da0..ed61139 100644 --- a/tests/autofix_files/async100_trio.py +++ b/tests/autofix_files/async100_trio.py @@ -21,8 +21,8 @@ async def nursery_start_soon(): async def nursery_start_soon_misnested(): async with trio.open_nursery() as n: - with trio.CancelScope(): - n.start_soon(trio.sleep, 0) + # error: 13, "trio", "CancelScope" + n.start_soon(trio.sleep, 0) async def nested_scope(): @@ -49,3 +49,27 @@ def foo(): # a false alarm in case we call foo()... but we can't check if they do foo() + + +# insert cancel point on nursery exit, not at the start_soon call +async def cancel_point_on_nursery_exit(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + # error: 17, "trio", "CancelScope" + n.start_soon(trio.sleep, 0) + + +# async100 does not consider *redundant* cancel scopes +async def redundant_cancel_scope(): + with trio.CancelScope(): + with trio.CancelScope(): + await trio.lowlevel.checkpoint() + + +# but if it did then none of these scopes should be marked redundant +# The inner checks task startup, the outer checks task exit +async def nursery_exit_blocks_with_start(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + with trio.CancelScope(): + await n.start(trio.sleep, 0) diff --git a/tests/autofix_files/async100_trio.py.diff b/tests/autofix_files/async100_trio.py.diff index f8ce6e5..e9ff0d1 100644 --- a/tests/autofix_files/async100_trio.py.diff +++ b/tests/autofix_files/async100_trio.py.diff @@ -13,7 +13,18 @@ # but it is a cancel point if the nursery contains a call to start_soon() -@@ x,11 x,11 @@ +@@ x,8 x,8 @@ + + async def nursery_start_soon_misnested(): + async with trio.open_nursery() as n: +- with trio.CancelScope(): # error: 13, "trio", "CancelScope" +- n.start_soon(trio.sleep, 0) ++ # error: 13, "trio", "CancelScope" ++ n.start_soon(trio.sleep, 0) + + + async def nested_scope(): +@@ x,22 x,22 @@ async def nested_function_call(): @@ -31,3 +42,16 @@ - foo() + # a false alarm in case we call foo()... but we can't check if they do + foo() + + + # insert cancel point on nursery exit, not at the start_soon call + async def cancel_point_on_nursery_exit(): + with trio.CancelScope(): + async with trio.open_nursery() as n: +- with trio.CancelScope(): # error: 17, "trio", "CancelScope" +- n.start_soon(trio.sleep, 0) ++ # error: 17, "trio", "CancelScope" ++ n.start_soon(trio.sleep, 0) + + + # async100 does not consider *redundant* cancel scopes diff --git a/tests/eval_files/async100_trio.py b/tests/eval_files/async100_trio.py index d14c8d3..d722185 100644 --- a/tests/eval_files/async100_trio.py +++ b/tests/eval_files/async100_trio.py @@ -21,7 +21,7 @@ async def nursery_start_soon(): async def nursery_start_soon_misnested(): async with trio.open_nursery() as n: - with trio.CancelScope(): + with trio.CancelScope(): # error: 13, "trio", "CancelScope" n.start_soon(trio.sleep, 0) @@ -49,3 +49,27 @@ def foo(): # a false alarm in case we call foo()... but we can't check if they do foo() + + +# insert cancel point on nursery exit, not at the start_soon call +async def cancel_point_on_nursery_exit(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + with trio.CancelScope(): # error: 17, "trio", "CancelScope" + n.start_soon(trio.sleep, 0) + + +# async100 does not consider *redundant* cancel scopes +async def redundant_cancel_scope(): + with trio.CancelScope(): + with trio.CancelScope(): + await trio.lowlevel.checkpoint() + + +# but if it did then none of these scopes should be marked redundant +# The inner checks task startup, the outer checks task exit +async def nursery_exit_blocks_with_start(): + with trio.CancelScope(): + async with trio.open_nursery() as n: + with trio.CancelScope(): + await n.start(trio.sleep, 0)