Exception group stage3

[iritkatriel]

No description provided.

[iritkatriel]

I'm not sure we want to use indentation for rendering, I used numbers instead.

Currently it does:

=================  <<1>> =================
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 21, in <module>
    raise ExceptionGroup("a nested group", f(), g(), h())
ExceptionGroup: a nested group
<<1>> ------------------------------ -
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 5, in f
    raise ValueError(42)
ValueError: 42
<<1>> ------------------------------ -
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 11, in g
    raise TypeError('bad type')
TypeError: bad type
<<1>> ------------------------------ -
=================  <<2>> =================
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 17, in h
    raise ExceptionGroup("a simple group", f(), g())
ExceptionGroup: a simple group
<<2>> ------------------------------ -
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 5, in f
    raise ValueError(42)
ValueError: 42
<<2>> ------------------------------ -
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 11, in g
    raise TypeError('bad type')
TypeError: bad type
=================  (2) =================
=================  (1) =================

for

def f():
    try:
        raise ValueError(42)
    except Exception as e:
        return e

def g():
    try:
        raise TypeError('bad type')
    except Exception as e:
        return e

def h():
    try:
        raise ExceptionGroup("a simple group", f(), g())
    except ExceptionGroup as e:
        return e

raise ExceptionGroup("a nested group", f(), g(), h())
def f():
    try:
        raise ValueError(42)
    except Exception as e:
        return e

def g():
    try:
        raise TypeError('bad type')
    except Exception as e:
        return e

def h():
    try:
        raise ExceptionGroup("a simple group", f(), g())
    except ExceptionGroup as e:
        return e

raise ExceptionGroup("a nested group", f(), g(), h())

[gvanrossum]

What's wrong with indentation?

[iritkatriel]

What's wrong with indentation?

It limits us to quite a small depth.

It's quite invasive in the C version - all the rendering functions just call PyFile_WriteString/PyFile_WriteObject and all those places will need to change. So I wanted to be sure that's what we want before implementing this. Do we want to render the whole nested structure, or flatten it to show only real exceptions? How do we want the limiting params to work?

[iritkatriel]

Also - what if the nested exceptions have cause and context? We show all of them?

[iritkatriel]

So we could for instance have a parameter saying how many exceptions to show (say 5) and then we say "ExceptionGroup: (the message) containing {n} exceptions, here are the first 5:" and then we print the first 5 leaf exceptions with their whole traceback and cause/context as they would normally be printed (except we need to glue together the sections of their traceback from the ExceptionGroup(s) they are in).

[gvanrossum]

I wouldn't worry too much about this. If a traceback is large, well, so it is (it's easy to get a 1000-line traceback from two mutually recursive functions, and I don't mind much -- scrollback buffers to the rescue :-).

