Skip to content

gh-129025: fix too wide source location for bytecodes emitted for except* #129026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+47 −2
Open

gh-129025: fix too wide source location for bytecodes emitted for except* #129026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6933f38
gh-129025: fix too wide source location for bytecodes emitted for and
iritkatriel Jan 19, 2025
bf78fe0
revert change to test_exceptions
iritkatriel Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions Lib/test/test_traceback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2919,6 +2919,47 @@ def exc():
report = self.get_report(exc)
self.assertEqual(report, expected)

def test_except_star_lineno(self):
def exc():
class Bad(ExceptionGroup):
def split(*args):
1/0

try:
raise Bad("", [ValueError(), TypeError()])
except* ValueError:
1
2
3

expected = (f' + Exception Group Traceback (most recent call last):\n'
f' | File "{__file__}", line {exc.__code__.co_firstlineno + 6}, in exc\n'
f' | raise Bad("", [ValueError(), TypeError()])\n'
f' | test.test_traceback.BaseExceptionReportingTests.test_except_star_lineno.<locals>.exc.<locals>.Bad: (2 sub-exceptions)\n'
f' +-+---------------- 1 ----------------\n'
f' | ValueError\n'
f' +---------------- 2 ----------------\n'
f' | TypeError\n'
f' +------------------------------------\n'
f'\n'
f'During handling of the above exception, another exception occurred:\n'
f'\n'
f'Traceback (most recent call last):\n'
f' File "{__file__}", line '
f'{self.callable_line}, in get_exception\n'
f' exception_or_callable()\n'
f' ~~~~~~~~~~~~~~~~~~~~~^^\n'
f' File "{__file__}", line {exc.__code__.co_firstlineno + 7}, in exc\n'
f' except* ValueError:\n'
f' \n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blank line is weird. I'd expect either an arrow and/or squiggles pointing at except or at the first character of the line or at the whole line, OR no arrows/squiggles and no blank line.

The old way showed the entire except block, but no extra blank line.

f' File "{__file__}", line {exc.__code__.co_firstlineno + 3}, in split\n'
f' 1/0\n'
f' ~^~\n'
f'ZeroDivisionError: division by zero\n')

report = self.get_report(exc)
self.assertEqual(report, expected)

def test_KeyboardInterrupt_at_first_line_of_frame(self):
# see GH-93249
def f():
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-01-19-14-36-37.gh-issue-129025.QFXeS2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix too wide source locations of instructions emitted for ``except`` and
``except*``.
6 changes: 4 additions & 2 deletions Python/codegen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2365,7 +2365,8 @@ codegen_try_except(compiler *c, stmt_ty s)
for (i = 0; i < n; i++) {
excepthandler_ty handler = (excepthandler_ty)asdl_seq_GET(
s->v.Try.handlers, i);
location loc = LOC(handler);
/* Set location to the entire line of the except keyword */
location loc = LOCATION(handler->lineno, handler->lineno, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the entire line? Would the location of the type would be better?

Is this legal syntax?

except *\
     ValueError:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the location of the type would be better?

The type has the location of the expression that defines it. This is for exceptions raised from the except* itself. If you look at the test - the exception is from split() on the exception group, which is called by except*.

Why the entire line?

We don't have accurate information in the AST about the location of the except*, so this is next best. If it is multiline, the first line is where the except* is, which is what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the CHECK_EG_MATCH instruction, IIUC. Since it is matching the type, wouldn't the location of the type be a reasonable location?

If the AST had all the location information for everything, what location would you give the CHECK_EG_MATCH instruction?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CHECK_EG_MATCH should have the location of except*, which is applying the match check. The location of calculating the type should be the location of the type expression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't generally attach locations to syntax like except *.
For example, the location of the calls to __enter__ and __exit__ in a with statement is the context expression, not the with keyword.

Maybe we should attach the locations to the keywords? Though, as you say, it will need changes to the AST.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this issue to add locations to the AST #129157.

But we need to do something here that we can backport.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the CHECK_EG_MATCH instruction, IIUC. Since it is matching the type, wouldn't the location of the type be a reasonable location?

If we go with the type, then we have the analogous situation with CHECK_EXC_MATCH for old-style except, where there may not be a type. We probably can't get an exception raised there, but do we mind if the location info is different in the code object is different for except and except*?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also valid (PEP 8 notwithstanding :-):

>>> try:
...     foo
... except\
... *\
... ValueError\
... :
...     pass
...

I agree with Irit that the location we want to show in the traceback is the except keyword, which in practice will be on the same line as the * and most likely also on the type.

(Sorry for the delay Irit. Today I am attempting to catch up with a lot of email I've been procrastinating on.)

Copy link
Member Author

@iritkatriel iritkatriel Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the discussion with @pablogsal on #129162 I think there are two different things here: (1) the location we put for the instruction in the code object and (2) what we display in the traceback.

For the traceback, I now think we should ideally hilight the whole except or except* statement, but place carets under the specific type (if there is one and it's relevant). If there is more than one type, we want to point to the one that cause the exception. This is similar to the case of a with statement with multiple context managers - we want to point to the with keyword, but also indicate which of the context managers was involved.

I think Pablo's PR should just add the missing location info to the AST, and then we can take it from there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find an example for plain except where putting the traceback on the expression might make more sense -- the CHECK_EXC_MATCH instruction has only one possible failure, which is when the expression returns something that's not an exception, and then we get

TypeError: catching classes that do not inherit from BaseException is not allowed

I'd like to see that attached to the expression that gave a non-exception rather than on except (if they are on different lines).

For except* it is more complicated (many failure points, not all related to the given type) and I think attaching to the except is more acceptable there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the extra blank line I noticed in the test output is because we use zeros for the column positions?

So maybe this needs to be more careful and figure out the position of the last character on the line, or just put it only on the except keyword? (Which would require figuring out the position of the except keyword, which we might not have.

I guess if it's hard to do that right, I'm okay with the blank line.

if (!handler->v.ExceptHandler.type && i < n-1) {
return _PyCompile_Error(c, loc, "default 'except:' must be last");
}
Expand Down Expand Up @@ -2546,7 +2547,8 @@ codegen_try_star_except(compiler *c, stmt_ty s)
for (Py_ssize_t i = 0; i < n; i++) {
excepthandler_ty handler = (excepthandler_ty)asdl_seq_GET(
s->v.TryStar.handlers, i);
location loc = LOC(handler);
/* Set location to the entire line of the except keyword */
location loc = LOCATION(handler->lineno, handler->lineno, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion here:

Suggested change
location loc = LOCATION(handler->lineno, handler->lineno, 0, 0);
location loc = LOCATION(handler->lineno, handler->lineno, handler->col_offset, handler->col_offset + 6); // len("except")

This highlights the except keyword.

For plain except you could do the same, or dig the coordinates out of the type expression (a bit harder because there are a lot of possibilities there).

NEW_JUMP_TARGET_LABEL(c, next_except);
except = next_except;
NEW_JUMP_TARGET_LABEL(c, except_with_error);
Expand Down
Loading