diff --git a/ChangeLog b/ChangeLog index b0a938e98e..2c90999d33 100644 --- a/ChangeLog +++ b/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. + * Fix false postive for ``invalid-all-format`` if the list or tuple builtin functions are used Closes #4711 diff --git a/doc/whatsnew/2.10.rst b/doc/whatsnew/2.10.rst index 67b6067a28..a754d0bf93 100644 --- a/doc/whatsnew/2.10.rst +++ b/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. diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index ffec05d6e3..9136419fae 100644 --- a/pylint/checkers/utils.py +++ b/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)): diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 433d14e8f2..7009bfd5ee 100644 --- a/pylint/checkers/variables.py +++ b/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) diff --git a/tests/functional/c/cellvar_escaping_loop.py b/tests/functional/c/cellvar_escaping_loop.py index 48d2291ddc..d2c1fb4a56 100644 --- a/tests/functional/c/cellvar_escaping_loop.py +++ b/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) diff --git a/tests/functional/c/cellvar_escaping_loop.txt b/tests/functional/c/cellvar_escaping_loop.txt index 49e8cb5fdd..4513d833fb 100644 --- a/tests/functional/c/cellvar_escaping_loop.txt +++ b/tests/functional/c/cellvar_escaping_loop.txt @@ -1,6 +1,13 @@ -cell-var-from-loop:70:27:bad_case.:Cell variable i defined in loop -cell-var-from-loop:75:20:bad_case2.:Cell variable i defined in loop -cell-var-from-loop:83:27:bad_case3.: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.:Cell variable i defined in loop -cell-var-from-loop:122:27:bad_case6.:Cell variable i defined in loop +cell-var-from-loop:101:27:bad_case.:Cell variable i defined in loop:HIGH +cell-var-from-loop:106:20:bad_case2.:Cell variable i defined in loop:HIGH +cell-var-from-loop:114:27:bad_case3.: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.:Cell variable i defined in loop:HIGH +cell-var-from-loop:153:27:bad_case6.:Cell variable i defined in loop:HIGH +cell-var-from-loop:161:12:bad_case7.:Cell variable x defined in loop:HIGH +cell-var-from-loop:162:14:bad_case7.:Cell variable y defined in loop:HIGH +cell-var-from-loop:171:27:bad_case8.:Cell variable j defined in loop:HIGH +cell-var-from-loop:181:27:bad_case9.: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.:Cell variable n defined in loop:HIGH +cell-var-from-loop:207:18:bad_case_issue2846.:Cell variable n defined in loop:HIGH diff --git a/tests/functional/c/consider/consider_using_enumerate.py b/tests/functional/c/consider/consider_using_enumerate.py index 7db4968a6d..bf29d68854 100644 --- a/tests/functional/c/consider/consider_using_enumerate.py +++ b/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])