GeezerVison

There is one more place where the move to using traceback from the exception can be visible from a program - in the eval loop when we reraise. See iritkatriel#32 . I think maybe we should add that change to this PR.

There is one more place where the move to using traceback from the exception can be visible from a program - in the eval loop when we reraise. See iritkatriel#32 . I think maybe we should add that change to this PR.

It comes down to how certain we are that that's not going to break anything. I don't see an assertion in there that checks that exc_info->exc_traceback equals whatever PyException_GetTraceback() returns.

There is one more place where the move to using traceback from the exception can be visible from a program - in the eval loop when we reraise. See iritkatriel#32 . I think maybe we should add that change to this PR.

It comes down to how certain we are that that's not going to break anything. I don't see an assertion in there that checks that exc_info->exc_traceback equals whatever PyException_GetTraceback() returns.

It will break the same edge case as the other changes in this pr - when you change the traceback on the handled exception in an except clause and then do ‘raise’.

It will break the same edge case as the other changes in this pr - when you change the traceback on the handled exception in an except clause and then do ‘raise’.

Okay, that's fine then. I was worried that it might in some cases insert or leave out a frame, but it sounds like that would only happen if you mess around with the exception (e.g. raising and catching it, like you showed earlier). That edge case is acceptable to me as long as we announce it clearly in what's new.

(I am still concerned about something else -- sometimes "raise e" and "raise" don't do the same thing even if "e" is the exception being handled. But that is not affected by these changes -- that difference will persist, and arguably it is correct (though it surprises me occasionally). Even if we wanted to change it, we couldn't, because it is not an obscure corner case -- the semantics are clearly described. Please ignore my mumbling here.)

FTR - an example of what will change for raise:

import sys

def f():
  raise TypeError(42)

try:
  f()
except Exception as e:
  exc = e
  print('1--', e.__traceback__, sys.exc_info()[2])
  try:
    raise e
  except:
    pass
  print('2--', e.__traceback__, sys.exc_info()[2])
  raise

Old:

iritkatriel@Irits-MBP cpython-1 % ../cpython/python.exe xx.py
1-- <traceback object at 0x10dc13980> <traceback object at 0x10dc13980>
2-- <traceback object at 0x10dc13840> <traceback object at 0x10dc13980>
Traceback (most recent call last):
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 7, in <module>
    f()
    ^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 4, in f
    raise TypeError(42)
    ^^^^^^^^^^^^^^^^^^^
TypeError: 42

New:

iritkatriel@Irits-MBP cpython-1 % ./python.exe xx.py
1-- <traceback object at 0x105748180> <traceback object at 0x105748180>
2-- <traceback object at 0x1057498c0> <traceback object at 0x1057498c0>
Traceback (most recent call last):
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 12, in <module>
    raise e
    ^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 7, in <module>
    f()
    ^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 4, in f
    raise TypeError(42)
    ^^^^^^^^^^^^^^^^^^^
TypeError: 42

(I am still concerned about something else -- sometimes "raise e" and "raise" don't do the same thing even if "e" is the exception being handled. But that is not affected by these changes -- that difference will persist, and arguably it is correct (though it surprises me occasionally). Even if we wanted to change it, we couldn't, because it is not an obscure corner case -- the semantics are clearly described. Please ignore my mumbling here.)

A related story: After we moved our system from python 2 to python 3 we got bug reports about nonsensical tracebacks. Turned out we had something like this:

import sys
import traceback

class Calc:
  def __init__(self):
     self.value = None
     self.exc_info = None

  def calc(self):
     raise TypeError(12)

  def calc_and_cache(self):
    try:
      self.value = self.calc()
      return self.value
    except Exception as e:
      self.exc_info = sys.exc_info()
      raise

  def get_cached_value(self):
    if self.exc_info is not None:
      raise self.exc_info[1]
    return self.value

c = Calc()

print('------ 1 ------')
try:
   c.calc_and_cache()
except Exception as e:
   traceback.print_exception(e)

print('------ 2 ------')
try:
   c.get_cached_value()
except Exception as e:
   traceback.print_exception(e)

The two tracebacks were the same in python 2, but in python 3 you get:

------ 1 ------
Traceback (most recent call last):
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 29, in <module>
    c.calc_and_cache()
    ^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 14, in calc_and_cache
    self.value = self.calc()
                 ^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 10, in calc
    raise TypeError(12)
    ^^^^^^^^^^^^^^^^^^^
TypeError: 12
------ 2 ------
Traceback (most recent call last):
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 35, in <module>
    c.get_cached_value()
    ^^^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 22, in get_cached_value
    raise self.exc_info[1]
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 29, in <module>
    c.calc_and_cache()
    ^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 14, in calc_and_cache
    self.value = self.calc()
                 ^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/xx.py", line 10, in calc
    raise TypeError(12)
    ^^^^^^^^^^^^^^^^^^^
TypeError: 12

The fix was to change the raise to raise self.exc_info[1].with_traceback(self.exc_info[2]).

A related story [...]

Hm, this makes me realize that there may be a bug in asyncio. When you call a Future's result() method, if the Future represents an error, it raises self.exception. But that means that each time you attempt to get the result, you'll get an extra line (or more) added to the traceback. This seems quite unintended -- the author of the code (me) apparently wasn't aware of this behavior. Curiously, nobody's ever complained about nonsensical tracebacks. To fix it, we'd have to add a separate attribute to hold the traceback.

Oh, and the same bug exists in concurrent.futures. (Which I did not write.) In fact now I suspect it exists all over the stdlib where exceptions are stored and raised later.

Curiously, nobody's ever complained about nonsensical tracebacks.

What made our tracebacks obviously nonsensical is that we raise the same exception from two different places. There is no way to get calc_and_cache and get_cached_value in the same stack.

I've created bpo-45924 to track this.

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 05af230 🤖

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