Skip to content

bpo-40631: Single-element starred elements in parenthesised expressions cannot be parsed #24014

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.

Sign up for GitHub

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

Closed
pablogsal wants to merge 1 commit into from
Closed

bpo-40631: Single-element starred elements in parenthesised expressions cannot be parsed #24014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions Grammar/python.gram
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ _PyPegen_parse(Parser *p)
'''
file[mod_ty]: a=[statements] ENDMARKER { _PyPegen_make_module(p, a) }
interactive[mod_ty]: a=statement_newline { Interactive(a, p->arena) }
eval[mod_ty]: a=expressions NEWLINE* ENDMARKER { Expression(a, p->arena) }
eval[mod_ty]: a=star_expressions NEWLINE* ENDMARKER { Expression(a, p->arena) }

@pablogsal pablogsal Dec 30, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lysnikolaou Do you remember why this was expressions and not star_expressions? I cannot see any edge case that makes this change incorrect but now I am curious.

@lysnikolaou lysnikolaou Dec 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this was for consistency with 3.8. While 3.8 and 3.9 currently fail at parser level for eval('*a'), this patch fails at bytecode compiler level.

@pablogsal pablogsal Dec 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make that fail at the parser level if you want but not sure if is worth it because we would need to factor that into some rule and I don't know if we can reuse the rest. What's your opinion?

@lysnikolaou lysnikolaou Dec 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't leaving it as is work?

The reason I'm pushing for the error to be in parser level is due to the quality of the error message. Here's an example:

Without this patch:

➜  cpython git:(bpo-40631) ✗ cat a.py       
a = [1, 2, 3]
eval('''


*a''')
➜  cpython git:(bpo-40631) ✗ python a.py
Traceback (most recent call last):
  File "/home/lysnikolaou/repos/cpython/a.py", line 2, in <module>
    eval('''
  File "<string>", line 4
    *a
    ^
SyntaxError: invalid syntax

With this patch:

