Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message for missing : before suites #87163

Closed
pablogsal opened this issue Jan 22, 2021 · 9 comments
Closed

Improve error message for missing : before suites #87163

pablogsal opened this issue Jan 22, 2021 · 9 comments
Labels

Comments

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jan 22, 2021

BPO 42997
Nosy @gvanrossum, @aroberge, @serhiy-storchaka, @lysnikolaou, @pablogsal
PRs
  • #24292
  • #24293
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-02-02.19:57:31.655>
    created_at = <Date 2021-01-22.01:20:05.352>
    labels = ['3.10']
    title = 'Improve error message for missing : before suites'
    updated_at = <Date 2021-02-02.19:57:31.654>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2021-02-02.19:57:31.654>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-02-02.19:57:31.655>
    closer = 'pablogsal'
    components = []
    creation = <Date 2021-01-22.01:20:05.352>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42997
    keywords = ['patch']
    message_count = 9.0
    messages = ['385463', '385465', '385468', '385481', '385720', '385725', '385726', '385728', '386164']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'aroberge', 'serhiy.storchaka', 'lys.nikolaou', 'pablogsal']
    pr_nums = ['24292', '24293']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42997'
    versions = ['Python 3.10']

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Jan 22, 2021

    Instead of displaying a generic syntax error:

    Python 3.8.6 (default, Oct 10 2020, 18:31:21)
    [GCC 10.2.0] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> for x in range
      File "<stdin>", line 1
        for x in range
                     ^
    SyntaxError: invalid syntax
    >>>

    we could display:

    >>> for x in range
      File "<stdin>", line 1
        for x in range
                      ^
    SyntaxError: expected ':'

    The same idea applies for every suite that has a missing ':'

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Jan 22, 2021

    There are several ways to implement this:

    In PR24292 I implemented this idea by adding a new element to the grammar: '&&'. This allows to hard-expect a token: if the token is not there the parsing hard-fails immediately without trying anything else and displays an appropriate error message

    The other possibility if we think that PR24292 is overkill is manually adding an "invalid_wathever" for every possible compound statement option that parses the statement with a !':' and then raises appropriately. This option is more verbose in the grammar but requires no new machinery.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Jan 22, 2021

    PR 24293 implements the second idea (only new rules and no new machinery)

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 22, 2021

    See also bpo-1634034.

    @lysnikolaou
    Copy link
    Contributor

    @lysnikolaou lysnikolaou commented Jan 26, 2021

    I propose to go with the first approach. I really like that and I can see us using it in various place in the future. It's basically a positive lookahead with a cut, but the extra feature that it raises a SyntaxError, right?

    I'll have a look at the PR now.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Jan 26, 2021

    See also bpo-1634034.

    The problem with bpo-1634034 is that is not anymore possible without manual changes to the grammar with the PEG parser (or new infrastructure). The new parser does not have a defined concept of "i expected this token and now I am going to hard fail because is not there", but instead it will backtrack and try something else. We also did some work on something similar and we deemed it not useful as it was:

    bugs.python.org/issue40599

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Jan 26, 2021

    It's basically a positive lookahead with a cut, but the extra feature that it raises a SyntaxError, right?

    Yeah, but the cut is "absolute" (because of the syntax Error). IIRC the normal cut only affects that rule, no?

    @lysnikolaou
    Copy link
    Contributor

    @lysnikolaou lysnikolaou commented Jan 26, 2021

    Yeah, but the cut is "absolute" (because of the syntax Error). IIRC the normal cut only affects that rule, no?

    Right.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Feb 2, 2021

    New changeset 58fb156 by Pablo Galindo in branch 'master':
    bpo-42997: Improve error message for missing : before suites (GH-24292)
    58fb156

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants