Skip to content

Commit

Permalink
Implemented new check unnecessary-dict-index-lookup (pylint-dev#4485)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yushao2
yushao2 committed May 23, 2021
1 parent e420ce5 commit b199fa6
Show file tree
Hide file tree
Showing 5 changed files with 220 additions and 3 deletions.
5 changes: 5 additions & 0 deletions ChangeLog
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?
===========================
Expand Down
2 changes: 2 additions & 0 deletions doc/whatsnew/2.9.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
=============
Expand Down
130 changes: 127 additions & 3 deletions pylint/checkers/refactoring/refactoring_checker.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import itertools
import tokenize
from functools import reduce
from typing import List
from typing import List, Union, cast

import astroid

Expand Down Expand Up @@ -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 = (
(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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)),),
)
63 changes: 63 additions & 0 deletions tests/functional/u/unnecessary/unnecessary_dict_index_lookup.py
View file
Original file line number Diff line number Diff line change
@@ -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
23 changes: 23 additions & 0 deletions tests/functional/u/unnecessary/unnecessary_dict_index_lookup.txt
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit b199fa6

Please sign in to comment.