| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| | 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?
Sorry, something went wrong.
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 '< >'?
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
thanks for looking into this, i'll revert the changes
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Like in the previous PR, check that there is no gap between tokens.
Sorry, something went wrong.
|
|
||
| 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.
Sorry, something went wrong.
|
@skirpichev thanks for the review, i've updated with helper function |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Should i revert the changes for this case ?
Sorry, something went wrong.
There was a problem hiding this comment.
This is my suggestion, yes.
Sorry, something went wrong.
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 😊
Sorry, something went wrong.
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:
Sorry, something went wrong.
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 :)
Sorry, something went wrong.
|
We wait for other PR to be merged first. |
Sorry, something went wrong.
…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, something went wrong.
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 rebase as i used to mess with this in past and this leads to creating my PR dirty that's the reason i built grebase(https://github.com/Aniketsy/grebase) But now i think i got alternative way to do . Thanks! |
Sorry, something went wrong.
|
gentle ping, i've resolved the conflict in parser.c, i think it would be great if we can move forward with this . Thanks :) |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
| "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?
Sorry, something went wrong.
| | 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?
Sorry, something went wrong.
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
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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 == )
Sorry, something went wrong.
| @@ -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.
Sorry, something went wrong.
| @@ -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?
Sorry, something went wrong.
There was a problem hiding this comment.
Can we remove this clause from the NEWS entry?
sure, make sense 👍
Sorry, something went wrong.
| "'%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.
Sorry, something went wrong.
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
Sorry, something went wrong.
|
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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
I have made the requested changes; please review again . |
Sorry, something went wrong.
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
Sorry, something went wrong.
|
@pablogsal gentle ping, please have a look in this, when you get chance. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
fixes #151303