gh-146495: Improve SyntaxError message for && and || operators

[Aniketsy]

fixes #146495

@Aniketsy I would prefer that a parser expert takes care of that. This is likely a non-trivial change so to make it easier to review, it would be better if a core dev or someone familiar with the parser takes care of this.

As there was not any pr for this for long time, so i thought to give a try for this as @skirpichev have shared the patch towards fix. Hoping it does not create much pain to review for members. Thank you !

• Issue: Consider specialized syntax error for && and || #146495

[sobolevn]

I like the idea! Before it was:

[pablogsal]

Thanks for the PR @Aniketsy! Unfortunately this is not the correct approach for such an error message. See my comment below

[bedevere-app]

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.

[serhiy-storchaka]

LGTM. 👍

As I proposed in #151129 (comment), we can also add suggestions for <> ("not equal" in Pascal and Python 2), "=<", "=>" and "=!" (typo in "<=", ">=" and "!="), "===" ("is" in JavaScript and PHP).

[Aniketsy]

LGTM. 👍

Thanks!

As I proposed in #151129 (comment), we can also add suggestions for <> ("not equal" in Pascal and Python 2), "=<", "=>" and "=!" (typo in "<=", ">=" and "!="), "===" ("is" in JavaScript and PHP).

I'd be happy to create a follow up PR or should i open issue and then link pr for this

[skirpichev]

"===" ("is" in JavaScript and PHP).

And Mathematica.

I'd be happy to create a follow up PR or should i open issue and then link pr for this

Yes, please open a separate issue.

[sobolevn]

Thank you!

[serhiy-storchaka]

I'd be happy to create a follow up PR or should i open issue and then link pr for this

Up to you. You can also include them in this PR.

Second look.

[skirpichev]

Sorry, this doesn't look acceptable for me:

>>> 1 & & 2
  File "<python-input-1>", line 1
    1 & & 2
      ^^^
SyntaxError: invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?

I think you should raise a different exception message only for cases, when '&&' or '||' could be interpreted as distinct tokens.

[Aniketsy]

Sorry, this doesn't look acceptable for me:

>>> 1 & & 2
  File "<python-input-1>", line 1
    1 & & 2
      ^^^
SyntaxError: invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?

I think you should raise a different exception message only for cases, when '&&' or '||' could be interpreted as distinct tokens.

should we fall back to generic error message if we have gap between them ?

[skirpichev]

should we fall back to generic error message if we have gap between them ?

Yes, I think so.

@serhiy-storchaka, @sobolevn ?

[Aniketsy]

with this patch

aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ git diff Grammar/python.gram
diff --git a/Grammar/python.gram b/Grammar/python.gram
index c45e2f3a28..823af82397 100644
--- a/Grammar/python.gram
+++ b/Grammar/python.gram
@@ -1642,7 +1642,19 @@ invalid_type_params:
             "Type parameter list cannot be empty")}

 invalid_bitwise_and:
-    | a=bitwise_and b='&' c='&' { RAISE_SYNTAX_ERROR_KNOWN_RANGE(b, c, "invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?") }
+    | a=bitwise_and b='&' c='&' {
+        (b->end_lineno == c->lineno)
+        ? ((b->end_col_offset == c->col_offset)
+            ? RAISE_SYNTAX_ERROR_KNOWN_RANGE(b, c, "invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?")
+            : NULL)
+        : NULL
+    }

 invalid_bitwise_or:
-    | a=bitwise_or b='|' c='|' { RAISE_SYNTAX_ERROR_KNOWN_RANGE(b, c, "invalid syntax.  Maybe you meant 'or' or '|' instead of '||'?") }
+    | a=bitwise_or b='|' c='|' {
+        (b->end_lineno == c->lineno)
+        ? ((b->end_col_offset == c->col_offset)
+            ? RAISE_SYNTAX_ERROR_KNOWN_RANGE(b, c, "invalid syntax.  Maybe you meant 'or' or '|' instead of '||'?")
+            : NULL)
+        : NULL
+    }

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 'and' or '&' instead of '&&'?
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "1 & & 2"
  File "<string>", line 1
    1 & & 2
        ^
