Implementation of RAISE_SYNTAX_ERROR_NO_COL_OFFSET seems incorrect #126

Activity

1. pablogsal

   changed the title [-]Implementation RAISE_SYNTAX_ERROR_NO_COL_OFFSET seems incorrect[/-] [+]Implementation of RAISE_SYNTAX_ERROR_NO_COL_OFFSET seems incorrect[/+] on May 10, 2020
2. gvanrossum commented on May 10, 2020

   

   gvanrossum

   on May 10, 2020

   More actions

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

   Maybe the value -1 can be used to indicate the offset is unknown?
3. pablogsal commented on May 10, 2020

   

   pablogsal

   on May 10, 2020

   Author

   More actions

   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.
4. gvanrossum commented on May 10, 2020

   

   gvanrossum

   on May 10, 2020

   More actions

   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).
5. gvanrossum commented on May 10, 2020

   

   gvanrossum

   on May 10, 2020

   More actions

   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).
6. pablogsal commented on May 10, 2020

   

   pablogsal

   on May 10, 2020

   Author

   More actions

   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
7. gvanrossum commented on May 10, 2020

   

   gvanrossum

   on May 10, 2020

   More actions

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

   👍React with 👍1Reacted by Pablo Galindo Salgado
8. pablogsal commented on May 10, 2020

   

   pablogsal

   on May 10, 2020

   Last edited by pablogsal

   Author

   More actions

   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).
9. gvanrossum commented on May 10, 2020

   

   gvanrossum

   on May 10, 2020

   More actions

   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).
10. lysnikolaou commented on May 11, 2020

    

    lysnikolaou

    on May 11, 2020

    Last edited by lysnikolaou

    Member

    More actions

    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).
11. pablogsal commented on May 11, 2020

    

    pablogsal

    on May 11, 2020

    Last edited by pablogsal

    Author

    More actions

    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.
12. lysnikolaou commented on May 11, 2020

    

    lysnikolaou

    on May 11, 2020

    Last edited by lysnikolaou

    Member

    More actions

    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.
13. pablogsal

    closed this as completedin python/cpython#20050on May 13, 2020