bpo-45292: [PEP 654] Update traceback display code to work with exception groups

[iritkatriel]

https://bugs.python.org/issue45292

[iritkatriel]

This is almost exactly the same as I used for the PEP - I just removed the "with X sub-exceptions" line from the tracebacks because I think it clutters them and is not very useful.

[gvanrossum]

This is almost exactly the same as I used for the PEP - I just removed the "with X sub-exceptions" line from the tracebacks because I think it clutters them and is not very useful.

Just be sure to update the PEP examples. :-)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 2052c77 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[bedevere-bot]

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

[erlend-aasland]

First round comments :) I'll take a new round on references and pointers later.

[1st1]

Do you have an opinion on whether it's better to box all EG tracebacks or only the nested ones?

I think it's OK to always box the EGs because they're pretty special, although I'd change the very first line of the output, from:

 | Traceback (most recent call last):
| File "{__file__}", line {exc.__code__.co_firstlineno + 9}, in exc
| raise EG("eg", [VE(1), exc, VE(4)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: eg
+-+---------------- context.1 ----------------
| ValueError: 1
+---------------- context.2 ----------------
| Traceback (most recent call last):
| File "{__file__}", line {exc.__code__.co_firstlineno + 6}, in exc
| raise EG("nested", [TE(2), TE(3)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: nested
+-+---------------- context.2.1 ----------------
| TypeError: 2
+---------------- context.2.2 ----------------
| TypeError: 3
+------------------------------------
+---------------- context.3 ----------------
| ValueError: 4
+------------------------------------

to:

 + Exception group traceback (most recent call last):
|
| File "{__file__}", line {exc.__code__.co_firstlineno + 9}, in exc
| raise EG("eg", [VE(1), exc, VE(4)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: eg
+-+---------------- context.1 ----------------
| ValueError: 1
+---------------- context.2 ----------------
| Traceback (most recent call last):
| File "{__file__}", line {exc.__code__.co_firstlineno + 6}, in exc
| raise EG("nested", [TE(2), TE(3)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: nested
+-+---------------- context.2.1 ----------------
| TypeError: 2
+---------------- context.2.2 ----------------
| TypeError: 3
+------------------------------------
+---------------- context.3 ----------------
| ValueError: 4
+------------------------------------

[iritkatriel]

@1st1 - see below.

 + Exception group traceback (most recent call last):
|
| File "{__file__}", line {exc.__code__.co_firstlineno + 9}, in exc
| raise EG("eg", [VE(1), exc, VE(4)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: eg
+-+---------------- context.1 ----------------
| ValueError: 1
+---------------- context.2 ----------------
| Traceback (most recent call last):
| File "{__file__}", line {exc.__code__.co_firstlineno + 6}, in exc
| raise EG("nested", [TE(2), TE(3)])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| ExceptionGroup: nested
+-+---------------- context.2.1 ----------------

The indentation of the next line is wrong. Is this a copy-paste error or a bug?

 | TypeError: 2
+---------------- context.2.2 ----------------
| TypeError: 3
+------------------------------------
+---------------- context.3 ----------------
| ValueError: 4
+------------------------------------

[1st1]

The indentation of the next line is wrong. Is this a copy-paste error or a bug?

Copy/paste/whitespace issue; no implementation bugs were observed!

[gvanrossum]

I like the max_group_{with,depth} solution and the refactor of the chaining loop!

I didn't review the C code carefully, it looks like others are on that.

The only things left from me are nits in the formatting of some of the overflow messages. Otherwise LGTM!

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 61fab3f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[gvanrossum]

LG!

[iritkatriel]

LG!

Thank you!

@gpshead@erlend-aasland@1st1 I'll wait a bit before merging in case you want to have another look.

[erlend-aasland]

Looks good; great job!

The only remark I have is that I find the error handling verbose and hard to read. I'm used to (and prefer):

intfunc()
{
intrc=first();
if (rc<0) {
goto error;
}
interr=second();
if (err!=0) {
goto error;
}
// etc.// at the end:return0;
error:
cleanup();
return-1;
}

The current approach of always verifying that the previous step did not fail seems to lead to a lot of indenting levels. I could count 7 (!) indents in one of the functions.

I would consider simplifying the error handling for the sake of readability.

Feel free to disregard this comment; It's only personal preference :)

[iritkatriel]

@erlend-aasland I agree with your comment. I tried using gotos but it was a bit delicate because you need to make sure the refcounting is still correct. This is a refactor that should be done in its own PR and not together with major functional changes. The current method preserves the control flow so it doesn't impact refcounts.

[erlend-aasland]

This is a refactor that should be done in its own PR and not together with major functional changes.

+1 🙂

[iritkatriel]

Thank you everyone for your help!

[merwok]

FYI running patchcheck for an unrelated PR just complained about this 5-space indent!
(and more in Lib/traceback.py)