Fix bugs in W0640 cell-var-from-loop checker

ChangeLog

@@ -97,6 +97,15 @@ Release date: TBA
 
   Closes #4102
 
+* Fixed ``cell-var-from-loop`` checker: handle cell variables in comprehensions within functions,
+  and function default argument expressions. Also handle basic variable shadowing.
+
+  Closes #2846
+  Closes #3107
+
+* Fixed bug with ``cell-var-from-loop`` checker: it no longer has false negatives when
+  both ``unused-variable`` and ``used-before-assignment`` are disabled.
+
 
 What's New in Pylint 2.9.6?
 ===========================

doc/whatsnew/2.10.rst

@@ -82,3 +82,12 @@ Other Changes
 * Added ``redundant-u-string-prefix`` checker: Emitted when the u prefix is added to a string
 
   Closes #4102
+
+* Fixed ``cell-var-from-loop`` checker: handle cell variables in comprehensions within functions,
+  and function default argument expressions. Also handle basic variable shadowing.
+
+  Closes #2846
+  Closes #3107
+
+* Fixed bug with ``cell-var-from-loop`` checker: it no longer has false negatives when
+  both ``unused-variable`` and ``used-before-assignment`` are disabled.

pylint/checkers/utils.py

@@ -455,6 +455,11 @@ def is_ancestor_name(
     return False
 
 
+def is_being_called(node: astroid.node_classes.NodeNG) -> bool:
+    """return True if node is the function being called in a Call node"""
+    return isinstance(node.parent, astroid.Call) and node.parent.func is node
+
+
 def assign_parent(node: astroid.node_classes.NodeNG) -> astroid.node_classes.NodeNG:
     """return the higher parent which is not an AssignName, Tuple or List node"""
     while node and isinstance(node, (astroid.AssignName, astroid.Tuple, astroid.List)):

pylint/checkers/variables.py

@@ -1034,8 +1034,7 @@ def visit_name(self, node):
                     and self._has_homonym_in_upper_function_scope(node, i)
                 )
             ):
-                defnode = utils.assign_parent(current_consumer.consumed[name][0])
-                self._check_late_binding_closure(node, defnode)
+                self._check_late_binding_closure(node)
                 self._loopvar_name(node, name)
                 break
 
@@ -1051,10 +1050,11 @@ def visit_name(self, node):
                 if used_before_assignment_is_enabled:
                     self.add_message("used-before-assignment", args=name, node=node)
 
+            self._check_late_binding_closure(node)
+
             if (
                 undefined_variable_is_enabled or used_before_assignment_is_enabled
             ) and defnode is not None:
-                self._check_late_binding_closure(node, defnode)
                 defstmt = defnode.statement()
                 defframe = defstmt.frame()
                 # The class reuses itself in the class scope.
@@ -1804,27 +1804,41 @@ def _check_unused_arguments(self, name, node, stmt, argnames):
 
         self.add_message("unused-argument", args=name, node=stmt, confidence=confidence)
 
-    def _check_late_binding_closure(self, node, assignment_node):
+    def _check_late_binding_closure(self, node: astroid.Name) -> None:
+        """Check whether node is a cell var that is assigned within a containing loop.
+
+        Special cases where we don't care about the error:
+        1. When the node's function is immediately called, e.g. (lambda: i)()
+        2. When the node's function is returned from within the loop, e.g. return lambda: i
+        """
         if not self.linter.is_message_enabled("cell-var-from-loop"):
             return
 
-        def _is_direct_lambda_call():
-            return (
-                isinstance(node_scope.parent, astroid.Call)
-                and node_scope.parent.func is node_scope
-            )
+        node_scope = node.frame()
+
+        # If node appears in a default argument expression,
+        # look at the next enclosing frame instead
+        if utils.is_default_argument(node, node_scope):
+            node_scope = node_scope.parent.frame()
 
-        node_scope = node.scope()
-        if not isinstance(node_scope, (astroid.Lambda, astroid.FunctionDef)):
+        # Check if node is a cell var
+        if (
+            not isinstance(node_scope, (astroid.Lambda, astroid.FunctionDef))
+            or node.name in node_scope.locals
+        ):
             return
-        if isinstance(node.parent, astroid.Arguments):
+
+        assign_scope, stmts = node.lookup(node.name)
+        if not stmts or not assign_scope.parent_of(node_scope):
             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)
+        if utils.is_comprehension(assign_scope):
+            self.add_message("cell-var-from-loop", node=node, args=node.name)
         else:
-            assign_scope = assignment_node.scope()
+            # Look for an enclosing For loop.
+            # Currently, we only consider the first assignment
+            assignment_node = stmts[0]
+
             maybe_for = assignment_node
             while maybe_for and not isinstance(maybe_for, astroid.For):
                 if maybe_for is assign_scope:
@@ -1834,7 +1848,7 @@ def _is_direct_lambda_call():
                 if (
                     maybe_for
                     and maybe_for.parent_of(node_scope)
-                    and not _is_direct_lambda_call()
+                    and not utils.is_being_called(node_scope)
                     and not isinstance(node_scope.statement(), astroid.Return)
                 ):
                     self.add_message("cell-var-from-loop", node=node, args=node.name)

