-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
…previous python versions
There was a problem hiding this 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
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
Co-authored-by: Zac Hatfield-Dodds <[email protected]>
messy and imperfect, but should catch most usage and hopefully Ruff can implement this properly 🤞
TODO:
reraise_child_exception
is clean but "child exception" is not established nomenclature.reraise_exception_group_sub_exception
?bad_exception_group_flattening
?