Implemented new check consider-using-dict-items (#4445)

Co-authored-by: Marc Mueller <[email protected]>

Author: yushao2

CONTRIBUTORS.txt

@@ -484,3 +484,5 @@ contributors:
 * Jiajunsu (victor): contributor
 
 * Andrew Haigh (nelfin): contributor
+
+* Pang Yu Shao (yushao2): contributor

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?
 ===========================

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
 =============
 

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

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

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

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]

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()