gh-141732: Fix ExceptionGroup repr changing when original exception sequence is mutated

[dr-carlos]

• BaseExceptionGroup.__repr__ now overrides BaseException.__repr__, using the group's exception tuple instead of the mutable argument passed in.
• ComplexExtendsException (a private macro in Objects/exceptions.c) now has a parameter to modify the exception's __repr__ and not just its __str__.

• Issue: ExceptionGroup repr is misleading if original exception sequence is mutated #141732

---

📚 Documentation preview 📚: https://cpython-previews--141736.org.readthedocs.build/

[iritkatriel]

Thanks.

We don't make user-visible changes to builtin types without a PEP, so this PR cannot be merged as is.

We could document current behaviour (i.e., don't mutate the list) if it's not clear from the docs and then consider if we want to change it.

If we do want to change it, maybe it should be to make a shallow copy of the list rather than make it a tuple (so the repr/str won't change and break existing code that relies on current behaviour).

[dr-carlos]

Thanks.

We don't make user-visible changes to builtin types without a PEP, so this PR cannot be merged as is.

We could document current behaviour (i.e., don't mutate the list) if it's not clear from the docs and then consider if we want to change it.

If we do want to change it, maybe it should be to make a shallow copy of the list rather than make it a tuple (so the repr/str won't change and break existing code that relies on current behaviour).

Thanks for the quick response! That all makes sense.

Just to clarify - is c87b66b not a very similar change in this release without a PEP? But maybe I'm missing something :)

[iritkatriel]

If you implement BaseExceptionGroup_repr so that the output doesn't change, then this change makes sense and can be merged and backported.

[iritkatriel]

the only way to reliably store the initial state of the repr on creation (without changing the repr) is to generate the repr when the ExceptionGroup is created and just re-print it when __repr__ is called.

That would be moderately expensive for a large list of exceptions, especially since we're not necessarily ever going to use the repr.

What if we did this only for the funky case where the sequence is not a list or tuple?

[dr-carlos]

What if we did this only for the funky case where the sequence is not a list or tuple?

Sounds like a great compromise! Just implemented now.

[dr-carlos]

I can also add some documentation to let users know that repr(exceptions_sequence) is going to be evaluated in the constructor of BaseExceptionGroup (and descendants) for non-tuple/list sequences, if that's warranted?

[iritkatriel]

Maybe not go into details, because this is an implementation detail, but could add a comment to the effect that while the exceptions may be any sequence, tuples and lists can be processed more efficiently by the implementation.

[dr-carlos]

Maybe not go into details, because this is an implementation detail, but could add a comment to the effect that while the exceptions may be any sequence, tuples and lists can be processed more efficiently by the implementation.

Done. I also noted that tuples are more efficient here than lists (this is because a new list has to be created in the repr if the original sequence was a list).

[iritkatriel]

I would be good to update all the examples in our docs to follow the "best practice" of using tuples. People will get the right thing when they copy-paste. Could you create an issue about the doc change?

[picnixz]

ditto

[dr-carlos]

Do we need to check the result of PySequence_List even though we've already run a PyList_Check?

[picnixz]

Yes. Because the errors are not only on the type.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[picnixz]

PyObject_Repr and PySequence_List can raise so we cannot continue calling the C API unless the calls succeeeded.

[picnixz]

Instead:

PyObject *exception_str = NULL;
if (!self->excs_str) {
    exception_str = ...
}
else {
    exception_str = Py_NewRef(self->excs_str);
}

assert(exception_str != NULL);
/* format */
Py_DECREF(exception_str);

It's not faster, but I think it's clearer to the reader. Up to you to make the change though.

[dr-carlos]

Wasn't sure so I attempted to clarify with a comment instead. Happy to do something different if that's preferable :)

[picnixz]

The problem wasn't the comment but rather the fact that we are using Py_XNewRef and then checking its result instead of first checking whethe Py_NewRef could be called.

[dr-carlos]

Currently we're using PyList_Check and PyTuple_Check to determine whether we can use the list or tuple repr - is it worth doing exact checks here instead, in case list or tuple are subclassed whilst overriding the __repr__?

[picnixz]

Since we only have a return NULL; you could remove the goto and inline evreything.