diff options
author | Jacob Walls <jacobtylerwalls@gmail.com> | 2022-04-02 07:51:24 -0400 |
---|---|---|
committer | Pierre Sassoulas <pierre.sassoulas@gmail.com> | 2022-04-04 09:30:15 +0200 |
commit | 22b5dc1ae716e870873ac0b7d8b3369ca9896c38 (patch) | |
tree | 44fa58a9c5faf556a33126ee455924c0bd691c21 | |
parent | 05990167978b3acbb1fbf37b079602a057ee4774 (diff) | |
download | pylint-git-22b5dc1ae716e870873ac0b7d8b3369ca9896c38.tar.gz |
Account for more node types in handling of except block homonyms with comprehensions (#6073)
Fixes a false positive for `used-before-assignment`.
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: Daniƫl van Noord <13665637+DanielNoord@users.noreply.github.com>
-rw-r--r-- | ChangeLog | 6 | ||||
-rw-r--r-- | pylint/checkers/variables.py | 117 | ||||
-rw-r--r-- | tests/functional/u/unused/unused_argument.py | 6 | ||||
-rw-r--r-- | tests/functional/u/unused/unused_argument.txt | 12 | ||||
-rw-r--r-- | tests/functional/u/used/used_before_assignment_comprehension_homonyms.py (renamed from tests/functional/u/used/used_before_assignment_filtered_comprehension.py) | 44 |
5 files changed, 116 insertions, 69 deletions
@@ -20,6 +20,12 @@ What's New in Pylint 2.13.5? ============================ Release date: TBA +* Fix false positive regression in 2.13.0 for ``used-before-assignment`` for + homonyms between variable assignments in try/except blocks and variables in + subscripts in comprehensions. + + Closes #6069 + * Narrow the scope of the ``unnecessary-ellipsis`` checker to: * functions & classes which contain both a docstring and an ellipsis. * A body which contains an ellipsis ``nodes.Expr`` node & at least one other statement. diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index b92769822..bbbb7eb57 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -623,8 +623,8 @@ scope_type : {self._atomic.scope_type} ): return found_nodes - # And is not part of a test in a filtered comprehension - if VariablesChecker._has_homonym_in_comprehension_test(node): + # And no comprehension is under the node's frame + if VariablesChecker._comprehension_between_frame_and_node(node): return found_nodes # Filter out assignments in ExceptHandlers that node is not contained in @@ -1276,8 +1276,23 @@ class VariablesChecker(BaseChecker): global_names = _flattened_scope_names(node.nodes_of_class(nodes.Global)) nonlocal_names = _flattened_scope_names(node.nodes_of_class(nodes.Nonlocal)) + comprehension_target_names: List[str] = [] + + for comprehension_scope in node.nodes_of_class(nodes.ComprehensionScope): + for generator in comprehension_scope.generators: + self._find_assigned_names_recursive( + generator.target, comprehension_target_names + ) + for name, stmts in not_consumed.items(): - self._check_is_unused(name, node, stmts[0], global_names, nonlocal_names) + self._check_is_unused( + name, + node, + stmts[0], + global_names, + nonlocal_names, + comprehension_target_names, + ) visit_asyncfunctiondef = visit_functiondef leave_asyncfunctiondef = leave_functiondef @@ -1405,7 +1420,7 @@ class VariablesChecker(BaseChecker): continue action, nodes_to_consume = self._check_consumer( - node, stmt, frame, current_consumer, i, base_scope_type + node, stmt, frame, current_consumer, base_scope_type ) if nodes_to_consume: # Any nodes added to consumed_uncertain by get_next_to_consume() @@ -1476,6 +1491,20 @@ class VariablesChecker(BaseChecker): return False + def _find_assigned_names_recursive( + self, + target: Union[nodes.AssignName, nodes.BaseContainer], + target_names: List[str], + ) -> None: + """Update `target_names` in place with the names of assignment + targets, recursively (to account for nested assignments). + """ + if isinstance(target, nodes.AssignName): + target_names.append(target.name) + elif isinstance(target, nodes.BaseContainer): + for elt in target.elts: + self._find_assigned_names_recursive(elt, target_names) + # pylint: disable=too-many-return-statements def _check_consumer( self, @@ -1483,30 +1512,16 @@ class VariablesChecker(BaseChecker): stmt: nodes.NodeNG, frame: nodes.LocalsDictNodeNG, current_consumer: NamesConsumer, - consumer_level: int, base_scope_type: Any, ) -> Tuple[VariableVisitConsumerAction, Optional[List[nodes.NodeNG]]]: """Checks a consumer for conditions that should trigger messages.""" # If the name has already been consumed, only check it's not a loop # variable used outside the loop. - # Avoid the case where there are homonyms inside function scope and - # comprehension current scope (avoid bug #1731) if node.name in current_consumer.consumed: - if utils.is_func_decorator(current_consumer.node) or not ( - current_consumer.scope_type == "comprehension" - and self._has_homonym_in_upper_function_scope(node, consumer_level) - # But don't catch homonyms against the filter of a comprehension, - # (like "if x" in "[x for x in expr() if x]") - # https://github.com/PyCQA/pylint/issues/5586 - and not ( - self._has_homonym_in_comprehension_test(node) - # Or homonyms against values to keyword arguments - # (like "var" in "[func(arg=var) for var in expr()]") - or ( - isinstance(node.scope(), nodes.ComprehensionScope) - and isinstance(node.parent, (nodes.Call, nodes.Keyword)) - ) - ) + # Avoid the case where there are homonyms inside function scope and + # comprehension current scope (avoid bug #1731) + if utils.is_func_decorator(current_consumer.node) or not isinstance( + node, nodes.ComprehensionScope ): self._check_late_binding_closure(node) self._loopvar_name(node) @@ -2259,8 +2274,14 @@ class VariablesChecker(BaseChecker): self.add_message("undefined-loop-variable", args=node.name, node=node) def _check_is_unused( - self, name, node, stmt, global_names, nonlocal_names: Iterable[str] - ): + self, + name, + node, + stmt, + global_names, + nonlocal_names: Iterable[str], + comprehension_target_names: List[str], + ) -> None: # Ignore some special names specified by user configuration. if self._is_name_ignored(stmt, name): return @@ -2282,7 +2303,8 @@ class VariablesChecker(BaseChecker): argnames = node.argnames() # Care about functions with unknown argument (builtins) if name in argnames: - self._check_unused_arguments(name, node, stmt, argnames, nonlocal_names) + if name not in comprehension_target_names: + self._check_unused_arguments(name, node, stmt, argnames, nonlocal_names) else: if stmt.parent and isinstance( stmt.parent, (nodes.Assign, nodes.AnnAssign, nodes.Tuple) @@ -2455,46 +2477,15 @@ class VariablesChecker(BaseChecker): def _allowed_redefined_builtin(self, name): return name in self.config.allowed_redefined_builtins - def _has_homonym_in_upper_function_scope( - self, node: nodes.Name, index: int - ) -> bool: - """Return whether there is a node with the same name in the - to_consume dict of an upper scope and if that scope is a function - - :param node: node to check for - :param index: index of the current consumer inside self._to_consume - :return: True if there is a node with the same name in the - to_consume dict of an upper scope and if that scope - is a function, False otherwise - """ - return any( - _consumer.scope_type == "function" and node.name in _consumer.to_consume - for _consumer in self._to_consume[index - 1 :: -1] - ) - @staticmethod - def _has_homonym_in_comprehension_test(node: nodes.Name) -> bool: - """Return True if `node`'s frame contains a comprehension employing an - identical name in a test. - - The name in the test could appear at varying depths: - - Examples: - [x for x in range(3) if name] - [x for x in range(3) if name.num == 1] - [x for x in range(3)] if call(name.num)] - """ - closest_comprehension = utils.get_node_first_ancestor_of_type( - node, nodes.Comprehension - ) - return ( - closest_comprehension is not None - and node.frame(future=True).parent_of(closest_comprehension) - and any( - test is node or test.parent_of(node) - for test in closest_comprehension.ifs - ) + def _comprehension_between_frame_and_node(node: nodes.Name) -> bool: + """Return True if a ComprehensionScope intervenes between `node` and its frame.""" + closest_comprehension_scope = utils.get_node_first_ancestor_of_type( + node, nodes.ComprehensionScope ) + return closest_comprehension_scope is not None and node.frame( + future=True + ).parent_of(closest_comprehension_scope) def _store_type_annotation_node(self, type_annotation): """Given a type annotation, store all the name nodes it refers to.""" diff --git a/tests/functional/u/unused/unused_argument.py b/tests/functional/u/unused/unused_argument.py index 91243ce75..af2ae8ae4 100644 --- a/tests/functional/u/unused/unused_argument.py +++ b/tests/functional/u/unused/unused_argument.py @@ -47,6 +47,12 @@ def metadata_from_dict(key): """ return {key: str(value) for key, value in key.items()} + +def metadata_from_dict_2(key): + """Similar, but with more nesting""" + return {key: (a, b) for key, (a, b) in key.items()} + + # pylint: disable=too-few-public-methods, misplaced-future,wrong-import-position from __future__ import print_function diff --git a/tests/functional/u/unused/unused_argument.txt b/tests/functional/u/unused/unused_argument.txt index f73b9b528..92795058e 100644 --- a/tests/functional/u/unused/unused_argument.txt +++ b/tests/functional/u/unused/unused_argument.txt @@ -1,9 +1,9 @@ unused-argument:3:16:3:21:test_unused:Unused argument 'first':HIGH unused-argument:3:23:3:29:test_unused:Unused argument 'second':HIGH unused-argument:32:29:32:32:Sub.newmethod:Unused argument 'aay':INFERENCE -unused-argument:54:13:54:16:function:Unused argument 'arg':HIGH -unused-argument:61:21:61:24:AAAA.method:Unused argument 'arg':INFERENCE -unused-argument:68:0:None:None:AAAA.selected:Unused argument 'args':INFERENCE -unused-argument:68:0:None:None:AAAA.selected:Unused argument 'kwargs':INFERENCE -unused-argument:87:23:87:26:BBBB.__init__:Unused argument 'arg':INFERENCE -unused-argument:98:34:98:39:Ancestor.set_thing:Unused argument 'other':INFERENCE +unused-argument:60:13:60:16:function:Unused argument 'arg':HIGH +unused-argument:67:21:67:24:AAAA.method:Unused argument 'arg':INFERENCE +unused-argument:74:0:None:None:AAAA.selected:Unused argument 'args':INFERENCE +unused-argument:74:0:None:None:AAAA.selected:Unused argument 'kwargs':INFERENCE +unused-argument:93:23:93:26:BBBB.__init__:Unused argument 'arg':INFERENCE +unused-argument:104:34:104:39:Ancestor.set_thing:Unused argument 'other':INFERENCE diff --git a/tests/functional/u/used/used_before_assignment_filtered_comprehension.py b/tests/functional/u/used/used_before_assignment_comprehension_homonyms.py index f0c569aad..feae58dbe 100644 --- a/tests/functional/u/used/used_before_assignment_filtered_comprehension.py +++ b/tests/functional/u/used/used_before_assignment_comprehension_homonyms.py @@ -49,3 +49,47 @@ def func5(): except ZeroDivisionError: k = None print(k, filtered) + + +def func6(data, keys): + """Similar, but with a subscript in a key-value pair rather than the test + See https://github.com/PyCQA/pylint/issues/6069""" + try: + results = {key: data[key] for key in keys} + except KeyError as exc: + key, *_ = exc.args + raise Exception(f"{key} not found") from exc + + return results + + +def func7(): + """Similar, but with a comparison""" + bools = [str(i) == i for i in range(3)] + + try: + 1 / 0 + except ZeroDivisionError: + i = None + print(i, bools) + + +def func8(): + """Similar, but with a container""" + pairs = [(i, i) for i in range(3)] + + try: + 1 / 0 + except ZeroDivisionError: + i = None + print(i, pairs) + + +# Module level cases + +module_ints = [j | j for j in range(3)] +try: + 1 / 0 +except ZeroDivisionError: + j = None + print(j, module_ints) |