bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE…#20020

pablogsal

Before:

>>> f(x for x in range(10),)
  File "<console>", line 1
    f(x for x in range(10),)
    ^
SyntaxError: Generator expression must be parenthesized

With this PR:

>>> f(x for x in range(10),)
  File "<console>", line 1
SyntaxError: Generator expression must be parenthesized

https://bugs.python.org/issue40334

I made this change in this PR because I was changing this error when I realized that the caret was always pointing to the beginning of the line when using RAISE_SYNTAX_ERROR_NO_COL_OFFSET

So this is a test of the PR and should not be committed?

I was proposing to commit this in tandem with the other change because currently, the caret points to the end of the call and that seems wrong to me:

>>> f(x for x in range(10), x, y, z) File "<console>", line 1 f(x for x in range(10), x, y, z) ^ SyntaxError: Generator expression must be parenthesized

Also, the old parser doesn't show a column offset:

./python -X oldparser Python 3.9.0a6+ (heads/no-collisions:6530eb2be9, May 10 2020, 01:41:04) >>> f(x for x in range(10), x, y, z) File "<console>", line 1 SyntaxError: Generator expression must be parenthesized

But omitting the column seems wrong too (there could be multiple calls on the line). Ideally the caret should point to the first character after the (.

Ideally the caret should point to the first character after the (.

In this case, I agree, but what about something like

f(a, b, c, x for x in range(10), x, y, z)

should it point to the beginning of x for x in range(10) or to the first caracter after the (?

IMO it's still better if it points to the start than to the end, because that's closer to the function name.

@lysnikolaou

I finally solved this mystery, I think.

The reason the old parser doesn't show the caret is because you tried it in the REPL. It seems the old parser doesn't show the source line for errors generated in some later pass (ast.c or the bytecode compiler), because the code that retrieves the source line reads the source file again. Compare these two:

~/cpython$ ./python.exe -X oldparser Python 3.9.0a6+ (heads/master:f453221c8b, May 12 2020, 10:45:53) [Clang 11.0.0 (clang-1100.0.33.8)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> (pass) File "<stdin>", line 1 (pass) ^ SyntaxError: invalid syntax >>> f() = 1 File "<stdin>", line 1 SyntaxError: cannot assign to function call >>>

Only the first of these shows the source line and the caret, because the error is detected by the (old) parser. The second comes from ast.c and there the attempt to retrieve the source line fails, causing the text member of the SyntaxError object to be None.

But if you put the second syntax error in a file, the old parser shows the source line and the caret (and as I've shown, the caret is in the right place too).

I'm going to close this PR; @lysnikolaou will fix it properly in #20050.

pablogsal

Wow, I am confused about this failing test in test_cmd_line_script:

def test_syntaxerror_unindented_caret_position(self): script = "1 + 1 = 2\n" with support.temp_dir() as script_dir: script_name = _make_test_script(script_dir, 'script', script) exitcode, stdout, stderr = assert_python_failure(script_name) text = io.TextIOWrapper(io.BytesIO(stderr), 'ascii').read() # Confirm that the caret is located under the first 1 character self.assertIn("\n 1 + 1 = 2\n ^", text)

that expects that the caret is on the first character but the error in invalid_assignment is using RAISE_SYNTAX_ERROR_NO_COL_OFFSET. @lysnikolaou Does RAISE_SYNTAX_ERROR_NO_COL_OFFSET means "put the column offset to the beginning" then? If so, I find that somewhat confusing.

gvanrossum

Wow, I am confused about this failing test in test_cmd_line_script:

That's a bug. The old parser puts the caret at the start of the "target". Example:

File "/Users/guido/cpython/@.py", line 2 if a: 1 + 1 = 2 ^ SyntaxError: cannot assign to operator

bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE… …

d3de8ad

…T is used

the-knights-who-say-ni added the CLA signed label May 10, 2020

bedevere-bot added the awaiting core review label May 10, 2020

pablogsal requested review from gvanrossum and lysnikolaou May 10, 2020 00:37

pablogsal commented May 10, 2020

View reviewed changes

pablogsal added the skip news label May 10, 2020

pablogsal mentioned this pull request May 10, 2020

Implementation of RAISE_SYNTAX_ERROR_NO_COL_OFFSET seems incorrect we-like-parsers/cpython#126

Closed

gvanrossum closed this May 12, 2020

pablogsal deleted the no-col-offset branch May 19, 2021 18:57

                   | args ',' '*' { RAISE_SYNTAX_ERROR("iterable argument unpacking follows keyword argument unpacking") }
                   | expression for_if_clauses ',' [args | expression for_if_clauses] {
-                      RAISE_SYNTAX_ERROR("Generator expression must be parenthesized") }
+                      RAISE_SYNTAX_ERROR_NO_COL_OFFSET("Generator expression must be parenthesized") }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-40334: Do not show error caret if RAISE_SYNTAX_ERROR_NO_COL_OFFSE…#20020