From 4cb66ae3d4c8db6e95a96381708aa181997dc03b Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Mon, 13 May 2024 19:14:58 +0200 Subject: [PATCH] add asyncio/anyio taskgroup support to ASYNC101 (#244) * add asyncio/anyio taskgroup support to ASYNC101. Fix func_has_decorator to recognize decorators that are calls but not attributes. Clarify difference between ASYNC101 and ASYNC119 * changelog and version bump * update docs for async101 to mention taskgroup functionality --- CHANGELOG.md | 4 + docs/rules.rst | 8 +- flake8_async/__init__.py | 2 +- flake8_async/visitors/helpers.py | 10 ++- flake8_async/visitors/visitor101.py | 14 +++- tests/eval_files/async101.py | 115 ++++++++++++++++++++------- tests/eval_files/async101_anyio.py | 10 +++ tests/eval_files/async101_asyncio.py | 82 +++++++++++++++++++ tests/eval_files/async101_trio.py | 17 ++++ tests/eval_files/async113_trio.py | 12 +++ 10 files changed, 237 insertions(+), 37 deletions(-) create mode 100644 tests/eval_files/async101_anyio.py create mode 100644 tests/eval_files/async101_asyncio.py create mode 100644 tests/eval_files/async101_trio.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b876101..11fcae2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Changelog *[CalVer, YY.month.patch](https://calver.org/)* +## 24.5.2 +- ASYNC101 now also warns on anyio & asyncio taskgroup.s +- Fixed a bug where ASYNC101 and ASYNC91x would not recognize decorators with parameters directly imported. I.e. `@fixture(...)` will now suppress errors. + ## 24.5.1 - Add ASYNC912: no checkpoints in with statement are guaranteed to run. - ASYNC100 now properly treats async for comprehensions as checkpoints. diff --git a/docs/rules.rst b/docs/rules.rst index 9fa5250..05d4ab5 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -6,7 +6,8 @@ General rules ============= - **ASYNC100**: A ``with [trio/anyio].fail_after(...):`` or ``with [trio/anyio].move_on_after(...):`` context does not contain any ``await`` statements. This makes it pointless, as the timeout can only be triggered by a checkpoint. This check also allows ``yield`` statements, since checkpoints can happen in the caller we yield to. -- **ASYNC101**: ``yield`` inside a trio/anyio nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling. +- **ASYNC101**: ``yield`` inside a trio nursery, anyio/asyncio TaskGroup, or in a timeout/cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling. See `this thread `_ for discussion of a future PEP. +This has substantial overlap with :ref:`ASYNC119 `, which will warn on almost all instances of ASYNC101, but ASYNC101 is about a conceptually different problem that will not get resolved by `PEP 533 `_. - **ASYNC102**: It's unsafe to await inside ``finally:`` or ``except BaseException/trio.Cancelled/anyio.get_cancelled_exc_class()/asyncio.exceptions.CancelledError`` unless you use a shielded cancel scope with a timeout. This is currently not able to detect asyncio shields. - **ASYNC103**: ``except BaseException/trio.Cancelled/anyio.get_cancelled_exc_class()/asyncio.exceptions.CancelledError``, or a bare ``except:`` with a code path that doesn't re-raise. If you don't want to re-raise ``BaseException``, add a separate handler for ``trio.Cancelled``/``anyio.get_cancelled_exc_class()``/``asyncio.exceptions.CancelledError`` before. - **ASYNC104**: ``trio.Cancelled``/``anyio.get_cancelled_exc_class()``/``asyncio.exceptions.CancelledError``/``BaseException`` must be re-raised. The same as ASYNC103, except specifically triggered on ``return`` or a different exception being raised. @@ -21,6 +22,9 @@ General rules - **ASYNC115**: Replace ``[trio/anyio].sleep(0)`` with the more suggestive ``[trio/anyio].lowlevel.checkpoint()``. - **ASYNC116**: ``[trio/anyio].sleep()`` with >24 hour interval should usually be ``[trio/anyio].sleep_forever()``. - **ASYNC118**: Don't assign the value of ``anyio.get_cancelled_exc_class()`` to a variable, since that breaks linter checks and multi-backend programs. + + .. _async119: + - **ASYNC119**: ``yield`` in context manager in async generator is unsafe, the cleanup may be delayed until ``await`` is no longer allowed. We strongly encourage you to read `PEP 533 `_ and use `async with aclosing(...) `_, or better yet avoid async generators entirely (see :ref:`ASYNC900 ` ) in favor of context managers which return an iterable `channel (trio) `_, `stream (anyio) `_, or `queue (asyncio) `_. .. TODO: use intersphinx(?) instead of having to specify full URL @@ -55,7 +59,7 @@ Optional rules disabled by default - **ASYNC910**: Exit or ``return`` from async function with no guaranteed checkpoint or exception since function definition. You might want to enable this on a codebase to make it easier to reason about checkpoints, and make the logic of ASYNC911 correct. - **ASYNC911**: Exit, ``yield`` or ``return`` from async iterable with no guaranteed checkpoint since possible function entry (yield or function definition) Checkpoints are ``await``, ``async for``, and ``async with`` (on one of enter/exit). --- **ASYNC912**: A timeout/cancelscope has 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. It instead shares logic with ASYNC910 and ASYNC911 for parsing conditionals and branches. +- **ASYNC912**: A timeout/cancelscope has 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. It instead shares logic with ASYNC910 and ASYNC911 for parsing conditionals and branches. Removed rules ================ diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index b808ba5..b15ab41 100644 --- a/flake8_async/__init__.py +++ b/flake8_async/__init__.py @@ -37,7 +37,7 @@ # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" -__version__ = "24.5.1" +__version__ = "24.5.2" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/helpers.py b/flake8_async/visitors/helpers.py index d33e099..89a859b 100644 --- a/flake8_async/visitors/helpers.py +++ b/flake8_async/visitors/helpers.py @@ -89,7 +89,8 @@ def _get_identifier(node: ast.expr) -> str: # ignores module and only checks the unqualified name of the decorator -# used in 101, 113, 900 and 910/911 +# used in 113. cst version used in 101, 900 and 910/911 +# matches @name, @foo.name, @name(...), and @foo.name(...) def has_decorator(node: ast.FunctionDef | ast.AsyncFunctionDef, *names: str): return any(_get_identifier(dec) in names for dec in node.decorator_list) @@ -343,7 +344,7 @@ def identifier_to_string(attr: cst.Name | cst.Attribute) -> str: def with_has_call( - node: cst.With, *names: str, base: Iterable[str] = ("trio", "anyio") + node: cst.With, *names: str, base: Iterable[str] | str = ("trio", "anyio") ) -> list[AttributeCall]: """Check if a with statement has a matching call, returning a list with matches. @@ -395,8 +396,13 @@ def func_has_decorator(func: cst.FunctionDef, *names: str) -> bool: func.decorators, m.Decorator( decorator=m.OneOf( + # @name oneof_names(*names), + # @foo.name m.Attribute(attr=oneof_names(*names)), + # @name(...) + m.Call(func=oneof_names(*names)), + # @foo.name(...) m.Call(func=m.Attribute(attr=oneof_names(*names))), ) ), diff --git a/flake8_async/visitors/visitor101.py b/flake8_async/visitors/visitor101.py index 0ad77d8..a5edcd9 100644 --- a/flake8_async/visitors/visitor101.py +++ b/flake8_async/visitors/visitor101.py @@ -40,12 +40,22 @@ def visit_With(self, node: cst.With): self.save_state(node, "_yield_is_error", copy=True) # if there's no safe decorator, # and it's not yet been determined that yield is error - # and this withitem opens a cancelscope: + # and this withitem opens a nursery/taskgroup/cancelscope: # then yielding is unsafe self._yield_is_error = ( not self._safe_decorator and not self._yield_is_error - and bool(with_has_call(node, "open_nursery", *cancel_scope_names)) + # It's not strictly necessary to specify the base, as raising errors on + # e.g. anyio.open_nursery isn't much of a problem. + and bool( + # nursery/taskgroup + with_has_call(node, "open_nursery", base="trio") + or with_has_call(node, "create_task_group", base="anyio") + or with_has_call(node, "TaskGroup", base="asyncio") + # cancel scopes + or with_has_call(node, "timeout", "timeout_at", base="asyncio") + or with_has_call(node, *cancel_scope_names, base=("trio", "anyio")) + ) ) def leave_With( diff --git a/tests/eval_files/async101.py b/tests/eval_files/async101.py index b0bdf76..88e3b34 100644 --- a/tests/eval_files/async101.py +++ b/tests/eval_files/async101.py @@ -1,74 +1,129 @@ -# ASYNCIO_NO_ERROR - nursery/cancelscope in asyncio is called taskgroup/timeout -# Note: this one *shouldn't* be giving the same errors for anyio (but it currently does) -# since it has TaskGroups instead of nurseries. #TODO #215 -# type: ignore +# ASYNCIO_NO_ERROR + +# This file contains errors shared between trio and anyio, since they have some +# overlap in naming. +# See async101_xxx which has errors specific to trio/asyncio/anyio. + + import contextlib import contextlib as bla from contextlib import asynccontextmanager, contextmanager, contextmanager as blahabla +import pytest +import pytest as blo +from pytest import fixture import trio -def foo0(): - with trio.open_nursery() as _: +# cancel scope aliases +async def foo_fail_after(): + with trio.fail_after(10): yield 1 # error: 8 -async def foo1(): - async with trio.open_nursery() as _: +async def foo_fail_at(): + with trio.fail_at(10): yield 1 # error: 8 -@contextmanager -def foo2(): - with trio.open_nursery() as _: - yield 1 # safe +async def foo_move_on_aft(): + with trio.move_on_after(10): + yield 1 # error: 8 -async def foo3(): - async with trio.CancelScope() as _: +# `as` makes no difference +async def foo_move_on_at(): + with trio.move_on_at(10) as _: yield 1 # error: 8 -@asynccontextmanager -async def foo4(): - async with trio.open_nursery() as _: - yield 1 # safe +async def foo_CancelScope(): + with trio.CancelScope() as _: + yield 1 # error: 8 +# the visitor does not care if the `with` is async +async def foo_async_with(): + async with trio.CancelScope() as _: # type: ignore[attr-defined] + yield 1 # error: 8 + + +# raises error at each yield +async def foo_multiple_yield(): + with trio.CancelScope() as _: + yield 1 # error: 8 + yield 1 # error: 8 + + +# nested method is safe async def foo5(): - async with trio.open_nursery(): + with trio.CancelScope(): yield 1 # error: 8 def foo6(): yield 1 # safe +# @[async]contextmanager suppresses the error +@contextmanager +def foo_contextmanager(): + with trio.CancelScope() as _: + yield 1 # safe + + +@asynccontextmanager +async def foo4(): + with trio.CancelScope() as _: + yield 1 # safe + + @contextlib.asynccontextmanager async def foo7(): - async with trio.open_nursery() as _: + with trio.CancelScope() as _: + yield 1 # safe + + +# pytest.fixture also silences async101, as they're morally context managers +@fixture +def foo_fixture(): + with trio.CancelScope() as _: + yield 1 # safe + + +@pytest.fixture +def foo_pytest_fixture(): + with trio.CancelScope() as _: yield 1 # safe +# it does not care about what library that [async]contextmanager or fixture is in @bla.contextmanager -def foo8(): - with trio.open_nursery() as _: +def foo_bla_contextmanager(): + with trio.CancelScope() as _: yield 1 # safe +@blo.fixture +def foo_blo_fixture(): + with trio.CancelScope() as _: + yield 1 # safe + + +# but any other decorator does nothing @blahabla -def foo9(): - with trio.open_nursery() as _: +def foo_blahabla(): + with trio.CancelScope() as _: yield 1 # error: 8 -@pytest.fixture() -def foo_false_alarm(): - with trio.open_nursery() as _: +# parentheses and parameters are also fine +@fixture() +def foo_pytest_fixture_paren(): + with trio.CancelScope() as _: yield 1 -@pytest.fixture -def foo_false_alarm_2(): - with trio.open_nursery() as _: +@pytest.fixture(autouse=True) +def foo_pytest_fixture_params(): + with trio.CancelScope() as _: yield 1 diff --git a/tests/eval_files/async101_anyio.py b/tests/eval_files/async101_anyio.py new file mode 100644 index 0000000..9f16b8c --- /dev/null +++ b/tests/eval_files/async101_anyio.py @@ -0,0 +1,10 @@ +# BASE_LIBRARY anyio +# TRIO_NO_ERROR +# ASYNCIO_NO_ERROR + +import anyio + + +async def foo(): + async with anyio.create_task_group(): + yield 1 # error: 8 diff --git a/tests/eval_files/async101_asyncio.py b/tests/eval_files/async101_asyncio.py new file mode 100644 index 0000000..b32fb10 --- /dev/null +++ b/tests/eval_files/async101_asyncio.py @@ -0,0 +1,82 @@ +# BASE_LIBRARY asyncio +# TRIO_NO_ERROR +# ANYIO_NO_ERROR +# TaskGroup and timeout[_at] was added in 3.11, we run type checking with 3.9 +# mypy: disable-error-code=attr-defined +import contextlib +import contextlib as bla +from contextlib import asynccontextmanager, contextmanager, contextmanager as blahabla + +import asyncio +import pytest + + +async def test_async_with(): + async with asyncio.TaskGroup() as _: + yield 1 # error: 8 + + +async def test_timeout(): + async with asyncio.timeout() as _: + yield 1 # error: 8 + + +async def test_timeout_at(): + async with asyncio.timeout_at() as _: + yield 1 # error: 8 + + +async def test_nested_method(): + async with asyncio.TaskGroup(): + yield 1 # error: 8 + + def foo6(): + yield 1 # safe + + +# TaskGroup is an async cm, but the visitor does not care about that +def test_with(): + with asyncio.TaskGroup() as _: + yield 1 # error: 8 + + +@contextmanager +def safe_1(): + with asyncio.TaskGroup() as _: + yield 1 # safe + + +@asynccontextmanager +async def safe_2(): + async with asyncio.TaskGroup() as _: + yield 1 # safe + + +@contextlib.asynccontextmanager +async def safe_3(): + async with asyncio.TaskGroup() as _: + yield 1 # safe + + +@bla.contextmanager +def safe_4(): + with asyncio.TaskGroup() as _: + yield 1 # safe + + +@blahabla +def test_unrelated_decorator(): + with asyncio.TaskGroup() as _: + yield 1 # error: 8 + + +@pytest.fixture() +def foo_false_alarm(): + with asyncio.TaskGroup() as _: + yield 1 + + +@pytest.fixture +def foo_false_alarm_2(): + with asyncio.TaskGroup() as _: + yield 1 diff --git a/tests/eval_files/async101_trio.py b/tests/eval_files/async101_trio.py new file mode 100644 index 0000000..3ceda36 --- /dev/null +++ b/tests/eval_files/async101_trio.py @@ -0,0 +1,17 @@ +# ASYNCIO_NO_ERROR +# ANYIO_NO_ERROR + +from contextlib import asynccontextmanager + +import trio + + +async def foo_open_nursery(): + async with trio.open_nursery() as _: + yield 1 # error: 8 + + +@asynccontextmanager +async def foo_open_nursery_contextmanager(): + async with trio.open_nursery() as _: + yield 1 # safe diff --git a/tests/eval_files/async113_trio.py b/tests/eval_files/async113_trio.py index 0875519..3d50c90 100644 --- a/tests/eval_files/async113_trio.py +++ b/tests/eval_files/async113_trio.py @@ -129,3 +129,15 @@ async def contextlib_import_alias_acm(): # code coverage for non-name, non-attribute decorator @None # type: ignore async def foo4(): ... + + +# while contextlib.asynccontextmanager does not allow parens or parameters, +# the visitor will still raise an error for them. +@asynccontextmanager() # type: ignore[call-arg] +async def foo_paren(): + nursery.start_soon(trio.run_process) # error: 4 + + +@asynccontextmanager(1, 2, 3) # type: ignore[call-arg] +async def foo_params(): + nursery.start_soon(trio.run_process) # error: 4