Sorry, something went wrong.
There was a problem hiding this comment.
Nice and straightforward. I have a few nits but really, this could go as-is.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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’. |
Sorry, something went wrong.
|
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.) |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
|
(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]). |
Sorry, something went wrong.
| (Contributed by Inada Naoki in :issue:`29410`.) | ||
|
|
||
| * When an active exception is re-raised by a :keyword:`raise` statement with no parameters, | ||
| the traceback attached to this exception is now always ``sys.exc_info()[1].__traceback__``. |
There was a problem hiding this comment.
The use of sys.exc_info()[1] always puts me off -- it's just too cryptic. Maybe "the traceback attached to this exception is now always its __traceback__ attribute? Or use the phrasing from the reference manual above?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
🤖 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM. Land any time!
Sorry, something went wrong.
| modified traceback. Previously, the exception was re-raised with the | ||
| traceback it had when it was caught. |
There was a problem hiding this comment.
Previously, the exception was re-raised with the traceback it had when it was caught.
Isn't this a bug and that particular change should therefore be backported? I recently got surprised by this behavior of raise when using BaseException.with_traceback in Python 3.10: I was able to set e.__traceback__ to a custom traceback with e.with_traceback(my_custom_tb), even verified with e.__traceback__ == my_custom_tb, yet a bare raise raised the exception like no changes had been made. (Also adding to this confusion: Changes to the original traceback like e.__traceback__.tb_next = None did successfully show up with bare raise.)
Sorry, something went wrong.
There was a problem hiding this comment.
So I originally wanted to post this as an issue but discovered that the behavior seemingly disappeared when using Python 3.11. With some help I found out about this PR where one of the changes fixes the behavior of bare raise. So I thought it would be the best idea to ask here via comment since @iritkatriel is also listed as the maintainer for the traceback module anyway? I hope that's okay.
Sorry, something went wrong.
There was a problem hiding this comment.
Here's a short example to explain what I mean:
Sorry, something went wrong.
There was a problem hiding this comment.
We can't backport this change, it's way too invasive for that. This behaviour existed since 3.0, unfortunately.
Sorry, something went wrong.
There was a problem hiding this comment.
If you "raise e" instead of "raise" then you will see the edited traceback (but with the current frame added).
Sorry, something went wrong.
https://bugs.python.org/issue45711