Traceback reporting needs to be altered to support exception chaining as
per PEP 3134.

Thinking about this, I realized that an exception can become its own
context if it is explicitly re-raised in its except handler (with "raise
variable", not bare "raise").
Not a critical bug, but it should be fixed in order to avoid delayed
garbage collection of such exceptions.

(as for the exception reporting code, it will have to detect recursivity)

>>> try: 1/0
... except Exception as e: raise e
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 1, in <module>
ZeroDivisionError: int division or modulo by zero
>>> e = sys.last_value
>>> e.__context__
ZeroDivisionError('int division or modulo by zero',)
>>> e.__context__ is e
True

Does anyone know why there is the following test in pythonrun.c:
http://hg.pitrou.net/public/py3k/py3k/file/c143699d8dee/Python/pythonrun.c#l1346

Can PyErr_Display be called with something else than a PyException instance?

Two other questions:

1) Should I expose a PyErr_DisplaySingle API to display an exception
without chaining?

2) Should PyErr_Display return an integer value (0: success, -1:
failure) rather than void as it currently does?

Here is a draft patch for those who want to take a look.

(it works but the final cleanup is waiting for the API decisions
mentioned above)

Yet another question. There is a slight discrepancy between tracebacks
generated by the builtin-reporting and tracebacks generated by traceback.py.

With built-in reporting:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "f.py", line 22, in raise_cause
inner_raise_cause()
  File "f.py", line 13, in inner_raise_cause
raise KeyError from e
KeyError

With traceback.py:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "f.py", line 22, in raise_cause
    inner_raise_cause()
  File "f.py", line 13, in inner_raise_cause
    raise KeyError from e
KeyError

As you see, indentation of code lines is different. Should we harmonize
those behaviours?

* cause/context cycles should be avoided.  Naive traceback printing
could become confused, and I can't think of any accidental way to
provoke it (besides the problem mentioned here.)

* I suspect PyErr_Display handled string exceptions in 2.x, and this is
an artifact of that

* No opinion on PyErr_DisplaySingle

* PyErr_Display is used by PyErr_Print, and it must end up with no
active exception.  Additionally, third party code may depend on this
semantic.  Maybe PyErr_DisplayEx?

* +1 on standardizing tracebacks

Le dimanche 22 juin 2008 à 07:04 +0000, Adam Olsen a écrit :
> Adam Olsen <rhamph@gmail.com> added the comment:
> 
> * cause/context cycles should be avoided.  Naive traceback printing
> could become confused, and I can't think of any accidental way to
> provoke it (besides the problem mentioned here.)

You mean they should be detected when the exception is set? I was afraid
that it may make exception raising slower. Reporting is not performance
sensitive in comparison to exception raising.

(the "problem mentioned here" is already avoided in the patch, but the
detection of other cycles is deferred to exception reporting for the
reason given above)

> * PyErr_Display is used by PyErr_Print, and it must end up with no
> active exception.  Additionally, third party code may depend on this
> semantic.  Maybe PyErr_DisplayEx?

I was not proposing to change the exception swallowing semantics, just
to add a return value indicating if any errors had occurred while
displaying the exception.

On Sun, Jun 22, 2008 at 8:07 AM, Antoine Pitrou <report@bugs.python.org> wrote:
> You mean they should be detected when the exception is set? I was afraid
> that it may make exception raising slower. Reporting is not performance
> sensitive in comparison to exception raising.
>
> (the "problem mentioned here" is already avoided in the patch, but the
> detection of other cycles is deferred to exception reporting for the
> reason given above)

I meant only that trivial cycles should be detected.  However, I
hadn't read your patch, so I didn't realize you already knew of a way
to create a non-trivial cycle.

This has placed a niggling doubt in my mind about chaining the
exceptions, rather than the tracebacks.  Hrm.

>> * PyErr_Display is used by PyErr_Print, and it must end up with no
>> active exception.  Additionally, third party code may depend on this
>> semantic.  Maybe PyErr_DisplayEx?
>
> I was not proposing to change the exception swallowing semantics, just
> to add a return value indicating if any errors had occurred while
> displaying the exception.

