From e52024583017b1b2e399681f8869855bcae5d4ee Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 11 Sep 2024 15:33:05 +0200 Subject: [PATCH 1/5] async100 now ignores trio.open_nursery and anyio.create_task_group --- docs/changelog.rst | 4 ++++ flake8_async/__init__.py | 2 +- flake8_async/visitors/visitor91x.py | 28 +++++++++++++++++++---- tests/autofix_files/async100.py | 6 +++++ tests/autofix_files/async100.py.diff | 10 ++++++++ tests/autofix_files/async100_trio.py | 10 ++++++++ tests/autofix_files/async100_trio.py.diff | 12 ++++++++++ tests/eval_files/async100.py | 6 +++++ tests/eval_files/async100_trio.py | 10 ++++++++ 9 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 tests/autofix_files/async100_trio.py create mode 100644 tests/autofix_files/async100_trio.py.diff create mode 100644 tests/eval_files/async100_trio.py diff --git a/docs/changelog.rst b/docs/changelog.rst index 2672b087..2480b3f1 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,10 @@ Changelog `CalVer, YY.month.patch `_ +24.11.1 +====== +- :ref:`ASYNC100 ` now ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group` as cancellation sources. + 24.10.2 ======= - :ref:`ASYNC102 ` now also warns about ``await()`` inside ``__aexit__``. diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 83c5d570..bbe6c353 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.10.2" +__version__ = "24.11.1" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 539fcee6..52c057b2 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, ) @@ -491,12 +492,32 @@ 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): + self.checkpoint() + break + assert isinstance(item.item.func, (cst.Attribute, cst.Name)) + 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 +576,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 diff --git a/tests/autofix_files/async100.py b/tests/autofix_files/async100.py index 524e320f..95613cb8 100644 --- a/tests/autofix_files/async100.py +++ b/tests/autofix_files/async100.py @@ -130,3 +130,9 @@ 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(): + ... diff --git a/tests/autofix_files/async100.py.diff b/tests/autofix_files/async100.py.diff index 268f7d82..3ffe6c35 100644 --- a/tests/autofix_files/async100.py.diff +++ b/tests/autofix_files/async100.py.diff @@ -130,3 +130,13 @@ with contextlib.suppress(Exception): with open("blah") as file: +@@ x,6 x,6 @@ + + + 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(): ++ ... diff --git a/tests/autofix_files/async100_trio.py b/tests/autofix_files/async100_trio.py new file mode 100644 index 00000000..8ab2dd5b --- /dev/null +++ b/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(): + ... diff --git a/tests/autofix_files/async100_trio.py.diff b/tests/autofix_files/async100_trio.py.diff new file mode 100644 index 00000000..dd355aae --- /dev/null +++ b/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(): ++ ... diff --git a/tests/eval_files/async100.py b/tests/eval_files/async100.py index 1460a545..10dcc1f7 100644 --- a/tests/eval_files/async100.py +++ b/tests/eval_files/async100.py @@ -130,3 +130,9 @@ 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(): + ... diff --git a/tests/eval_files/async100_trio.py b/tests/eval_files/async100_trio.py new file mode 100644 index 00000000..ccf5a1ea --- /dev/null +++ b/tests/eval_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(): + with trio.CancelScope(): # error: 9, "trio", "CancelScope" + async with trio.open_nursery(): + ... From 4df62687a1462044e0140cdaa1fc1070fc9e4bf3 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 15 Nov 2024 16:32:11 +0100 Subject: [PATCH 2/5] don't crash on weird 'with' call --- flake8_async/visitors/visitor91x.py | 6 ++++-- tests/autofix_files/async100.py | 5 +++++ tests/autofix_files/async100.py.diff | 5 ++++- tests/eval_files/async100.py | 5 +++++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 52c057b2..26980549 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -501,10 +501,12 @@ def _checkpoint_with(self, node: cst.With): """ if getattr(node, "asynchronous", None): for item in node.items: - if not isinstance(item.item, cst.Call): + if not isinstance(item.item, cst.Call) or not isinstance( + item.item.func, (cst.Attribute, cst.Name) + ): self.checkpoint() break - assert isinstance(item.item.func, (cst.Attribute, cst.Name)) + func = identifier_to_string(item.item.func) assert func is not None if func not in ("trio.open_nursery", "anyio.create_task_group"): diff --git a/tests/autofix_files/async100.py b/tests/autofix_files/async100.py index 95613cb8..ecd6946b 100644 --- a/tests/autofix_files/async100.py +++ b/tests/autofix_files/async100.py @@ -136,3 +136,8 @@ 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)(): + ... diff --git a/tests/autofix_files/async100.py.diff b/tests/autofix_files/async100.py.diff index 3ffe6c35..7f074e60 100644 --- a/tests/autofix_files/async100.py.diff +++ b/tests/autofix_files/async100.py.diff @@ -130,7 +130,7 @@ with contextlib.suppress(Exception): with open("blah") as file: -@@ x,6 x,6 @@ +@@ x,9 x,9 @@ async def nursery_no_cancel_point(): @@ -140,3 +140,6 @@ + # error: 9, "trio", "CancelScope" + async with anyio.create_task_group(): + ... + + + async def dont_crash_on_non_name_or_attr_call(): diff --git a/tests/eval_files/async100.py b/tests/eval_files/async100.py index 10dcc1f7..4dc100f5 100644 --- a/tests/eval_files/async100.py +++ b/tests/eval_files/async100.py @@ -136,3 +136,8 @@ 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)(): + ... From 9c3c10e9cb9594eb7d531e2846da60b72387fda0 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sun, 17 Nov 2024 14:49:09 +0100 Subject: [PATCH 3/5] add more documentation. fix bad formatting for 24.9.3 in changelog --- docs/changelog.rst | 3 ++- docs/glossary.rst | 34 ++++++++++++++++++++++++++++++++-- docs/rules.rst | 1 + 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2480b3f1..cd9fb6ce 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,7 +5,7 @@ Changelog `CalVer, YY.month.patch `_ 24.11.1 -====== +======= - :ref:`ASYNC100 ` now ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group` as cancellation sources. 24.10.2 @@ -27,6 +27,7 @@ Changelog 24.9.3 ====== - :ref:`ASYNC102 ` and :ref:`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 diff --git a/docs/glossary.rst b/docs/glossary.rst index 7dde9b7b..edc5dc13 100644 --- a/docs/glossary.rst +++ b/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 ` and +:ref:`can switch which task is running `, 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,6 +99,8 @@ 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 ` will still not guarantee checkpoints. @@ -116,6 +118,34 @@ To insert a checkpoint with no other side effects, you can use :func:`trio.lowlevel.checkpoint`/:func:`anyio.lowlevel.checkpoint`/:func:`asyncio.sleep(0) ` +.. _schedule_point: +.. _schedule_points: + +Schedule Point +-------------- +A partial `checkpoint` that allows the async backend to switch the running task, but doesn't check for cancellation. This is 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(0) + +asyncio does not have any direct equivalents due to their cancellation model being different. + + +.. _cancel_point: +.. _cancel_points: + +Cancel Point +------------ +A partial `checkpoint` that will raise :ref:`cancelled` if the enclosing cancel scope has been cancelled, but does not allow the scheduler to switch to a different task. +This is 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 diff --git a/docs/rules.rst b/docs/rules.rst index c3b7566d..f7a404b3 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`. See :ref:`ASYNC912 ` which will in addition guarantee checkpoints on every code path. ASYNC101 : yield-in-cancel-scope From 851b2b42b0b7cc9b3920fbea1a9b60f0deb5fccf Mon Sep 17 00:00:00 2001 From: Zac Hatfield-Dodds Date: Sun, 17 Nov 2024 14:45:07 -0800 Subject: [PATCH 4/5] tweak docs --- docs/changelog.rst | 4 +++- docs/glossary.rst | 11 ++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index cd9fb6ce..4a2a8938 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,7 +6,9 @@ Changelog 24.11.1 ======= -- :ref:`ASYNC100 ` now ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group` as cancellation sources. +- :ref:`ASYNC100 ` now ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group` + as cancellation sources, because they are :ref:`schedule points ` but not + :ref:`cancellation points `. 24.10.2 ======= diff --git a/docs/glossary.rst b/docs/glossary.rst index edc5dc13..d491db47 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -103,7 +103,7 @@ The one exception is :func:`trio.open_nursery` and :func:`anyio.create_task_grou asyncio does not place any guarantees on if or when asyncio functions will checkpoint. This means that enabling and adhering to :ref:`ASYNC91x ` -will still not guarantee checkpoints. +will still not guarantee checkpoints on asyncio. For anyio it will depend on the current backend. @@ -123,7 +123,8 @@ To insert a checkpoint with no other side effects, you can use Schedule Point -------------- -A partial `checkpoint` that allows the async backend to switch the running task, but doesn't check for cancellation. This is available as :func:`trio.lowlevel.cancel_shielded_checkpoint`/:func:`anyio.lowlevel.cancel_shielded_checkpoint`, and equivalent to +A schedule point is half of a full `checkpoint`, which allows the async backend to switch the running task, but doesn't check for cancellation (the other half is a `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 @@ -132,7 +133,7 @@ A partial `checkpoint` that allows the async backend to switch the running task, # from anyio import CancelScope, lowlevel with CancelScope(shield=True): - await lowlevel.checkpoint(0) + await lowlevel.checkpoint() asyncio does not have any direct equivalents due to their cancellation model being different. @@ -142,8 +143,8 @@ asyncio does not have any direct equivalents due to their cancellation model bei Cancel Point ------------ -A partial `checkpoint` that will raise :ref:`cancelled` if the enclosing cancel scope has been cancelled, but does not allow the scheduler to switch to a different task. -This is available as :func:`trio.lowlevel.checkpoint_if_cancelled`/:func:`anyio.lowlevel.checkpoint_if_cancelled`. +A schedule point is half of a full `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 `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: From 25d74de79626f450045f305ea23747314e9c9847 Mon Sep 17 00:00:00 2001 From: Zac Hatfield-Dodds Date: Sun, 17 Nov 2024 14:49:32 -0800 Subject: [PATCH 5/5] fix cross-refs --- docs/glossary.rst | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/glossary.rst b/docs/glossary.rst index d491db47..36574f32 100644 --- a/docs/glossary.rst +++ b/docs/glossary.rst @@ -103,9 +103,7 @@ The one exception is :func:`trio.open_nursery` and :func:`anyio.create_task_grou asyncio does not place any guarantees on if or when asyncio functions will checkpoint. This means that enabling and adhering to :ref:`ASYNC91x ` -will still not guarantee checkpoints on asyncio. - -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 @@ -123,7 +121,7 @@ To insert a checkpoint with no other side effects, you can use Schedule Point -------------- -A schedule point is half of a full `checkpoint`, which allows the async backend to switch the running task, but doesn't check for cancellation (the other half is a `cancel_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 @@ -143,7 +141,7 @@ asyncio does not have any direct equivalents due to their cancellation model bei Cancel Point ------------ -A schedule point is half of a full `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 `schedule_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`.