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

Author: yushao2

CONTRIBUTORS.txt

@@ -482,3 +482,5 @@ contributors:
 * das-intensity: contributor
 
 * Jiajunsu (victor): contributor
+
+* Pang Yu Shao (yushao2): contributor

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

@@ -26,6 +26,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 the keys of a dictionary are iterated and the items of the "
+            "dictionary is accessed by indexing with the key in each iteration. "
+            "Both the key and value can be accessed by iterating using the .items() "
+            "method of the dictionary instead.",
+        ),
     }
 
     @staticmethod
@@ -53,8 +61,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")
+    @utils.check_messages("consider-using-enumerate", "consider-using-dict-items")
     def visit_for(self, node):
+        self._check_consider_using_enumerate(node)
+        self._check_consider_using_dict_items(node)
+
+    def _check_consider_using_enumerate(self, node):
         """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 +131,60 @@ def visit_for(self, node):
                     continue
                 self.add_message("consider-using-enumerate", node=node)
                 return
+
+    def _check_consider_using_dict_items(self, node):
+        """Emit a convention whenever range and len are used for indexing."""
+        # 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.
+
+        if not isinstance(node.iter, astroid.Call):
+            # Is it a dictionary?
+            if not isinstance(node.iter, astroid.Name):
+                return
+            inferred = utils.safe_infer(node.iter)
+            if not isinstance(inferred, astroid.Dict) and not isinstance(
+                inferred, astroid.DictComp
+            ):
+                return
+            iterating_object_name = node.iter.as_string()
+
+        else:
+            # Is it a proper keys call?
+            if isinstance(node.iter.func, astroid.Name):
+                return
+            if node.iter.func.attrname != "keys":
+                return
+            inferred = utils.safe_infer(node.iter.func)
+            if not isinstance(inferred, astroid.BoundMethod) or not isinstance(
+                inferred.bound, astroid.Dict
+            ):
+                return
+            iterating_object_name = node.iter.as_string().split(".")[0]
+
+        # 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):
+                if not isinstance(subscript.value, astroid.Name):
+                    continue
+
+                value = subscript.slice
+                if isinstance(value, astroid.Index):
+                    value = value.value
+                if not isinstance(value, astroid.Name):
+                    continue
+                if value.name != node.target.name:
+                    continue
+                if iterating_object_name != subscript.value.name:
+                    continue
+                if subscript.value.scope() != node.scope():
+                    # Ignore this subscript if it's not in the same
+                    # scope. This means that in the body of the for
+                    # loop, another scope was created, where the same
+                    # name for the iterating object was used.
+                    continue
+                self.add_message("consider-using-dict-items", node=node)
+                return

pylint/checkers/typecheck.py

@@ -1436,8 +1436,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

tests/functional/c/consider/consider_using_dict_items.py

@@ -0,0 +1,17 @@
+"""Emit a message for iteration through dict keys and subscripting dict with key."""
+
+# pylint: disable=missing-docstring, import-error, useless-object-inheritance, 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)

tests/functional/c/consider/consider_using_dict_items.txt

@@ -0,0 +1,2 @@
+consider-using-dict-items:7:4:bad:Consider iterating with .items()
+consider-using-dict-items:10:4:bad:Consider iterating with .items()