gh-151303 : Improve SyntaxError suggestions for common operator typos and cross-language mistakes - #151375
gh-151303 : Improve SyntaxError suggestions for common operator typos and cross-language mistakes#151375Aniketsy wants to merge 14 commits into
SyntaxError suggestions for common operator typos and cross-language mistakes#151375Conversation
This comment was marked as resolved.
This comment was marked as resolved.
| | invalid_eqeqeq | ||
|
|
||
| invalid_diamond_op: | ||
| | a='<' b='>' { RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant '!=' instead of '<>'?") } |
There was a problem hiding this comment.
Won't this break when __future__.CO_FUTURE_BARRY_AS_BDFL is true?
There was a problem hiding this comment.
hmm , yes
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "
__futu> from __future__ import barry_as_FLUFL
> 1 < > 2
> "
File "<string>", line 3
1 < > 2
^^^
SyntaxError: invalid syntax. Maybe you meant '!=' instead of '<>'?
please let me know your thoughts on this ...
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ git diff Grammar/python.gram
diff --git a/Grammar/python.gram b/Grammar/python.gram
index 61a6d5ea90..19ef406437 100644
--- a/Grammar/python.gram
+++ b/Grammar/python.gram
@@ -805,7 +805,11 @@ compare_op_bitwise_or_pair[CmpopExprPair*]:
| invalid_eqeqeq
invalid_diamond_op:
- | a='<' b='>' { RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant '!=' instead of '<>'?") }
+ | a='<' b='>' {
+ (p->flags & PyPARSE_BARRY_AS_BDFL)
+ ? RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant '<>' instead of '< >'?")
+ : RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant '!=' instead of '<>'?")
+ }
eq_bitwise_or[CmpopExprPair*]: '==' a=bitwise_or { _PyPegen_cmpop_expr_pair(p, Eq, a) }
invalid_eqeqeq:
| a='==' b='=' { RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant '==' instead of '==='?") }
output:
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "1 < > 2"
File "<string>", line 1
1 < > 2
^^^
SyntaxError: invalid syntax. Maybe you meant '!=' instead of '<>'?
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "
__futu> from __future__ import barry_as_FLUFL
> 1 < > 2
> "
File "<string>", line 3
1 < > 2
^^^
SyntaxError: invalid syntax. Maybe you meant '<>' instead of '< >'?
There was a problem hiding this comment.
hmm, yes
No, this example doesn't break __future__ import. But I suspect, that __future__.CO_FUTURE_BARRY_AS_BDFL is the reason that your code doesn't work without spaces. This can be "fixed" by this patch:
diff --git a/Tools/build/generate_token.py b/Tools/build/generate_token.py
index 9ee5ec86e75..28a0ce1ff78 100755
--- a/Tools/build/generate_token.py
+++ b/Tools/build/generate_token.py
@@ -178,7 +178,7 @@ def generate_chars_to_token(mapping, n=1):
def make_c(infile, outfile='Parser/token.c'):
tok_names, ERRORTOKEN, string_to_tok = load_tokens(infile)
- string_to_tok['<>'] = string_to_tok['!=']
+# string_to_tok['<>'] = string_to_tok['!=']
chars_to_token = {}
for string, value in string_to_tok.items():
assert 1 <= len(string) <= 3
Look at the difference:
$ cat a.py
1 =! 2
$ cat a.py | ./python -m tokenize
1,0-1,1: NUMBER '1'
1,2-1,3: OP '='
1,3-1,4: OP '!'
1,5-1,6: NUMBER '2'
1,6-1,7: NEWLINE '\n'
2,0-2,0: ENDMARKER ''
vs
$ cat a.py
1 <> 2
$ cat a.py | ./python -m tokenize
1,0-1,1: NUMBER '1'
1,2-1,4: OP '<>'
1,5-1,6: NUMBER '2'
1,6-1,7: NEWLINE '\n'
2,0-2,0: ENDMARKER ''
Regardless on a joke, it looks as a tokenizer bug. I'll open a separate issue.
There was a problem hiding this comment.
thanks for looking into this, i'll revert the changes
There was a problem hiding this comment.
No, this example doesn't break future import.
It still does break it, we're suggesting != for < > which is invalid syntax when __future__.CO_FUTURE_BARRY_AS_BDFL is true.
There was a problem hiding this comment.
Yes, this is wrong, see my comment below. But nothing to do with __future__.barry_as_FLUFL.
Currently, we have a hidden token (<> is alias for !=) in the grammar. So, this patch will not produce an error without a space between. See #151464.
This comment was marked as resolved.
This comment was marked as resolved.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Like in the previous PR, check that there is no gap between tokens.
|
|
||
| def test_diamond_operator(self): | ||
| self._check_error( | ||
| "1 < > 2", |
There was a problem hiding this comment.
There should not be space in the middle.
|
@skirpichev thanks for the review, i've updated with helper function |
| if (strcmp(tok_str, "<>") == 0) { | ||
| RAISE_SYNTAX_ERROR("invalid syntax. Are you trying to overthrow the SC? Use operator \"!=\"!"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
I would prefer not use this workaround for #151464. Leave "<>" case for another PR.
There was a problem hiding this comment.
Should i revert the changes for this case ?
There was a problem hiding this comment.
I would prefer not use this workaround for #151464. Leave "<>" case for another PR.
@skirpichev are you planning to work on this PR as issue was assigned to you, or i can open a follow up pr after this . please let me know 😊
There was a problem hiding this comment.
Feel free to do this.
I'm waiting for opinion of core developers. Beware that posted patch is only a partial solution. Proper support of this easter egg will require much more work. Do we need this? I'm not sure. Other options:
- call this not a bug
- remove joke.
There was a problem hiding this comment.
I'm waiting for opinion of core developers.
in that case i think i should also wait for the opinion.
Beware that posted patch is only a partial solution. Proper support of this easter egg will require much more work.
and meanwhile, i'll explore this, and will update you in the issue ... Thanks :)
|
We wait for other PR to be merged first. |
…e-151303.mzlJxi.rst
|
For future -- never use rebase, amend and force-push. This forces us to review the entire PR from the beginning, instead of just the new changes. |
sorry for creating extra works, i didn't know we can solve conflicts without rebase, i'll keep in mind and i always wanted to never use |
|
gentle ping, i've resolved the conflict in |
| def test_diamond_operator(self): | ||
| self._check_error( | ||
| "1<>2", | ||
| "invalid syntax", | ||
| lineno=1, | ||
| end_lineno=1, | ||
| offset=2, | ||
| end_offset=4, | ||
| ) | ||
|
|
||
| def test_diamond_operator_barry_as_flufl(self): |
There was a problem hiding this comment.
I think you should remove these tests.
There was a problem hiding this comment.
yeah make sense , as we have left this fix for other PR. thanks for pointing , i'll remove
| "invalid syntax", | ||
| lineno=2, | ||
| end_lineno=2, | ||
| offset=5, |
There was a problem hiding this comment.
Unless I am missing something, this _check_error lost the end_offset=6 check it had before this PR , looks like an accident from moving the new tests around. Can you restore it?
| | invalid_eqeqeq | ||
|
|
||
| eq_bitwise_or[CmpopExprPair*]: '==' a=bitwise_or { _PyPegen_cmpop_expr_pair(p, Eq, a) } | ||
| invalid_eqeqeq: |
There was a problem hiding this comment.
Nit: we define the invalid_* rules below the START OF INVALID RULES marker (see how invalid_bitwise_and is referenced from bitwise_and but defined there). Can we move invalid_eqeqeq to that section and regenerate?
There was a problem hiding this comment.
Can we move invalid_eqeqeq to that section and regenerate?
sure, thanks for pointing out, i'll update
| invalid_eqeqeq: | ||
| | a='==' b='=' { | ||
| _PyPegen_tokens_are_adjacent(a, b) | ||
| ? RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, b, "invalid syntax. Maybe you meant 'is' instead of '==='?") |
There was a problem hiding this comment.
Why are we suggesting is here instead of ==? In JavaScript and PHP === is (strict) equality, so == is what these users almost always mean, and suggesting is can push people into x is 'foo' identity bugs, no?
There was a problem hiding this comment.
In JavaScript and PHP === is (strict) equality, so == is what these users almost always mean, and suggesting is can push people into x is 'foo' identity bugs, no?
thanks, this is something interesting 😄 we missed, and make sense (being honest, i got confused for sometime, but yes we should use == )
| @@ -0,0 +1 @@ | |||
| Improve :exc:`SyntaxError` messages for common operator typos coming from other languages: ``=<``, ``=>``, and ``=!`` now suggest ``<=``, ``>=``, and ``!=`` respectively, ``===`` suggests ``is``, and ``<>`` shows a tailored message suggesting ``!=``. | |||
There was a problem hiding this comment.
The NEWS entry still says that <> shows a tailored message suggesting !=, but that part was reverted , 1<>2 still raises plain invalid syntax and your own test_diamond_operator checks exactly that. Please, update the entry to drop the <> part.
| @@ -0,0 +1 @@ | |||
| Improve :exc:`SyntaxError` messages for common operator typos coming from other languages: ``=<``, ``=>``, and ``=!`` now suggest ``<=``, ``>=``, and ``!=`` respectively, ``===`` suggests ``is``, and ``<>`` shows a tailored message suggesting ``!=``. | |||
There was a problem hiding this comment.
<> still reports plain invalid syntax; the tailored message was reverted. Can we remove this clause from the NEWS entry?
There was a problem hiding this comment.
Can we remove this clause from the NEWS entry?
sure, make sense 👍
| "'%s' is an illegal expression for augmented assignment", | ||
| _PyPegen_get_expr_name(a) | ||
| )} | ||
| | star_expressions a='=' b='<' { |
There was a problem hiding this comment.
These only fire for bare statements, so if a => b: still gives the generic error, no? That is probably fine for a first iteration (the = < tokenization makes it look like an assignment), but maybe we should add a test documenting it or a follow-up issue for the if/while case.
There was a problem hiding this comment.
i'll open a follow up issue for this, please let me know if you lean towards adding test i fine with that too
|
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 |
|
I have made the requested changes; please review again . |
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
fixes #151303
SyntaxErrorsuggestions for common operator typos and cross-language mistakes #151303