diff --git a/docs/changelog.rst b/docs/changelog.rst index 2d19510..f70171e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,11 @@ Changelog *[CalVer, YY.month.patch](https://calver.org/)* +24.6.1 +====== +- Add :ref:`ASYNC120 ` await-in-except. +- Fix false alarm with :ref:`ASYNC102 ` with function definitions inside finally/except. + 24.5.6 ====== - Make :ref:`ASYNC913 ` disabled by default, as originally intended. diff --git a/docs/rules.rst b/docs/rules.rst index 14e0610..1b9b95b 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -20,8 +20,10 @@ ASYNC101 : yield-in-cancel-scope 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 : await-in-finally-or-cancelled +_`ASYNC102` : await-in-finally-or-cancelled ``await`` inside ``finally`` or :ref:`cancelled-catching ` ``except:`` must have shielded :ref:`cancel scope ` with timeout. + If not, the async call will immediately raise a new cancellation, suppressing the cancellation that was caught. + See :ref:`ASYNC120 ` for the general case where other exceptions might get suppressed. This is currently not able to detect asyncio shields. ASYNC103 : no-reraise-cancelled @@ -73,6 +75,12 @@ _`ASYNC119` : yield-in-cm-in-async-gen ``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 :ref:`channel/stream/queue `. +_`ASYNC120` : await-in-except + Dangerous :ref:`checkpoint` inside an ``except`` block. + If this checkpoint is cancelled, the current active exception will be replaced by the ``Cancelled`` exception, and cannot be reraised later. + This will not trigger when :ref:`ASYNC102 ` does, and if you don't care about losing non-cancelled exceptions you could disable this rule. + This is currently not able to detect asyncio shields. + Blocking sync calls in async functions ====================================== diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 3402408..b85a828 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__ = "24.5.6" +__version__ = "24.6.1" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 52177dd..be69bec 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -23,6 +23,10 @@ class Visitor102(Flake8AsyncVisitor): "await inside {0.name} on line {0.lineno} must have shielded cancel " "scope with a timeout." ), + "ASYNC120": ( + "checkpoint inside {0.name} on line {0.lineno} will discard the active " + "exception if cancelled." + ), } class TrioScope: @@ -52,6 +56,15 @@ def __init__(self, *args: Any, **kwargs: Any): self._trio_context_managers: list[Visitor102.TrioScope] = [] self.cancelled_caught = False + # list of dangerous awaits inside a non-critical except handler, + # which will emit errors upon reaching a raise. + # Entries are only added to the list inside except handlers, + # so with the `save_state` in visit_ExceptHandler any entries not followed + # by a raise will be thrown out when exiting the except handler. + self._potential_120: list[ + tuple[ast.Await | ast.AsyncFor | ast.AsyncWith, Statement] + ] = [] + # if we're inside a finally or critical except, and we're not inside a scope that # doesn't have both a timeout and shield def async_call_checker( @@ -60,7 +73,16 @@ def async_call_checker( if self._critical_scope is not None and not any( cm.has_timeout and cm.shielded for cm in self._trio_context_managers ): - self.error(node, self._critical_scope) + # non-critical exception handlers have the statement name set to "except" + if self._critical_scope.name == "except": + self._potential_120.append((node, self._critical_scope)) + else: + self.error(node, self._critical_scope, error_code="ASYNC102") + + def visit_Raise(self, node: ast.Raise): + for err_node, scope in self._potential_120: + self.error(err_node, scope, error_code="ASYNC120") + self._potential_120.clear() def is_safe_aclose_call(self, node: ast.Await) -> bool: return ( @@ -122,16 +144,21 @@ def visit_Try(self, node: ast.Try): self.visit_nodes(node.finalbody) def visit_ExceptHandler(self, node: ast.ExceptHandler): - if self.cancelled_caught: - return - res = critical_except(node) - if res is None: + # if we're inside a critical scope, a nested except should never override that + if self._critical_scope is not None and self._critical_scope.name != "except": return - self.save_state(node, "_critical_scope", "_trio_context_managers") - self.cancelled_caught = True + self.save_state( + node, "_critical_scope", "_trio_context_managers", "_potential_120" + ) self._trio_context_managers = [] - self._critical_scope = res + self._potential_120 = [] + + if self.cancelled_caught or (res := critical_except(node)) is None: + self._critical_scope = Statement("except", node.lineno, node.col_offset) + else: + self._critical_scope = res + self.cancelled_caught = True def visit_Assign(self, node: ast.Assign): # checks for .shield = [True/False] @@ -147,3 +174,24 @@ def visit_Assign(self, node: ast.Assign): and isinstance(node.value, ast.Constant) ): last_scope.shielded = node.value.value + + def visit_FunctionDef( + self, node: ast.FunctionDef | ast.AsyncFunctionDef | ast.Lambda + ): + self.save_state( + node, + "_critical_scope", + "_trio_context_managers", + "_potential_120", + "cancelled_caught", + ) + self._critical_scope = None + self._trio_context_managers = [] + self.cancelled_caught = False + + self._potential_120 = [] + + visit_AsyncFunctionDef = visit_FunctionDef + # lambda can't contain await, try, except, raise, with, or assignments. + # You also can't do assignment expressions with attributes. So we don't need to + # do any special handling for them. diff --git a/tests/eval_files/async102.py b/tests/eval_files/async102.py index b6b7278..fba16e4 100644 --- a/tests/eval_files/async102.py +++ b/tests/eval_files/async102.py @@ -1,4 +1,5 @@ # type: ignore +# ARG --enable=ASYNC102,ASYNC120 # NOASYNCIO # TODO: support asyncio shields from contextlib import asynccontextmanager @@ -170,7 +171,7 @@ async def foo4(): try: ... except ValueError: - await foo() # safe + await foo() except BaseException: await foo() # error: 8, Statement("BaseException", lineno-1) except: @@ -237,7 +238,7 @@ async def foo_nested_excepts(): try: await foo() # error: 12, Statement("BaseException", lineno-3) except BaseException: - await foo() # error: 12, Statement("BaseException", lineno-1) + await foo() # error: 12, Statement("BaseException", lineno-5) except: # unsafe, because we're waiting inside the parent baseexception await foo() # error: 12, Statement("BaseException", lineno-8) @@ -265,3 +266,13 @@ async def foo_nested_async_for(): j ) in trio.bypasslinters: ... + + +# nested funcdef (previously false alarm) +async def foo_nested_funcdef(): + try: + ... + except: + + async def foobar(): + await foo() diff --git a/tests/eval_files/async102_aclose.py b/tests/eval_files/async102_aclose.py index df1343d..4cba907 100644 --- a/tests/eval_files/async102_aclose.py +++ b/tests/eval_files/async102_aclose.py @@ -1,8 +1,12 @@ # type: ignore +# ARG --enable=ASYNC102,ASYNC120 -# exclude finally: await x.aclose() from async102, with trio/anyio +# exclude finally: await x.aclose() from async102 and async120, with trio/anyio + +# These magical markers are the ones that ensure trio & anyio don't raise errors: # ANYIO_NO_ERROR # TRIO_NO_ERROR + # See also async102_aclose_args.py - which makes sure trio/anyio raises errors if there # are arguments to aclose(). diff --git a/tests/eval_files/async120.py b/tests/eval_files/async120.py new file mode 100644 index 0000000..f969f45 --- /dev/null +++ b/tests/eval_files/async120.py @@ -0,0 +1,123 @@ +# ARG --enable=ASYNC102,ASYNC120 +# NOASYNCIO # TODO: support asyncio shields (?) + +import trio + + +def condition() -> bool: + return False + + +async def foo(): + try: + ... + except ValueError: + await foo() # ASYNC120: 8, Stmt("except", lineno-1) + if condition(): + raise + await foo() + + try: + ... + except ValueError: + await foo() # ASYNC120: 8, Stmt("except", lineno-1) + raise + + # don't error if the raise is in a separate excepthandler + try: + ... + except ValueError: + await foo() + except TypeError: + raise + + # does not support conditional branches + try: + ... + except ValueError: + if ...: + await foo() # ASYNC120: 12, Stmt("except", lineno-2) + else: + raise + + # don't trigger on cases of ASYNC102 (?) + try: + ... + except: + await foo() # ASYNC102: 8, Stmt("bare except", lineno-1) + raise + + # shielded awaits with timeouts don't trigger 120 + try: + ... + except: + with trio.fail_after(10) as cs: + cs.shield = True + await foo() + raise + + try: + ... + except: + with trio.fail_after(10) as cs: + cs.shield = True + await foo() + raise + + # ************************ + # Weird nesting edge cases + # ************************ + + # nested excepthandlers should not trigger 120 on awaits in + # their parent scope + try: + ... + except ValueError: + await foo() + try: + ... + except TypeError: + raise + + # but the other way around probably should(?) + try: + ... + except ValueError: + try: + ... + except TypeError: + await foo() + raise + + # but only when they're properly nested, this should not give 120 + try: + ... + except TypeError: + await foo() + if condition(): + raise + + try: + ... + except ValueError: + await foo() # ASYNC120: 8, Statement("except", lineno-1) + try: + await foo() # ASYNC120: 12, Statement("except", lineno-3) + except BaseException: + await foo() # ASYNC102: 12, Statement("BaseException", lineno-1) + except: + await foo() + await foo() # ASYNC120: 8, Statement("except", lineno-8) + raise + + +# nested funcdef +async def foo_nested_funcdef(): + try: + ... + except ValueError: + + async def foobar(): + await foo() + + raise