Added additional logic and tests for (pylint-dev#3389)

ChangeLog

@@ -31,7 +31,7 @@ Release date: TBA
 * Stdlib deprecated modules check is moved to stdlib checker. New deprecated
 modules are added.
 
-* ``consider-using-dict-items``, emitted when iterating over dictionary keys and then
+* 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

doc/whatsnew/2.9.rst

@@ -12,7 +12,7 @@ Summary -- Release highlights
 New checkers
 ============
 
-* ``consider-using-dict-items``, emitted when iterating over dictionary keys and then
+* ``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,6 +1,8 @@
 # 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
 
 from pylint import checkers, interfaces
@@ -29,8 +31,8 @@ class RecommendationChecker(checkers.BaseChecker):
         "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. "
+            "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.",
         ),
@@ -62,11 +64,11 @@ def visit_call(self, node):
             self.add_message("consider-iterating-dictionary", node=node)
 
     @utils.check_messages("consider-using-enumerate", "consider-using-dict-items")
-    def visit_for(self, node):
+    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):
+    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
@@ -132,8 +134,8 @@ def _check_consider_using_enumerate(self, node):
                 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."""
+    def _check_consider_using_dict_items(self, node: astroid.For) -> None:
+        """Emit a convention when dict key used to index dict."""
         # 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.
@@ -143,48 +145,47 @@ def _check_consider_using_dict_items(self, node):
             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
-            ):
+            if not isinstance(inferred, (astroid.Dict, 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":
+            if not (
+                isinstance(node.iter.func, astroid.Attribute)
+                and 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
-            ):
+            if not isinstance(inferred, (astroid.BoundMethod, astroid.Dict)):
                 return
-            iterating_object_name = node.iter.as_string().split(".")[0]
+            iterating_object_name = node.iter.as_string().partition(".")[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):
+                subscript = cast(astroid.Subscript, 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):
+                if (
+                    not isinstance(value, astroid.Name)
+                    or value.name != node.target.name
+                    or iterating_object_name != subscript.value.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.
+                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
                 self.add_message("consider-using-dict-items", node=node)
                 return

tests/functional/c/consider/consider_using_dict_items.py

@@ -1,17 +1,30 @@
 """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
+# pylint: disable=missing-docstring,unsubscriptable-object
 
 def bad():
     a_dict = {1:1, 2:2, 3:3}
-    for k in a_dict:# [consider-using-dict-items]
+    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]
+    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])

tests/functional/c/consider/consider_using_dict_items.txt

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