From 04ea64cfe46dcaf794028cb18951792c04d65b20 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 29 May 2024 16:18:22 +0200 Subject: [PATCH 1/8] add async120, await-in-except --- docs/changelog.rst | 4 ++++ docs/rules.rst | 10 +++++++++- flake8_async/__init__.py | 2 +- flake8_async/visitors/visitor102.py | 24 +++++++++++++++++------- tests/eval_files/async102.py | 17 +++++++++-------- 5 files changed, 40 insertions(+), 17 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2d19510..b4a5ea6 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,10 @@ Changelog *[CalVer, YY.month.patch](https://calver.org/)* +24.5.7 +====== +- Add :ref:`ASYNC120 ` await-in-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..a05fab2 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 + ``await`` inside ``except`` must have shielded :ref:`cancel scope ` with timeout. + If not, the async call might get cancelled, suppressing the exception that was caught. + 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..b01fd88 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.5.7" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 52177dd..62cacef 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": ( + "await inside {0.name} on line {0.lineno} must have shielded cancel " + "scope with a timeout." + ), } class TrioScope: @@ -60,7 +64,11 @@ 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) + if self._critical_scope.name == "except": + error_code = "ASYNC120" + else: + error_code = "ASYNC102" + self.error(node, self._critical_scope, error_code=error_code) def is_safe_aclose_call(self, node: ast.Await) -> bool: return ( @@ -122,16 +130,18 @@ 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._trio_context_managers = [] - self._critical_scope = res + + 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] diff --git a/tests/eval_files/async102.py b/tests/eval_files/async102.py index b6b7278..7645219 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,11 +171,11 @@ async def foo4(): try: ... except ValueError: - await foo() # safe + await foo() # ASYNC120: 8, Statement("except", lineno-1) except BaseException: await foo() # error: 8, Statement("BaseException", lineno-1) except: - await foo() + await foo() # ASYNC120: 8, Statement("except", lineno-1) # safe, since BaseException will catch Cancelled # also fully redundant and will never be executed, but that's for another linter @@ -186,7 +187,7 @@ async def foo5(): with trio.CancelScope(deadline=30, shield=True): await foo() # safe except: - await foo() + await foo() # ASYNC120: 8, Statement("except", lineno-1) try: ... @@ -237,20 +238,20 @@ 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) await foo() # error: 8, Statement("BaseException", lineno-9) except: - await foo() + await foo() # ASYNC120: 8, Statement("except", lineno-1) try: - await foo() + await foo() # ASYNC120: 12, Statement("except", lineno-3) except BaseException: await foo() # error: 12, Statement("BaseException", lineno-1) except: - await foo() - await foo() + await foo() # ASYNC120: 12, Statement("except", lineno-1) + await foo() # ASYNC120: 8, Statement("except", lineno-8) await foo() From c9398a7b60ec8b9414343320daf628e3b390afef Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 29 May 2024 16:23:20 +0200 Subject: [PATCH 2/8] add link to async120 --- docs/rules.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules.rst b/docs/rules.rst index a05fab2..ecc2b05 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -75,7 +75,7 @@ _`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 +_`ASYNC120` : await-in-except ``await`` inside ``except`` must have shielded :ref:`cancel scope ` with timeout. If not, the async call might get cancelled, suppressing the exception that was caught. This will not trigger when :ref:`ASYNC102 ` does, and if you don't care about losing non-cancelled exceptions you could disable this rule. From 0778402f00f90c1585d02ab33ffb168f0144f960 Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Mon, 3 Jun 2024 15:23:01 +0200 Subject: [PATCH 3/8] Apply suggestions from code review Co-authored-by: Zac Hatfield-Dodds --- docs/rules.rst | 4 ++-- flake8_async/visitors/visitor102.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/rules.rst b/docs/rules.rst index ecc2b05..77ab88a 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -76,8 +76,8 @@ _`ASYNC119` : yield-in-cm-in-async-gen 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 - ``await`` inside ``except`` must have shielded :ref:`cancel scope ` with timeout. - If not, the async call might get cancelled, suppressing the exception that was caught. + 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. diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 62cacef..1484aed 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -24,8 +24,8 @@ class Visitor102(Flake8AsyncVisitor): "scope with a timeout." ), "ASYNC120": ( - "await inside {0.name} on line {0.lineno} must have shielded cancel " - "scope with a timeout." + "checkpoint inside {0.name} on line {0.lineno} will discard the active " + "exception if cancelled." ), } From b4656cba1067e617434d302f30ecb008efe62012 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 3 Jun 2024 13:24:51 +0000 Subject: [PATCH 4/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- docs/rules.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules.rst b/docs/rules.rst index 77ab88a..1b9b95b 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -77,7 +77,7 @@ _`ASYNC119` : yield-in-cm-in-async-gen _`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. + 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. From e18121a4408fcf3f62e2fe5d80eb9c9df8339c74 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 3 Jun 2024 15:36:47 +0200 Subject: [PATCH 5/8] add clarifying comment, enable 120 in async102_aclose --- flake8_async/visitors/visitor102.py | 1 + tests/eval_files/async102_aclose.py | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 1484aed..3546960 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -64,6 +64,7 @@ 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 ): + # non-critical exception handlers have the statement name set to "except" if self._critical_scope.name == "except": error_code = "ASYNC120" else: 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(). From b2a285866ac6b8bdef8211911d939b553e334786 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 3 Jun 2024 17:02:20 +0200 Subject: [PATCH 6/8] async120 now only triggers if excepthandler would raise. Fix false alarm on nested function definitions for 102 & 120 --- docs/changelog.rst | 1 + flake8_async/visitors/visitor102.py | 43 +++++++++- tests/eval_files/async102.py | 31 +++++-- tests/eval_files/async120.py | 123 ++++++++++++++++++++++++++++ 4 files changed, 187 insertions(+), 11 deletions(-) create mode 100644 tests/eval_files/async120.py diff --git a/docs/changelog.rst b/docs/changelog.rst index b4a5ea6..e6510fc 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -7,6 +7,7 @@ Changelog 24.5.7 ====== - Add :ref:`ASYNC120 ` await-in-except. +- Fix false alarm with :ref:`ASYNC102 ` with function definitions inside finally/except. 24.5.6 ====== diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 3546960..56cb1df 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -56,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( @@ -66,10 +75,14 @@ def async_call_checker( ): # non-critical exception handlers have the statement name set to "except" if self._critical_scope.name == "except": - error_code = "ASYNC120" + self._potential_120.append((node, self._critical_scope)) else: - error_code = "ASYNC102" - self.error(node, self._critical_scope, error_code=error_code) + 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 ( @@ -135,8 +148,11 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler): if self._critical_scope is not None and self._critical_scope.name != "except": return - self.save_state(node, "_critical_scope", "_trio_context_managers") + self.save_state( + node, "_critical_scope", "_trio_context_managers", "_potential_120" + ) self._trio_context_managers = [] + self._potential_120 = [] if self.cancelled_caught or (res := critical_except(node)) is None: self._critical_scope = Statement("except", node.lineno, node.col_offset) @@ -158,3 +174,22 @@ 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 + visit_Lambda = visit_FunctionDef diff --git a/tests/eval_files/async102.py b/tests/eval_files/async102.py index 7645219..0abb246 100644 --- a/tests/eval_files/async102.py +++ b/tests/eval_files/async102.py @@ -171,11 +171,11 @@ async def foo4(): try: ... except ValueError: - await foo() # ASYNC120: 8, Statement("except", lineno-1) + await foo() except BaseException: await foo() # error: 8, Statement("BaseException", lineno-1) except: - await foo() # ASYNC120: 8, Statement("except", lineno-1) + await foo() # safe, since BaseException will catch Cancelled # also fully redundant and will never be executed, but that's for another linter @@ -187,7 +187,7 @@ async def foo5(): with trio.CancelScope(deadline=30, shield=True): await foo() # safe except: - await foo() # ASYNC120: 8, Statement("except", lineno-1) + await foo() try: ... @@ -244,14 +244,14 @@ async def foo_nested_excepts(): await foo() # error: 12, Statement("BaseException", lineno-8) await foo() # error: 8, Statement("BaseException", lineno-9) except: - await foo() # ASYNC120: 8, Statement("except", lineno-1) + await foo() try: - await foo() # ASYNC120: 12, Statement("except", lineno-3) + await foo() except BaseException: await foo() # error: 12, Statement("BaseException", lineno-1) except: - await foo() # ASYNC120: 12, Statement("except", lineno-1) - await foo() # ASYNC120: 8, Statement("except", lineno-8) + await foo() + await foo() await foo() @@ -266,3 +266,20 @@ 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() + + +async def foo_nested_funcdef(): + try: + ... + except: + x = lambda: await foo() 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 From 3c69f2bc4e30f1fcea4ec02eb97ab4a22329895e Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Wed, 12 Jun 2024 11:04:35 +0200 Subject: [PATCH 7/8] Update flake8_async/__init__.py Co-authored-by: Zac Hatfield-Dodds --- flake8_async/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index b01fd88..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.7" +__version__ = "24.6.1" # taken from https://github.com/Zac-HD/shed From 73d21c359c9d035b85b5ec641dcae9d22a77f600 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 12 Jun 2024 11:17:09 +0200 Subject: [PATCH 8/8] bump __version__ in __init__, remove lambda handling --- docs/changelog.rst | 2 +- flake8_async/visitors/visitor102.py | 4 +++- tests/eval_files/async102.py | 7 ------- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index e6510fc..f70171e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,7 +4,7 @@ Changelog *[CalVer, YY.month.patch](https://calver.org/)* -24.5.7 +24.6.1 ====== - Add :ref:`ASYNC120 ` await-in-except. - Fix false alarm with :ref:`ASYNC102 ` with function definitions inside finally/except. diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index 56cb1df..be69bec 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -192,4 +192,6 @@ def visit_FunctionDef( self._potential_120 = [] visit_AsyncFunctionDef = visit_FunctionDef - visit_Lambda = 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 0abb246..fba16e4 100644 --- a/tests/eval_files/async102.py +++ b/tests/eval_files/async102.py @@ -276,10 +276,3 @@ async def foo_nested_funcdef(): async def foobar(): await foo() - - -async def foo_nested_funcdef(): - try: - ... - except: - x = lambda: await foo()