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

Grammar change to prevent shift/reduce problem with varargslist #46293

Closed
dalke mannequin opened this issue Feb 5, 2008 · 11 comments
Closed

Grammar change to prevent shift/reduce problem with varargslist #46293

dalke mannequin opened this issue Feb 5, 2008 · 11 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@dalke
Copy link
Mannequin

dalke mannequin commented Feb 5, 2008

BPO 2009
Nosy @mdickinson, @ncoghlan
Files
  • refactor_varargslist.patch
  • 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 2010-07-12.14:15:52.003>
    created_at = <Date 2008-02-05.00:19:49.674>
    labels = ['interpreter-core', 'type-feature']
    title = 'Grammar change to prevent shift/reduce problem with varargslist'
    updated_at = <Date 2010-07-12.14:15:52.002>
    user = 'https://bugs.python.org/dalke'

    bugs.python.org fields:

    activity = <Date 2010-07-12.14:15:52.002>
    actor = 'mark.dickinson'
    assignee = 'none'
    closed = True
    closed_date = <Date 2010-07-12.14:15:52.003>
    closer = 'mark.dickinson'
    components = ['Interpreter Core']
    creation = <Date 2008-02-05.00:19:49.674>
    creator = 'dalke'
    dependencies = []
    files = ['17914']
    hgrepos = []
    issue_num = 2009
    keywords = ['patch']
    message_count = 11.0
    messages = ['62055', '62065', '62066', '62071', '109699', '109701', '109705', '109706', '109707', '109800', '110088']
    nosy_count = 4.0
    nosy_names = ['nnorwitz', 'dalke', 'mark.dickinson', 'ncoghlan']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue2009'
    versions = ['Python 3.2']

    @dalke
    Copy link
    Mannequin Author

    dalke mannequin commented Feb 5, 2008

    I wrote a translator from the CFG used in the Grammar file into a form for PLY. I
    found one problem with

    varargslist: ((fpdef ['=' test] ',')*
    ('*' NAME [',' '**' NAME] | '**' NAME) |
    fpdef ['=' test] (',' fpdef ['=' test])* [','])

    This grammar definition is ambiguous until the presence/lack of a "*". PLY
    complains:

    state 469

    (28) varargslist -> fpdef EQUAL test COMMA .
    (32) varargslist_star -> fpdef EQUAL test COMMA .
    (35) varargslist_star3 -> COMMA . fpdef
    (36) varargslist_star3 -> COMMA . fpdef EQUAL test
    (39) fpdef -> . NAME
    (40) fpdef -> . LPAR fplist RPAR
    

    ! shift/reduce conflict for NAME resolved as shift.
    ! shift/reduce conflict for LPAR resolved as shift.

    RPAR            reduce using rule 28 (varargslist -\> fpdef EQUAL test COMMA .)
    COLON           reduce using rule 28 (varargslist -\> fpdef EQUAL test COMMA .)
    STAR            reduce using rule 32 (varargslist_star -\> fpdef EQUAL test 
    

    COMMA .)
    DOUBLESTAR reduce using rule 32 (varargslist_star -> fpdef EQUAL test
    COMMA .)
    NAME shift and go to state 165
    LPAR shift and go to state 163

    ! NAME [ reduce using rule 32 (varargslist_star -> fpdef EQUAL test
    COMMA
    .) ]
    ! LPAR [ reduce using rule 32 (varargslist_star -> fpdef EQUAL test
    COMMA
    .) ]

    fpdef                          shift and go to state 515
    

    My fix was to use this definition when I did the translation.

    varargslist: ((fpdef ['=' test] (',' fpdef ['=' test])*
    (',' '*' NAME [',' '**' NAME] | ',' '**' NAME | [','])) |
    ('*' NAME [',' '**' NAME]) |
    ('**' NAME))

    So far I've not found a functional difference between these two definitions, and
    the only change to ast.c is to update the comment based on this section.

    By making this change it would be easier for the handful of people who write
    parsers for Python based on a yacc-like look-ahead(1) parser to use that file more
    directly.

    @dalke dalke mannequin added the type-feature A feature request or enhancement label Feb 5, 2008
    @tiran tiran added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Feb 5, 2008
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 5, 2008

    I don't have any objection in principle, but I would like to see the
    updated rules based on the 2.6 and 3.0 Grammar files (they should only
    be minor variants of what you have already done for 2.5)

    @dalke
    Copy link
    Mannequin Author

    dalke mannequin commented Feb 5, 2008

    I've been working from the Grammar file from CVS for 2.6 ... I thought.
    For example, I see "# except_clause: 'except' [test [('as' | ',')
    test]]" which is a 2.6-ism.

    "svn log" says it hasn't changed since 2007-05-19, when except/as was
    added.

    What did I miss?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Feb 5, 2008

    Sorry, I misread slightly (or else just got confused as to which of the
    3 Grammar files I was looking at on my local machine). What you've
    posted already is indeed the relevant change for 2.6 - it's only 3.0
    that is slightly different because it allows the definition of
    keyword-only arguments (I believe that feature may also be on the list
    of items that may be backported to 2.6).

    @mdickinson
    Copy link
    Member

    +!. I had to do a very similar refactoring recently when trying to fix the parser module to understand keyword-only arguments and annotations.

    Taking keyword-only arguments into account, I think the replacement needs to be:

    varargslist: (vfpdef ['=' test] (',' vfpdef ['=' test])* [','
    ['*' [vfpdef] (',' vfpdef ['=' test])* [',' '**' vfpdef] | '**' vfpdef]]
    | '*' [vfpdef] (',' vfpdef ['=' test])* [',' '**' vfpdef] | '**' vfpdef)

    and the analogous replacement needs to be made in the production for 'typedargslist', too.

    BTW, I'm a bit surprised that the grammar doesn't allow for trailing commas after keyword-only arguments: that is,

    def f(a, b,): ...     is fine, but
    def f(*, a, b,): ...  is a SyntaxError

    @mdickinson
    Copy link
    Member

    Here's a patch.

    I'm assuming that this doesn't violate the moratorium, since it's not so much a change to the grammar itself as a modification to the way that it's expressed.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 9, 2010

    BTW, I'm a bit surprised that the grammar doesn't allow for trailing commas after keyword-only arguments:  that is,

    def f(a, b,): ...     is fine, but
    def f(*, a, b,): ...  is a SyntaxError

    That seems more like an oversight than a deliberate restriction. Does
    your patch eliminate the discrepancy?

    (And agreed the moratorium doesn't apply).

    @mdickinson
    Copy link
    Member

    No, "def f(, a,): ..." is still a SyntaxError with the patch. I'd think that allowing that extra comma *would be a violation of the moratorium, though maybe it's debatable. Perhaps worth bringing up on python-dev?

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 9, 2010

    No,  "def f(, a,): ..." is still a SyntaxError with the patch.  I'd think that allowing that extra comma *would be a violation of the moratorium, though maybe it's debatable.  Perhaps worth bringing up on python-dev?

    Good point - I just wouldn't worry about it in that case.

    @mdickinson
    Copy link
    Member

    Good point - I just wouldn't worry about it in that case.

    Ah, now I have to apologise: after some discussion on #python-dev IRC, I'm afraid I've (perhaps foolishly) rejected this advice. :) See python-dev discussion starting at

    http://mail.python.org/pipermail/python-dev/2010-July/101636.html

    I'll hold off on the modification in this issue until that discussion resolves.

    @mdickinson
    Copy link
    Member

    Applied in r82837. I'll open a separate issue for the trailing commas.

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants