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

iritkatriel · 2025-01-19T14:38:15Z

Issue: Too wide location info for CHECK_EG_MATCH #129025

…or and

markshannon

Looks good.
I have one question about the choice of location.

markshannon · 2025-01-20T16:06:24Z

Python/codegen.c

@@ -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);


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

Is this legal syntax?

except *\ ValueError:

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.

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?

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.

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.

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

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

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*?

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.)

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.

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.

gvanrossum · 2025-01-24T21:42:32Z

Lib/test/test_traceback.py

+                    f'    ~~~~~~~~~~~~~~~~~~~~~^^\n'
+                    f'  File "{__file__}", line {exc.__code__.co_firstlineno + 7}, in exc\n'
+                    f'    except* ValueError:\n'
+                    f'    \n'


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.

gvanrossum · 2025-01-24T21:55:58Z

Python/codegen.c

@@ -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);


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.

gvanrossum · 2025-01-24T22:14:28Z

Python/codegen.c

@@ -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);


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.

gvanrossum · 2025-01-24T22:33:38Z

Python/codegen.c

@@ -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);


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).

iritkatriel added type-bug An unexpected behavior, bug, or error needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jan 19, 2025

iritkatriel requested a review from markshannon as a code owner January 19, 2025 14:38

bedevere-app bot mentioned this pull request Jan 19, 2025

Too wide location info for CHECK_EG_MATCH #129025

Open

bedevere-app bot added the awaiting core review label Jan 19, 2025

iritkatriel requested a review from gvanrossum January 19, 2025 14:39

iritkatriel mentioned this pull request Jan 19, 2025

Unexpected location of naked exception wrapped in except* block #128799

Closed

pythongh-129025: fix too wide source location for bytecodes emitted f…

6933f38

…or and

iritkatriel force-pushed the except_lineno branch from c6018f3 to 6933f38 Compare January 19, 2025 16:27

markshannon reviewed Jan 20, 2025

View reviewed changes

iritkatriel changed the title ~~gh-129025: fix too wide source location for bytecodes emitted for and~~ gh-129025: fix too wide source location for bytecodes emitted for except* Jan 20, 2025

revert change to test_exceptions

bf78fe0

iritkatriel mentioned this pull request Jan 21, 2025

Add to AST location info for compound exception headers or keywords #129157

Open

gvanrossum reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iritkatriel commented Jan 19, 2025 •

edited by bedevere-app bot

Loading

markshannon left a comment

markshannon Jan 20, 2025

iritkatriel Jan 20, 2025

markshannon Jan 20, 2025

iritkatriel Jan 20, 2025

markshannon Jan 21, 2025

iritkatriel Jan 22, 2025

iritkatriel Jan 22, 2025

gvanrossum Jan 24, 2025

iritkatriel Jan 24, 2025 •

edited

Loading

gvanrossum Jan 24, 2025

gvanrossum Jan 24, 2025

gvanrossum Jan 24, 2025

gvanrossum Jan 24, 2025

gvanrossum Jan 24, 2025

	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")

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

Are you sure you want to change the base?

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

Conversation

iritkatriel commented Jan 19, 2025 • edited by bedevere-app bot Loading

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel commented Jan 19, 2025 •

edited by bedevere-app bot

Loading

iritkatriel Jan 24, 2025 •

edited

Loading