➜  cpython git:(bpo-40631) ✗ cat a.py
a = [1, 2, 3]
eval('''


*a''')
➜  cpython git:(bpo-40631) ✗ ./python a.py
Traceback (most recent call last):
  File "/home/lysnikolaou/repos/cpython/a.py", line 2, in <module>
    eval('''
  File "<string>", line 4
SyntaxError: can't use starred expression here

The first one makes a much better job of making clear what the error is I feel. But if it's too much effort and leaving it as is doesn't work, I'm okay with it.

@pablogsal pablogsal Dec 31, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much agree. I have closed the PR based on this discussion and Guido's request. Thanks for the examples and the comment.

func_type[mod_ty]: '(' a=[type_expressions] ')' '->' b=expression NEWLINE* ENDMARKER { FunctionType(a, b, p->arena) }
fstring[expr_ty]: star_expressions

Expand Down Expand Up @@ -311,18 +311,22 @@ block[asdl_stmt_seq*] (memo):
| simple_stmts
| invalid_block

star_element[expr_ty]:
| '*' a=bitwise_or { _Py_Starred(a, Load, EXTRA) }
| '(' '*' a=bitwise_or ')' { _Py_Starred(a, Load, EXTRA) }

@gousaiyang gousaiyang Dec 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch makes (*x), y valid, but with multiple layers of parentheses (((*x))), y is still considered invalid.


star_expressions[expr_ty]:
| a=star_expression b=(',' c=star_expression { c })+ [','] {
_Py_Tuple(CHECK(asdl_expr_seq*, _PyPegen_seq_insert_in_front(p, a, b)), Load, EXTRA) }
| a=star_expression ',' { _Py_Tuple(CHECK(asdl_expr_seq*, _PyPegen_singleton_seq(p, a)), Load, EXTRA) }
| star_expression
star_expression[expr_ty] (memo):
| '*' a=bitwise_or { _Py_Starred(a, Load, EXTRA) }
| star_element
| expression

star_named_expressions[asdl_expr_seq*]: a[asdl_expr_seq*]=','.star_named_expression+ [','] { a }
star_named_expression[expr_ty]:
| '*' a=bitwise_or { _Py_Starred(a, Load, EXTRA) }
| star_element
| named_expression
named_expression[expr_ty]:
| a=NAME ':=' ~ b=expression { _Py_NamedExpr(CHECK(expr_ty, _PyPegen_set_expr_context(p, a, Store)), b, EXTRA) }
Expand All @@ -331,11 +335,6 @@ named_expression[expr_ty]:

annotated_rhs[expr_ty]: yield_expr | star_expressions

expressions[expr_ty]:
| a=expression b=(',' c=expression { c })+ [','] {
_Py_Tuple(CHECK(asdl_expr_seq*, _PyPegen_seq_insert_in_front(p, a, b)), Load, EXTRA) }
| a=expression ',' { _Py_Tuple(CHECK(asdl_expr_seq*, _PyPegen_singleton_seq(p, a)), Load, EXTRA) }
| expression
expression[expr_ty] (memo):
| a=disjunction 'if' b=disjunction 'else' c=expression { _Py_IfExp(b, a, c, EXTRA) }
| disjunction
Expand Down
9 changes: 9 additions & 0 deletions Lib/test/test_ast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,12 @@ def to_tuple(t):
"1,2,3",
# Tuple
"(1,2,3)",
# Tuple with starred
"(1,*b,2)",
# Tuple with parenthesized starred
"(1,(*b),2)",
# Implicit tuple with parenthesized starred
"1,(*b),2",
# Empty tuple
"()",
# Combination
Expand Down Expand Up @@ -2169,6 +2175,9 @@ def main():
('Expression', ('List', (1, 0, 1, 2), [], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 5), [('Constant', (1, 0, 1, 1), 1, None), ('Constant', (1, 2, 1, 3), 2, None), ('Constant', (1, 4, 1, 5), 3, None)], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 7), [('Constant', (1, 1, 1, 2), 1, None), ('Constant', (1, 3, 1, 4), 2, None), ('Constant', (1, 5, 1, 6), 3, None)], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 8), [('Constant', (1, 1, 1, 2), 1, None), ('Starred', (1, 3, 1, 5), ('Name', (1, 4, 1, 5), 'b', ('Load',)), ('Load',)), ('Constant', (1, 6, 1, 7), 2, None)], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 10), [('Constant', (1, 1, 1, 2), 1, None), ('Starred', (1, 3, 1, 7), ('Name', (1, 5, 1, 6), 'b', ('Load',)), ('Load',)), ('Constant', (1, 8, 1, 9), 2, None)], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 8), [('Constant', (1, 0, 1, 1), 1, None), ('Starred', (1, 2, 1, 6), ('Name', (1, 4, 1, 5), 'b', ('Load',)), ('Load',)), ('Constant', (1, 7, 1, 8), 2, None)], ('Load',))),
('Expression', ('Tuple', (1, 0, 1, 2), [], ('Load',))),
('Expression', ('Call', (1, 0, 1, 17), ('Attribute', (1, 0, 1, 7), ('Attribute', (1, 0, 1, 5), ('Attribute', (1, 0, 1, 3), ('Name', (1, 0, 1, 1), 'a', ('Load',)), 'b', ('Load',)), 'c', ('Load',)), 'd', ('Load',)), [('Subscript', (1, 8, 1, 16), ('Attribute', (1, 8, 1, 11), ('Name', (1, 8, 1, 9), 'a', ('Load',)), 'b', ('Load',)), ('Slice', (1, 12, 1, 15), ('Constant', (1, 12, 1, 13), 1, None), ('Constant', (1, 14, 1, 15), 2, None), None), ('Load',))], [])),
]
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2020-12-30-21-13-43.bpo-40631.ejfreV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a bug in the PEG parser that prevents parsing tuples with
single-element, parenthesized unpacking. Patch by Pablo Galindo
Loading