← 返回首页
bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE… by pablogsal · Pull Request #20020 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE… #20020

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

Closed
pablogsal wants to merge 1 commit into from
Conversation 10 Commits 1 Checks 0 Files changed
Closed

bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE… #20020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .c  (2) .gram  (1) All 2 file types selected Viewed files
Conversations
Failed to load comments. Retry
Loading
Jump to
Jump to file
Failed to load files. Retry
Loading
Diff view
Unified
Split
Hide whitespace
Apply and reload
Show whitespace
Diff view
Unified
Split
Hide whitespace
Apply and reload
2 changes: 1 addition & 1 deletion Grammar/python.gram
Show comments
View file Edit file Delete file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ t_atom[expr_ty]:
incorrect_arguments:
| args ',' '*' { RAISE_SYNTAX_ERROR("iterable argument unpacking follows keyword argument unpacking") }
| expression for_if_clauses ',' [args | expression for_if_clauses] {
RAISE_SYNTAX_ERROR("Generator expression must be parenthesized") }
RAISE_SYNTAX_ERROR_NO_COL_OFFSET("Generator expression must be parenthesized") }

pablogsal May 10, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I made this change in this PR because I was changing this error when I realized that the caret was always pointing to the beginning of the line when using RAISE_SYNTAX_ERROR_NO_COL_OFFSET

gvanrossum May 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

So this is a test of the PR and should not be committed?

pablogsal May 10, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

So this is a test of the PR and should not be committed?

I was proposing to commit this in tandem with the other change because currently, the caret points to the end of the call and that seems wrong to me:

>>> f(x for x in range(10), x, y, z) File "<console>", line 1 f(x for x in range(10), x, y, z) ^ SyntaxError: Generator expression must be parenthesized

pablogsal May 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Also, the old parser doesn't show a column offset:

./python -X oldparser Python 3.9.0a6+ (heads/no-collisions:6530eb2be9, May 10 2020, 01:41:04) >>> f(x for x in range(10), x, y, z) File "<console>", line 1 SyntaxError: Generator expression must be parenthesized

gvanrossum May 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

But omitting the column seems wrong too (there could be multiple calls on the line). Ideally the caret should point to the first character after the (.

pablogsal May 10, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Ideally the caret should point to the first character after the (.

In this case, I agree, but what about something like

f(a, b, c, x for x in range(10), x, y, z)

should it point to the beginning of x for x in range(10) or to the first caracter after the (?

gvanrossum May 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

IMO it's still better if it points to the start than to the end, because that's closer to the function name.

gvanrossum May 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I finally solved this mystery, I think.

The reason the old parser doesn't show the caret is because you tried it in the REPL. It seems the old parser doesn't show the source line for errors generated in some later pass (ast.c or the bytecode compiler), because the code that retrieves the source line reads the source file again. Compare these two:

~/cpython$ ./python.exe -X oldparser Python 3.9.0a6+ (heads/master:f453221c8b, May 12 2020, 10:45:53) [Clang 11.0.0 (clang-1100.0.33.8)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> (pass) File "<stdin>", line 1 (pass) ^ SyntaxError: invalid syntax >>> f() = 1 File "<stdin>", line 1 SyntaxError: cannot assign to function call >>>

Only the first of these shows the source line and the caret, because the error is detected by the (old) parser. The second comes from ast.c and there the attempt to retrieve the source line fails, causing the text member of the SyntaxError object to be None.

But if you put the second syntax error in a file, the old parser shows the source line and the caret (and as I've shown, the caret is in the right place too).

I'm going to close this PR; @lysnikolaou will fix it properly in #20050.
| a=args ',' args { _PyPegen_arguments_parsing_error(p, a) }
invalid_kwarg:
| expression '=' { RAISE_SYNTAX_ERROR("expression cannot contain assignment, perhaps you meant \"==\"?") }
Expand Down
2 changes: 1 addition & 1 deletion Parser/pegen/parse.c
Show comments
View file Edit file Delete file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -10550,7 +10550,7 @@ incorrect_arguments_rule(Parser *p)
(opt_var = _tmp_125_rule(p), 1) // [args | expression for_if_clauses]
)
{
res = RAISE_SYNTAX_ERROR ( "Generator expression must be parenthesized" );
res = RAISE_SYNTAX_ERROR_NO_COL_OFFSET ( "Generator expression must be parenthesized" );
if (res == NULL && PyErr_Occurred()) {
p->error_indicator = 1;
return NULL;
Expand Down
2 changes: 1 addition & 1 deletion Parser/pegen/pegen.c
Show comments
View file Edit file Delete file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ _PyPegen_raise_error(Parser *p, PyObject *errtype, int with_col_number, const ch
}
col_number = byte_offset_to_character_offset(loc, col_offset);
}
else if (!loc) {
else {
Py_INCREF(Py_None);
loc = Py_None;
}
Expand Down
Toggle all file notes Toggle all file annotations

Footer

© 2026 GitHub, Inc.