Warn on ExceptionGroup subclasses without a .derive() method (#448)

* Add ASYNC126 exceptiongroup-subclass-missing-derive

Warn when a class inherits from `ExceptionGroup` / `BaseExceptionGroup`
but doesn't override `derive`. Without that override, `split`/`subgroup`
(as used by nursery and TaskGroup implementations) silently produce
plain `ExceptionGroup` instances instead of the custom subclass.

Fixes #334.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Exempt aclose_forcefully from ASYNC102

`trio.aclose_forcefully` (and `anyio.aclose_forcefully`) are designed
specifically for cleanup and cancel immediately by design, so calling
them in a `finally:` / critical `except:` does not introduce the
cancellation-replacing hazard that ASYNC102 targets. Treat them as safe,
same as `.aclose()` with no arguments.

Fixes #446.

* Fix ASYNC126 eval file after auto-formatter wrapping

The pre-commit ruff/black formatter wrapped some `class Foo(Bar):` lines
across multiple lines, which moved the trailing `# error:` annotation off
the line where the visitor actually emits the error (the `class` keyword
line). Use shorter class names so the formatter leaves them on a single
line.

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: 3 people

docs/changelog.rst

@@ -9,6 +9,8 @@ Unreleased
 - Autofix for :ref:`ASYNC910 <async910>` / :ref:`ASYNC911 <async911>` no longer inserts checkpoints inside ``except`` clauses (which would trigger :ref:`ASYNC120 <async120>`); instead the checkpoint is added at the top of the function or of the enclosing loop. `(issue #403) <https://github.com/python-trio/flake8-async/issues/403>`_
 - :ref:`ASYNC910 <async910>` and :ref:`ASYNC911 <async911>` now accept ``__aenter__`` / ``__aexit__`` methods when the partner method provides the checkpoint, or when only one of the two is defined on a class that inherits from another class (charitably assuming the partner is inherited and contains a checkpoint). `(issue #441) <https://github.com/python-trio/flake8-async/issues/441>`_
 - :ref:`ASYNC300 <async300>` no longer triggers when the result of ``asyncio.create_task()`` is returned from a function. `(issue #398) <https://github.com/python-trio/flake8-async/issues/398>`_
+- Add :ref:`ASYNC126 <async126>` exceptiongroup-subclass-missing-derive. `(issue #334) <https://github.com/python-trio/flake8-async/issues/334>`_
+- :ref:`ASYNC102 <async102>` no longer warns on ``await trio.aclose_forcefully(...)`` / ``await anyio.aclose_forcefully(...)``, which are designed for cleanup and cancel immediately by design. `(issue #446) <https://github.com/python-trio/flake8-async/issues/446>`_
 
 25.7.1
 ======

docs/rules.rst

@@ -25,6 +25,7 @@ _`ASYNC102` : await-in-finally-or-cancelled
     ``await`` inside ``finally``, :ref:`cancelled-catching <cancelled>` ``except:``, or ``__aexit__`` must have shielded :ref:`cancel scope <cancel_scope>` with timeout.
     If not, the async call will immediately raise a new cancellation, suppressing any cancellation that was caught.
     Not applicable to asyncio due to edge-based cancellation semantics it uses as opposed to level-based used by trio and anyio.
+    Calls to ``.aclose()`` (with no arguments) and to :func:`trio.aclose_forcefully` / :func:`anyio.aclose_forcefully` are exempt, as they are intended for use in cleanup.
     See :ref:`ASYNC120 <async120>` for the general case where other exceptions might get suppressed.
 
 ASYNC103 : no-reraise-cancelled
@@ -123,6 +124,14 @@ _`ASYNC125`: constant-absolute-deadline
     :func:`anyio.move_on_after`, or the ``relative_deadline`` parameter to
     :class:`trio.CancelScope`.
 
+_`ASYNC126`: exceptiongroup-subclass-missing-derive
+    A subclass of :class:`ExceptionGroup` or :class:`BaseExceptionGroup` must override
+    :meth:`~BaseExceptionGroup.derive` to return an instance of itself, otherwise
+    :meth:`~BaseExceptionGroup.split` and :meth:`~BaseExceptionGroup.subgroup` - which
+    are used by e.g. :ref:`taskgroup_nursery` implementations - will silently produce
+    plain ``ExceptionGroup`` instances and lose the custom subclass.
+    See `trio#3175 <https://github.com/python-trio/trio/issues/3175>`_ for motivation.
+
 Blocking sync calls in async functions
 ======================================
 

flake8_async/visitors/visitor102_120.py

@@ -84,17 +84,22 @@ def visit_Raise(self, node: ast.Raise):
         self._potential_120.clear()
 
     def is_safe_aclose_call(self, node: ast.Await) -> bool:
-        return (
-            isinstance(node.value, ast.Call)
-            # only known safe if no arguments
+        if not isinstance(node.value, ast.Call):
+            return False
+        # allow `<x>.aclose()` with no arguments
+        if (
+            isinstance(node.value.func, ast.Attribute)
+            and node.value.func.attr == "aclose"
             and not node.value.args
             and not node.value.keywords
-            and isinstance(node.value.func, ast.Attribute)
-            and node.value.func.attr == "aclose"
-        )
+        ):
+            return True
+        # allow `trio.aclose_forcefully(<x>)` / `anyio.aclose_forcefully(<x>)`,
+        # which are specifically designed for cleanup and cancel immediately by design
+        return get_matching_call(node.value, "aclose_forcefully") is not None
 
     def visit_Await(self, node: ast.Await):
-        # allow calls to `.aclose()`
+        # allow calls to `.aclose()` and `[trio/anyio].aclose_forcefully(...)`
         if not (self.is_safe_aclose_call(node)):
             self.async_call_checker(node)
 

flake8_async/visitors/visitors.py

@@ -532,6 +532,40 @@ def is_constant(value: ast.expr) -> bool:
             )
 
 
