Implemented new check unnecessary-dict-index-lookup

ChangeLog

@@ -62,6 +62,11 @@ modules are added.
 
 * Fix documentation errors in "Block disables" paragraph of User Guide.
 
+* New checker ``unnecessary-dict-index-lookup``. Emitted when iterating over dictionary items
+  (key-value pairs) and accessing the value by index lookup.
+
+  Closes #4470
+
 
 What's New in Pylint 2.8.2?
 ===========================

doc/whatsnew/2.9.rst

@@ -20,6 +20,8 @@ New checkers
 
 * An ``ignore_signatures`` option has been added to the similarity checker. It will permits  to reduce false positives when multiple functions have the same parameters.
 
+* ``unnecessary-dict-index-lookup``: Emitted when iterating over dictionary items
+  (key-value pairs) and accessing the value by index lookup.
 
 Other Changes
 =============

pylint/checkers/refactoring/refactoring_checker.py

@@ -6,7 +6,7 @@
 import itertools
 import tokenize
 from functools import reduce
-from typing import List
+from typing import List, Union, cast
 
 import astroid
 
@@ -347,6 +347,13 @@ class RefactoringChecker(checkers.BaseTokenChecker):
             "Emitted if a resource-allocating assignment or call may be replaced by a 'with' block. "
             "By using 'with' the release of the allocated resources is ensured even in the case of an exception.",
         ),