SyntaxError: invalid syntax
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "1 || 2"
  File "<string>", line 1
    1 || 2
      ^^
SyntaxError: invalid syntax.  Maybe you meant 'or' or '|' instead of '||'?
aniket@DESKTOP-074O80J:/mnt/d/cpython/cpython$ ./python.exe -c "1 | | 2"
  File "<string>", line 1
    1 | | 2
        ^
SyntaxError: invalid syntax

previously i tried something like this but it was throwing warning during compiling

invalid_bitwise_and:
   | a=bitwise_and b='&' c='&' {
        (b->end_lineno == c->lineno && b->end_col_offset == c->col_offset)
        ? RAISE_SYNTAX_ERROR_KNOWN_RANGE(b, c, "invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?")
        : NULL
    }

these were the warning :--

lude    -DPy_BUILD_CORE -o Parser/parser.o Parser/parser.c
Parser/parser.c: In function ‘invalid_bitwise_and_rule’:
Parser/parser.c:28511:77: warning: comparison between pointer and integer
28511 |             _res = ( b -> end_lineno == c -> lineno & & b -> end_col_offset == c -> col_offset ) ? RAISE_SYNTAX_ERROR_KNOWN_RANGE ( b , c , "invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?" ) : NULL;
      |                                                                             ^~
Parser/parser.c:28511:38: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]
28511 |             _res = ( b -> end_lineno == c -> lineno & & b -> end_col_offset == c -> col_offset ) ? RAISE_SYNTAX_ERROR_KNOWN_RANGE ( b , c , "invalid syntax.  Maybe you meant 'and' or '&' instead of '&&'?" ) : NULL;
      |                      ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~
Parser/parser.c: In function ‘invalid_bitwise_or_rule’:
Parser/parser.c:28560:77: warning: comparison between pointer and integer
28560 |             _res = ( b -> end_lineno == c -> lineno & & b -> end_col_offset == c -> col_offset ) ? RAISE_SYNTAX_ERROR_KNOWN_RANGE ( b , c , "invalid syntax.  Maybe you meant 'or' or '|' instead of '||'?" ) : NULL;
      |                                                                             ^~
Parser/parser.c:28560:38: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]
28560 |             _res = ( b -> end_lineno == c -> lineno & & b -> end_col_offset == c -> col_offset ) ? RAISE_SYNTAX_ERROR_KNOWN_RANGE ( b , c , "invalid syntax.  Maybe you meant 'or' or '|' instead of '||'?" ) : NULL;
      |                      ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~

[skirpichev]

with this patch

Yes, like that. Though, I think it could be factored to a helper function.

these were the warning

You should take into account operator precedence rules in C.

[serhiy-storchaka]

The code was correct, but errorprone, thus the compiler warning. Simply add parentheses: ((b->end_lineno == c->lineno) && (b->end_col_offset == c->col_offset)).

[serhiy-storchaka]

Hmm, it seems there is a bug in the python.gram parser or the Parser/parser.c generator which parses && in C code as two tokens & and & and then inserts a space between them.

[serhiy-storchaka]

Anyway, if you add a helper that tests this condition in Parser/pegen.h, you can simply use it in python.gram.

[sobolevn]

In my opinion && and & & are both incorrect, the error message is good enough.

[Aniketsy]

In my opinion && and & & are both incorrect, the error message is good enough.

+1 make sense

Hmm, it seems there is a bug in the python.gram parser or the Parser/parser.c generator which parses && in C code as two tokens & and & and then inserts a space between them.

should we take care of this in follow up

[Aniketsy]

Also please let me know your thoughts on this as well #151375

[skirpichev]

In my opinion && and & & are both incorrect, the error message is good enough.

I typed '& &', not '&&'. And maybe actually meant something like '& 7' (a typo).

[skirpichev]

Add tests with space between operators, e.g. a & & b.

[serhiy-storchaka]

This is actually a regexp. So escape ? (in other test too).