Add new checker (unnecessary-comprehension) (close #2905)

pylint-dev · Jul 16, 2019 · 18d9e99 · 18d9e99
1 parent f6df515
commit 18d9e99
Show file tree

Hide file tree

Showing 18 changed files with 111 additions and 13 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -7,6 +7,13 @@ What's New in Pylint 2.4.0?
 
 Release date: TBA
 
+* Add a check `unnecessary-comprehension` that detects unnecessary comprehensions.
+
+  This check is emitted when ``pylint`` finds list-, set- or dict-comprehensions,
+  that are unnecessary and can be rewritten with the list-, set- or dict-constructors.
+
+  Close #2905
+
 * Excluded PEP 526 instance and class variables from ``no-member``. Close #2945
 
 * Excluded `attrs` from `too-few-public-methods` check. Close #2988.

diff --git a/doc/whatsnew/2.4.rst b/doc/whatsnew/2.4.rst
@@ -13,6 +13,13 @@ Summary -- Release highlights
 New checkers
 ============
 
+* Added `unnecessary-comprehension` that detects unnecessary comprehensions.
+
+  This check is emitted when ``pylint`` finds list-, set- or dict-comprehensions,
+  that are unnecessary and can be rewritten with the list-, set- or dict-constructors.
+
+  Close #2905
+
 * Added `subprocess-run-check` to handle subprocess.run without explicitly set `check` keyword.
 
   Close #2848

diff --git a/pylint/checkers/refactoring.py b/pylint/checkers/refactoring.py
@@ -266,6 +266,13 @@ class RefactoringChecker(checkers.BaseTokenChecker):
             "following a chain of ifs, all of them containing a "
             "raise statement.",
         ),
