Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial implementation of ASYNC123 #303

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Oct 18, 2024

messy and imperfect, but should catch most usage and hopefully Ruff can implement this properly 🤞

TODO:

  • changelog, bump version
  • write documentation
  • figure out a name. reraise_child_exception is clean but "child exception" is not established nomenclature. reraise_exception_group_sub_exception? bad_exception_group_flattening?

@jakkdl jakkdl requested a review from Zac-HD October 19, 2024 13:20
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation and tests look great, thanks @jakkdl!

I've suggested some changes to the docs which I think will give a bit more context on what happens and why it's bad, as well as recommending "don't do this at all" as our top suggestion. Merge when you're happy with it, with or without edits 🙂

docs/rules.rst Outdated
@@ -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 <https://github.com/python-trio/trio/issues/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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much better, thanks! Just one small nit on it

@jakkdl jakkdl linked an issue Oct 21, 2024 that may be closed by this pull request
Co-authored-by: Zac Hatfield-Dodds <[email protected]>
docs/rules.rst Outdated Show resolved Hide resolved
@Zac-HD Zac-HD merged commit 051204c into python-trio:main Oct 21, 2024
10 checks passed
@jakkdl jakkdl deleted the unwrap_exceptiongroup branch October 21, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New rule: raising ExceptionGroup child exception loses context/cause
2 participants