From e63d5803143ced7b8e66ecc987ef586ca98b9823 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 23 Apr 2024 14:32:45 +0200 Subject: [PATCH] add async119: yield in contextmanager in async generator --- CHANGELOG.md | 2 ++ README.md | 1 + docs/rules.rst | 5 +++ flake8_async/visitors/visitors.py | 41 +++++++++++++++++++++++ tests/eval_files/async119.py | 55 +++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+) create mode 100644 tests/eval_files/async119.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dd2d283..a4145db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog *[CalVer, YY.month.patch](https://calver.org/)* +## 24.4.23 +- Add ASYNC119: yield in contextmanager in async generator. ## 24.4.1 - ASYNC91X fix internal error caused by multiple `try/except` incorrectly sharing state. diff --git a/README.md b/README.md index b98f4dc..654d406 100644 --- a/README.md +++ b/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. diff --git a/docs/rules.rst b/docs/rules.rst index 007a548..eabaf54 100644 --- a/docs/rules.rst +++ b/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 `_ 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 Blocking sync calls in async functions ====================================== @@ -42,6 +45,8 @@ 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(...)``. +.. _async900: + Optional rules disabled by default ================================== diff --git a/flake8_async/visitors/visitors.py b/flake8_async/visitors/visitors.py index 07ceaab..de91e2f 100644 --- a/flake8_async/visitors/visitors.py +++ b/flake8_async/visitors/visitors.py @@ -281,6 +281,47 @@ 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: ast.AsyncFunctionDef | None = None + self.contextmanager: ast.With | ast.AsyncWith | None = None + + def visit_AsyncFunctionDef( + self, node: ast.AsyncFunctionDef | ast.FunctionDef | ast.Lambda + ): + self.save_state(node, "unsafe_function", "contextmanager") + self.contextmanager = None + if isinstance(node, ast.AsyncFunctionDef) and not has_decorator( + node, "asynccontextmanager" + ): + self.unsafe_function = node + else: + self.unsafe_function = None + + def visit_With(self, node: ast.With | ast.AsyncWith): + self.save_state(node, "contextmanager") + self.contextmanager = node + + def visit_Yield(self, node: ast.Yield): + if self.unsafe_function is not None and self.contextmanager is not None: + # Decision point: the error could point to the method, or context manager, + # or the yield. + self.error(node) + # only warn once per method (?) + self.unsafe_function = None + + visit_AsyncWith = visit_With + visit_FunctionDef = visit_AsyncFunctionDef + visit_Lambda = visit_AsyncFunctionDef + + @error_class @disabled_by_default class Visitor900(Flake8AsyncVisitor): diff --git a/tests/eval_files/async119.py b/tests/eval_files/async119.py new file mode 100644 index 0000000..3f173c4 --- /dev/null +++ b/tests/eval_files/async119.py @@ -0,0 +1,55 @@ +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 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