Ahh, harmless then, but to what benefit?  Wouldn't the traceback
module be better suited to any possible error reporting?

Le dimanche 22 juin 2008 à 17:17 +0000, Adam Olsen a écrit :
> I meant only that trivial cycles should be detected.  However, I
> hadn't read your patch, so I didn't realize you already knew of a way
> to create a non-trivial cycle.
> 
> This has placed a niggling doubt in my mind about chaining the
> exceptions, rather than the tracebacks.  Hrm.

Chaining the tracebacks rather than the exceptions loses important
information: what is the nature of the exception which is the cause or
context of the current exception?

It is improbable to create such a cycle involuntarily, it means you
raise an old exception in replacement of a newer one caused by the
older, which I think is quite contorted. It is also quite easy to avoid
creating the cycle, simply by re-raising outside of any except handler.

> Ahh, harmless then, but to what benefit?

I don't know, I've never used that API, I just proposed returning that
error code since it is computed inside the function anyway.
But let's drop that proposal. Same for PyErr_DisplaySingle if nobody
cares.

> Wouldn't the traceback
> module be better suited to any possible error reporting?

I suppose built-in reporting is needed when reporting a grave error such
as a MemoryError (C code can avoid doing any memory allocations), or an
error that occurred during startup before any pure Python modules could
be initialized (how would you report a syntax error in the traceback
module itself?).

But application code should use the traceback module rather than try to
access the C API.

On Sun, Jun 22, 2008 at 1:04 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 17:17 +0000, Adam Olsen a écrit :
>> I meant only that trivial cycles should be detected.  However, I
>> hadn't read your patch, so I didn't realize you already knew of a way
>> to create a non-trivial cycle.
>>
>> This has placed a niggling doubt in my mind about chaining the
>> exceptions, rather than the tracebacks.  Hrm.
>
> Chaining the tracebacks rather than the exceptions loses important
> information: what is the nature of the exception which is the cause or
> context of the current exception?

I assumed each leg of the traceback would reference the relevant exception.

Although.. this is effectively the same as creating a new exception
instance when reraised, rather than modifying the old one.  Reusing
the old is done for performance I believe.

> It is improbable to create such a cycle involuntarily, it means you
> raise an old exception in replacement of a newer one caused by the
> older, which I think is quite contorted. It is also quite easy to avoid
> creating the cycle, simply by re-raising outside of any except handler.

I'm not convinced.

    try:
        ...  # Lookup
    except A as a:  # Lookup failed
        try:
            ...  # Fallback
        except B as b:  # Fallback failed
            raise a  # The original exception is of the type we want

For this behaviour, this is the most natural way to write it.
Conceptually, there shouldn't be a cycle - the traceback should be the
lookup, then the fallback, then whatever code is about this - exactly
the order the code executed in.

Le dimanche 22 juin 2008 à 19:23 +0000, Adam Olsen a écrit :
> For this behaviour, this is the most natural way to write it.
> Conceptually, there shouldn't be a cycle

I agree your example is not far-fetched. How about avoiding cycles for
implicit chaining, and letting users shoot themselves in the foot with
explicit recursive chaining if they want? Detection would be cheap
enough, just a simple loop without any memory allocation.

> the traceback should be the
> lookup, then the fallback, then whatever code is about this - exactly
> the order the code executed in.

It would be the reverse: first the fallback, then the re-raised
exception. The last caught exception is always reported last, because
it's supposed to be the "main" or "highest-level" one.

On Sun, Jun 22, 2008 at 1:48 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 19:23 +0000, Adam Olsen a écrit :
>> For this behaviour, this is the most natural way to write it.
>> Conceptually, there shouldn't be a cycle
>
> I agree your example is not far-fetched. How about avoiding cycles for
> implicit chaining, and letting users shoot themselves in the foot with
> explicit recursive chaining if they want? Detection would be cheap
> enough, just a simple loop without any memory allocation.

That's still O(n).  I'm not so easily convinced it's cheap enough.

And for that matter, I'm not convinced it's correct.  The inner
exception's context becomes clobbered when we modify the outer
exception's traceback.  The inner's context should reference the
traceback as it was at that point.

