bpo-40599: Improve error messages with expected keywords

[pablogsal]

The current errors are due to tests that don't expect the extra information in the error messages.

https://bugs.python.org/issue40599

[pablogsal]

I open this PR as a draft because I still have some questions about the approach but we can start a discussion to see how to improve it :)

Some examples of the errors with this PR:

>>> from x impirt l
  File "<stdin>", line 1
    from x impirt l
           ^
SyntaxError: Invalid syntax. Expected one of: import
>>> with x os y
  File "<stdin>", line 1
    with x os y
           ^
SyntaxError: Invalid syntax. Expected one of: as, or, if, in, is, not, and
>>> l = 3 if x elso 4
  File "<stdin>", line 1
    l = 3 if x elso 4
               ^
SyntaxError: Invalid syntax. Expected one of: or, in, is, else, not, and
>>> if x:
...   ...
... elif y:
...   ...
... elso
  File "<stdin>", line 5
    elso
    ^
SyntaxError: Invalid syntax. Expected one of: elif, else

With some work, we could extend this to general tokens (although it may be very verbose).

[gvanrossum]

This looks like scope creep for the original issue. Even if you can solve this using the new parser, please open a new bpo issue and have a discussion about it there (or perhaps on python-ideas or python-dev?).

[pablogsal]

This looks like scope creep for the original issue. Even if you can solve this using the new parser, please open a new bpo issue and have a discussion about it there (or perhaps on python-ideas or python-dev?).

Ok, will do. In any case, I opened the draft PR to get some initial opinions from you and @lysnikolaou to see if is worth pursuing.

[pablogsal]

I have linked this against this new issue:

https://bugs.python.org/issue40599

and unlinked it from the original issue.

[aeros]

I have a bit of feedback on some of the examples. When/if I find the time, I'll experiment further with the PR locally and try to provide more in-depth feedback.

>>> from x impirt l
  File "<stdin>", line 1
    from x impirt l
           ^
SyntaxError: Invalid syntax. Expected one of: import

Huge +1 for offering a specific suggestion when it's unambiguous. However, I think that it could look a significantly better to omit the "one of" when there is only when possible suggestion (assuming it's not too much of an implementation issue). E.g. SyntaxError: Invalid syntax. Expected: import.

>>> with x os y
  File "<stdin>", line 1
    with x os y
           ^
SyntaxError: Invalid syntax. Expected one of: as, or, if, in, is, not, and

I'm -0 on the behavior in the above example. Offering 5+ possible suggestions (7 in this case) for keywords doesn't seem especially helpful and results in a rather long exception message.

My personal preference would be to only display the possible suggestion(s) if there are only 4 or less (maybe 3?) possible keywords, and omit "one of" when there's only one.

As for the implementation details, my knowledge of the C-API and especially the parser is rather limited, but would it be possible to check the number of possible keywords with something like Py_ssize_t err_tokens_size = PySet_Size(p->err_tokens)?

If the above or something along those lines would be feasible, I would be +1 on the PR overall. Otherwise, I'm +0. It seems like a good idea, but I think the error message would be a bit too bloated when there's too many possible keywords.

[pablogsal]

Thanks for the feedback @aeros !

I think that the problem is that, as Guido mentions, without tokens this is not very helpful and once we add tokens the possible items in the set will increase a lot. For instance, if you do x y it will complain that it was expecting any operator and all possible operators will be displayed. The same stands for other tokens like ";" or ",". In those errors, is not possible for the parser to know what is the most "likely" one so restricting to only report with "n" tokens will only trigger this on very specific cases.

As I mentioned in the issue, I opened this draft to start a discussion but I and more and more unsure that this approach will give us better debugability than the noise it adds :(

[aeros]

In those errors, is not possible for the parser to know what is the most "likely" one so restricting to only report with "n" tokens will only trigger this on very specific cases.

I would personally be okay with that, it could be quite useful at occasionally detecting unambiguous misspellings, such as the import example. Although one may argue that it's more of a job of a linter to pick up on that.

As I mentioned in the issue, I opened this draft to start a discussion but I and more and more unsure that this approach will give us better debugability than the noise it adds :(

Either way, I think this discussion is a good start and gives a general direction (and perhaps more importantly, establishes what it shouldn't do). Thanks for starting the discussion.