start_soon now makes open_nursery a cancel point on exit

docs/changelog.rst

@@ -6,7 +6,7 @@ Changelog
 
 24.11.4
 =======
-- :ref:`ASYNC100 <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 <cancel_point>`.
+- :ref:`ASYNC100 <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
 =======

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 <schedule_point>` but not :ref:`cancel points <cancel_point>`.
+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>`.
 
 asyncio does not place any guarantees on if or when asyncio functions will
 checkpoint. This means that enabling and adhering to :ref:`ASYNC91x <ASYNC910>`

docs/rules.rst

@@ -13,7 +13,7 @@ _`ASYNC100` : cancel-scope-no-checkpoint
     A :ref:`timeout_context` does not contain any :ref:`checkpoints <checkpoint>`.
     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 <schedule_point>` but not :ref:`cancel points <cancel_point>` (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 <schedule_point>` but not :ref:`cancel points <cancel_point>` (unless they have tasks started in them).
     See :ref:`ASYNC912 <async912>` which will in addition guarantee checkpoints on every code path.
 
 _`ASYNC101` : yield-in-cancel-scope

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

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)

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

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)