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
+1,027 −680
Open

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

Show file tree
Hide file tree
Changes from all 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
8 changes: 6 additions & 2 deletions Grammar/python.gram
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1432,11 +1432,15 @@ invalid_import:
| 'import' token=NEWLINE {
RAISE_SYNTAX_ERROR_STARTING_FROM(token, "Expected one or more names after 'import'") }
invalid_dotted_as_name:
| dotted_name 'as' !(NAME (',' | ')' | ';' | NEWLINE)) a=expression {
| a=dotted_name b=['as' NAME] c=dotted_name ('as' | ',' | ')' | ';' | NEWLINE) {
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 (',' | ')' | ';' | NEWLINE) {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,
"cannot use %s as import target", _PyPegen_get_expr_name(a)) }
invalid_import_from_as_name:
| NAME 'as' !(NAME (',' | ')' | ';' | NEWLINE)) a=expression {
| [NAME 'as'] a=NAME b=NAME ('as' | ',' | ')' | ';' | NEWLINE) {
RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "expected comma between import clauses") }
| NAME 'as' !(NAME (',' | ')' | ';' | NEWLINE)) a=expression (',' | ')' | ';' | NEWLINE) {
RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,
"cannot use %s as import target", _PyPegen_get_expr_name(a)) }

Expand Down
105 changes: 105 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 @@ -2206,6 +2206,25 @@
Traceback (most recent call last):
SyntaxError: cannot assign to None

# Check that we don't raise a "cannot use name as import target" error
# when there's a syntax error after the import target.

>>> import a as b(-)
Traceback (most recent call last):
SyntaxError: invalid syntax

>>> import a as b None
Traceback (most recent call last):
SyntaxError: invalid syntax

>>> import a as b as c
Traceback (most recent call last):
SyntaxError: invalid syntax

>>> from x import a as b None
Traceback (most recent call last):
SyntaxError: invalid syntax

# Check that we dont raise the "trailing comma" error if there is more
# input to the left of the valid part that we parsed.

Expand All @@ -2225,6 +2244,58 @@
Traceback (most recent call last):
SyntaxError: Expected one or more names after 'import'

# Missing comma between import clauses

>>> 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, b c
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

>>> import a.a as a b.b as 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

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

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

# Check that we don't raise the "missing comma" error for invalid import targets.

>>> import a b()
Traceback (most recent call last):
SyntaxError: invalid syntax

>>> from x import a b[c]
Traceback (most recent call last):
SyntaxError: invalid syntax

>>> (): int
Traceback (most recent call last):
SyntaxError: only single target (not tuple) can be annotated
Expand Down Expand Up @@ -3505,6 +3576,40 @@ def test_ifexp_body_stmt_else_stmt(self):
]:
self._check_error(f"x = {lhs_stmt} if 1 else {rhs_stmt}", msg)

def test_import_missing_comma(self):
self._check_error("import a.a b.b",
"expected comma between import clauses",
offset=8, end_offset=8 + len("a.a b.b"))
self._check_error(
"""if 1:
from x import (
a,
b
c,
)
""",
errtext="expected comma between import clauses",
lineno=4,
end_lineno=5,
offset=17,
end_offset=18,
)
self._check_error(
"""if 1:
from x import (
a,
b as b
c as c,
)
""",
errtext="expected comma between import clauses",
lineno=4,
end_lineno=5,
offset=22,
end_offset=18,
)



class LazyImportRestrictionTestCase(SyntaxErrorTestCase):
"""Test syntax restrictions for lazy imports."""
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