New rule: raising ExceptionGroup child exception loses context/cause

def raise_error():
    try:
        1/0
    finally:
        raise ValueError

def wrap_error():
    try:
        raise_error()
    except Exception as e:
        raise ExceptionGroup("aoeu", [e])

def unwrap_error():
    try:
        wrap_error()
    except ExceptionGroup as exc_group:
        if ...:
            # Oops: we now lost the DivisionByZeroError as cause/context
            raise exc_group.exceptions[0] from exc_group
        if ...:
            # could catch this with type tracking
            e = exc_group.exceptions[0]
            raise e from None
        if ...:
            # this is fine
            raise NewError from exc_group

I realized I've done this in several PRs (at least the one in trio-websocket), and nobody has commented on the fact. It could also be a better fit for flake8-bugbear (although they don't have any type-tracking infrastructure).
I haven't yet figured out the best way of avoiding it though, options include:

def unwrap_error2():
    my_exc = None
    try:
        wrap_error()
    except ExceptionGroup as exc_group:
        if ...:
            my_exc = exc_group.exceptions[0]
    # child exception cause/context is fully preserved, but you lose all info about the
    # group. This might be good or bad depending on the situation
    if my_exc is not None:
        raise my_exc

def unwrap_error_3():
    try:
        wrap_error()
    except ExceptionGroup as exc_group:
        # this seems sketchy??
        import copy
        my_exc = copy.copy(exc_group.exceptions[0])
        raise my_exc from exc_group

also see HypothesisWorks/hypothesis#4115 for context

after discussion in gitter and thinking about it we can likely suggest copy.copy() as a robust solution. Even if users have complex data structures on their exceptions they probably won't manipulate them on several levels of the context chain.

oooh, yeah, copy.copy() sounds like a good solution.

I'd be happy to have a lint rule for this, it seems like a common mistake in a rare-ish situation and I'd definitely prefer to get the full context when it happens.

edit: actually I think you don't even need from ... to trigger this, so it's not even that rare - but pretty much the most obvious way that someone would try to unwrap a group!

~~ooooh, I just realized I could check for this in trio.testing.RaisesGroup! Perhaps unconventional, but that would have a wide reach and would be more robust than trying to lint for it.~~ nvm where this is actually needed is in pytest.raises since the vast majority of the time you will only ever encounter this when expecting a single unwrapped exception.

