Visitar URL original
bpo-28837: Fix lib2to3 handling of map/zip/filter by stuarteberg · Pull Request #24 · python/cpython · GitHub
Skip to content

bpo-28837: Fix lib2to3 handling of map/zip/filter #24

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

Merged
benjaminp merged 1 commit into from
Apr 6, 2017
Merged

bpo-28837: Fix lib2to3 handling of map/zip/filter #24

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
Issue #28837: Fix lib2to3 handling of map/zip/filter calls when follo… 
…wed with a 'trailer', e.g. zip()[x]
  • Loading branch information
@stuarteberg
stuarteberg committed Feb 11, 2017
commit 5cdd2dbf2c8107f3b01cfa1a59930cbc365d88e0
21 changes: 18 additions & 3 deletions Lib/lib2to3/fixes/fix_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@

# Local imports
from .. import fixer_base
from ..fixer_util import Name, Call, ListComp, in_special_context
from ..pytree import Node
from ..pygram import python_symbols as syms
from ..fixer_util import Name, ArgList, ListComp, in_special_context


class FixFilter(fixer_base.ConditionalFix):
BM_compatible = True
Expand All @@ -34,16 +37,19 @@ class FixFilter(fixer_base.ConditionalFix):
>
')'
>
[extra_trailers=trailer*]
>
|
power<
'filter'
trailer< '(' arglist< none='None' ',' seq=any > ')' >
[extra_trailers=trailer*]
>
|
power<
'filter'
args=trailer< '(' [any] ')' >
[extra_trailers=trailer*]
>
"""

Expand All @@ -53,23 +59,32 @@ def transform(self, node, results):
if self.should_skip(node):
return

trailers = []
if 'extra_trailers' in results:
for t in results['extra_trailers']:
trailers.append(t.clone())

if "filter_lambda" in results:
new = ListComp(results.get("fp").clone(),
results.get("fp").clone(),
results.get("it").clone(),
results.get("xp").clone())
new = Node(syms.power, [new] + trailers, prefix="")

elif "none" in results:
new = ListComp(Name("_f"),
Name("_f"),
results["seq"].clone(),
Name("_f"))
new = Node(syms.power, [new] + trailers, prefix="")

else:
if in_special_context(node):
return None
new = node.clone()

args = results['args'].clone()
new = Node(syms.power, [Name("filter"), args], prefix="")
new = Node(syms.power, [Name("list"), ArgList([new])] + trailers)
new.prefix = ""
new = Call(Name("list"), [new])
new.prefix = node.prefix
return new
37 changes: 28 additions & 9 deletions Lib/lib2to3/fixes/fix_map.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
# Local imports
from ..pgen2 import token
from .. import fixer_base
from ..fixer_util import Name, Call, ListComp, in_special_context
from ..fixer_util import Name, ArgList, Call, ListComp, in_special_context
from ..pygram import python_symbols as syms
from ..pytree import Node


class FixMap(fixer_base.ConditionalFix):
BM_compatible = True
Expand All @@ -32,6 +34,7 @@ class FixMap(fixer_base.ConditionalFix):
map_none=power<
'map'
trailer< '(' arglist< 'None' ',' arg=any [','] > ')' >
[extra_trailers=trailer*]
>
|
map_lambda=power<
Expand All @@ -47,10 +50,12 @@ class FixMap(fixer_base.ConditionalFix):
>
')'
>
[extra_trailers=trailer*]
>
|
power<
'map' trailer< '(' [arglist=any] ')' >
'map' args=trailer< '(' [any] ')' >

@benjaminp benjaminp Apr 5, 2017

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.

What's the reason for this change?

@stuarteberg stuarteberg Apr 5, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What's the reason for this change?

On line 100 (below), I need to clone the argument list:

new = Node(syms.power, [Name("map"), args.clone()])

If I leave the above line unchanged, then I could have written this, instead:

new = Node(syms.power, [Name("map"), ArgList([args.clone()])])

...but there's a problem with that: It doesn't clone the ( and ) tokens -- it creates them from scratch. As a result, one of the tests fails, due to an unpreserved prefix before the ) token:

======================================================================
FAIL: test_prefix_preservation (Lib.lib2to3.tests.test_fixers.Test_map)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bergs/Documents/workspace/cpython-git/Lib/lib2to3/tests/test_fixers.py", line 3036, in test_prefix_preservation
    self.check(b, a)
  File "/Users/bergs/Documents/workspace/cpython-git/Lib/lib2to3/tests/test_fixers.py", line 3031, in check
    super(Test_map, self).check(b, a)
  File "/Users/bergs/Documents/workspace/cpython-git/Lib/lib2to3/tests/test_fixers.py", line 36, in check
    tree = self._check(before, after)
  File "/Users/bergs/Documents/workspace/cpython-git/Lib/lib2to3/tests/test_fixers.py", line 32, in _check
    self.assertEqual(after, str(tree))
AssertionError: "x =    list(map(   f,    'abc'   ))\n\n" != "x =    list(map(   f,    'abc'))\n\n"
- x =    list(map(   f,    'abc'   ))
?                               ---
+ x =    list(map(   f,    'abc'))

Hence, I think it's best to capture and clone the whole trailer. But if I'm missing something, please let me know.

@benjaminp benjaminp Apr 6, 2017

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.

Okay, thanks for the explanation.

[extra_trailers=trailer*]
>
"""

Expand All @@ -60,6 +65,11 @@ def transform(self, node, results):
if self.should_skip(node):
return

trailers = []
if 'extra_trailers' in results:
for t in results['extra_trailers']:
trailers.append(t.clone())

if node.parent.type == syms.simple_stmt:
self.warning(node, "You should use a for loop here")
new = node.clone()
Expand All @@ -69,23 +79,32 @@ def transform(self, node, results):
new = ListComp(results["xp"].clone(),
results["fp"].clone(),
results["it"].clone())
new = Node(syms.power, [new] + trailers, prefix="")

