Added check for an inner loop reusing an outer loop's target(s)

Closes #999

12 files changed, 125 insertions, 9 deletions

diff --git a/ChangeLog b/ChangeLog
index a8f4b310d..9c22e21dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -115,6 +115,12 @@ Release date: tba
       occurred when a class docstring uses the 'For the parameters, see'
       magic string but the class __init__ docstring does not, or vice versa.
 
+    * `redefined-outer-name` is now also emitted when a nested loop's target
+      variable is the same as a target variable in an outer loop.
+
+      Closes issue #911.
+
+
 What's new in Pylint 1.6.2?
 ===========================
 
diff --git a/doc/whatsnew/2.0.rst b/doc/whatsnew/2.0.rst
index 57dbd1951..4aea6fd3e 100644
--- a/doc/whatsnew/2.0.rst
+++ b/doc/whatsnew/2.0.rst
@@ -127,6 +127,15 @@ Other Changes
            def baz(self, first):
                # Not keyword-only
 
+* ``redefined-outer-name`` is now also emitted when a
+  nested loop's target variable is the same as an outer loop.
+
+  .. code-block:: python
+
+      for i, j in [(1, 2), (3, 4)]:
+          for j in range(i):
+              print(j)
+
 
 Bug fixes
 =========
diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py
index 45a27fb11..734d2687e 100644
--- a/pylint/checkers/imports.py
+++ b/pylint/checkers/imports.py
@@ -141,10 +141,10 @@ def _dependencies_graph(filename, dep_info):
     for modname, dependencies in sorted(six.iteritems(dep_info)):
         done[modname] = 1
         printer.emit_node(modname)
-        for modname in dependencies:
-            if modname not in done:
-                done[modname] = 1
-                printer.emit_node(modname)
+        for depmodname in dependencies:
+            if depmodname not in done:
+                done[depmodname] = 1
+                printer.emit_node(depmodname)
     for depmodname, dependencies in sorted(six.iteritems(dep_info)):
         for modname in dependencies:
             printer.emit_edge(modname, depmodname)
diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py
index 901f78ebb..7806e88e4 100644
--- a/pylint/checkers/typecheck.py
+++ b/pylint/checkers/typecheck.py
@@ -42,8 +42,8 @@ def _unflatten(iterable):
     for index, elem in enumerate(iterable):
         if (isinstance(elem, collections.Sequence) and
                 not isinstance(elem, six.string_types)):
-            for elem in _unflatten(elem):
-                yield elem
+            for single_elem in _unflatten(elem):
+                yield single_elem
         elif elem and not index:
             # We're interested only in the first element.
             yield elem
diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index cf9cdd465..d189c301e 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -3,6 +3,7 @@
 
 """variables checkers for Python code
 """
+import collections
 import itertools
 import os
 import sys
@@ -52,7 +53,9 @@ def _is_from_future_import(stmt, name):
 def in_for_else_branch(parent, stmt):
     """Returns True if stmt in inside the else branch for a parent For stmt."""
     return (isinstance(parent, astroid.For) and
-            any(else_stmt.parent_of(stmt) for else_stmt in parent.orelse))
+            any(else_stmt.parent_of(stmt) or else_stmt == stmt
+                for else_stmt in parent.orelse))
+
 
 @lru_cache(maxsize=1000)
 def overridden_method(klass, name):
@@ -217,6 +220,29 @@ def _flattened_scope_names(iterator):
     return set(itertools.chain.from_iterable(values))
 
 
