From 59fcee719ebb4a30724c9c02814e50a9a78681ff Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Thu, 19 May 2022 09:08:25 -0400 Subject: [PATCH] Emit modified-iterating-list|dict|set for literals or usage of `del` --- doc/whatsnew/2/2.15/index.rst | 5 +++ pylint/checkers/modified_iterating_checker.py | 35 ++++++++++++++++--- pylint/checkers/utils.py | 14 +++++++- pylint/checkers/variables.py | 23 +++--------- tests/functional/m/modified_iterating.py | 12 +++++++ tests/functional/m/modified_iterating.txt | 10 ++++-- 6 files changed, 72 insertions(+), 27 deletions(-) diff --git a/doc/whatsnew/2/2.15/index.rst b/doc/whatsnew/2/2.15/index.rst index 519232b22e..6a285fe028 100644 --- a/doc/whatsnew/2/2.15/index.rst +++ b/doc/whatsnew/2/2.15/index.rst @@ -31,6 +31,11 @@ False positives fixed False negatives fixed ===================== +* Emit ``modified-iterating-list`` and analogous messages for dicts and sets when iterating + literals, or when using the ``del`` keyword. + + Closes #6648 + Other bug fixes =============== diff --git a/pylint/checkers/modified_iterating_checker.py b/pylint/checkers/modified_iterating_checker.py index a4e8520937..039257b6a6 100644 --- a/pylint/checkers/modified_iterating_checker.py +++ b/pylint/checkers/modified_iterating_checker.py @@ -56,9 +56,8 @@ class ModifiedIterationChecker(checkers.BaseChecker): ) def visit_for(self, node: nodes.For) -> None: iter_obj = node.iter - if isinstance(iter_obj, nodes.Name): - for body_node in node.body: - self._modified_iterating_check_on_node_and_children(body_node, iter_obj) + for body_node in node.body: + self._modified_iterating_check_on_node_and_children(body_node, iter_obj) def _modified_iterating_check_on_node_and_children( self, body_node: nodes.NodeNG, iter_obj: nodes.NodeNG @@ -72,7 +71,19 @@ def _modified_iterating_check( self, node: nodes.NodeNG, iter_obj: nodes.NodeNG ) -> None: msg_id = None - if self._modified_iterating_list_cond(node, iter_obj): + if isinstance(node, nodes.Delete) and any( + self._deleted_iteration_target_cond(t, iter_obj) for t in node.targets + ): + inferred = utils.safe_infer(iter_obj) + if isinstance(inferred, nodes.List): + msg_id = "modified-iterating-list" + elif isinstance(inferred, nodes.Dict): + msg_id = "modified-iterating-dict" + elif isinstance(inferred, nodes.Set): + msg_id = "modified-iterating-set" + elif not isinstance(iter_obj, nodes.Name): + pass + elif self._modified_iterating_list_cond(node, iter_obj): msg_id = "modified-iterating-list" elif self._modified_iterating_dict_cond(node, iter_obj): msg_id = "modified-iterating-dict" @@ -150,6 +161,22 @@ def _modified_iterating_set_cond( and node.value.func.attrname in _SET_MODIFIER_METHODS ) + def _deleted_iteration_target_cond( + self, node: nodes.DelName, iter_obj: nodes.NodeNG + ) -> bool: + if not isinstance(node, nodes.DelName): + return False + if not isinstance(iter_obj.parent, nodes.For): + return False + if not isinstance( + iter_obj.parent.target, (nodes.AssignName, nodes.BaseContainer) + ): + return False + return any( + t == node.name + for t in utils.find_assigned_names_recursive(iter_obj.parent.target) + ) + def register(linter: PyLinter) -> None: linter.register_checker(ModifiedIterationChecker(linter)) diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 8ae5f354b5..f13e0e2b3b 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -12,7 +12,7 @@ import re import string import warnings -from collections.abc import Iterable +from collections.abc import Iterable, Iterator from functools import lru_cache, partial from re import Match from typing import TYPE_CHECKING, Callable, TypeVar @@ -1792,3 +1792,15 @@ def in_for_else_branch(parent: nodes.NodeNG, stmt: nodes.Statement) -> bool: return isinstance(parent, nodes.For) and any( else_stmt.parent_of(stmt) or else_stmt == stmt for else_stmt in parent.orelse ) + + +def find_assigned_names_recursive( + target: nodes.AssignName | nodes.BaseContainer, +) -> Iterator[str]: + """Yield the names of assignment targets, accounting for nested ones.""" + if isinstance(target, nodes.AssignName): + if target.name is not None: + yield target.name + elif isinstance(target, nodes.BaseContainer): + for elt in target.elts: + yield from find_assigned_names_recursive(elt) diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 59d2fa0307..3e503849da 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -1234,13 +1234,12 @@ def leave_functiondef(self, node: nodes.FunctionDef) -> None: 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] = [] + comprehension_target_names: set[str] = set() 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 in utils.find_assigned_names_recursive(generator.target): + comprehension_target_names.add(name) for name, stmts in not_consumed.items(): self._check_is_unused( @@ -1449,20 +1448,6 @@ def _should_node_be_skipped( return False - def _find_assigned_names_recursive( - self, - target: 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, @@ -2264,7 +2249,7 @@ def _check_is_unused( stmt, global_names, nonlocal_names: Iterable[str], - comprehension_target_names: list[str], + comprehension_target_names: Iterable[str], ) -> None: # Ignore some special names specified by user configuration. if self._is_name_ignored(stmt, name): diff --git a/tests/functional/m/modified_iterating.py b/tests/functional/m/modified_iterating.py index 8817c1db0e..2f2798d507 100644 --- a/tests/functional/m/modified_iterating.py +++ b/tests/functional/m/modified_iterating.py @@ -47,6 +47,18 @@ item_set.remove(4) # [modified-iterating-set] item_list.remove(1) # [modified-iterating-list] +for item in [1, 2, 3]: + del item # [modified-iterating-list] + +for inner_first, inner_second in [[1, 2], [1, 2]]: + del inner_second # [modified-iterating-list] + +for k in my_dict: + del k # [modified-iterating-dict] + +for element in item_set: + del element # [modified-iterating-set] + # Check for nested for loops and changes to iterators for l in item_list: item_list.append(1) # [modified-iterating-list] diff --git a/tests/functional/m/modified_iterating.txt b/tests/functional/m/modified_iterating.txt index 77001b1720..90cabc22ea 100644 --- a/tests/functional/m/modified_iterating.txt +++ b/tests/functional/m/modified_iterating.txt @@ -5,6 +5,10 @@ modified-iterating-set:39:4:39:27::Iterated set 'item_set' is being modified ins modified-iterating-list:46:8:46:27::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE modified-iterating-set:47:8:47:26::Iterated set 'item_set' is being modified inside for loop body, iterate through a copy of it instead.:INFERENCE modified-iterating-list:48:4:48:23::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE -modified-iterating-list:52:4:52:23::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE -modified-iterating-list:55:12:55:31::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE -modified-iterating-list:57:16:57:35::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE +modified-iterating-list:51:4:51:12::Iterated list 'list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE +modified-iterating-list:54:4:54:20::Iterated list 'list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE +modified-iterating-dict:57:4:57:9::Iterated dict 'my_dict' is being modified inside for loop body, iterate through a copy of it instead.:INFERENCE +modified-iterating-set:60:4:60:15::Iterated set 'item_set' is being modified inside for loop body, iterate through a copy of it instead.:INFERENCE +modified-iterating-list:64:4:64:23::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE +modified-iterating-list:67:12:67:31::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE +modified-iterating-list:69:16:69:35::Iterated list 'item_list' is being modified inside for loop body, consider iterating through a copy of it instead.:INFERENCE