else:
if "map_none" in results:
new = results["arg"].clone()
new.prefix = ""
else:
if "arglist" in results:
args = results["arglist"]
if args.type == syms.arglist and \
args.children[0].type == token.NAME and \
args.children[0].value == "None":
if "args" in results:
args = results["args"]
if args.type == syms.trailer and \
args.children[1].type == syms.arglist and \
args.children[1].children[0].type == token.NAME and \
args.children[1].children[0].value == "None":
self.warning(node, "cannot convert map(None, ...) "
"with multiple arguments because map() "
"now truncates to the shortest sequence")
return

new = Node(syms.power, [Name("map"), args.clone()])
new.prefix = ""

if in_special_context(node):
return None
new = node.clone()

new = Node(syms.power, [Name("list"), ArgList([new])] + trailers)
new.prefix = ""
new = Call(Name("list"), [new])

new.prefix = node.prefix
return new
21 changes: 16 additions & 5 deletions Lib/lib2to3/fixes/fix_zip.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@

# Local imports
from .. import fixer_base
from ..fixer_util import Name, Call, in_special_context
from ..pytree import Node
from ..pygram import python_symbols as syms
from ..fixer_util import Name, ArgList, in_special_context


class FixZip(fixer_base.ConditionalFix):

BM_compatible = True
PATTERN = """
power< 'zip' args=trailer< '(' [any] ')' >
power< 'zip' args=trailer< '(' [any] ')' > [trailers=trailer*]
>
"""

Expand All @@ -28,8 +31,16 @@ def transform(self, node, results):
if in_special_context(node):
return None

new = node.clone()
new.prefix = ""
new = Call(Name("list"), [new])
args = results['args'].clone()
args.prefix = ""

trailers = []
if 'trailers' in results:
trailers = [n.clone() for n in results['trailers']]
for n in trailers:
n.prefix = ""

new = Node(syms.power, [Name("zip"), args], prefix="")
new = Node(syms.power, [Name("list"), ArgList([new])] + trailers)
new.prefix = node.prefix
return new
56 changes: 47 additions & 9 deletions Lib/lib2to3/tests/test_fixers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2954,10 +2954,23 @@ def test_filter_basic(self):
a = """x = [x for x in range(10) if x%2 == 0]"""
self.check(b, a)

# XXX This (rare) case is not supported
## b = """x = filter(f, 'abc')[0]"""
## a = """x = list(filter(f, 'abc'))[0]"""
## self.check(b, a)
def test_filter_trailers(self):
b = """x = filter(None, 'abc')[0]"""
a = """x = [_f for _f in 'abc' if _f][0]"""
self.check(b, a)

b = """x = len(filter(f, 'abc')[0])"""
a = """x = len(list(filter(f, 'abc'))[0])"""
self.check(b, a)

b = """x = filter(lambda x: x%2 == 0, range(10))[0]"""
a = """x = [x for x in range(10) if x%2 == 0][0]"""
self.check(b, a)

# Note the parens around x
b = """x = filter(lambda (x): x%2 == 0, range(10))[0]"""
a = """x = [x for x in range(10) if x%2 == 0][0]"""
self.check(b, a)

def test_filter_nochange(self):
a = """b.join(filter(f, 'abc'))"""
Expand Down Expand Up @@ -3022,6 +3035,23 @@ def test_prefix_preservation(self):
a = """x = list(map( f, 'abc' ))"""
self.check(b, a)

def test_map_trailers(self):
b = """x = map(f, 'abc')[0]"""
a = """x = list(map(f, 'abc'))[0]"""
self.check(b, a)

b = """x = map(None, l)[0]"""
a = """x = list(l)[0]"""
self.check(b, a)

b = """x = map(lambda x:x, l)[0]"""
a = """x = [x for x in l][0]"""
self.check(b, a)

b = """x = map(f, 'abc')[0][1]"""
a = """x = list(map(f, 'abc'))[0][1]"""
self.check(b, a)

def test_trailing_comment(self):
b = """x = map(f, 'abc') # foo"""
a = """x = list(map(f, 'abc')) # foo"""
Expand Down Expand Up @@ -3066,11 +3096,6 @@ def test_map_basic(self):
"""
self.warns(b, a, "You should use a for loop here")

# XXX This (rare) case is not supported
## b = """x = map(f, 'abc')[0]"""
## a = """x = list(map(f, 'abc'))[0]"""
## self.check(b, a)

def test_map_nochange(self):
a = """b.join(map(f, 'abc'))"""
self.unchanged(a)
Expand Down Expand Up @@ -3130,6 +3155,10 @@ def check(self, b, a):
super(Test_zip, self).check(b, a)

def test_zip_basic(self):
b = """x = zip()"""
a = """x = list(zip())"""
self.check(b, a)

b = """x = zip(a, b, c)"""
a = """x = list(zip(a, b, c))"""
self.check(b, a)
Expand All @@ -3138,6 +3167,15 @@ def test_zip_basic(self):
a = """x = len(list(zip(a, b)))"""
self.check(b, a)

def test_zip_trailers(self):
b = """x = zip(a, b, c)[0]"""
a = """x = list(zip(a, b, c))[0]"""
self.check(b, a)

b = """x = zip(a, b, c)[0][1]"""
a = """x = list(zip(a, b, c))[0][1]"""
self.check(b, a)

def test_zip_nochange(self):
a = """b.join(zip(a, b))"""
self.unchanged(a)
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ Andrew Bennetts
Andy Bensky
Bennett Benson
Ezra Berch
Stuart Berg
Michel Van den Bergh
Julian Berman
Brice Berna
Expand Down