Repository navigation
Minor improvement to errors with (no) missing colon #92858
Description
Activity
cc @pablogsal
Would it be OK for me to submit a PR making these changes?
Yeah, go ahead! Thanks for looking into this :)
Take care because the reason this may not be already made is because there may be some hairy incompatibility down the line (I cannot remember any so it is likely that I just forgot) so make sure to run the test suite :)
Reacted by wookie184 and ajoinoNote that the error is much friendlier with Python 3.11's (edit: meant 3.10) better tracebacks, since the carets point the location of the error, so you can't confuse it for another colon on the line:
λ python3.11 Python 3.11.0b1+ (heads/3.11:6546af3, May 10 2022, 12:01:20) [Clang 13.0.0 (clang-1300.0.27.3)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> with block ad something: pass File "<stdin>", line 1 with block ad something: pass ^^ SyntaxError: expected ':'which I think I prefer compared to the error in the proposed PR:
λ ./python.exe Python 3.12.0a0 (heads/improve-error-message:c2e406f851, May 17 2022, 16:27:27) [Clang 13.0.0 (clang-1300.0.27.3)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> with block ad something: pass File "<stdin>", line 1 with block ad something: pass ^^ SyntaxError: invalid syntaxI read
SyntaxError: expected ':'as "you probably meant to put a colon there", so felt it makes most sense to only use it where that's likely to be the issue.In cases other than that I don't think the fact that the parser expected a colon adds much information over a plain syntax error (and may be slightly confusing).
Note that the error is much friendlier with Python 3.11's better tracebacks
Unless I am missing something very fundamental, there is nothing in 3.11 for this, this should work also on Python 3.10 as I added the changes required for range highlighting in syntax errors early in the 3.10 dev cycle.
I read SyntaxError: expected ':' as "you probably meant to put a colon there", so felt it makes most sense to only use it where that's likely to be the issue.
Technically means "the parser expected a colon here" which can be compatible with a colon being late in the line. Is not technically a suggestion but more than an explanation on what's goin on.
In cases other than that
Sorry, not sure I follow. What cases are you referring to? The example that you showed? In general, missing colon errors have been one of the most requested specialized error messages so I think it does make sense to have them appear instead of "invalid syntax" as much as possible.
Regarding this case that you show: the problem is that currently we have some suites working like you show and others not working like that (the ones using the mode that you are using in your PR). In general, I prefer to be consistent if we can, because that simplifies expectations, which is an important (although not fundamenteal) part of the UX here.
Sorry, not sure I follow. What cases are you referring to? The example that you showed? In general, missing colon errors have been one of the most requested specialized error messages so I think it does make sense to have them appear instead of "invalid syntax" as much as possible.
Sorry, I misspoke there (remove 'in cases'). I was trying to say that I think the missing colon error isn't really helpful when the solution isn't to add/move a colon.
In the case mentioned,
with block ad something: passthere is a syntax error before the colon (adshould beas) and the fact the parser expected a colon isn't really relevant (there's already a colon).It also seemed to be a similar case for pretty much all cases that my PR changes (see added test cases on the PR). The only exception I could think of is where there is a colon missing and the body is on the same line, e.g.
for i in range(10) print(i)but that's ambiguous and sort of ugly anyway.There are cases such as
for i in range 10which are missing a colon and also otherwise syntactically invalid. I think the more general error is also nicer here as the issue isn't just a missing colon.Regarding this case that you show: the problem is that currently we have some suites working like you show and others not working like that (the ones using the mode that you are using in your PR). In general, I prefer to be consistent if we can, because that simplifies expectations, which is an important (although not fundamenteal) part of the UX here.
Agreed. I felt changing the
for-like behaviour to match thewhile-like behaviour made more sense rather than the other way round, although it seems others may disagree. I'd be happy to change to whatever the consensus is (if it's decided any change should be made).Agreed. I felt changing the
for-like behaviour to match thewhile-like behaviour made more sense rather than the other way round, although it seems others may disagree. I'd be happy to change to whatever the consensus is (if it's decided any change should be made).Yeah, I think is going to be better to do follow what while loops are doing. The errors that way are less surprising 👍
I will review your PR soon. Thanks a lot for working on this :)
Reacted by wookie184- added 5 commits that reference this issue
on Jun 23, 2022 - added a commit that references this issue
on Jun 26, 2022
Some invalid
forstatements currently give the specific errorexpected ':'even if there is a colon on the line. I think it would be a slight improvement to raise a more general error in these cases. This is howwhileandifalready behave.Current behaviour:
Proposed behaviour:
Proposed Implementation:
Would do it the same way
whileandifdo it. For the for loop example it would be something like this:I'd also apply this to other suites where it makes sense (e.g.
with,match/case, andtry)Would it be OK for me to submit a PR making these changes? (this is my first time looking into the CPython parser so apologies if I made any obvious mistakes!)