This would all be a lot easier if reraising always created a new
exception.  Can you think of a way to skip that only when we can be
sure its safe?  Maybe as simple as counting the references to it?

Le dimanche 22 juin 2008 à 19:57 +0000, Adam Olsen a écrit :
> That's still O(n).  I'm not so easily convinced it's cheap enough.

O(n) when n will almost never be greater than 5 (and very often equal to
1 or 2), and when the unit is the cost of a pointer dereference plus the
cost of a pointer comparison, still sounds cheap. We could bench it
anyway.

> And for that matter, I'm not convinced it's correct.  The inner
> exception's context becomes clobbered when we modify the outer
> exception's traceback.  The inner's context should reference the
> traceback as it was at that point.

Yes, I've just thought about that, it's a bit annoying... We have to
decide what is more annoying: that, or a reference cycle that can delay
deallocation of stuff attached to an exception (including local
variables attached to the tracebacks)?

(just a small note: it's exception objects that are chained, not
tracebacks... we never modify tracebacks at any point)

> This would all be a lot easier if reraising always created a new
> exception.

How do you duplicate an instance of an user-defined exception? Using an
equivalent of copy.deepcopy()? It will probably end up much more
expensive than the above-mentioned O(n) search.

> Can you think of a way to skip that only when we can be
> sure its safe?  Maybe as simple as counting the references to it?

I don't think so, the exception can be referenced in an unknown number
of local variables (themselves potentially referenced by tracebacks).

On Sun, Jun 22, 2008 at 2:20 PM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> Antoine Pitrou <pitrou@free.fr> added the comment:
>
> Le dimanche 22 juin 2008 à 19:57 +0000, Adam Olsen a écrit :
>> That's still O(n).  I'm not so easily convinced it's cheap enough.
>
> O(n) when n will almost never be greater than 5 (and very often equal to
> 1 or 2), and when the unit is the cost of a pointer dereference plus the
> cost of a pointer comparison, still sounds cheap. We could bench it
> anyway.

Indeed.

>> And for that matter, I'm not convinced it's correct.  The inner
>> exception's context becomes clobbered when we modify the outer
>> exception's traceback.  The inner's context should reference the
>> traceback as it was at that point.
>
> Yes, I've just thought about that, it's a bit annoying... We have to
> decide what is more annoying: that, or a reference cycle that can delay
> deallocation of stuff attached to an exception (including local
> variables attached to the tracebacks)?

The cycle is only created by broken behaviour.  The more I think about
it, the more I want to fix it (by not reusing the exception).

>> This would all be a lot easier if reraising always created a new
>> exception.
>
> How do you duplicate an instance of an user-defined exception? Using an
> equivalent of copy.deepcopy()? It will probably end up much more
> expensive than the above-mentioned O(n) search.

Passing in e.args is probably sufficient.  All this would need to be
discussed on python-dev (or python-3000?) though.

>> Can you think of a way to skip that only when we can be
>> sure its safe?  Maybe as simple as counting the references to it?
>
> I don't think so, the exception can be referenced in an unknown number
> of local variables (themselves potentially referenced by tracebacks).

Can be, or will be?  Only the most common behaviour needs to be optimized.

Le dimanche 22 juin 2008 à 20:40 +0000, Adam Olsen a écrit :
> > How do you duplicate an instance of an user-defined exception? Using
> an
> > equivalent of copy.deepcopy()? It will probably end up much more
> > expensive than the above-mentioned O(n) search.
> 
> Passing in e.args is probably sufficient.

I think it's very optimistic :-) Some exception objects can hold dynamic
state which is simply not stored in the "args" tuple. See Twisted's
Failure objects for an extreme example:
http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py

(yes, it is used an an exception: see "raise self" in the trap() method)

> Can be, or will be?  Only the most common behaviour needs to be optimized.

Well, the "most common behaviour" is a very short context chain, which
is already optimized by my reference-avoidance proposal...

