Added a new warning for closing over variables that are defined in loops.

7 files changed, 163 insertions, 0 deletions

diff --git a/ChangeLog b/ChangeLog
index 3002eeb..b5ca9d1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -8,6 +8,9 @@ ChangeLog for Pylint
     * Emit [assignment-from-none] when the function contains bare returns.
       Fixes BitBucket issue #191.
 
+    * Added a new warning for closing over variables that are
+      defined in loops. Fixes Bitbucket issue #176.
+
     * Extend the checking for unbalanced-tuple-unpacking and
       unpacking-non-sequence to instance attribute unpacking as well.
 
diff --git a/checkers/variables.py b/checkers/variables.py
index 2ce7fcf..a6123de 100644
--- a/checkers/variables.py
+++ b/checkers/variables.py
@@ -146,6 +146,12 @@ MSGS = {
               'Used when something which is not '
               'a sequence is used in an unpack assignment'),
 
+    'W0640': ('Cell variable %s defined in loop',
+              'cell-var-from-loop', 
+              'A variable used in a closure is defined in a loop. '
+              'This will result in all closures using the same value for '
+              'the closed-over variable.'),
+
     }
 
 class VariablesChecker(BaseChecker):
@@ -423,6 +429,25 @@ builtins. Remember that you should avoid to define new builtins when possible.'
         if default_message:
             self.add_message('global-statement', node=node)
 
+    def _check_late_binding_closure(self, node, assignment_node, scope_type):
+        node_scope = node.scope()
+        if not isinstance(node_scope, (astroid.Lambda, astroid.Function)):
+            return
+
+        if isinstance(assignment_node, astroid.Comprehension):
+            if assignment_node.parent.parent_of(node.scope()):
+                self.add_message('cell-var-from-loop', node=node, args=node.name)
+        else:
+            assign_scope = assignment_node.scope()
+            maybe_for = assignment_node
+            while not isinstance(maybe_for, astroid.For):
+                if maybe_for is assign_scope:
+                    break
+                maybe_for = maybe_for.parent
+            else:
+                if maybe_for.parent_of(node_scope) and not isinstance(node_scope.statement(), astroid.Return):
+                    self.add_message('cell-var-from-loop', node=node, args=node.name)
+        
     def _loopvar_name(self, node, name):
         # filter variables according to node's scope
         # XXX used to filter parents but don't remember why, and removing this
@@ -509,6 +534,8 @@ builtins. Remember that you should avoid to define new builtins when possible.'
             # the name has already been consumed, only check it's not a loop
             # variable used outside the loop
             if name in consumed:
+                defnode = assign_parent(consumed[name][0])
+                self._check_late_binding_closure(node, defnode, scope_type)
                 self._loopvar_name(node, name)
                 break
             # mark the name as consumed if it's defined in this scope
@@ -520,6 +547,7 @@ builtins. Remember that you should avoid to define new builtins when possible.'
             # checks for use before assignment
             defnode = assign_parent(to_consume[name][0])
             if defnode is not None:
+                self._check_late_binding_closure(node, defnode, scope_type)
                 defstmt = defnode.statement()
                 defframe = defstmt.frame()
                 maybee0601 = True
