diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index fb422cf7ba..7d1cd868b7 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -484,3 +484,5 @@ contributors: * Jiajunsu (victor): contributor * Andrew Haigh (nelfin): contributor + +* Pang Yu Shao (yushao2): contributor diff --git a/ChangeLog b/ChangeLog index de7a504220..b9e279ab9a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -33,6 +33,11 @@ modules are added. * Fix raising false-positive ``no-member`` on abstract properties +* New checker ``consider-using-dict-items``. Emitted when iterating over dictionary keys and then + indexing the same dictionary with the key within loop body. + + Closes #3389 + What's New in Pylint 2.8.2? =========================== diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index a0053e98f0..8f5bd43f2f 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -12,6 +12,9 @@ Summary -- Release highlights New checkers ============ +* ``consider-using-dict-items``: Emitted when iterating over dictionary keys and then + indexing the same dictionary with the key within loop body. + Other Changes ============= diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index b1175f03e0..6efc4190c4 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -1,5 +1,6 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html # For details: https://github.com/PyCQA/pylint/blob/master/LICENSE +from typing import cast import astroid @@ -26,6 +27,14 @@ class RecommendationChecker(checkers.BaseChecker): "method. It is enough to just iterate through the dictionary itself, as " 'in "for key in dictionary".', ), + "C0206": ( + "Consider iterating with .items()", + "consider-using-dict-items", + "Emitted when iterating over the keys of a dictionary and accessing the " + "value by index lookup." + "Both the key and value can be accessed by iterating using the .items() " + "method of the dictionary instead.", + ), } @staticmethod @@ -53,8 +62,12 @@ def visit_call(self, node): if isinstance(node.parent, (astroid.For, astroid.Comprehension)): self.add_message("consider-iterating-dictionary", node=node) - @utils.check_messages("consider-using-enumerate") - def visit_for(self, node): + @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") + def visit_for(self, node: astroid.For) -> None: + self._check_consider_using_enumerate(node) + self._check_consider_using_dict_items(node) + + def _check_consider_using_enumerate(self, node: astroid.For) -> None: """Emit a convention whenever range and len are used for indexing.""" # Verify that we have a `range([start], len(...), [stop])` call and # that the object which is iterated is used as a subscript in the @@ -119,3 +132,81 @@ def visit_for(self, node): continue self.add_message("consider-using-enumerate", node=node) return + + def _check_consider_using_dict_items(self, node: astroid.For) -> None: + """Add message when accessing dict values by index lookup.""" + # Verify that we have a .keys() call and + # that the object which is iterated is used as a subscript in the + # body of the for. + + iterating_object_name = utils.get_iterating_dictionary_name(node) + if iterating_object_name is None: + return + + # Verify that the body of the for loop uses a subscript + # with the object that was iterated. This uses some heuristics + # in order to make sure that the same object is used in the + # for body. + for child in node.body: + for subscript in child.nodes_of_class(astroid.Subscript): + subscript = cast(astroid.Subscript, subscript) + + if not isinstance(subscript.value, (astroid.Name, astroid.Attribute)): + continue + + value = subscript.slice + if isinstance(value, astroid.Index): + value = value.value + if ( + not isinstance(value, astroid.Name) + or value.name != node.target.name + or iterating_object_name != subscript.value.as_string() + ): + continue + last_definition_lineno = value.lookup(value.name)[1][-1].lineno + if last_definition_lineno > node.lineno: + # Ignore this subscript if it has been redefined after + # the for loop. This checks for the line number using .lookup() + # to get the line number where the iterating object was last + # defined and compare that to the for loop's line number + continue + if ( + isinstance(subscript.parent, astroid.Assign) + and subscript in subscript.parent.targets + or isinstance(subscript.parent, astroid.AugAssign) + and subscript == subscript.parent.target + ): + # Ignore this subscript if it is the target of an assignment + continue + + self.add_message("consider-using-dict-items", node=node) + return + + @utils.check_messages("consider-using-dict-items") + def visit_comprehension(self, node: astroid.Comprehension) -> None: + iterating_object_name = utils.get_iterating_dictionary_name(node) + if iterating_object_name is None: + return + + children = list(node.parent.get_children()) + if node.ifs: + children.extend(node.ifs) + for child in children: + for subscript in child.nodes_of_class(astroid.Subscript): + subscript = cast(astroid.Subscript, subscript) + + if not isinstance(subscript.value, (astroid.Name, astroid.Attribute)): + continue + + value = subscript.slice + if isinstance(value, astroid.Index): + value = value.value + if ( + not isinstance(value, astroid.Name) + or value.name != node.target.name + or iterating_object_name != subscript.value.as_string() + ): + continue + + self.add_message("consider-using-dict-items", node=node) + return diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py index 8ce0c9171e..f64960a218 100644 --- a/pylint/checkers/typecheck.py +++ b/pylint/checkers/typecheck.py @@ -1439,8 +1439,8 @@ def visit_call(self, node): args=(display_name, callable_name), ) - for name in kwparams: - defval, assigned = kwparams[name] + for name, val in kwparams.items(): + defval, assigned = val if ( defval is None and not assigned diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 6c11eeb9d9..8cf97d8074 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1492,3 +1492,33 @@ def is_assign_name_annotated_with(node: astroid.AssignName, typing_name: str) -> ): return True return False + + +def get_iterating_dictionary_name( + node: Union[astroid.For, astroid.Comprehension] +) -> Optional[str]: + """Get the name of the dictionary which keys are being iterated over on + a `astroid.For` or `astroid.Comprehension` node. + + If the iterating object is not either the keys method of a dictionary + or a dictionary itself, this returns None. + """ + # Is it a proper keys call? + if ( + isinstance(node.iter, astroid.Call) + and isinstance(node.iter.func, astroid.Attribute) + and node.iter.func.attrname == "keys" + ): + inferred = safe_infer(node.iter.func) + if not isinstance(inferred, astroid.BoundMethod): + return None + return node.iter.as_string().rpartition(".keys")[0] + + # Is it a dictionary? + if isinstance(node.iter, (astroid.Name, astroid.Attribute)): + inferred = safe_infer(node.iter) + if not isinstance(inferred, astroid.Dict): + return None + return node.iter.as_string() + + return None diff --git a/tests/functional/c/consider/consider_using_dict_items.py b/tests/functional/c/consider/consider_using_dict_items.py new file mode 100644 index 0000000000..1d6c86741b --- /dev/null +++ b/tests/functional/c/consider/consider_using_dict_items.py @@ -0,0 +1,86 @@ +"""Emit a message for iteration through dict keys and subscripting dict with key.""" +# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods + +def bad(): + a_dict = {1: 1, 2: 2, 3: 3} + for k in a_dict: # [consider-using-dict-items] + print(a_dict[k]) + another_dict = dict() + for k in another_dict: # [consider-using-dict-items] + print(another_dict[k]) + + +def good(): + a_dict = {1: 1, 2: 2, 3: 3} + for k in a_dict: + print(k) + +out_of_scope_dict = dict() + +def another_bad(): + for k in out_of_scope_dict: # [consider-using-dict-items] + print(out_of_scope_dict[k]) + +def another_good(): + for k in out_of_scope_dict: + k = 1 + k = 2 + k = 3 + print(out_of_scope_dict[k]) + + +b_dict = {} +for k2 in b_dict: # Should not emit warning, key access necessary + b_dict[k2] = 2 + +for k2 in b_dict: # Should not emit warning, key access necessary (AugAssign) + b_dict[k2] += 2 + +# Warning should be emitted in this case +for k6 in b_dict: # [consider-using-dict-items] + val = b_dict[k6] + b_dict[k6] = 2 + +for k3 in b_dict: # [consider-using-dict-items] + val = b_dict[k3] + +for k4 in b_dict.keys(): # [consider-iterating-dictionary,consider-using-dict-items] + val = b_dict[k4] + +class Foo: + c_dict = {} + +# Should emit warning when iterating over a dict attribute of a class +for k5 in Foo.c_dict: # [consider-using-dict-items] + val = Foo.c_dict[k5] + +c_dict = {} + +# Should NOT emit warning whey key used to access a different dict +for k5 in Foo.c_dict: # This is fine + val = b_dict[k5] + +for k5 in Foo.c_dict: # This is fine + val = c_dict[k5] + +# Should emit warning within a list/dict comprehension +val = {k9: b_dict[k9] for k9 in b_dict} # [consider-using-dict-items] +val = [(k7, b_dict[k7]) for k7 in b_dict] # [consider-using-dict-items] + +# Should emit warning even when using dict attribute of a class within comprehension +val = [(k7, Foo.c_dict[k7]) for k7 in Foo.c_dict] # [consider-using-dict-items] +val = any(True for k8 in Foo.c_dict if Foo.c_dict[k8]) # [consider-using-dict-items] + +# Should emit warning when dict access done in ``if`` portion of comprehension +val = any(True for k8 in b_dict if b_dict[k8]) # [consider-using-dict-items] + +# Should NOT emit warning whey key used to access a different dict +val = [(k7, b_dict[k7]) for k7 in Foo.c_dict] +val = any(True for k8 in Foo.c_dict if b_dict[k8]) + +# Should NOT emit warning, essentially same check as above +val = [(k7, c_dict[k7]) for k7 in Foo.c_dict] +val = any(True for k8 in Foo.c_dict if c_dict[k8]) + +# Should emit warning, using .keys() of Foo.c_dict +val = any(True for k8 in Foo.c_dict.keys() if Foo.c_dict[k8]) # [consider-iterating-dictionary,consider-using-dict-items] diff --git a/tests/functional/c/consider/consider_using_dict_items.txt b/tests/functional/c/consider/consider_using_dict_items.txt new file mode 100644 index 0000000000..8ead82351b --- /dev/null +++ b/tests/functional/c/consider/consider_using_dict_items.txt @@ -0,0 +1,15 @@ +consider-using-dict-items:6:4:bad:Consider iterating with .items() +consider-using-dict-items:9:4:bad:Consider iterating with .items() +consider-using-dict-items:21:4:another_bad:Consider iterating with .items() +consider-using-dict-items:40:0::Consider iterating with .items() +consider-using-dict-items:44:0::Consider iterating with .items() +consider-iterating-dictionary:47:10::Consider iterating the dictionary directly instead of calling .keys() +consider-using-dict-items:47:0::Consider iterating with .items() +consider-using-dict-items:54:0::Consider iterating with .items() +consider-using-dict-items:67:0::Consider iterating with .items() +consider-using-dict-items:68:0::Consider iterating with .items() +consider-using-dict-items:71:0::Consider iterating with .items() +consider-using-dict-items:72:0::Consider iterating with .items() +consider-using-dict-items:75:0::Consider iterating with .items() +consider-iterating-dictionary:86:25::Consider iterating the dictionary directly instead of calling .keys() +consider-using-dict-items:86:0::Consider iterating with .items()