Make futurize --stage1 leave next() method alone (issue #81)

Author: edschofield

docs/futurize.rst

@@ -131,7 +131,7 @@ The complete set of fixers applied by ``futurize --stage1`` is::
     lib2to3.fixes.fix_isinstance
     lib2to3.fixes.fix_methodattrs
     lib2to3.fixes.fix_ne
-    lib2to3.fixes.fix_next
+    libfuturize.fixes.fix_next_call
     lib2to3.fixes.fix_numliterals
     lib2to3.fixes.fix_paren
     lib2to3.fixes.fix_reduce
@@ -161,6 +161,17 @@ this. The new fixer both makes implicit relative imports explicit and
 adds the declaration ``from __future__ import absolute_import`` at the top
 of each relevant module.
 
+.. code-block:: python
+
+    lib2to3.fixes.fix_next
+
+The ``fix_next_call`` fixer in ``libfuturize.fixes`` is applied instead of
+``fix_next`` in stage 1. The new fixer changes any ``obj.next()`` calls to
+``next(obj)``, which is Py2/3 compatible, but doesn't change any ``next`` method
+names to ``__next__``, which would break Py2 compatibility.
+
+``fix_next`` is applied in stage 2.
+
 .. code-block:: python
 
     lib2to3.fixes.fix_print

docs/whatsnew.rst

@@ -9,6 +9,9 @@ What's new in version 0.12.5
 
 - Use a private logger instead of the global logger in
   ``future.standard_library`` (issue #82).
+- Stage 1 of ``futurize`` no longer renames ``next`` methods to ``__next__``
+  (issue #81). It still converts ``obj.next()`` method calls to
+  ``next(obj)`` correctly.
 
 
 .. whats-new-0.12.4:

future/tests/test_futurize.py

@@ -156,7 +156,7 @@ def test_oldstyle_classes_iterator(self):
         class Upper:
             def __init__(self, iterable):
                 self._iter = iter(iterable)
-            def next(self):                 # note the Py3 interface
+            def next(self):                 # note the Py2 interface
                 return next(self._iter).upper()
             def __iter__(self):
                 return self
@@ -183,7 +183,7 @@ def __iter__(self):
         class Upper():
             def __init__(self, iterable):
                 self._iter = iter(iterable)
-            def next(self):                 # note the Py3 interface
+            def next(self):                 # note the Py2 interface
                 return next(self._iter).upper()
             def __iter__(self):
                 return self
@@ -677,6 +677,79 @@ def f(a, b):
         '''
         self.convert_check(before, after, stages=[1])
 
+    def test_next_1(self):
+        """
+        Custom next methods should not be converted to __next__ in stage1, but
+        any obj.next() calls should be converted to next(obj).
+        """
+        before = """
+        class Upper:
+            def __init__(self, iterable):
+                self._iter = iter(iterable)
+            def next(self):                 # note the Py2 interface
+                return next(self._iter).upper()
+            def __iter__(self):
+                return self
+
+        itr = Upper('hello')
+        assert itr.next() == 'H'
+        assert next(itr) == 'E'
+        assert list(itr) == list('LLO')
+        """
+
+        after = """
+        class Upper:
+            def __init__(self, iterable):
+                self._iter = iter(iterable)
+            def next(self):                 # note the Py2 interface
+                return next(self._iter).upper()
+            def __iter__(self):
+                return self
+
+        itr = Upper('hello')
+        assert next(itr) == 'H'
+        assert next(itr) == 'E'
+        assert list(itr) == list('LLO')
+        """
+        self.convert_check(before, after, stages=[1])
+
+    @unittest.expectedFailure
+    def test_next_2(self):
+        """
+        This version of the above doesn't currently work: the self._iter.next() call in
+        line 5 isn't converted to next(self._iter).
+        """
+        before = """
+        class Upper:
+            def __init__(self, iterable):
+                self._iter = iter(iterable)
+            def next(self):                 # note the Py2 interface
+                return self._iter.next().upper()
+            def __iter__(self):
+                return self
+
+        itr = Upper('hello')
+        assert itr.next() == 'H'
+        assert next(itr) == 'E'
+        assert list(itr) == list('LLO')
+        """
+
+        after = """
+        class Upper(object):
+            def __init__(self, iterable):
+                self._iter = iter(iterable)
+            def next(self):                 # note the Py2 interface
+                return next(self._iter).upper()
+            def __iter__(self):
+                return self
+
+        itr = Upper('hello')
+        assert next(itr) == 'H'
+        assert next(itr) == 'E'
+        assert list(itr) == list('LLO')
+        """
+        self.convert_check(before, after, stages=[1])
+
     def test_xrange(self):
         """
         xrange should not be changed by futurize --stage1

libfuturize/fixes/__init__.py

@@ -17,7 +17,8 @@
     'lib2to3.fixes.fix_isinstance',
     'lib2to3.fixes.fix_methodattrs',
     'lib2to3.fixes.fix_ne',
-    'lib2to3.fixes.fix_next',
+    # 'lib2to3.fixes.fix_next',         # would replace ``next`` method names
+                                        # with ``__next__``.
     'lib2to3.fixes.fix_numliterals',    # turns 1L into 1, 0755 into 0o755
     'lib2to3.fixes.fix_paren',
     # 'lib2to3.fixes.fix_print',        # see the libfuturize fixer that also
@@ -57,6 +58,7 @@
     'lib2to3.fixes.fix_long',
     'lib2to3.fixes.fix_map',
     # 'lib2to3.fixes.fix_metaclass', # causes SyntaxError in Py2! Use the one from ``six`` instead
+    'lib2to3.fixes.fix_next',
     'lib2to3.fixes.fix_nonzero',     # TODO: cause this to import ``object`` and/or add a decorator for mapping __bool__ to __nonzero__
     'lib2to3.fixes.fix_operator',    # we will need support for this by e.g. extending the Py2 operator module to provide those functions in Py3
     'lib2to3.fixes.fix_raw_input',
@@ -69,6 +71,9 @@
 libfuturize_fix_names_stage1 = set([
     'libfuturize.fixes.fix_absolute_import',
     'libfuturize.fixes.fix_division',
+    'libfuturize.fixes.fix_next_call',  # obj.next() -> next(obj). Unlike
+                                        # lib2to3.fixes.fix_next, doesn't change
+                                        # the ``next`` method to ``__next__``.
     'libfuturize.fixes.fix_print_with_import',
     'libfuturize.fixes.fix_raise',
     'libfuturize.fixes.fix_order___future__imports',  # TODO: consolidate to a single line to simplify testing

libfuturize/fixes/fix_next_call.py

@@ -0,0 +1,104 @@
+"""
+Based on fix_next.py by Collin Winter.
+
+Replaces it.next() -> next(it), per PEP 3114.
+
+Unlike fix_next.py, this fixer doesn't replace the name of a next method with __next__,
+which would break Python 2 compatibility without further help from fixers in
+stage 2.
+"""
+
+# Local imports
+from lib2to3.pgen2 import token
+from lib2to3.pygram import python_symbols as syms
+from lib2to3 import fixer_base
+from lib2to3.fixer_util import Name, Call, find_binding
+
+bind_warning = "Calls to builtin next() possibly shadowed by global binding"
+
+
+class FixNextCall(fixer_base.BaseFix):
+    BM_compatible = True
+    PATTERN = """
+    power< base=any+ trailer< '.' attr='next' > trailer< '(' ')' > >
+    |
+    power< head=any+ trailer< '.' attr='next' > not trailer< '(' ')' > >
+    |
+    global=global_stmt< 'global' any* 'next' any* >
+    """
+
+    order = "pre" # Pre-order tree traversal
+
+    def start_tree(self, tree, filename):
+        super(FixNextCall, self).start_tree(tree, filename)
+
+        n = find_binding('next', tree)
+        if n:
+            self.warning(n, bind_warning)
+            self.shadowed_next = True
+        else:
+            self.shadowed_next = False
+
+    def transform(self, node, results):
+        assert results
+
+        base = results.get("base")
+        attr = results.get("attr")
+        name = results.get("name")
+
+        if base:
+            if self.shadowed_next:
+                # Omit this:
+                # attr.replace(Name("__next__", prefix=attr.prefix))
+                pass
+            else:
+                base = [n.clone() for n in base]
+                base[0].prefix = ""
+                node.replace(Call(Name("next", prefix=node.prefix), base))
+        elif name:
+            # Omit this:
+            # n = Name("__next__", prefix=name.prefix)
+            # name.replace(n)
+            pass
+        elif attr:
+            # We don't do this transformation if we're assigning to "x.next".
+            # Unfortunately, it doesn't seem possible to do this in PATTERN,
+            #  so it's being done here.
+            if is_assign_target(node):
+                head = results["head"]
+                if "".join([str(n) for n in head]).strip() == '__builtin__':
+                    self.warning(node, bind_warning)
+                return
+            # Omit this:
+            # attr.replace(Name("__next__"))
+        elif "global" in results:
+            self.warning(node, bind_warning)
+            self.shadowed_next = True
+
+
+### The following functions help test if node is part of an assignment
+###  target.
+
+def is_assign_target(node):
+    assign = find_assign(node)
+    if assign is None:
+        return False
+
+    for child in assign.children:
+        if child.type == token.EQUAL:
+            return False
+        elif is_subtree(child, node):
+            return True
+    return False
+
+def find_assign(node):
+    if node.type == syms.expr_stmt:
+        return node
+    if node.type == syms.simple_stmt or node.parent is None:
+        return None
+    return find_assign(node.parent)
+
+def is_subtree(root, node):
+    if root == node:
+        return True
+    return any(is_subtree(c, node) for c in root.children)