diff --git a/test/input/func_loopvar_in_closure.py b/test/input/func_loopvar_in_closure.py
new file mode 100644
index 0000000..32b7a6c
--- /dev/null
+++ b/test/input/func_loopvar_in_closure.py
@@ -0,0 +1,114 @@
+"""Tests for loopvar-in-closure."""
+
+__revision__ = 0
+
+
+def good_case():
+    """No problems here."""
+    lst = []
+    for i in range(10):
+        lst.append(i)
+
+
+def good_case2():
+    """No problems here."""
+    return [i for i in range(10)]
+
+
+def good_case3():
+    """No problems here."""
+    lst = []
+    for i in range(10):
+        lst.append(lambda i=i: i)
+
+
+def good_case4():
+    """No problems here."""
+    lst = []
+    for i in range(10):
+        print i
+        lst.append(lambda i: i)
+
+
+def good_case5():
+    """No problems here."""
+    return (i for i in range(10))
+
+
+def good_case6():
+    """Accept use of the variable after the loop.
+
+    There's already a warning about possibly undefined loop variables, and
+    the value will not change any more."""
+    for i in range(10):
+        print i
+    return lambda: i
+
+
+def good_case7():
+    """Accept use of the variable inside return."""
+    for i in range(10):
+        if i == 8:
+            return lambda: i
+    return lambda: -1
+
+
+def bad_case():
+    """Closing over a loop variable."""
+    lst = []
+    for i in range(10):
+        print i
+        lst.append(lambda: i)
+
+
+def bad_case2():
+    """Closing over a loop variable."""
+    return [lambda: i for i in range(10)]
+
+
+def bad_case3():
+    """Closing over variable defined in loop."""
+    lst = []
+    for i in range(10):
+        j = i * i
+        lst.append(lambda: j)
+    return lst
+
+
+def bad_case4():
+    """Closing over variable defined in loop."""
+    lst = []
+    for i in range(10):
+        def nested():
+            """Nested function."""
+            return i**2
+        lst.append(nested)
+    return lst
+
+
+def bad_case5():
+    """Problematic case.
+
+    If this function is used as
+
+    >>> [x() for x in bad_case5()]
+
+    it behaves 'as expected', i.e. the result is range(10).
+
+    If it's used with
+
+    >>> lst = list(bad_case5())
+    >>> [x() for x in lst]
+
+    the result is [9] * 10 again.
+    """
+    return (lambda: i for i in range(10))
+
+
+def bad_case6():
+    """Closing over variable defined in loop."""
+    lst = []
+    for i, j in zip(range(10), range(10, 20)):
+        print j
+        lst.append(lambda: i)
+    return lst
diff --git a/test/input/func_loopvar_in_dict_comp_py27.py b/test/input/func_loopvar_in_dict_comp_py27.py
new file mode 100644
index 0000000..312eee7
--- /dev/null
+++ b/test/input/func_loopvar_in_dict_comp_py27.py
@@ -0,0 +1,8 @@
+"""Tests for loopvar-in-closure."""
+
+__revision__ = 0
+
+
+def bad_case():
+    """Loop variable from dict comprehension."""
+    return {x: lambda: x for x in range(10)}
diff --git a/test/messages/func_break_or_return_in_try_finally.txt b/test/messages/func_break_or_return_in_try_finally.txt
index 4b674ae..04f27fe 100644
--- a/test/messages/func_break_or_return_in_try_finally.txt
+++ b/test/messages/func_break_or_return_in_try_finally.txt
@@ -1,2 +1,3 @@
 W: 18:insidious_break_and_return: break statement in finally block may swallow exception
 W: 20:insidious_break_and_return: return statement in finally block may swallow exception
+W: 39:break_and_return.strange: Cell variable my_var defined in loop
diff --git a/test/messages/func_loopvar_in_closure.txt b/test/messages/func_loopvar_in_closure.txt
new file mode 100644
index 0000000..6ca613a
--- /dev/null
+++ b/test/messages/func_loopvar_in_closure.txt
@@ -0,0 +1,8 @@
+W: 21:good_case3: Unused variable 'i'
+W: 45:good_case6.<lambda>: Using possibly undefined loop variable 'i'
+W: 61:bad_case.<lambda>: Cell variable i defined in loop
+W: 66:bad_case2.<lambda>: Cell variable i defined in loop
+W: 74:bad_case3.<lambda>: Cell variable j defined in loop
+W: 84:bad_case4.nested: Cell variable i defined in loop
+W:105:bad_case5.<lambda>: Cell variable i defined in loop
+W:113:bad_case6.<lambda>: Cell variable i defined in loop
diff --git a/test/messages/func_loopvar_in_dict_comp_py27.txt b/test/messages/func_loopvar_in_dict_comp_py27.txt
new file mode 100644
index 0000000..bc11121
--- /dev/null
+++ b/test/messages/func_loopvar_in_dict_comp_py27.txt
@@ -0,0 +1 @@
+W:  8:bad_case.<lambda>: Cell variable x defined in loop