From ba564213ee5dc98502465327ffc2ad8385a98f3b Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 18 Oct 2024 16:40:41 +0200 Subject: [PATCH 1/6] initial implementation of ASYNC123 --- .pre-commit-config.yaml | 2 + flake8_async/visitors/__init__.py | 1 + flake8_async/visitors/visitor123.py | 110 ++++++++++++++++++++++++++ tests/eval_files/async123.py | 116 ++++++++++++++++++++++++++++ tests/test_flake8_async.py | 1 + 5 files changed, 230 insertions(+) create mode 100644 flake8_async/visitors/visitor123.py create mode 100644 tests/eval_files/async123.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fa1fbc5..9cff073 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -40,6 +40,8 @@ repos: rev: v1.11.2 hooks: - id: mypy + # uses py311 syntax, mypy configured for py39 + exclude: tests/eval_files/async123.py - repo: https://github.com/RobertCraigie/pyright-python rev: v1.1.384 diff --git a/flake8_async/visitors/__init__.py b/flake8_async/visitors/__init__.py index 0b05011..f1b6199 100644 --- a/flake8_async/visitors/__init__.py +++ b/flake8_async/visitors/__init__.py @@ -36,6 +36,7 @@ visitor105, visitor111, visitor118, + visitor123, visitor_utility, visitors, ) diff --git a/flake8_async/visitors/visitor123.py b/flake8_async/visitors/visitor123.py new file mode 100644 index 0000000..0fd99bb --- /dev/null +++ b/flake8_async/visitors/visitor123.py @@ -0,0 +1,110 @@ +"""foo.""" + +from __future__ import annotations + +import ast +from typing import TYPE_CHECKING, Any + +from .flake8asyncvisitor import Flake8AsyncVisitor +from .helpers import error_class + +if TYPE_CHECKING: + from collections.abc import Mapping + + +@error_class +class Visitor123(Flake8AsyncVisitor): + error_codes: Mapping[str, str] = { + "ASYNC123": ( + "Raising a child exception of an exception group loses" + " context, cause, and/or traceback of the exception inside the group." + ) + } + + def __init__(self, *args: Any, **kwargs: Any): + super().__init__(*args, **kwargs) + self.try_star = False + self.exception_group_names: set[str] = set() + self.child_exception_list_names: set[str] = set() + self.child_exception_names: set[str] = set() + + def _is_exception_group(self, node: ast.expr) -> bool: + return ( + (isinstance(node, ast.Name) and node.id in self.exception_group_names) + or ( + # a child exception might be an ExceptionGroup + self._is_child_exception(node) + ) + or ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and self._is_exception_group(node.func.value) + and node.func.attr in ("subgroup", "split") + ) + ) + + def _is_exception_list(self, node: ast.expr | None) -> bool: + return ( + isinstance(node, ast.Name) and node.id in self.child_exception_list_names + ) or ( + isinstance(node, ast.Attribute) + and node.attr == "exceptions" + and self._is_exception_group(node.value) + ) + + def _is_child_exception(self, node: ast.expr | None) -> bool: + return ( + isinstance(node, ast.Name) and node.id in self.child_exception_names + ) or (isinstance(node, ast.Subscript) and self._is_exception_list(node.value)) + + def visit_Raise(self, node: ast.Raise): + if self._is_child_exception(node.exc): + self.error(node) + + def visit_ExceptHandler(self, node: ast.ExceptHandler): + self.save_state( + node, + "exception_group_names", + "child_exception_list_names", + "child_exception_names", + copy=True, + ) + if node.name is None or ( + not self.try_star + and (node.type is None or "ExceptionGroup" not in ast.unparse(node.type)) + ): + self.novisit = True + return + self.exception_group_names = {node.name} + + # ast.TryStar added in py311 + def visit_TryStar(self, node: ast.TryStar): # type: ignore[name-defined] + self.save_state(node, "try_star", copy=False) + self.try_star = True + + def visit_Assign(self, node: ast.Assign | ast.AnnAssign): + if node.value is None or not self.exception_group_names: + return + targets = (node.target,) if isinstance(node, ast.AnnAssign) else node.targets + if self._is_child_exception(node.value): + for target in targets: + if isinstance(target, ast.Name): + self.child_exception_names.add(target.id) + elif self._is_exception_list(node.value): + if len(targets) == 1 and isinstance(targets[0], ast.Name): + self.child_exception_list_names.add(targets[0].id) + # unpacking tuples and Starred and shit. Not implemented + elif self._is_exception_group(node.value): + for target in targets: + if isinstance(target, ast.Name): + self.exception_group_names.add(target.id) + elif isinstance(target, ast.Tuple): + for t in target.elts: + if isinstance(t, ast.Name): + self.exception_group_names.add(t.id) + + visit_AnnAssign = visit_Assign + + def visit_For(self, node: ast.For): + if self._is_exception_list(node.iter) and isinstance(node.target, ast.Name): + self.child_exception_names.add(node.target.id) diff --git a/tests/eval_files/async123.py b/tests/eval_files/async123.py new file mode 100644 index 0000000..36c5208 --- /dev/null +++ b/tests/eval_files/async123.py @@ -0,0 +1,116 @@ +import copy +import sys +from typing import Any + +if sys.version_info < (3, 11): + from exceptiongroup import BaseExceptionGroup, ExceptionGroup + + +def condition() -> bool: + return True + + +def any_fun(arg: Exception) -> Exception: + return arg + + +try: + ... +except ExceptionGroup as e: + if condition(): + raise e.exceptions[0] # error: 8 + elif condition(): + raise copy.copy(e.exceptions[0]) # safe + elif condition(): + raise copy.deepcopy(e.exceptions[0]) # safe + else: + raise any_fun(e.exceptions[0]) # safe +try: + ... +except BaseExceptionGroup as e: + raise e.exceptions[0] # error: 4 +try: + ... +except ExceptionGroup as e: + my_e = e.exceptions[0] + raise my_e # error: 4 +try: + ... +except ExceptionGroup as e: + excs = e.exceptions + my_e = excs[0] + raise my_e # error: 4 +try: + ... +except ExceptionGroup as e: + excs_2 = e.subgroup(bool) + if excs_2: + raise excs_2.exceptions[0] # error: 8 +try: + ... +except ExceptionGroup as e: + excs_1, excs_2 = e.split(bool) + if excs_1: + raise excs_1.exceptions[0] # error: 8 + if excs_2: + raise excs_2.exceptions[0] # error: 8 + +try: + ... +except ExceptionGroup as e: + f = e + raise f.exceptions[0] # error: 4 +try: + ... +except ExceptionGroup as e: + excs = e.exceptions + excs2 = excs + raise excs2[0] # error: 4 +try: + ... +except ExceptionGroup as e: + my_exc = e.exceptions[0] + my_exc2 = my_exc + raise my_exc2 # error: 4 + +try: + ... +except* Exception as e: + raise e.exceptions[0] # error: 4 + +try: + ... +except ExceptionGroup as e: + raise e.exceptions[0].exceptions[0] # error: 4 +try: + ... +except ExceptionGroup as e: + excs = e.exceptions + for exc in excs: + if ...: + raise exc # error: 12 + raise +try: + ... +except ExceptionGroup as e: + ff: ExceptionGroup[Exception] = e + raise ff.exceptions[0] # error: 4 +try: + ... +except ExceptionGroup as e: + raise e.subgroup(bool).exceptions[0] # type: ignore # error: 4 + +# not implemented +try: + ... +except ExceptionGroup as e: + a, *b = e.exceptions + raise a + +# not implemented +try: + ... +except ExceptionGroup as e: + x: Any = object() + x.y = e + raise x.y.exceptions[0] diff --git a/tests/test_flake8_async.py b/tests/test_flake8_async.py index 5219f27..9676bbc 100644 --- a/tests/test_flake8_async.py +++ b/tests/test_flake8_async.py @@ -482,6 +482,7 @@ def _parse_eval_file( # doesn't check for it "ASYNC121", "ASYNC122", + "ASYNC123", "ASYNC300", "ASYNC912", } From 06874cc481cba00b73ef987d10b26795c5e54812 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 19 Oct 2024 15:19:04 +0200 Subject: [PATCH 2/6] add docs, version --- docs/changelog.rst | 4 ++++ docs/rules.rst | 3 +++ flake8_async/__init__.py | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 9a4e78f..d81bb8a 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,10 @@ Changelog `CalVer, YY.month.patch `_ +24.10.1 +======= +- Add :ref:`ASYNC123 ` bad-exception-group-flattening + 24.9.5 ====== - Fix crash when analyzing code with infinite loop inside context manager. diff --git a/docs/rules.rst b/docs/rules.rst index a181536..a4ca99f 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -89,6 +89,9 @@ _`ASYNC121`: control-flow-in-taskgroup _`ASYNC122`: delayed-entry-of-relative-cancelscope :func:`trio.move_on_after`, :func:`trio.fail_after`, :func:`anyio.move_on_after` and :func:`anyio.fail_after` behaves unintuitively if initialization and entry are separated, with the timeout starting on initialization. Trio>=0.27 changes this behaviour, so if you don't support older versions you should disable this check. See `Trio issue #2512 `_. +_`ASYNC123`: bad-exception-group-flattening + Raising an exception that was inside an `ExceptionGroup` inside the ``except`` handler of that group will set the group as the `__context__` for the exception, which overrides the previous `__context__` that exception had, truncating the context tree. The same is true for ``__cause__`` (unless explicitly setting ``from``) and `__traceback__`. The easiest fix is to `copy.copy` the exception before raising it, but beware that `trio.Cancelled` did not support copying before `trio==0.27.1`. + Blocking sync calls in async functions ====================================== diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 82fdf43..b9bbaf9 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.9.5" +__version__ = "24.10.1" # taken from https://github.com/Zac-HD/shed From 2e807d2b9cb32884fb3a4ac38082d61b0554061a Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 19 Oct 2024 15:37:32 +0200 Subject: [PATCH 3/6] fix code coverage --- flake8_async/visitors/visitor123.py | 6 ++--- tests/eval_files/async123.py | 38 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/flake8_async/visitors/visitor123.py b/flake8_async/visitors/visitor123.py index 0fd99bb..2db28c9 100644 --- a/flake8_async/visitors/visitor123.py +++ b/flake8_async/visitors/visitor123.py @@ -87,9 +87,9 @@ def visit_Assign(self, node: ast.Assign | ast.AnnAssign): return targets = (node.target,) if isinstance(node, ast.AnnAssign) else node.targets if self._is_child_exception(node.value): - for target in targets: - if isinstance(target, ast.Name): - self.child_exception_names.add(target.id) + # not normally possible to assign single exception to multiple targets + if len(targets) == 1 and isinstance(targets[0], ast.Name): + self.child_exception_names.add(targets[0].id) elif self._is_exception_list(node.value): if len(targets) == 1 and isinstance(targets[0], ast.Name): self.child_exception_list_names.add(targets[0].id) diff --git a/tests/eval_files/async123.py b/tests/eval_files/async123.py index 36c5208..967f78e 100644 --- a/tests/eval_files/async123.py +++ b/tests/eval_files/async123.py @@ -114,3 +114,41 @@ def any_fun(arg: Exception) -> Exception: x: Any = object() x.y = e raise x.y.exceptions[0] + +# coverage +try: + ... +except ExceptionGroup: + ... + +# not implemented +try: + ... +except ExceptionGroup as e: + (a, *b), (c, *d) = e.split(bool) + if condition(): + raise a + if condition(): + raise b[0] + if condition(): + raise c + if condition(): + raise d[0] + +# coverage (skip irrelevant assignments) +x = 0 + +# coverage (ignore multiple targets when assign target is child exception) +try: + ... +except ExceptionGroup as e: + exc = e.exceptions[0] + b, c = exc + if condition(): + raise b # not handled, and probably shouldn't raise + else: + raise c # same + +# coverage (skip irrelevant loop) +for x in range(5): + ... From 10c78ee498a2f96c88a8ca70098c1d4eb63be6c8 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Sat, 19 Oct 2024 16:29:09 +0200 Subject: [PATCH 4/6] split out py311+-specific async123 test code to not crash CI runs on previous python versions --- .pre-commit-config.yaml | 2 +- flake8_async/visitors/visitor123.py | 3 ++- tests/eval_files/async123.py | 5 ----- tests/eval_files/async123_py311.py | 4 ++++ 4 files changed, 7 insertions(+), 7 deletions(-) create mode 100644 tests/eval_files/async123_py311.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9cff073..b9fe3fb 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -41,7 +41,7 @@ repos: hooks: - id: mypy # uses py311 syntax, mypy configured for py39 - exclude: tests/eval_files/async123.py + exclude: tests/eval_files/.*_py311.py - repo: https://github.com/RobertCraigie/pyright-python rev: v1.1.384 diff --git a/flake8_async/visitors/visitor123.py b/flake8_async/visitors/visitor123.py index 2db28c9..58237c1 100644 --- a/flake8_async/visitors/visitor123.py +++ b/flake8_async/visitors/visitor123.py @@ -78,7 +78,8 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler): self.exception_group_names = {node.name} # ast.TryStar added in py311 - def visit_TryStar(self, node: ast.TryStar): # type: ignore[name-defined] + # we run strict codecov on all python versions, this one doesn't run on Exception: my_exc2 = my_exc raise my_exc2 # error: 4 -try: - ... -except* Exception as e: - raise e.exceptions[0] # error: 4 - try: ... except ExceptionGroup as e: diff --git a/tests/eval_files/async123_py311.py b/tests/eval_files/async123_py311.py new file mode 100644 index 0000000..4410dab --- /dev/null +++ b/tests/eval_files/async123_py311.py @@ -0,0 +1,4 @@ +try: + ... +except* Exception as e: + raise e.exceptions[0] # error: 4 From 1a35daebdcd7f9ba52d52a21388887223d042f9e Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Mon, 21 Oct 2024 13:29:17 +0200 Subject: [PATCH 5/6] Update docs/rules.rst Co-authored-by: Zac Hatfield-Dodds --- docs/rules.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/rules.rst b/docs/rules.rst index a4ca99f..413f5b8 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -90,7 +90,9 @@ _`ASYNC122`: delayed-entry-of-relative-cancelscope :func:`trio.move_on_after`, :func:`trio.fail_after`, :func:`anyio.move_on_after` and :func:`anyio.fail_after` behaves unintuitively if initialization and entry are separated, with the timeout starting on initialization. Trio>=0.27 changes this behaviour, so if you don't support older versions you should disable this check. See `Trio issue #2512 `_. _`ASYNC123`: bad-exception-group-flattening - Raising an exception that was inside an `ExceptionGroup` inside the ``except`` handler of that group will set the group as the `__context__` for the exception, which overrides the previous `__context__` that exception had, truncating the context tree. The same is true for ``__cause__`` (unless explicitly setting ``from``) and `__traceback__`. The easiest fix is to `copy.copy` the exception before raising it, but beware that `trio.Cancelled` did not support copying before `trio==0.27.1`. + Raising one of the contained exceptions will mutate it, replacing the original ``.__context__`` with the group, and erasing the ``.__traceback__``. + Dropping this information makes diagnosing errors much more difficult. + We recommend ``raise SomeNewError(...) from group`` if possible; or consider using `copy.copy` to shallow-copy the exception before re-raising (for copyable types), or re-raising the error from outside the `except` block. Blocking sync calls in async functions ====================================== From 1dbae0de7bea4e85f414d1beca00efb85168cd35 Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Mon, 21 Oct 2024 13:30:15 +0200 Subject: [PATCH 6/6] Update docs/rules.rst --- docs/rules.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules.rst b/docs/rules.rst index 413f5b8..c3c8edb 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -90,7 +90,7 @@ _`ASYNC122`: delayed-entry-of-relative-cancelscope :func:`trio.move_on_after`, :func:`trio.fail_after`, :func:`anyio.move_on_after` and :func:`anyio.fail_after` behaves unintuitively if initialization and entry are separated, with the timeout starting on initialization. Trio>=0.27 changes this behaviour, so if you don't support older versions you should disable this check. See `Trio issue #2512 `_. _`ASYNC123`: bad-exception-group-flattening - Raising one of the contained exceptions will mutate it, replacing the original ``.__context__`` with the group, and erasing the ``.__traceback__``. + Raising one of the exceptions contained in an exception group will mutate it, replacing the original ``.__context__`` with the group, and erasing the ``.__traceback__``. Dropping this information makes diagnosing errors much more difficult. We recommend ``raise SomeNewError(...) from group`` if possible; or consider using `copy.copy` to shallow-copy the exception before re-raising (for copyable types), or re-raising the error from outside the `except` block.