+        "R1721": (
+            "Unnecessary use of a comprehension",
+            "unnecessary-comprehension",
+            "Instead of using an identitiy comprehension, "
+            "consider using the list, dict or set constructor. "
+            "It is faster and simpler.",
+        )
     }
     options = (
         (
@@ -964,6 +971,40 @@ def _check_consider_using_join(self, aug_assign):
     def visit_augassign(self, node):
         self._check_consider_using_join(node)
 
+    @utils.check_messages("unnecessary-comprehension")
+    def visit_comprehension(self, node):
+        self._check_unnecessary_comprehension(node)
+
+    def _check_unnecessary_comprehension(self, node):
+        if (isinstance(node.parent, astroid.GeneratorExp)
+                or len(node.ifs) != 0
+                or len(node.parent.generators) != 1
+                or node.is_async):
+            return
+
+        if (isinstance(node.parent, astroid.DictComp)
+                and isinstance(node.parent.key, astroid.Name)
+                and isinstance(node.parent.value, astroid.Name)
+                and isinstance(node.target, astroid.Tuple)
+                and all(isinstance(elt, astroid.AssignName)
+                        for elt in node.target.elts)):
+            expr_list = [node.parent.key.name, node.parent.value.name]
+            target_list = [elt.name for elt in node.target.elts]
+
+        elif isinstance(node.parent, (astroid.ListComp, astroid.SetComp)):
+            expr = node.parent.elt
+            expr_list = expr.name if isinstance(expr, astroid.Name) else (
+                [elt.name for elt in expr.elts if isinstance(elt, astroid.Name)]
+                if isinstance(expr, astroid.Tuple) else [])
+            target = node.parent.generators[0].target
+            target_list = target.name if isinstance(target, astroid.AssignName) else (
+                [elt.name for elt in target.elts if isinstance(elt, astroid.AssignName)]
+                if isinstance(target, astroid.Tuple) else [])
+        else:
+            return
+        if expr_list == target_list != []:
+            self.add_message("unnecessary-comprehension", node=node)
+
     @staticmethod
     def _is_and_or_ternary(node):
         """

diff --git a/tests/functional/cellvar_escaping_loop.py b/tests/functional/cellvar_escaping_loop.py
@@ -1,4 +1,4 @@
-# pylint: disable=print-statement
+# pylint: disable=print-statement, unnecessary-comprehension
 """Tests for loopvar-in-closure."""
 from __future__ import print_function
 

diff --git a/tests/functional/consider_iterating_dictionary.py b/tests/functional/consider_iterating_dictionary.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods, no-member, import-error, no-self-use, line-too-long, useless-object-inheritance
+# pylint: disable=missing-docstring, expression-not-assigned, too-few-public-methods, no-member, import-error, no-self-use, line-too-long, useless-object-inheritance, unnecessary-comprehension
 
 from unknown import Unknown
 

diff --git a/tests/functional/consider_using_set_comprehension.py b/tests/functional/consider_using_set_comprehension.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, invalid-name
+# pylint: disable=missing-docstring, invalid-name, unnecessary-comprehension
 
 numbers = [1, 2, 3, 4, 5, 6]
 

diff --git a/tests/functional/defined_and_used_on_same_line.py b/tests/functional/defined_and_used_on_same_line.py
@@ -1,5 +1,5 @@
 """Check for definitions and usage happening on the same line."""
-#pylint: disable=missing-docstring,multiple-statements,no-absolute-import,parameter-unpacking,wrong-import-position
+#pylint: disable=missing-docstring,multiple-statements,no-absolute-import,parameter-unpacking,wrong-import-position,unnecessary-comprehension
 from __future__ import print_function
 print([index
        for index in range(10)])

diff --git a/tests/functional/iterable_context.py b/tests/functional/iterable_context.py
@@ -2,7 +2,7 @@
 Checks that primitive values are not used in an
 iterating/mapping context.
 """
-# pylint: disable=missing-docstring,invalid-name,too-few-public-methods,no-init,no-self-use,import-error,unused-argument,bad-mcs-method-argument,wrong-import-position,no-else-return, useless-object-inheritance
+# pylint: disable=missing-docstring,invalid-name,too-few-public-methods,no-init,no-self-use,import-error,unused-argument,bad-mcs-method-argument,wrong-import-position,no-else-return, useless-object-inheritance, unnecessary-comprehension
 from __future__ import print_function
 
 # primitives

diff --git a/tests/functional/iterable_context_py36.py b/tests/functional/iterable_context_py36.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring,too-few-public-methods,unused-variable
+# pylint: disable=missing-docstring,too-few-public-methods,unused-variable,unnecessary-comprehension
 import asyncio
 
 class AIter:

diff --git a/tests/functional/name_styles.py b/tests/functional/name_styles.py
@@ -1,5 +1,5 @@
 """Test for the invalid-name warning."""
-# pylint: disable=no-absolute-import, useless-object-inheritance, unnecessary-pass
+# pylint: disable=no-absolute-import, useless-object-inheritance, unnecessary-pass, unnecessary-comprehension
 from __future__ import print_function
 import abc
 import collections

diff --git a/tests/functional/star_needs_assignment_target_py35.py b/tests/functional/star_needs_assignment_target_py35.py
@@ -3,7 +3,7 @@
 https://www.python.org/dev/peps/pep-0448/
 """
 
-# pylint: disable=superfluous-parens
+# pylint: disable=superfluous-parens, unnecessary-comprehension
 
 UNPACK_TUPLE = (*range(4), 4)
 UNPACK_LIST = [*range(4), 4]

diff --git a/tests/functional/statement_without_effect.py b/tests/functional/statement_without_effect.py
@@ -1,5 +1,5 @@
 """Test for statements without effects."""
-# pylint: disable=too-few-public-methods, useless-object-inheritance
+# pylint: disable=too-few-public-methods, useless-object-inheritance, unnecessary-comprehension
 
 # +1:[pointless-string-statement]
 """inline doc string should use a separated message"""

diff --git a/tests/functional/unnecessary_comprehension.py b/tests/functional/unnecessary_comprehension.py
@@ -0,0 +1,35 @@
+# pylint: disable=undefined-variable, pointless-statement, missing-docstring, line-too-long, bad-whitespace
+# For name-reference see https://docs.python.org/3/reference/expressions.html#displays-for-lists-sets-and-dictionaries
+
+# List comprehensions
+[x for x in iterable]  # [unnecessary-comprehension]
+[y for x in iterable]  # expression != target_list
+[x for x,y,z in iterable]  # expression != target_list
+[(x,y,z) for x,y,z in iterable]  # [unnecessary-comprehension]
+[(x,y,z) for (x,y,z) in iterable]  # [unnecessary-comprehension]
+[x for x, *y in iterable]  # expression != target_list
+[x for x in iterable if condition]  # exclude comp_if
+[y for x in iterable for y in x]  # exclude nested comprehensions
+[2 * x for x in iterable]  # exclude useful comprehensions
+
+# Set comprehensions
+{x for x in iterable}  # [unnecessary-comprehension]
+{y for x in iterable}  # expression != target_list
+{x for x,y,z in iterable}  # expression != target_list
+{(x,y,z) for x,y,z in iterable}  # [unnecessary-comprehension]
+{(x,y,z) for (x, y, z) in iterable}  # [unnecessary-comprehension]
+{(x,y,z) for x in iterable}  # expression != target_list
+{(x,y,(a,b,c)) for x in iterable}  # expression != target_list
+{x for x, *y in iterable}  # expression != target_list
+{x for x in iterable if condition}  # exclude comp_if
+{y for x in iterable for y in x}  # exclude nested comprehensions
+
+# Dictionary comprehensions
+{x: y for x, y in iterable}  # [unnecessary-comprehension]
+{y: x for x, y in iterable}  # key value wrong order
+{x: y for (x, y) in iterable}  # [unnecessary-comprehension]
+{x: y for x,y,z in iterable}  # expression != target_list
+{x: y for x, y in iterable if condition}  # exclude comp_if
+{y: z for x in iterable for y, z in x}  # exclude nested comprehensions
+{x: 1 for x in iterable}  # expression != target_list
+{2 * x: 3 + x for x in iterable}  # exclude useful comprehensions
diff --git a/tests/functional/unnecessary_comprehension.txt b/tests/functional/unnecessary_comprehension.txt
@@ -0,0 +1,8 @@
+unnecessary-comprehension:5::Unnecessary use of a comprehension
+unnecessary-comprehension:8::Unnecessary use of a comprehension
+unnecessary-comprehension:9::Unnecessary use of a comprehension
+unnecessary-comprehension:16::Unnecessary use of a comprehension
+unnecessary-comprehension:19::Unnecessary use of a comprehension
+unnecessary-comprehension:20::Unnecessary use of a comprehension
+unnecessary-comprehension:28::Unnecessary use of a comprehension
+unnecessary-comprehension:30::Unnecessary use of a comprehension
diff --git a/tests/functional/unsubscriptable_value.py b/tests/functional/unsubscriptable_value.py
@@ -2,7 +2,7 @@
 Checks that value used in a subscript supports subscription
 (i.e. defines __getitem__ method).
 """
-# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position
+# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position, unnecessary-comprehension
 # pylint: disable=too-few-public-methods,import-error,invalid-name,wrong-import-order, useless-object-inheritance
 import six
 

diff --git a/tests/functional/unsupported_assignment_operation.py b/tests/functional/unsupported_assignment_operation.py
@@ -2,7 +2,7 @@
 Checks that value used in a subscript support assignments
 (i.e. defines __setitem__ method).
 """
-# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position
+# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position,unnecessary-comprehension
 # pylint: disable=too-few-public-methods,import-error,invalid-name,wrong-import-order, useless-object-inheritance
 import six
 

diff --git a/tests/functional/unsupported_delete_operation.py b/tests/functional/unsupported_delete_operation.py
@@ -2,7 +2,7 @@
 Checks that value used in a subscript support deletion
 (i.e. defines __delitem__ method).
 """
-# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position
+# pylint: disable=missing-docstring,pointless-statement,expression-not-assigned,wrong-import-position,unnecessary-comprehension
 # pylint: disable=too-few-public-methods,import-error,invalid-name,wrong-import-order, useless-object-inheritance
 import six
 

diff --git a/tests/functional/using_constant_test.py b/tests/functional/using_constant_test.py
@@ -1,7 +1,7 @@
 """Verify if constant tests are used inside if statements."""
 # pylint: disable=invalid-name, missing-docstring,too-few-public-methods
 # pylint: disable=no-init,expression-not-assigned, useless-object-inheritance
-# pylint: disable=missing-parentheses-for-call-in-test
+# pylint: disable=missing-parentheses-for-call-in-test, unnecessary-comprehension
 
 
 import collections