2to3 parser failure caused by a comma after a generator expression

[jstasiak]

BPO	27494
Nosy	@benjaminp, @ambv, @serhiy-storchaka, @jstasiak, @jayvdb, @csabella, @miss-islington
PRs	bpo-27494: Fix a 2to3 parser bug (trailing comma) #60 bpo-27494: Fix 2to3 handling of trailing comma after a generator expression #3771 Revert "closes bpo-27494: Fix 2to3 handling of trailing comma after a generator expression (#3771)" #8241 [3.7] Revert "closes bpo-27494: Fix 2to3 handling of trailing comma after a generator expression (GH-3771)" (GH-8241) #8580

^{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 2018-07-31.06:34:34.045>
created_at = <Date 2016-07-12.10:29:13.785>
labels = ['3.7', '3.8', 'type-bug', 'expert-2to3']
title = '2to3 parser failure caused by a comma after a generator expression'
updated_at = <Date 2018-08-09.13:19:59.882>
user = 'https://github.com/jstasiak'

bugs.python.org fields:

activity = <Date 2018-08-09.13:19:59.882>
actor = 'lukasz.langa'
assignee = 'none'
closed = True
closed_date = <Date 2018-07-31.06:34:34.045>
closer = 'serhiy.storchaka'
components = ['2to3 (2.x to 3.x conversion tool)']
creation = <Date 2016-07-12.10:29:13.785>
creator = 'jstasiak'
dependencies = []
files = []
hgrepos = []
issue_num = 27494
keywords = ['patch']
message_count = 13.0
messages = ['270236', '295246', '303058', '303500', '303744', '306146', '306269', '322731', '322734', '322760', '322768', '322780', '323321']
nosy_count = 7.0
nosy_names = ['benjamin.peterson', 'lukasz.langa', 'serhiy.storchaka', 'jstasiak', 'jayvdb', 'cheryl.sabella', 'miss-islington']
pr_nums = ['60', '3771', '8241', '8580']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27494'
versions = ['Python 3.7', 'Python 3.8']

[jstasiak]

Test file (test.py):

print(set(x for x in range(2),))

Python runs it nicely:

% python2 test.py
set([0, 1])
% python3 test.py
{0, 1}

2to3 parser (on both Python 2.7.11 and 3.5.2) chokes on it though:

% /usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/bin/2to3 test.py
RefactoringTool: Skipping optional fixer: buffer
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: set_literal
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Can't parse test.py: ParseError: bad input: type=8, value=u')', context=('', (1, 30))
RefactoringTool: No files need to be modified.
RefactoringTool: There was 1 error:
RefactoringTool: Can't parse test.py: ParseError: bad input: type=8, value=u')', context=('', (1, 30))

% /usr/local/Cellar/python3/3.5.2/Frameworks/Python.framework/Versions/3.5/bin/2to3 test.py
RefactoringTool: Skipping optional fixer: buffer
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: set_literal
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Can't parse test.py: ParseError: bad input: type=8, value=')', context=('', (1, 30))
RefactoringTool: No files need to be modified.
RefactoringTool: There was 1 error:
RefactoringTool: Can't parse test.py: ParseError: bad input: type=8, value=')', context=('', (1, 30))

For reference: smarkets/flake8-strict#9 (project using lib2to3 parser)

[csabella]

Adding benjamin.peterson as he is listed as the expert for 2to3.

[serhiy-storchaka]

set(x for x in range(2),) can be interpreted as set(x for x in (range(2),)).

Wouldn't be better to forbid such ambiguous syntax? The trailing comma in argument list is supported because it helps to add new arguments (or temporary comment out arguments).

    foo(x,
        y,
        #z,
       )

But set(x for x in range(2),) is not syntactically valid if add an argument after the comma. Parenthesis around a generator expression can be omitted only if it is the only argument in a function call. I think that it would be better to forbid a trailing comma in this case.

[jstasiak]

By "forbid" do you mean "forbid in Python" (as in change Python syntax)? I like the idea but that seems like a more serious change and 2to3 arguably needs to handle code targeting older Python versions anyway.

[benjaminp]

New changeset af810b3 by Benjamin Peterson (Jakub Stasiak) in branch 'master':
closes bpo-27494: Fix 2to3 handling of trailing comma after a generator expression (bpo-3771)
af810b3

[serhiy-storchaka]

Actually this syntax isn't allowed by the Python language specification. See bpo-32012 for fixing the Python parser.

[serhiy-storchaka]

Since this syntax never was valid according to the Python language specification, and it causes SyntaxError in 3.7 (see bpo-32012), I think that this change should be reverted.

[serhiy-storchaka]

New changeset 4b8a7f5 by Serhiy Storchaka in branch 'master':
Revert "closes bpo-27494: Fix 2to3 handling of trailing comma after a generator expression (GH-3771)" (bpo-8241)
4b8a7f5

[miss-islington]

New changeset 9ecbe33 by Miss Islington (bot) in branch '3.7':
Revert "closes bpo-27494: Fix 2to3 handling of trailing comma after a generator expression (GH-3771)" (GH-8241)
9ecbe33

[jstasiak]

Apologies for only responding now, I've not received any notifications after my original pull request had been merged. I only learned about the change being reverted from #8580, so let me leave my two cents here:

I don't think the syntax not being valid (formally - since forever, practically - since 3.7) is good enough reason to make (lib)2to3 reject it. 2to3 is supposed to handle old syntax, isn't it? I'd argue that since it is (or was) possible to use this syntax in Python 2.x it should be handled gracefully.

[serhiy-storchaka]

Sorry, I missed that you didn't receive a notification about creating of the reverting PR. I should announce this explicitly.

2to3 handles the old syntax, but a comma after a generator expression was not a valid old syntax. With your patch it accepted this syntax and produced invalid Python program.

$ ./python -m lib2to3 -w t9.py
RefactoringTool: Skipping optional fixer: buffer
RefactoringTool: Skipping optional fixer: idioms
RefactoringTool: Skipping optional fixer: set_literal
RefactoringTool: Skipping optional fixer: ws_comma
RefactoringTool: Refactored t9.py
--- t9.py       (original)
+++ t9.py       (refactored)
@@ -1 +1 @@
-print(set(x for x in range(2),))
+print((set(x for x in list(range(2)),)))
RefactoringTool: Files that were modified:
RefactoringTool: t9.py
$ ./python t9.py 
  File "t9.py", line 1
    print((set(x for x in list(range(2)),)))
              ^
SyntaxError: Generator expression must be parenthesized

[jstasiak]

I appreciate the example, but I'd claim that's a "missing fixer" issue, not a "parser accepts too much" issue. Considering the syntax wasn't ambiguous (I think) and had been accepted before 3.7 I'll remain not totally convinced here.

[ambv]

This is one of those issues where it's clear that the parser and 2to3 should be separate.