gh-129605: Add more exception handling when computing suggestions in traceback.py #129632
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| @@ -1504,24 +1504,27 @@ def _compute_suggestion_error(exc_value, tb, wrong_name): | |||
| return None | |||
| else: | |||
| assert isinstance(exc_value, NameError) | |||
| # find most recent frame | |||
| if tb is None: | |||
| try: | |||
There was a problem hiding this comment.
This is a very broad try-except block. If you scoped it only around the hasattr call would the new test cases still pass?
There was a problem hiding this comment.
Yes they would still pass. It's usually good practice to minimise code in a try block, but I think this is an exceptional case. This function attempts to provide "Did you mean foo?" suggestions after there has already been an error in user code. Hence a different exception propagating from this function is very unhelpful (and the cause of the issue). So I think it's worth having wider try blocks here to minimise the risk of unhandled exceptions in the future. The existing code has wide try blocks ignoring all Exception subclasses when doing processing for AttributeError (L1485-1494) and ImportError (L1499-1502). I think we we should treat the processing for NameError the same. (Although - I haven't found a way to cause an exception in L1508-1517, so this is mostly theoretical.) Happy to narrow the scope of the try if folks feel strongly.
In
_compute_suggestion_errorintraceback.py, the code that handlesAttributeErrorandImportErroris wrapped intry-excepts, but the code for NameError is not wrapped in atry-except. However, thehasattr(self, wrong_name)call fails if accessing an attribute raises an exception. This is what happens in the reproducer in the issue.I wrapped the handling of
NameErrorin a try-except to match the handling of the other two exception types.I also added three related tests to test_traceback.py. All three of the new tests crash both PyREPL and the test_traceback.py test suite without the fix. The property test needed some creativity to trigger the bug and I'm a bit proud of it 😛
Let me know if the tests also need to be added to the PyREPL test suite.
__getattr__raising #129605