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

@@ -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.

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 <https://discuss.python.org/t/preventing-yield-inside-certain-context-managers/1091/23>`_ for discussion of a future PEP.
+This has substantial overlap with :ref:`ASYNC119 <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 <https://peps.python.org/pep-0533/>`_.
 - **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 <https://peps.python.org/pep-0533/>`_ and use `async with aclosing(...) <https://docs.python.org/3/library/contextlib.html#contextlib.aclosing>`_, or better yet avoid async generators entirely (see :ref:`ASYNC900 <async900>` ) in favor of context managers which return an iterable `channel (trio) <https://trio.readthedocs.io/en/stable/reference-core.html#channels>`_, `stream (anyio) <https://anyio.readthedocs.io/en/stable/streams.html#streams>`_, or `queue (asyncio) <https://docs.python.org/3/library/asyncio-queue.html>`_.
 
   .. 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
 ================

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

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))),
                 )
             ),

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(

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

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

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

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

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