From fa6bdbbc942913fb6ce72deab02043e34299f52d Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 31 Jan 2025 17:57:19 +0100 Subject: [PATCH 1/3] make ASYNC912 and 913 care about cancel points, and ignore schedule points --- docs/changelog.rst | 4 ++++ docs/rules.rst | 5 +++-- docs/usage.rst | 2 +- flake8_async/__init__.py | 2 +- flake8_async/visitors/visitor91x.py | 14 ++++++++++---- tests/autofix_files/async913_trio_anyio.py | 12 ++++++++++++ tests/autofix_files/async913_trio_anyio.py.diff | 9 +++++++++ tests/eval_files/async912.py | 9 ++++++++- tests/eval_files/async913_trio_anyio.py | 11 +++++++++++ tests/test_flake8_async.py | 9 ++++++++- tox.ini | 1 - 11 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 tests/autofix_files/async913_trio_anyio.py create mode 100644 tests/autofix_files/async913_trio_anyio.py.diff create mode 100644 tests/eval_files/async913_trio_anyio.py diff --git a/docs/changelog.rst b/docs/changelog.rst index f1658de3..9bd7e36f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,10 @@ Changelog `CalVer, YY.month.patch `_ +25.2.1 +======= +- :ref:`ASYNC912 ` and :ref:`ASYNC913 ` will now trigger if there's no *cancel* points. This means that :func:`trio.open_nursery`/`anyio.create_task_group` will not silence them on their own, unless they're guaranteed to start tasks. + 25.1.1 ======= - Add :ref:`ASYNC124 ` async-function-could-be-sync diff --git a/docs/rules.rst b/docs/rules.rst index b2bef0bd..9d32e144 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -197,12 +197,13 @@ _`ASYNC911` : async-generator-no-checkpoint Exit, ``yield`` or ``return`` from async iterable with no guaranteed :ref:`checkpoint` since possible function entry (``yield`` or function definition). _`ASYNC912` : cancel-scope-no-guaranteed-checkpoint - A timeout/cancelscope has :ref:`checkpoints `, but they're not guaranteed to run. - Similar to `ASYNC100`_, but it does not warn on trivial cases where there is no checkpoint at all. + A timeout/cancelscope has :ref:`cancel points `, but they're not guaranteed to run. + Similar to `ASYNC100`_, but it does not warn on trivial cases where there is no cancel point at all. It instead shares logic with `ASYNC910`_ and `ASYNC911`_ for parsing conditionals and branches. _`ASYNC913` : indefinite-loop-no-guaranteed-checkpoint An indefinite loop (e.g. ``while True``) has no guaranteed :ref:`checkpoint `. This could potentially cause a deadlock. + This will also error if there's no guaranteed :ref:`cancel point`, where even though it won't deadlock the loop might become an uncancelable dry-run loop. .. _autofix-support: diff --git a/docs/usage.rst b/docs/usage.rst index d72e8123..e6e116f7 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -33,7 +33,7 @@ adding the following to your ``.pre-commit-config.yaml``: minimum_pre_commit_version: '2.9.0' repos: - repo: https://github.com/python-trio/flake8-async - rev: 25.1.1 + rev: 25.2.1 hooks: - id: flake8-async # args: [--enable=ASYNC, --disable=ASYNC9, --autofix=ASYNC] diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 405aa28c..aa715cbf 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__ = "25.1.1" +__version__ = "25.2.1" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 0036c793..b664e64b 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -354,10 +354,10 @@ class Visitor91X(Flake8AsyncVisitor_cst, CommonVisitors): "on line {1.lineno}." ), "ASYNC912": ( - "CancelScope with no guaranteed checkpoint. This makes it potentially " + "CancelScope with no guaranteed cancel point. This makes it potentially " "impossible to cancel." ), - "ASYNC913": ("Indefinite loop with no guaranteed checkpoint."), + "ASYNC913": ("Indefinite loop with no guaranteed cancel points."), "ASYNC100": ( "{0}.{1} context contains no checkpoints, remove the context or add" " `await {0}.lowlevel.checkpoint()`." @@ -401,10 +401,16 @@ def checkpoint_cancel_point(self) -> None: self.taskgroup_has_start_soon.clear() def checkpoint_schedule_point(self) -> None: - self.uncheckpointed_statements = set() + # ASYNC912&ASYNC913 only cares about cancel points, so don't remove + # them if we only do a schedule point + self.uncheckpointed_statements = { + s + for s in self.uncheckpointed_statements + if isinstance(s, ArtificialStatement) + } def checkpoint(self) -> None: - self.checkpoint_schedule_point() + self.uncheckpointed_statements = set() self.checkpoint_cancel_point() def checkpoint_statement(self) -> cst.SimpleStatementLine: diff --git a/tests/autofix_files/async913_trio_anyio.py b/tests/autofix_files/async913_trio_anyio.py new file mode 100644 index 00000000..76157ed4 --- /dev/null +++ b/tests/autofix_files/async913_trio_anyio.py @@ -0,0 +1,12 @@ +# ARG --enable=ASYNC913 +# AUTOFIX +# ASYNCIO_NO_ERROR + +import trio + + +async def nursery_no_cancel_point(): + while True: # ASYNC913: 4 + await trio.lowlevel.checkpoint() + async with trio.open_nursery(): + pass diff --git a/tests/autofix_files/async913_trio_anyio.py.diff b/tests/autofix_files/async913_trio_anyio.py.diff new file mode 100644 index 00000000..dc167287 --- /dev/null +++ b/tests/autofix_files/async913_trio_anyio.py.diff @@ -0,0 +1,9 @@ +--- ++++ +@@ x,5 x,6 @@ + + async def nursery_no_cancel_point(): + while True: # ASYNC913: 4 ++ await trio.lowlevel.checkpoint() + async with trio.open_nursery(): + pass diff --git a/tests/eval_files/async912.py b/tests/eval_files/async912.py index 1813ff26..040c39c1 100644 --- a/tests/eval_files/async912.py +++ b/tests/eval_files/async912.py @@ -125,7 +125,7 @@ def customWrapper(a: T) -> T: with (res := trio.fail_at(10)): ... # but saving with `as` does - with trio.fail_at(10) as res: # ASYNC912: 9 + with trio.fail_at(10) as res2: # ASYNC912: 9 if bar(): await trio.lowlevel.checkpoint() @@ -189,3 +189,10 @@ async def check_yield_logic(): if bar(): await trio.lowlevel.checkpoint() yield + + +async def nursery_no_cancel_point(): + with trio.move_on_after(10): # ASYNC912: 9 + async with trio.open_nursery(): + if bar(): + await trio.lowlevel.checkpoint() diff --git a/tests/eval_files/async913_trio_anyio.py b/tests/eval_files/async913_trio_anyio.py new file mode 100644 index 00000000..28fc0a1f --- /dev/null +++ b/tests/eval_files/async913_trio_anyio.py @@ -0,0 +1,11 @@ +# ARG --enable=ASYNC913 +# AUTOFIX +# ASYNCIO_NO_ERROR + +import trio + + +async def nursery_no_cancel_point(): + while True: # ASYNC913: 4 + async with trio.open_nursery(): + pass diff --git a/tests/test_flake8_async.py b/tests/test_flake8_async.py index 733ec0c7..08957642 100644 --- a/tests/test_flake8_async.py +++ b/tests/test_flake8_async.py @@ -103,7 +103,14 @@ def diff_strings(first: str, second: str, /) -> str: # replaces all instances of `original` with `new` in string # unless it's preceded by a `-`, which indicates it's part of a command-line flag def replace_library(string: str, original: str = "trio", new: str = "anyio") -> str: - return re.sub(rf"(? str: + return re.sub(rf"(? Date: Fri, 31 Jan 2025 18:13:18 +0100 Subject: [PATCH 2/3] fix doc reference --- docs/rules.rst | 2 +- tox.ini | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/rules.rst b/docs/rules.rst index 9d32e144..974d99cc 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -203,7 +203,7 @@ _`ASYNC912` : cancel-scope-no-guaranteed-checkpoint _`ASYNC913` : indefinite-loop-no-guaranteed-checkpoint An indefinite loop (e.g. ``while True``) has no guaranteed :ref:`checkpoint `. This could potentially cause a deadlock. - This will also error if there's no guaranteed :ref:`cancel point`, where even though it won't deadlock the loop might become an uncancelable dry-run loop. + This will also error if there's no guaranteed :ref:`cancel point `, where even though it won't deadlock the loop might become an uncancelable dry-run loop. .. _autofix-support: diff --git a/tox.ini b/tox.ini index b7579d7e..2bcca72d 100644 --- a/tox.ini +++ b/tox.ini @@ -30,6 +30,7 @@ allowlist_externals = make changedir = docs skip_install = True commands = + make clean make html # Settings for other tools From 5df97af30bcfbeb052cf3b5c7ebdd719e9f26e6c Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 3 Feb 2025 16:41:32 +0100 Subject: [PATCH 3/3] dirty temp fix to deal with non-912/913 in another PR --- tests/test_flake8_async.py | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/tests/test_flake8_async.py b/tests/test_flake8_async.py index 08957642..86673b4d 100644 --- a/tests/test_flake8_async.py +++ b/tests/test_flake8_async.py @@ -102,14 +102,20 @@ def diff_strings(first: str, second: str, /) -> str: # replaces all instances of `original` with `new` in string # unless it's preceded by a `-`, which indicates it's part of a command-line flag -def replace_library(string: str, original: str = "trio", new: str = "anyio") -> str: +def replace_library( + string: str, + original: str = "trio", + new: str = "anyio", + replace_nursery: bool = False, +) -> str: def replace_str(string: str, original: str, new: str) -> str: return re.sub(rf"(?