Implementation of RAISE_SYNTAX_ERROR_NO_COL_OFFSET seems incorrect

[pablogsal]

From python#20020 (comment), it seems that the way RAISE_SYNTAX_ERROR_NO_COL_OFFSET works in not correct as is always assigning the caret to the beginning of the line instead of where the error should be.

[gvanrossum]

Well, what do you expect given the name of the macro? :-)

Maybe the value -1 can be used to indicate the offset is unknown?

[pablogsal]

Well, what do you expect given the name of the macro? :-)

I expected that when the macro is used, no column offset was displayed (so there is no error caret on the final error message).

Maybe the value -1 can be used to indicate the offset is unknown?

Exactly! That is what I was thinking about. It sounds odd to me somehow to point always to the beginning of the line.

[gvanrossum]

The macro itself looks innocent. If anything's the matter, it's in _PyPegen_raise_error. I experimented with both raising SyntaxError and printing it using the traceback module, and then read the code. The only way to consistently suppress the caret is by setting the offset attribute of the SyntaxError object to None. In Python you can do this like this:

err = SyntaxError("message", ("file.py", 1, None, "text"))

The C equivalent is similar (you still end up having to construct a 4-tuple).

[gvanrossum]

FWIW while we're at it, we might want to fix all the inconsistencies between the traceback module and the code in pythonrun.c for various out-of-bound values of the offset (both 0 and below and len(text) and above).

[pablogsal]

The C equivalent is similar (you still end up having to construct a 4-tuple).

We have a code path that does this, but currently is not activated if with_col_number is 0, only if loc is NULL:

https://github.com/python/cpython/blob/master/Parser/pegen/pegen.c#L417-L418

[gvanrossum]

No, ‘loc’ here is confusingly named — it’s the text of the line.

[pablogsal]

No, ‘loc’ here is confusingly named — it’s the text of the line.

Indeed, that is confusingly named! We could then pass the None changing the Py_BuildValue to (OiON), passing PyNone if with_col_number=0 instead of col_number and manually constructing the integer value in the happy path (for what currently is just col_number).

[gvanrossum]

Or just two different calls to Py_BuildValue. The confusing loc name was copied from ast_error() in Python/ast.c. I suppose it means "location" but I don't understand why the code is like that -- probably an ancient mistake when that code was refactored (some of it probably predated the AST).

[lysnikolaou]

The RAISE_SYNTAX_ERROR_NO_COL_OFFSET macro was introduced in python#19782 by @isidentical, where he explained to me that this is actually the expected behaviour in some tests. See python#19782 (comment).

[pablogsal]

where he explained to me that this is actually the expected behaviour in some tests.

Yeah, but that works only for that particular case. If you make it a bit different it fails. For example:

Old parser:

  File "<stdin>", line 1
    if a: 1 + 1 = 2
          ^
SyntaxError: cannot assign to operator

New Parser:

  File "<stdin>", line 1
    if a: 1 + 1 = 2
    ^
SyntaxError: cannot assign to operator

That's why I am saying that always pointing to the beginning of the line is not correct.

[lysnikolaou]

Oh you're right, this is not correct. It seems that ast_error always set the column number of the SyntaxError to the column offset of the offending node, so maybe that's what we want to do as well.