Skip to content

gh-146492: Improve SyntaxError for missing comma between import clauses #146493

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
brianschubert wants to merge 6 commits into
base: main
Choose a base branch
from
Open

gh-146492: Improve SyntaxError for missing comma between import clauses #146493

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b4d41c
Improve SyntaxError for missing comma between import clauses
brianschubert Mar 26, 2026
a52ae91
Add news entry
brianschubert Mar 26, 2026
66e3bec
Tweak news
brianschubert Mar 27, 2026
e0b46ea
Additional tests
brianschubert Mar 28, 2026
ca28947
Exclude trailing invalid import targets
brianschubert Mar 28, 2026
1fae8ed
Fix other "cannot use name as import target" cases
brianschubert Mar 29, 2026
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
4 changes: 4 additions & 0 deletions Grammar/python.gram
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,10 +1432,14 @@ invalid_import:
| 'import' token=NEWLINE {
RAISE_SYNTAX_ERROR_STARTING_FROM(token, "Expected one or more names after 'import'") }
invalid_dotted_as_name:
| a=dotted_name b=['as' NAME] c=dotted_name {
RAISE_SYNTAX_ERROR_KNOWN_RANGE(b ? (expr_ty) b : a, c, "expected comma between import clauses") }
Copy link
Copy Markdown
Member

@pablogsal pablogsal Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Unless I'm missing something, is this rule a bit too broad now?

compile("import a b()", "<repro>", "exec")
compile("import a b + c", "<repro>", "exec")
compile("from x import a b[c]", "<repro>", "exec")

This prints:

expected comma between import clauses | line=1 col=8 end_line=1 end_col=11
expected comma between import clauses | line=1 col=8 end_line=1 end_col=11
expected comma between import clauses | line=1 col=15 end_line=1 end_col=18

In all three cases, adding a comma still wouldn't make the code valid, so this doesn't seem like a real missing comma between import clauses situation. Should this only trigger when the would-be second clause is actually a valid import target?

Copy link
Copy Markdown
Contributor Author

@brianschubert brianschubert Mar 29, 2026

Choose a reason for hiding this comment

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

Hmm, I see what you mean. I might argue that the missing comma is the "first" error in each of those cases, and that the rest is more-or-less an unrelated additional syntax error. Adding a comma would at least make those cases more correct and move the syntax error location start of the "real" problem. But I'm happy to exclude those if you prefer.

I pushed a commit that prevents this from triggering for those cases. It still triggers for this case:

>>> import a b as c()
  File "<python-input-0>", line 1
    import a b as c()
           ^^^
SyntaxError: expected comma between import clauses

but I'm not sure there's a good way to exclude this too without making it much more complicated.

| dotted_name 'as' !(NAME (',' | ')' | ';' | NEWLINE)) a=expression {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,
"cannot use %s as import target", _PyPegen_get_expr_name(a)) }
invalid_import_from_as_name:
| [NAME 'as'] a=NAME b=NAME {
RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "expected comma between import clauses") }
| NAME 'as' !(NAME (',' | ')' | ';' | NEWLINE)) a=expression {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,
"cannot use %s as import target", _PyPegen_get_expr_name(a)) }
Expand Down
16 changes: 16 additions & 0 deletions Lib/test/test_syntax.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2225,6 +2225,22 @@
Traceback (most recent call last):
SyntaxError: Expected one or more names after 'import'

>>> import a b
Copy link
Copy Markdown
Member

@pablogsal pablogsal Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Are we only pinning the message text for a few simple one-line cases here? For example, these both reproduce the new behavior:

compile("import a, b c", "<repro>", "exec")
compile("from x import (a\n b)", "<repro>", "exec")

with:

expected comma between import clauses | line=1 col=11 end_line=1 end_col=14
expected comma between import clauses | line=1 col=16 end_line=2 end_col=3

But from what I can see, the new tests here don't assert the caret/range at all, and they also don't cover either a later-clause case or the parenthesized multiline form. Would it make sense to add at least one range-aware assertion and one nontrivial shape, so the main user-visible part of the change is actually locked down?

Copy link
Copy Markdown
Contributor Author

@brianschubert brianschubert Mar 29, 2026

Choose a reason for hiding this comment

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

Added some more tests to exercise some other forms and to assert the expected error range. Let me know if there are any other forms you think would be worth covering

Traceback (most recent call last):
SyntaxError: expected comma between import clauses

>>> import a.a as a b.b
Traceback (most recent call last):
SyntaxError: expected comma between import clauses

>>> from x import a b
Traceback (most recent call last):
SyntaxError: expected comma between import clauses

>>> from x import a as a b
Traceback (most recent call last):
SyntaxError: expected comma between import clauses

>>> (): int
Traceback (most recent call last):
SyntaxError: only single target (not tuple) can be annotated
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2026-03-26-17-32-25.gh-issue-146492.zBMZmC.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Improve :exc:`SyntaxError` message for missing comma between import clauses
in :keyword:`import statements <import>`. Patch by Brian Schubert.
Loading
Loading