initial implementation of ASYNC123#303
Merged
Zac-HD merged 6 commits intopython-trio:mainfrom Oct 21, 2024
Merged
Conversation
…previous python versions
Zac-HD
approved these changes
Oct 20, 2024
Member
Zac-HD
left a comment
There was a problem hiding this comment.
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
| :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`. |
Member
There was a problem hiding this comment.
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. |
Member
Author
There was a problem hiding this comment.
That's much better, thanks! Just one small nit on it
Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
jakkdl
commented
Oct 21, 2024
Zac-HD
approved these changes
Oct 21, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
messy and imperfect, but should catch most usage and hopefully Ruff can implement this properly 🤞
TODO:
reraise_child_exceptionis clean but "child exception" is not established nomenclature.reraise_exception_group_sub_exception?bad_exception_group_flattening?