tests/functional/c/cellvar_escaping_loop.py

@@ -62,6 +62,37 @@ def func(bound_i=i):
     return funs
 
 
+def good_case9():
+    """Ignore when the cell var is not defined in a loop"""
+    i = 10
+    lst = []
+    for _ in range(10):
+        lst.append(lambda: i)
+    return lst
+
+
+def good_case10():
+    """Ignore when a loop variable is showdowed by an inner function"""
+    lst = []
+    for i in range(10):  # pylint: disable=unused-variable
+        def func():
+            i = 100
+            def func2(arg=i):
+                return arg
+
+            return func2
+
+        lst.append(func)
+    return lst
+
+
+def good_case_issue3107():
+    """Eager binding of cell variable when used in a non-trivial default argument expression.
+    """
+    for i in [[2], [3]]:
+        next(filter(lambda j, ix=i[0]: j == ix, [1, 3]))
+
+
 def bad_case():
     """Closing over a loop variable."""
     lst = []
@@ -123,6 +154,63 @@ def bad_case6():
     return lst
 
 
+def bad_case7():
+    """Multiple variables unpacked in comprehension."""
+    return [
+        lambda: (
+            x  # [cell-var-from-loop]
+            + y)  # [cell-var-from-loop]
+        for x, y in ((1, 2), (3, 4), (5, 6))
+    ]
+
+
+def bad_case8():
+    """Closing over variable defined in loop below the function."""
+    lst = []
+    for i in range(10):
+        lst.append(lambda: j)  # [cell-var-from-loop]
+        j = i * i
+    return lst
+
+
+def bad_case9():
+    """Detect when loop variable shadows an outer assignment."""
+    lst = []
+    i = 100
+    for i in range(10):
+        lst.append(lambda: i)  # [cell-var-from-loop]
+    return lst
+
+
+def bad_case10():
+    """Detect when a loop variable is the default argument for a nested function"""
+    lst = []
+    for i in range(10):
+        def func():
+            def func2(arg=i):  # [cell-var-from-loop]
+                return arg
+
+            return func2
+
+        lst.append(func)
+    return lst
+
+
+def bad_case_issue2846():
+    """Closing over variable that is used within a comprehension in the function body."""
+    lst_a = [
+        (lambda: n)  # [cell-var-from-loop]
+        for n in range(3)
+    ]
+
+    lst_b = [
+        (lambda: [n for _ in range(3)])  # [cell-var-from-loop]
+        for n in range(3)
+    ]
+
+    return lst_a, lst_b
+
+
 class Test(Enum):
     TEST = (40, 160)
 

tests/functional/c/cellvar_escaping_loop.txt

@@ -1,6 +1,13 @@
-cell-var-from-loop:70:27:bad_case.<lambda>:Cell variable i defined in loop
-cell-var-from-loop:75:20:bad_case2.<lambda>:Cell variable i defined in loop
-cell-var-from-loop:83:27:bad_case3.<lambda>:Cell variable j defined in loop
-cell-var-from-loop:93:19:bad_case4.nested:Cell variable i defined in loop
-cell-var-from-loop:114:20:bad_case5.<lambda>:Cell variable i defined in loop
-cell-var-from-loop:122:27:bad_case6.<lambda>:Cell variable i defined in loop
+cell-var-from-loop:101:27:bad_case.<lambda>:Cell variable i defined in loop:HIGH
+cell-var-from-loop:106:20:bad_case2.<lambda>:Cell variable i defined in loop:HIGH
+cell-var-from-loop:114:27:bad_case3.<lambda>:Cell variable j defined in loop:HIGH
+cell-var-from-loop:124:19:bad_case4.nested:Cell variable i defined in loop:HIGH
+cell-var-from-loop:145:20:bad_case5.<lambda>:Cell variable i defined in loop:HIGH
+cell-var-from-loop:153:27:bad_case6.<lambda>:Cell variable i defined in loop:HIGH
+cell-var-from-loop:161:12:bad_case7.<lambda>:Cell variable x defined in loop:HIGH
+cell-var-from-loop:162:14:bad_case7.<lambda>:Cell variable y defined in loop:HIGH
+cell-var-from-loop:171:27:bad_case8.<lambda>:Cell variable j defined in loop:HIGH
+cell-var-from-loop:181:27:bad_case9.<lambda>:Cell variable i defined in loop:HIGH
+cell-var-from-loop:190:26:bad_case10.func.func2:Cell variable i defined in loop:HIGH
+cell-var-from-loop:202:17:bad_case_issue2846.<lambda>:Cell variable n defined in loop:HIGH
+cell-var-from-loop:207:18:bad_case_issue2846.<lambda>:Cell variable n defined in loop:HIGH

tests/functional/c/consider/consider_using_enumerate.py

@@ -56,7 +56,7 @@ def good():
 
     for index in range(len(iterable)):
         def test(iterable):
-            return iterable[index]
+            return iterable[index]  # pylint: disable=cell-var-from-loop
         yield test([1, 2, 3])