+def _for_loop_assign_names(node):
+    """
+    Get the list of loop variables assigned to by a for loop.
+
+    :param node: An assigned-to variable. Usually this is the result of
+        `for_node.target`.
+    :type node: astroid.NodeNG
+
+    :returns: A generator of variable names that the for loop assigns to.
+    :rtype: generator
+    """
+    queue = collections.deque([node])
+    while queue:
+        elem = queue.popleft()
+        if isinstance(elem, astroid.Starred):
+            elem = elem.value
+
+        if isinstance(elem, astroid.AssignName):
+            yield elem.name
+        elif isinstance(elem, astroid.Tuple):
+            queue.extendleft(reversed(list(elem.get_children())))
+
+
 MSGS = {
     'E0601': ('Using variable %r before assignment',
               'used-before-assignment',
@@ -359,6 +385,7 @@ class VariablesChecker(BaseChecker):
         BaseChecker.__init__(self, linter)
         self._to_consume = None  # list of tuples: (to_consume:dict, consumed:dict, scope_type:str)
         self._checking_mod_attr = None
+        self._loop_variables = []
 
     # Relying on other checker's options, which might not have been initialized yet.
     @decorators.cachedproperty
@@ -370,6 +397,31 @@ class VariablesChecker(BaseChecker):
         return get_global_option(self, 'ignored-modules', default=[])
 
 
+    @check_messages('redefined-outer-name')
+    def visit_for(self, node):
+        assigned_to = _for_loop_assign_names(node.target)
+
+        # Only check variables that are used
+        dummy_rgx = self.config.dummy_variables_rgx
+        assigned_to = [var for var in assigned_to if not dummy_rgx.match(var)]
+
+        for variable in assigned_to:
+            for outer_for, outer_variables in self._loop_variables:
+                if (variable in outer_variables
+                        and not in_for_else_branch(outer_for, node)):
+                    self.add_message(
+                        'redefined-outer-name',
+                        args=(variable, outer_for.fromlineno),
+                        node=node
+                    )
+                    break
+
+        self._loop_variables.append((node, assigned_to))
+
+    @check_messages('redefined-outer-name')
+    def leave_for(self, _):
+        self._loop_variables.pop()
+
     def visit_module(self, node):
         """visit module : update consumption analysis variable
         checks globals doesn't overrides builtins
diff --git a/pylint/test/functional/reused_outer_loop_variable.py b/pylint/test/functional/reused_outer_loop_variable.py
new file mode 100644
index 000000000..b244e99f5
--- /dev/null
+++ b/pylint/test/functional/reused_outer_loop_variable.py
@@ -0,0 +1,37 @@
+"""Tests for redefining an outer loop variable."""
+from __future__ import print_function
+__revision__ = 0
+
+# Simple nested loop
+for i in range(10):
+    for i in range(10): #[redefined-outer-name]
+        print(i)
+
+# When outer loop unpacks a tuple
+for i, i_again in enumerate(range(10)):
+    for i in range(10): #[redefined-outer-name]
+        print(i, i_again)
+
+# When inner loop unpacks a tuple
+for i in range(10):
+    for i, i_again in range(10): #[redefined-outer-name]
+        print(i, i_again)
+
+# With nested tuple unpacks
+for (a, (b, c)) in [(1, (2, 3))]:
+    for i, a in range(10): #[redefined-outer-name]
+        print(i, a, b, c)
+
+# Ignores when in else
+for i in range(10):
+    print(i)
+    if i > 5:
+        break
+else:
+    for i in range(2):
+        print(i)
+
+# Ignores dummy variables
+for _ in range(10):
+    for _ in range(10):
+        print("Hello")
diff --git a/pylint/test/functional/reused_outer_loop_variable.txt b/pylint/test/functional/reused_outer_loop_variable.txt
new file mode 100644
index 000000000..1252ac9d6
--- /dev/null
+++ b/pylint/test/functional/reused_outer_loop_variable.txt
@@ -0,0 +1,4 @@
+redefined-outer-name:7::Redefining name 'i' from outer scope (line 6)
+redefined-outer-name:12::Redefining name 'i' from outer scope (line 11)
+redefined-outer-name:17::Redefining name 'i' from outer scope (line 16)
+redefined-outer-name:22::Redefining name 'a' from outer scope (line 21)
diff --git a/pylint/test/functional/reused_outer_loop_variable_py3.py b/pylint/test/functional/reused_outer_loop_variable_py3.py
new file mode 100644
index 000000000..6eaa1f51c
--- /dev/null
+++ b/pylint/test/functional/reused_outer_loop_variable_py3.py
@@ -0,0 +1,5 @@
+"""Python >= 3 tests for redefining an outer loop's variable."""
+
+for i, *j in [(1, 2, 3, 4)]:
+    for j in range(i): #[redefined-outer-name]
+        print(j)
diff --git a/pylint/test/functional/reused_outer_loop_variable_py3.rc b/pylint/test/functional/reused_outer_loop_variable_py3.rc
new file mode 100644
index 000000000..c093be204
--- /dev/null
+++ b/pylint/test/functional/reused_outer_loop_variable_py3.rc
@@ -0,0 +1,2 @@
+[testoptions]
+min_pyver=3.0
diff --git a/pylint/test/functional/reused_outer_loop_variable_py3.txt b/pylint/test/functional/reused_outer_loop_variable_py3.txt
new file mode 100644
index 000000000..ac3a6ead2
--- /dev/null
+++ b/pylint/test/functional/reused_outer_loop_variable_py3.txt
@@ -0,0 +1 @@
+redefined-outer-name:4::Redefining name 'j' from outer scope (line 3)
diff --git a/pylint/test/functional/useless_else_on_loop.py b/pylint/test/functional/useless_else_on_loop.py
index 2fcde8b39..bd8534f53 100644
--- a/pylint/test/functional/useless_else_on_loop.py
+++ b/pylint/test/functional/useless_else_on_loop.py
@@ -46,7 +46,7 @@ def test_return_for2():
     https://bitbucket.org/logilab/pylint/issue/117/useless-else-on-loop-false-positives
     """
     for i in range(10):
-        for i in range(i):
+        for _ in range(i):
             if i % 2:
                 break
         else:
diff --git a/pylint/test/input/func_break_or_return_in_try_finally.py b/pylint/test/input/func_break_or_return_in_try_finally.py
index f880926b0..f242f911e 100644
--- a/pylint/test/input/func_break_or_return_in_try_finally.py
+++ b/pylint/test/input/func_break_or_return_in_try_finally.py
@@ -29,7 +29,7 @@ def break_and_return():
         try:
             my_var += 1.0/i
         finally:
-            for i in range(2):
+            for _ in range(2):
                 if True:
                     break # :D
             else: