add async119: yield in contextmanager in async generator (#238)

* add async119: yield in contextmanager in async generator

* fix version

* make async900 label point directly to the async900 rule. Save bools instead of nodes since we don't need to warn on them or refer to them. Warn on each yield, add test case for that.

Author: jakkdl

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Changelog
 *[CalVer, YY.month.patch](https://calver.org/)*
 
+## 24.4.2
+- Add ASYNC119: yield in contextmanager in async generator.
 
 ## 24.4.1
 - ASYNC91X fix internal error caused by multiple `try/except` incorrectly sharing state.

README.md

@@ -41,6 +41,7 @@ pip install flake8-async
 - **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**: `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 ASYNC900) in favor of context managers which return an iterable channel/queue.
 
 ### Warnings for blocking sync calls in async functions
 Note: 22X, 23X and 24X has not had asyncio-specific suggestions written.

docs/rules.rst

@@ -21,6 +21,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**: ``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
 
 Blocking sync calls in async functions
 ======================================
@@ -42,9 +45,12 @@ Note: 22X, 23X and 24X has not had asyncio-specific suggestions written.
 - **ASYNC250**: Builtin ``input()`` should not be called from async function. Wrap in ``[trio/anyio].to_thread.run_sync()`` or ``asyncio.loop.run_in_executor()``.
 - **ASYNC251**: ``time.sleep(...)`` should not be called from async function. Use ``[trio/anyio/asyncio].sleep(...)``.
 
+
 Optional rules disabled by default
 ==================================
 
+.. _async900:
+
 - **ASYNC900**: Async generator without ``@asynccontextmanager`` not allowed. You might want to enable this on a codebase since async generators are inherently unsafe and cleanup logic might not be performed. See https://github.com/python-trio/flake8-async/issues/211 and https://discuss.python.org/t/using-exceptiongroup-at-anthropic-experience-report/20888/6 for discussion.
 - **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)

flake8_async/__init__.py

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

flake8_async/visitors/visitors.py

@@ -281,6 +281,45 @@ def visit_Call(self, node: ast.Call):
                 self.error(node, m[2])
 
 
+@error_class
+class Visitor119(Flake8AsyncVisitor):
+    error_codes: Mapping[str, str] = {
+        "ASYNC119": "Yield in contextmanager in async generator might not trigger"
+        " cleanup. Use `@asynccontextmanager` or refactor."
+    }
+
+    def __init__(self, *args: Any, **kwargs: Any):
+        super().__init__(*args, **kwargs)
+        self.unsafe_function: bool = False
+        self.contextmanager: bool = False
+
+    def visit_AsyncFunctionDef(
+        self, node: ast.AsyncFunctionDef | ast.FunctionDef | ast.Lambda
+    ):
+        self.save_state(node, "unsafe_function", "contextmanager")
+        self.contextmanager = False
+        if isinstance(node, ast.AsyncFunctionDef) and not has_decorator(
+            node, "asynccontextmanager"
+        ):
+            self.unsafe_function = True
+        else:
+            self.unsafe_function = False
+
+    def visit_With(self, node: ast.With | ast.AsyncWith):
+        self.save_state(node, "contextmanager")
+        self.contextmanager = True
+
+    def visit_Yield(self, node: ast.Yield):
+        if self.unsafe_function and self.contextmanager:
+            self.error(node)
+
+    visit_AsyncWith = visit_With
+    visit_FunctionDef = visit_AsyncFunctionDef
+    # it's not possible to yield or open context managers in lambda's, so this
+    # one isn't strictly needed afaik.
+    visit_Lambda = visit_AsyncFunctionDef
+
+
 @error_class
 @disabled_by_default
 class Visitor900(Flake8AsyncVisitor):

tests/eval_files/async119.py

@@ -0,0 +1,64 @@
+import contextlib
+
+from contextlib import asynccontextmanager
+
+
+async def unsafe_yield():
+    with open(""):
+        yield  # error: 8
+
+
+async def async_with():
+    async with unsafe_yield():
+        yield  # error: 8
+
+
+async def warn_on_yeach_yield():
+    with open(""):
+        yield  # error: 8
+        yield  # error: 8
+    with open(""):
+        yield  # error: 8
+        yield  # error: 8
+
+
+async def yield_not_in_contextmanager():
+    yield
+    with open(""):
+        ...
+    yield
+
+
+async def yield_in_nested_function():
+    with open(""):
+
+        def foo():
+            yield
+
+
+async def yield_in_nested_async_function():
+    with open(""):
+
+        async def foo():
+            yield
+
+
+async def yield_after_nested_async_function():
+    with open(""):
+
+        async def foo():
+            yield
+
+        yield  # error: 8
+
+
+@asynccontextmanager
+async def safe_in_contextmanager():
+    with open(""):
+        yield
+
+
+@contextlib.asynccontextmanager
+async def safe_in_contextmanager2():
+    with open(""):
+        yield