gh-146492: Improve SyntaxError for missing comma between import clauses#146493
gh-146492: Improve SyntaxError for missing comma between import clauses#146493brianschubert wants to merge 3 commits intopython:mainfrom
Conversation
| 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") } |
There was a problem hiding this comment.
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?
| Traceback (most recent call last): | ||
| SyntaxError: Expected one or more names after 'import' | ||
|
|
||
| >>> import a b |
There was a problem hiding this comment.
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?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Demo: