initial implementation of ASYNC123 (#303)

* initial implementation of ASYNC123

* add docs, version

* fix code coverage

* split out py311+-specific async123 test code to not crash CI runs on previous python versions

* Update docs/rules.rst

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

* Update docs/rules.rst

---------

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

.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/.*_py311.py
 
     - repo: https://github.com/RobertCraigie/pyright-python
       rev: v1.1.384

docs/changelog.rst

@@ -4,6 +4,10 @@ Changelog
 
 `CalVer, YY.month.patch <https://calver.org/>`_
 
+24.10.1
+=======
+- Add :ref:`ASYNC123 <async123>` bad-exception-group-flattening
+
 24.9.5
 ======
 - Fix crash when analyzing code with infinite loop inside context manager.

docs/rules.rst

@@ -89,6 +89,11 @@ _`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 <https://github.com/python-trio/trio/issues/2512>`_.
 
+_`ASYNC123`: bad-exception-group-flattening
+    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.
+
 Blocking sync calls in async functions
 ======================================
 

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

flake8_async/visitors/__init__.py

@@ -36,6 +36,7 @@
     visitor105,
     visitor111,
     visitor118,
+    visitor123,
     visitor_utility,
     visitors,
 )

flake8_async/visitors/visitor123.py

@@ -0,0 +1,111 @@
+"""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
+    # we run strict codecov on all python versions, this one doesn't run on <py311
+    def visit_TryStar(self, node: ast.TryStar):  # type: ignore[name-defined]  # pragma: no cover
+        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):
+            # 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)
+            # 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)

tests/eval_files/async123.py

@@ -0,0 +1,149 @@
+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 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]
+
+# 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):
+    ...

tests/eval_files/async123_py311.py

@@ -0,0 +1,4 @@
+try:
+    ...
+except* Exception as e:
+    raise e.exceptions[0]  # error: 4

tests/test_flake8_async.py

@@ -482,6 +482,7 @@ def _parse_eval_file(
     # doesn't check for it
     "ASYNC121",
     "ASYNC122",
+    "ASYNC123",
     "ASYNC300",
     "ASYNC912",
 }