On Sun, Jun 22, 2008 at 2:56 PM, Antoine Pitrou <report@bugs.python.org> wrote:
> Le dimanche 22 juin 2008 à 20:40 +0000, Adam Olsen a écrit :
>> Passing in e.args is probably sufficient.
>
> I think it's very optimistic :-) Some exception objects can hold dynamic
> state which is simply not stored in the "args" tuple. See Twisted's
> Failure objects for an extreme example:
> http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py
>
> (yes, it is used an an exception: see "raise self" in the trap() method)

Failure doesn't have an args tuple and doesn't subclass Exception (or
BaseException) - it already needs modification in 3.0.  It's heaped
full of complexity and implementation details.  I wouldn't be
surprised if your changes break it in subtle ways too.

In short, if forcing Failure to be rewritten is the only consequence
of using .args, it's an acceptable tradeoff of not corrupting
exception contexts.

Le lundi 23 juin 2008 à 06:16 +0000, Adam Olsen a écrit :
> Failure doesn't have an args tuple and doesn't subclass Exception (or
> BaseException) - it already needs modification in 3.0.  It's heaped
> full of complexity and implementation details.  I wouldn't be
> surprised if your changes break it in subtle ways too.

Failure was only an example, I'm sure there are lots of other ones. The
point is that the assertion that user-defined exceptions can be safely
cloned by cloning their args is wrong, unless you want to put very
strong restrictions on how user-defined exceptions can behave.

Here is the final patch. Features:
- cleanup of internal APIs
- standardize traceback indentation (source lines are prefixed with 4
spaces)
- break cycles along the context chain (a synthetic benchmark with a
6-level deep context chain shows a very moderate slowdown of 10-15%)

Potential reviewers, let's make life easier for you:
http://codereview.appspot.com/2448

I think there is a problem when the source file cannot be opened (a .pyc
without its .py): the four spaces are printed, but not the line, and the
following level is not properly indented.

See issue3342 for a competing patch that corrects the indentation problem.

Le vendredi 11 juillet 2008 à 12:12 +0000, Amaury Forgeot d'Arc a
écrit :
> I think there is a problem when the source file cannot be opened (a .pyc
> without its .py): the four spaces are printed, but not the line, and the
> following level is not properly indented.
> 
> See issue3342 for a competing patch that corrects the indentation problem.

Great! Do you plan to commit it soon or should I include it into my
patch?

(and do you have other observations on the present patch?)

I committed r64881, which invalidates your patch a bit :-|

BTW, I don't like your comment "Can't be bothered to check all those
PyFile_WriteString() calls". 
In general, it is not a good idea to execute python code with an
exception set, this leads to subtle problems, and some "XXX undetected
error" prints in debug mode. Chaining several calls to PyDict_SetItem
for example is usually not a problem, but PyFile_WriteString does
execute python code in python3.0.

Le vendredi 11 juillet 2008 à 22:18 +0000, Amaury Forgeot d'Arc a
écrit :
> I committed r64881, which invalidates your patch a bit :-|

Apparently you committed in trunk rather than py3k? Could you svnmerge
into py3k as well? Then it should be quite easy to rework my patch
around it.

> BTW, I don't like your comment "Can't be bothered to check all those
> PyFile_WriteString() calls". 

It's not my comment, it was already there. I agree it doesn't sound very
meticulous. :-)

> In general, it is not a good idea to execute python code with an
> exception set, this leads to subtle problems, and some "XXX undetected
> error" prints in debug mode.
> Chaining several calls to PyDict_SetItem
> for example is usually not a problem, but PyFile_WriteString does
> execute python code in python3.0.

Well, as said above, I just kept the original method of doing things...
If you think of a simple solution to make things better (and a test case
to validate it), I'm open to integrating it in the patch.

Here is a new patch incorporating Amaury's better indentation fix for
tracebacks. It should be ready for consumption (all the tests pass).

And for the code review junkies: http://codereview.appspot.com/2448

Antonine, do you have a patch address the review comments?

> Antonine, do you have a patch address the review comments?

Yes, although I've forgotten to upload it here - you will find it at
http://codereview.appspot.com/2448

PS: it is Antoine not Antonine :)

Sorry, Antoine! The patch is in with r64965.