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):

int
func()
{
    int rc = first();
    if (rc < 0) {
        goto error;
    }
    int err = second();
    if (err != 0) {
        goto error;
    }
    // etc.

    // at the end:
    return 0;
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)