gh-146582: Improve syntax error for non-case statement inside match statement#146583
gh-146582: Improve syntax error for non-case statement inside match statement#146583brianschubert wants to merge 2 commits intopython:mainfrom
case statement inside match statement#146583Conversation
|
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 |
Grammar/python.gram
Outdated
| | "case" patterns guard? NEWLINE { RAISE_SYNTAX_ERROR("expected ':'") } | ||
| | a="case" patterns guard? ':' NEWLINE !INDENT { | ||
| RAISE_INDENTATION_ERROR("expected an indented block after 'case' statement on line %d", a->lineno) } | ||
| | !"case" { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("expected 'case' statement inside 'match' statement") } |
There was a problem hiding this comment.
Unless I'm missing something, doesn't this now hijack later syntax errors after an otherwise valid match body?
I checked it on the built branch with:
compile("match 1:\n case 1:\n pass\n1 2\n", "<repro>", "exec")and got:
SyntaxError: expected 'case' statement inside 'match' statement @ line 4, col 3
I also checked:
compile("match 1:\n case 1:\n pass\ncase 2:\n pass\n", "<repro>", "exec")and got:
SyntaxError: expected 'case' statement inside 'match' statement @ line 4, col 6
Shouldn't those still be reported as the later syntax error / top-level case error instead of being turned into the new match-body message?
There was a problem hiding this comment.
yikes, good catch! I didn't consider that this could match the trailing DEDENT. Pushed a commit that should fix this case
Grammar/python.gram
Outdated
| | "case" patterns guard? NEWLINE { RAISE_SYNTAX_ERROR("expected ':'") } | ||
| | a="case" patterns guard? ':' NEWLINE !INDENT { | ||
| RAISE_INDENTATION_ERROR("expected an indented block after 'case' statement on line %d", a->lineno) } | ||
| | !"case" { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("expected 'case' statement inside 'match' statement") } |
There was a problem hiding this comment.
Unless I'm missing something, is this still incomplete for invalid statements that start with soft-keyword case?
For example with
compile("match 1:\n case = 1\n", "<repro>", "exec")
compile("match 1:\n case += 1\n", "<repro>", "exec")
compile("match 1:\n case, x = 1, 2\n", "<repro>", "exec")you get:
SyntaxError: invalid syntax @ line 2, col 10
SyntaxError: invalid syntax @ line 2, col 10
SyntaxError: invalid syntax @ line 2, col 9
So the new diagnostic still doesn't seem to catch these case-starting non-case statements inside a match body. Should those be covered too?
There was a problem hiding this comment.
Should those be covered too?
Hmm, possibly? It occurred that me that this wouldn't catch cases like this, but I figured that this would be tricky to do right and only represents a vanishingly small set of statements in practice, so I didn't give it much thought.
Do you think it's worth trying to catch these too? I'd worry that a rule that matches these cases would also run the risk of matching an actual malformed case statement, which could make for a rather confusing error for the user
Lib/test/test_syntax.py
Outdated
| "a = 1", | ||
| "if a:", | ||
| "else:", | ||
| "match 1:" |
There was a problem hiding this comment.
Unless I'm reading this wrong, is this test case actually exercising a multiline nested match?
I tried the same adjacent-string shape used here:
stmt_list = [
"a",
"a = 1",
"if a:",
"else:",
"match 1:"
"pass",
]
print(repr(stmt_list[4]))and got:
'match 1:pass'
So this looks like string literal concatenation, not a multiline nested match.
There was a problem hiding this comment.
ah, that was a typo. It was supposed to exercise a single-line (incomplete) match statement. Pushed a commit that adds the missing comma
Demo:
casestatement insidematchstatement #146582