+@error_class
+class Visitor126(Flake8AsyncVisitor):
+    error_codes: Mapping[str, str] = {
+        "ASYNC126": (
+            "ExceptionGroup subclass {} should override `derive`, otherwise"
+            " `split`/`subgroup` (used by e.g. nursery/TaskGroup"
+            " implementations) will silently produce plain `ExceptionGroup`"
+            " instances instead of `{}`."
+        )
+    }
+
+    def visit_ClassDef(self, node: ast.ClassDef):
+        def base_name(base: ast.expr) -> str:
+            # strip generic subscripts like `ExceptionGroup[Foo]`
+            if isinstance(base, ast.Subscript):
+                base = base.value
+            unparsed = ast.unparse(base)
+            return unparsed.rsplit(".", 1)[-1]
+
+        if not any(
+            base_name(b) in ("ExceptionGroup", "BaseExceptionGroup") for b in node.bases
+        ):
+            return
+
+        for item in node.body:
+            if (
+                isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef))
+                and item.name == "derive"
+            ):
+                return
+
+        self.error(node, node.name, node.name)
+
+
 @error_class_cst
 class Visitor300(Flake8AsyncVisitor_cst):
     error_codes: Mapping[str, str] = {

tests/eval_files/async102.py

@@ -345,3 +345,22 @@ async def foo():
         await x.aclose(bar=foo)  # ASYNC102: 8, Statement("try/finally", lineno-9)
         await x.aclose(*foo)  # ASYNC102: 8, Statement("try/finally", lineno-10)
         await x.aclose(None)  # ASYNC102: 8, Statement("try/finally", lineno-11)
+
+
+# aclose_forcefully is designed for cleanup and is safe in finally/except
+# see https://github.com/python-trio/flake8-async/issues/446
+async def foo_aclose_forcefully():
+    x = None
+
+    try:
+        ...
+    except BaseException:
+        await trio.aclose_forcefully(x)
+    finally:
+        await trio.aclose_forcefully(x)
+
+    # unqualified or unknown-base call is still treated as unsafe
+    try:
+        ...
+    finally:
+        await aclose_forcefully(x)  # ASYNC102: 8, Statement("try/finally", lineno-3)

tests/eval_files/async126.py

@@ -0,0 +1,67 @@
+import sys
+
+if sys.version_info < (3, 11):
+    from exceptiongroup import BaseExceptionGroup, ExceptionGroup
+
+
+class NoDerive(ExceptionGroup):  # error: 0, "NoDerive", "NoDerive"
+    pass
+
+
+class NoDeriveBE(BaseExceptionGroup):  # error: 0, "NoDeriveBE", "NoDeriveBE"
+    pass
+
+
+class NoDeriveG(ExceptionGroup[Exception]):  # error: 0, "NoDeriveG", "NoDeriveG"
+    pass
+
+
+import exceptiongroup as eg
+
+
+class NoDeriveQ(eg.ExceptionGroup):  # error: 0, "NoDeriveQ", "NoDeriveQ"
+    pass
+
+
+class _Mixin: ...
+
+
+class MultiBase(_Mixin, ExceptionGroup):  # error: 0, "MultiBase", "MultiBase"
+    pass
+
+
+# safe - overrides derive
+class HasDerive(ExceptionGroup):
+    def derive(self, excs):
+        return HasDerive(self.message, excs)
+
+
+class HasDeriveBE(BaseExceptionGroup):
+    def derive(self, excs):
+        return HasDeriveBE(self.message, excs)
+
+
+class HasDeriveG(ExceptionGroup[Exception]):
+    def derive(self, excs):
+        return HasDeriveG(self.message, excs)
+
+
+# async derive is weird but counts
+class AsyncDer(ExceptionGroup):
+    async def derive(self, excs):  # type: ignore
+        return AsyncDer(self.message, excs)
+
+
+# not an ExceptionGroup subclass
+class NotAnEG(Exception):
+    pass
+
+
+# nested class
+class Outer:
+    class InnerNo(ExceptionGroup):  # error: 4, "InnerNo", "InnerNo"
+        pass
+
+    class InnerHas(ExceptionGroup):
+        def derive(self, excs):
+            return Outer.InnerHas(self.message, excs)

tests/test_flake8_async.py

@@ -521,6 +521,7 @@ def _parse_eval_file(
     "ASYNC122",
     "ASYNC123",
     "ASYNC125",
+    "ASYNC126",
     "ASYNC300",
     "ASYNC400",
     "ASYNC912",