+        "R1733": (
+            "Unnecessary dictionary index lookup, use '%s' instead",
+            "unnecessary-dict-index-lookup",
+            "Emitted when iterating over the dictionary items (key-item pairs) and accessing the "
+            "value by index lookup. "
+            "The value can be accessed directly instead.",
+        ),
     }
     options = (
         (
@@ -534,9 +541,14 @@ def _check_redefined_argument_from_local(self, name_node):
                     args=(name_node.name,),
                 )
 
-    @utils.check_messages("redefined-argument-from-local", "too-many-nested-blocks")
+    @utils.check_messages(
+        "redefined-argument-from-local",
+        "too-many-nested-blocks",
+        "unnecessary-dict-index-lookup",
+    )
     def visit_for(self, node):
         self._check_nested_blocks(node)
+        self._check_unnecessary_dict_index_lookup(node)
 
         for name in node.target.nodes_of_class(astroid.AssignName):
             self._check_redefined_argument_from_local(name)
@@ -1330,9 +1342,10 @@ def _check_consider_using_join(self, aug_assign):
     def visit_augassign(self, node):
         self._check_consider_using_join(node)
 
-    @utils.check_messages("unnecessary-comprehension")
+    @utils.check_messages("unnecessary-comprehension", "unnecessary-dict-index-lookup")
     def visit_comprehension(self, node):
         self._check_unnecessary_comprehension(node)
+        self._check_unnecessary_dict_index_lookup(node)
 
     def _check_unnecessary_comprehension(self, node):
         if (
@@ -1601,3 +1614,114 @@ def _check_return_at_the_end(self, node):
             # return None"
             elif isinstance(last.value, astroid.Const) and (last.value.value is None):
                 self.add_message("useless-return", node=node)
+
+    def _check_unnecessary_dict_index_lookup(
+        self, node: Union[astroid.For, astroid.Comprehension]
+    ) -> None:
+        """Add message when accessing dict values by index lookup."""
+        # Verify that we have a .items() call and
+        # that the object which is iterated is used as a subscript in the
+        # body of the for.
+        # Is it a proper items call?
+        if (
+            isinstance(node.iter, astroid.Call)
+            and isinstance(node.iter.func, astroid.Attribute)
+            and node.iter.func.attrname == "items"
+        ):
+            inferred = utils.safe_infer(node.iter.func)
+            if not isinstance(inferred, astroid.BoundMethod):
+                return
+            iterating_object_name = node.iter.func.expr.as_string()
+
+            # 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.
+
+            children = (
+                node.body
+                if isinstance(node, astroid.For)
+                else node.parent.get_children()
+            )
+            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 isinstance(node, astroid.For) and (
+                        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
+
+                    # Case where .items is assigned to k,v (i.e., for k, v in d.items())
+                    if isinstance(value, astroid.Name):
+                        if (
+                            not isinstance(node.target, astroid.Tuple)
+                            or value.name != node.target.elts[0].name
+                            or iterating_object_name != subscript.value.as_string()
+                        ):
+                            continue
+
+                        if (
+                            isinstance(node, astroid.For)
+                            and value.lookup(value.name)[1][-1].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
+
+                        self.add_message(
+                            "unnecessary-dict-index-lookup",
+                            node=subscript,
+                            args=(node.target.elts[1].as_string()),
+                        )
+
+                    # Case where .items is assigned to single var (i.e., for item in d.items())
+                    elif isinstance(value, astroid.Subscript):
+                        if (
+                            not isinstance(node.target, astroid.AssignName)
+                            or node.target.name != value.value.name
+                            or iterating_object_name != subscript.value.as_string()
+                        ):
+                            continue
+
+                        if (
+                            isinstance(node, astroid.For)
+                            and value.value.lookup(value.value.name)[1][-1].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
+
+                        # check if subscripted by 0 (key)
+                        subscript_val = value.slice
+                        if isinstance(subscript_val, astroid.Index):
+                            subscript_val = subscript_val.value
+                        inferred = utils.safe_infer(subscript_val)
+                        if (
+                            not isinstance(inferred, astroid.Const)
+                            or inferred.value != 0
+                        ):
+                            continue
+                        self.add_message(
+                            "unnecessary-dict-index-lookup",
+                            node=subscript,
+                            args=("1".join(value.as_string().rsplit("0", maxsplit=1)),),
+                        )

tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py

@@ -0,0 +1,63 @@
+# pylint: disable=missing-docstring, too-few-public-methods, expression-not-assigned, line-too-long
+
+a_dict = dict()
+b_dict = dict()
+
+for k, v in a_dict.items():
+    print(a_dict[k])  # [unnecessary-dict-index-lookup]
+    print(b_dict[k])  # Should not emit warning, accessing other dictionary
+    a_dict[k] = 123  # Should not emit warning, key access necessary
+    a_dict[k] += 123  # Should not emit warning, key access necessary
+    print(a_dict[k])  # [unnecessary-dict-index-lookup]
+    k = "another key"
+    print(a_dict[k])  # This is fine, key reassigned
+
+
+# Tests on comprehensions
+{v: 1 for k, v in a_dict.items() if a_dict[k]}  # [unnecessary-dict-index-lookup]
+{v: 1 for k, v in a_dict.items() if k}  # This is fine, no indexing
+{a_dict[k]: 1 for k, v in a_dict.items() if k}  # [unnecessary-dict-index-lookup]
+{a_dict[k]: 1 for k, v in a_dict.items() if a_dict[k]}  # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup]
+
+[v for k, v in a_dict.items() if a_dict[k]]  # [unnecessary-dict-index-lookup]
+[v for k, v in a_dict.items() if k]  # This is fine, no indexing
+[a_dict[k] for k, v in a_dict.items() if k]  # [unnecessary-dict-index-lookup]
+[a_dict[k] for k, v in a_dict.items() if a_dict[k]]  # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup]
+
+
+# Tests on dict attribute of a class
+class Foo:
+    c_dict = {}
+
+for k, v in Foo.c_dict.items():
+    print(b_dict[k])  # Should not emit warning, accessing other dictionary
+    print(Foo.c_dict[k])  # [unnecessary-dict-index-lookup]
+    Foo.c_dict[k] += Foo.c_dict[k]  # [unnecessary-dict-index-lookup]
+    Foo.c_dict[k] += v  # key access necessary
+
+# Tests on comprehensions
+{v: 1 for k, v in Foo.c_dict.items() if Foo.c_dict[k]}  # [unnecessary-dict-index-lookup]
+{v: 1 for k, v in Foo.c_dict.items() if k}  # This is fine, no indexing
+{Foo.c_dict[k]: 1 for k, v in Foo.c_dict.items() if k}  # [unnecessary-dict-index-lookup]
+{Foo.c_dict[k]: 1 for k, v in Foo.c_dict.items() if Foo.c_dict[k]}  # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup]
+
+[v for k, v in Foo.c_dict.items() if Foo.c_dict[k]]  # [unnecessary-dict-index-lookup]
+[v for k, v in Foo.c_dict.items() if k]  # This is fine, no indexing
+[Foo.c_dict[k] for k, v in Foo.c_dict.items() if k]  # [unnecessary-dict-index-lookup]
+[Foo.c_dict[k] for k, v in Foo.c_dict.items() if Foo.c_dict[k]]  # [unnecessary-dict-index-lookup, unnecessary-dict-index-lookup]
+
+# Test assigning d.items() to a single variable
+d = {1: "a", 2: "b"}
+for item in d.items():
+    print(item[0])
+    print(d[item[0]])  # [unnecessary-dict-index-lookup]
+
+[item[0] for item in d.items()]
+[d[item[0]] for item in d.items()]  # [unnecessary-dict-index-lookup]
+
+# Reassigning single var
+for item in d.items():
+    print(item[0])
+    print(d[item[0]])  # [unnecessary-dict-index-lookup]
+    item = (2, "b")
+    print(d[item[0]])  # This is fine, no warning thrown as key has been reassigned

tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt

@@ -0,0 +1,23 @@
+unnecessary-dict-index-lookup:7:10::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:11:10::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:17:36::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:19:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:20:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:20:44::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:22:33::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:24:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:25:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:25:41::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:34:10::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:35:21::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:39:40::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:41:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:42:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:42:52::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:44:37::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:46:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:47:1::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:47:49::Unnecessary dictionary index lookup, use 'v' instead
+unnecessary-dict-index-lookup:53:10::Unnecessary dictionary index lookup, use 'item[1]' instead
+unnecessary-dict-index-lookup:56:1::Unnecessary dictionary index lookup, use 'item[1]' instead
+unnecessary-dict-index-lookup:61:10::Unnecessary dictionary index lookup, use 'item[1]' instead