async100 now ignores trio.open_nursery and anyio.create_task_group (#317)

* async100 now ignores trio.open_nursery and anyio.create_task_group

* don't crash on weird 'with' call

* add more documentation. fix bad formatting for 24.9.3 in changelog

* tweak docs

* fix cross-refs

---------

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

Author: jakkdl

docs/changelog.rst

@@ -4,6 +4,12 @@ Changelog
 
 `CalVer, YY.month.patch <https://calver.org/>`_
 
+24.11.1
+=======
+- :ref:`ASYNC100 <async100>` now ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`
+  as cancellation sources, because they are :ref:`schedule points <schedule_points>` but not
+  :ref:`cancellation points <cancel_points>`.
+
 24.10.2
 =======
 - :ref:`ASYNC101 <async101>` and :ref:`ASYNC119 <async119>` are now silenced for decorators in :ref:`transform-async-generator-decorators`
@@ -24,6 +30,7 @@ Changelog
 24.9.3
 ======
 - :ref:`ASYNC102 <async102>` and :ref:`ASYNC120 <async120>`:
+
   - handles nested cancel scopes
   - detects internal cancel scopes of nurseries as a way to shield&deadline
   - no longer treats :func:`trio.open_nursery` or :func:`anyio.create_task_group` as cancellation sources

docs/glossary.rst

@@ -88,8 +88,8 @@ Exception classes:
 
 Checkpoint
 ----------
-Checkpoints are points where the async backend checks for cancellation and
-can switch which task is running, in an ``await``, ``async for``, or ``async with``
+Checkpoints are points where the async backend checks for :ref:`cancellation <cancel_point>` and
+:ref:`can switch which task is running <schedule_point>`, in an ``await``, ``async for``, or ``async with``
 expression.  Regular checkpoints can be important for both performance and correctness.
 
 Trio has extensive and detailed documentation on the concept of
@@ -99,11 +99,11 @@ 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`.
+
 asyncio does not place any guarantees on if or when asyncio functions will
 checkpoint. This means that enabling and adhering to :ref:`ASYNC91x <ASYNC910>`
-will still not guarantee checkpoints.
-
-For anyio it will depend on the current backend.
+will still not guarantee checkpoints on asyncio (even if used via anyio).
 
 When using Trio (or an AnyIO library that people might use on Trio), it can be
 very helpful to ensure that your own code adheres to the same guarantees as
@@ -116,6 +116,35 @@ To insert a checkpoint with no other side effects, you can use
 :func:`trio.lowlevel.checkpoint`/:func:`anyio.lowlevel.checkpoint`/:func:`asyncio.sleep(0)
 <asyncio.sleep>`
 
+.. _schedule_point:
+.. _schedule_points:
+
+Schedule Point
+--------------
+A schedule point is half of a full :ref:`checkpoint`, which allows the async backend to switch the running task, but doesn't check for cancellation (the other half is a :ref:`cancel_point`).
+While you are unlikely to need one, they are available as :func:`trio.lowlevel.cancel_shielded_checkpoint`/:func:`anyio.lowlevel.cancel_shielded_checkpoint`, and equivalent to
+
+.. code-block:: python
+
+   from trio import CancelScope, lowlevel
+   # or
+   # from anyio import CancelScope, lowlevel
+
+   with CancelScope(shield=True):
+       await lowlevel.checkpoint()
+
+asyncio does not have any direct equivalents due to their cancellation model being different.
+
+
+.. _cancel_point:
+.. _cancel_points:
+
+Cancel Point
+------------
+A schedule point is half of a full :ref:`checkpoint`, which will raise :ref:`cancelled` if the enclosing cancel scope has been cancelled, but does not allow the scheduler to switch to a different task (the other half is a :ref:`schedule_point`).
+While you are unlikely to need one, they are available as :func:`trio.lowlevel.checkpoint_if_cancelled`/:func:`anyio.lowlevel.checkpoint_if_cancelled`.
+Users of asyncio might want to use :meth:`asyncio.Task.cancelled`.
+
 .. _channel_stream_queue:
 
 Channel / Stream / Queue

docs/rules.rst

@@ -13,6 +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` but not :ref:`cancel_points`.
     See :ref:`ASYNC912 <async912>` which will in addition guarantee checkpoints on every code path.
 
 _`ASYNC101` : yield-in-cancel-scope

flake8_async/__init__.py

@@ -38,7 +38,7 @@
 
 
 # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
-__version__ = "24.10.2"
+__version__ = "24.11.1"
 
 
 # taken from https://github.com/Zac-HD/shed

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,
 )
@@ -491,12 +492,34 @@ def _is_exception_suppressing_context_manager(self, node: cst.With) -> bool:
             is not None
         )
 
+    def _checkpoint_with(self, node: cst.With):
+        """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.
+        """
+        if getattr(node, "asynchronous", None):
+            for item in node.items:
+                if not isinstance(item.item, cst.Call) or not isinstance(
+                    item.item.func, (cst.Attribute, cst.Name)
+                ):
+                    self.checkpoint()
+                    break
+
+                func = identifier_to_string(item.item.func)
+                assert func is not None
+                if func not in ("trio.open_nursery", "anyio.create_task_group"):
+                    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._checkpoint_with(node)
 
         # if this might suppress exceptions, we cannot treat anything inside it as
         # checkpointing.
@@ -555,8 +578,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)
         return updated_node
 
     # error if no checkpoint since earlier yield or function entry

tests/autofix_files/async100.py

@@ -130,3 +130,14 @@ async def fn(timeout):
             if condition():
                 return
             await trio.sleep(1)
+
+
+async def nursery_no_cancel_point():
+    # error: 9, "trio", "CancelScope"
+    async with anyio.create_task_group():
+        ...
+
+
+async def dont_crash_on_non_name_or_attr_call():
+    async with contextlib.asynccontextmanager(agen_fn)():
+        ...

tests/autofix_files/async100.py.diff

@@ -130,3 +130,16 @@
 
      with contextlib.suppress(Exception):
          with open("blah") as file:
+@@ x,9 x,9 @@
+
+
+ async def nursery_no_cancel_point():
+-    with trio.CancelScope():  # error: 9, "trio", "CancelScope"
+-        async with anyio.create_task_group():
+-            ...
++    # error: 9, "trio", "CancelScope"
++    async with anyio.create_task_group():
++        ...
+
+
+ async def dont_crash_on_non_name_or_attr_call():

tests/autofix_files/async100_trio.py

@@ -0,0 +1,10 @@
+# AUTOFIX
+# ASYNCIO_NO_ERROR # asyncio.open_nursery doesn't exist
+# ANYIO_NO_ERROR # anyio.open_nursery doesn't exist
+import trio
+
+
+async def nursery_no_cancel_point():
+    # error: 9, "trio", "CancelScope"
+    async with trio.open_nursery():
+        ...

tests/autofix_files/async100_trio.py.diff

@@ -0,0 +1,12 @@
+---
++++
+@@ x,6 x,6 @@
+
+
+ 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():
++        ...

tests/eval_files/async100.py

@@ -130,3 +130,14 @@ async def fn(timeout):
             if condition():
                 return
             await trio.sleep(1)
+
+
+async def nursery_no_cancel_point():
+    with trio.CancelScope():  # error: 9, "trio", "CancelScope"
+        async with anyio.create_task_group():
+            ...
+
+
+async def dont_crash_on_non_name_or_attr_call():
+    async with contextlib.asynccontextmanager(agen_fn)():
+        ...