If there is a cause or context we should render those exactly the way they are done currently: render context if present, else cause, then the connecting phrase, then the exception that was raised. (In experiments I couldn't come up with a situation where both cause and context are rendered -- I don't know if that's because only one or the other is present, or if the rendering code chooses one over the other.)

I don't think the C code currently has a way to limit the amount of output, right? It's just limited by the maximum stack depth.

I would just do something like this:

Traceback (most recent call last):
  File .........
  File .........
ExceptionGroup: message

This exception group has 11 sub-exceptions:

    ---------- 1/11 ----------
    Traceback (most recent call last):
      File .........
      File .........
    SomeException: Some message

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File .........
    AnotherException: Another message
    --------- 2/11 ----------
    Traceback (most recent call last):
      File .........
     YetAnotherException: Yet another message
     ---------- 3/11 ---------
     .
     .
     .
     ---------- end of 11 ----------

If any of the sub-exceptions is itself an ExceptionGroup, just repeat the whole structure indented. Maybe show the nesting level in the numbering as well, e.g. if sub-exception 2 is a group with 3 sub-sub-exceptions, number then 2.1/3, 2.2/3, 2.3/3?

If you have to invent a little data structure to pass around between the rendering helper functions to keep track of the levels, so be it.

[iritkatriel]

(In experiments I couldn't come up with a situation where both cause and context are rendered -- I don't know if that's because only one or the other is present, or if the rendering code chooses one over the other.)

That's in the rendering code - context is not printed if there is cause.

I don't think the C code currently has a way to limit the amount of output, right? It's just limited by the maximum stack depth.

I don't see a limit on the number of chained exceptions (just cycle detection).

Ok - I'll try something like you suggest, perhaps with one tweak. I was just reading bpos about tracebacks and came across this comment https://bugs.python.org/issue12535#msg140176 .

The ordering is as it is so that the last line in the displayed traceback corresponds to the exception that was actually caught.
What was caught was the ExceptionGroup, so its message should probably be at the end. No?

[gvanrossum]

I was just reading bpos about tracebacks and came across this comment https://bugs.python.org/issue12535#msg140176 .

The ordering is as it is so that the last line in the displayed traceback corresponds to the exception that was actually caught.
What was caught was the ExceptionGroup, so its message should probably be at the end. No?

That's a tough one, but you have a point here. I think the relationship between an exception and its cause/context is subtly different than that between an exception group and its sub-exceptions though. In the context/cause case we have this:

try:
    <some code that raises A>
except ...:
    <handler that also raises B>

Note that the tracebacks are printed in the same order:

Traceback (most recent call last):
  <traceback for A>
A: ...

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  <traceback for B>
B: ...

If we wanted to do the same thing for exception groups, the code would look something like this:

exceptions = []
for task in t1, t2, t3:
    try:
        task()
    except ... as e:
        exceptions.append(e)
raise ExceptionGroup("message",  exceptions)

And we'd end up with output like this:

<traceback 1>

Concurrently, another exception happened:

<traceback 2>

Concurrently, another exception happened:

<traceback 3>

These tracebacks were combined in an exception group:

<traceback for ExceptionGroup>

That doesn't look great, and also note that the ExceptionGroup isn't raised from inside the except clause. In a sense the ExceptionGroup doesn't have multiple causes or contexts; it's more that there are multiple concurrent exceptions with a common "start" (or "tail"?) of the traceback.

So, despite that precedent, I still feel that the group (common) traceback should be printed before the sub-exceptions.

[gvanrossum]

BTW, I'm curious how Trio handles this dilemma.

[iritkatriel]

BTW, I'm curious how Trio handles this dilemma.

I think this is it: https://github.com/python-trio/trio/blob/master/trio/_core/_multierror.py#L422

[gvanrossum]

Okay, so that's my proposal with 2-space indents, roughly, right?

[iritkatriel]

Yes.

[iritkatriel]

I get this output now (with single indent):

Running Release|x64 interpreter...
Traceback (most recent call last):
  File "C:\Users\User\src\cpython\x.py", line 21, in <module>
    raise ExceptionGroup("a nested group", f(), g(), h())
ExceptionGroup: a nested group
This exception has 3 sub-exceptions:
 ----------------- 1/3 -----------------
 Traceback (most recent call last):
   File "C:\Users\User\src\cpython\x.py", line 5, in f
     raise ValueError(42)
 ValueError: 42
 ----------------- end of 1 -----------------
 ----------------- 2/3 -----------------
 Traceback (most recent call last):
   File "C:\Users\User\src\cpython\x.py", line 11, in g
     raise TypeError('bad type')
 TypeError: bad type
 ----------------- end of 2 -----------------
 ----------------- 3/3 -----------------
 Traceback (most recent call last):
   File "C:\Users\User\src\cpython\x.py", line 17, in h
     raise ExceptionGroup("a simple group", f(), g())
 ExceptionGroup: a simple group
 This exception has 2 sub-exceptions:
  ----------------- 3.1/2 -----------------
  Traceback (most recent call last):
    File "C:\Users\User\src\cpython\x.py", line 5, in f
      raise ValueError(42)
  ValueError: 42
  ----------------- end of 3.1 -----------------
  ----------------- 3.2/2 -----------------
  Traceback (most recent call last):
    File "C:\Users\User\src\cpython\x.py", line 11, in g
      raise TypeError('bad type')
  TypeError: bad type
  ----------------- end of 3.2 -----------------
 ----------------- end of 3 -----------------

[iritkatriel]

I wanted to miminize code changes now, but I think the "seen" set should move to be part of the new context struct.

I also think we need a limit on the number of exceptions printed. It is unlikely that someone gets a chain with 100s of cause/context links, but it is very likely that an async task returns many many exceptions in a group (many repetitions of the same exception).

[aeros]

I believe this could result in a reference leak for the traceback object tb (since PyException_GetTraceback() returns a new ref) if the goto error on line 753 is reached. Shouldn't there be a Py_XDECREF(tb) to decref tb when it's unable to be set to the exception?

Suggested change

	tb = PyException_GetTraceback((PyObject*)orig);
	if (tb) {
	if (PyException_SetTraceback(eg, tb) == -1) {
	goto error;
	}
	}
	context = PyException_GetContext((PyObject*)orig);
	if (context) {
	PyException_SetContext(eg, context);
	}
	cause = PyException_GetCause((PyObject*)orig);
	if (cause) {
	PyException_SetCause(eg, cause);
	}
	return eg;
	error:
	Py_XDECREF(eg);
	return NULL;
	tb = PyException_GetTraceback((PyObject*)orig);
	if (tb) {
	if (PyException_SetTraceback(eg, tb) == -1) {
	goto error;
	}
	}
	context = PyException_GetContext((PyObject*)orig);
	if (context) {
	PyException_SetContext(eg, context);
	}
	cause = PyException_GetCause((PyObject*)orig);
	if (cause) {
	PyException_SetCause(eg, cause);
	}
	return eg;
	error:
	Py_XDECREF(eg);
	Py_XDECREF(tb);
	return NULL;

[iritkatriel]

I think you're right. But should the Py_XDECREF(tb); be at the end or just before the goto error?

Right now there is no difference, but imagine some other case of "goto error" is added in the future, after tb was set successfully set on eg. Then could we not end up with double deallocation of tb (one from eg's clear)?

[aeros]

Right now there is no difference, but imagine some other case of "goto error" is added in the future, after tb was set successfully set on eg. Then could we not end up with double deallocation of tb (one from eg's clear)?

Hmm, good point. That seems reasonable to me.

Although while investigating this further, I also noticed something else regarding the interaction between PyException_GetTraceback and PyException_SetTraceback vs the other getter/setter function pairs of PyException_* (for context and cause). For all three getters, PyException_GetTraceback, PyException_GetContext, and PyException_GetCause, a new reference is returned. However, only two of the setters, PyException_SetContext and PyException_SetCause, actually "steal" a reference, using Py_XSETREF directly (e.g. resulting in +1 references for the context object from PyException_GetContext and -1 from PyException_SetContext).

In PyException_SetTraceback, there seems to be a net 0 difference though, since in its underlying implementation function (BaseException_set_tb), there is a Py_Incref(tb) just before the Py_XSETREF, unlike the other two setters. So overall, for the traceback, we get +1 references to the traceback from PyException_GetTraceback, +1 from the Py_Incref, and -1 from Py_XSETREF. This seems to result in a net difference of +1 references for the traceback object (within exceptiongroup_subset), so don't we want to decref tb after PyException_SetTraceback, regardless of whether PyException_SetTraceback succeeds (assuming tb is not null, of course)?

Note that the C-API isn't really my area of expertise, so I could just be misunderstanding something here. But I wanted to mention it anyways in case it was overlooked and for my own clarification.

[iritkatriel]

Yes, I think you're right about this as well. It should be something equivalent to this then:

if (tb) {
        if (PyException_SetTraceback(eg, tb) == -1) {
            Py_XDECREF(tb);
            goto error;
        } else {
            Py_XDECREF(tb);
        }
    }

[iritkatriel]

I'm closing this because I'm working on exceptionGroup-stage4 branch now and don't want to make any more changes here.