hm, I guess you can't universally copy.copy exceptions

    |   File "/home/h/Git/trio-websocket/exc_group_cause_context/trio_websocket/_impl.py", line 226, in open_websocket
    |     raise copy.copy(user_error) from user_error.__cause__
    |           ^^^^^^^^^^^^^^^^^^^^^
    |   File "/usr/lib/python3.12/copy.py", line 97, in copy
    |     return _reconstruct(x, None, *rv)
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |   File "/usr/lib/python3.12/copy.py", line 253, in _reconstruct
    |     y = func(*args)
    |         ^^^^^^^^^^^
    |   File "/home/h/Git/trio-websocket/.venv/lib/python3.12/site-packages/trio/_util.py", line 374, in __call__
    |     raise TypeError(
    | TypeError: trio.Cancelled has no public constructor

Note that copy.copying an exception doesn't copy most of its metadata. You'll lose the traceback, context, etc. All you get is the type, args (which is usually just a message) and dict (which includes notes, but is otherwise empty unless you put something there). So I'm not sure if it's wise to suggest this idiom?

ah, I think in most cases it doesn't matter that we copy none of the metadata - since those will be set on the new exception object as we raise it. Writing docs/suggestion on this will be a major pain though.

It appears it's not just trio.Cancelled that causes trouble for copy.copy.

    |   File "/home/h/Git/trio-websocket/exc_group_cause_context/trio_websocket/_impl.py", line 101, in copy_exc
    |     return copy.copy(e)
    |            ^^^^^^^^^^^^
    |   File "/usr/lib/python3.12/copy.py", line 97, in copy
    |     return _reconstruct(x, None, *rv)
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |   File "/usr/lib/python3.12/copy.py", line 253, in _reconstruct
    |     y = func(*args)
    |         ^^^^^^^^^^^
    | TypeError: ConnectionRejected.__init__() missing 3 required positional arguments: 'status_code', 'headers', and 'body'

I've narrowed it down to this minimal repro:

import copy

class MyExc(Exception):
    def __init__(self, x):
        super().__init__()

a = MyExc(5)
copy.copy(a)

I have no clue why copy.copy fails to pick up the args if you define an __init__ which calls super().__init__()

Regardless, it seems like copy.copy is not viable as a suggestion. My current workaround is

def copy_exc(e):
    cls = type(e)
    result = cls.__new__(cls)
    result.__dict__ = copy.copy(e.__dict__)
    return result

which seems to handle everything I've thrown at it so far.

That'll misbehave for any class where the __init__ method does more than just assign values to attributes, unfortunately.

That'll misbehave for any class where the __init__ method does more than just assign values to attributes, unfortunately.

do people do that in exception classes? And if they do, do we want to re-execute whatever code is in the __init__? I guess there's no universal answer... which sucks for libraries. Reraising outside the except also gets real tricky once you have a finally (that could raise). But I'll give that another go in trio-websocket I suppose.

I opened python/cpython#125782 to see if upstream wants to address copy.copy+BaseException.__init__

I have no clue why copy.copy fails to pick up the args if you define an __init__ which calls super().__init__()

In your example, you aren't passing the args to parent __init__, so the original exception object doesn't have them either!

>>> class MyExc(Exception):
...     def __init__(self, x):
...         super().__init__()
...
>>> a = MyExc(5)
>>> a
MyExc()

If you fix that, they do get copied.

>>> class MyExc(Exception):
...     def __init__(self, x):
...         super().__init__(x)
...
>>> a = MyExc(5)
>>> import copy
>>> copy.copy(a)
MyExc(5)

Regardless, it seems like copy.copy is not viable as a suggestion. My current workaround is [...] which seems to handle everything I've thrown at it so far.

I don't think you looked at the results very carefully...

>>> def copy_exc(e):
...     cls = type(e)
...     result = cls.__new__(cls)
...     result.__dict__ = copy.copy(e.__dict__)
...     return result
...
>>> copy_exc(RuntimeError("hello world"))
RuntimeError()

Exception objects store their state in three places:

slots for the most of the "dunders": traceback, cause, context, suppress_context
an args tuple, which is just whatever you passed to BaseException.__init__
a dictionary, which gets any attributes you assign later, as well as the __notes__ since it doesn't have a dedicated slot

copy.copy() copies the args and dictionary but not the dunder slots. Your copy_exc only copies the dictionary, which is strictly less good. Most of the problems discussed in this thread are from not copying the dunder slots. I don't think there's any brief way to copy them (the non-brief way is by making four setattr calls).

I have no clue why copy.copy fails to pick up the args if you define an __init__ which calls super().__init__()

In your example, you aren't passing the args to parent __init__, so the original exception object doesn't have them either!

Yeah that was intentional, though I perhaps went too far in minifying the repro only to cause confusion. The original code in trio-websocket is https://github.com/python-trio/trio-websocket/blob/f5fd6d77db16a7b527d670c4045fa1d53e621c35/trio_websocket/_impl.py#L587

This also displays a further thorn if trying to write a linter for it, because it doesn't directly inherit from [Base]Exception. If the cpython issue does end up as docs-only I'll likely give a best-effort rule in flake8-bugbear a shot anyway.

copy.copy() copies the args and dictionary but not the dunder slots. Your copy_exc only copies the dictionary, which is strictly less good. Most of the problems discussed in this thread are from not copying the dunder slots. I don't think there's any brief way to copy them (the non-brief way is by making four setattr calls).

we don't need to copy the dunder slots for this usage, because they will all be set on the copy when we raise it. But yeah it appears my testing was overly brief.

Thank you for your thorough research! ❤️

Navigation Menu

Repository navigation

New rule: raising ExceptionGroup child exception loses context/cause #298

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

New rule: raising ExceptionGroup child exception loses context/cause #298

Description

Activity

jakkdl commented on Oct 2, 2024

Zac-HD commented on Oct 3, 2024

jakkdl commented on Oct 9, 2024

jakkdl commented on Oct 9, 2024

oremanj commented on Oct 9, 2024

jakkdl commented on Oct 9, 2024

jakkdl commented on Oct 20, 2024

Zac-HD commented on Oct 20, 2024

jakkdl commented on Oct 21, 2024

oremanj commented on Oct 21, 2024

jakkdl commented on Oct 21, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions