From ed3449fee063d91f050c6b733030d3b3d7ad719f Mon Sep 17 00:00:00 2001 From: Arianna Y <92831762+areveny@users.noreply.github.com> Date: Mon, 25 Oct 2021 02:09:43 -0500 Subject: Add extension checker that suggests any/all statements from for loops (#5196) * Add extension checker that suggests any/all statements from for loops * Suggest all/not all: If there are negated conditions, we can suggest an all/not all statement to move the 'not' out of the comprehension and call it only once. Co-authored-by: Pierre Sassoulas --- CONTRIBUTORS.txt | 2 + ChangeLog | 6 + doc/whatsnew/2.12.rst | 4 + pylint/checkers/utils.py | 9 ++ pylint/extensions/for_any_all.py | 73 +++++++++++ tests/extensions/test_for_any_all.py | 247 +++++++++++++++++++++++++++++++++++ tests/functional/ext/for_any_all.py | 54 ++++++++ tests/functional/ext/for_any_all.rc | 2 + tests/functional/ext/for_any_all.txt | 7 + 9 files changed, 404 insertions(+) create mode 100644 pylint/extensions/for_any_all.py create mode 100644 tests/extensions/test_for_any_all.py create mode 100644 tests/functional/ext/for_any_all.py create mode 100644 tests/functional/ext/for_any_all.rc create mode 100644 tests/functional/ext/for_any_all.txt diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index fd5d627eb..545691405 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -558,3 +558,5 @@ contributors: * James DesLauriers: contributor * Youngsoo Sung: contributor + +* Arianna Yang: contributor diff --git a/ChangeLog b/ChangeLog index be49bc113..12365068a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,12 @@ Release date: TBA .. Put new features here and also in 'doc/whatsnew/2.12.rst' +* Add an optional extension ``consider-using-any-or-all`` : Emitted when a ``for`` loop only + produces a boolean and could be replaced by ``any`` or ``all`` using a generator. Also suggests + a suitable any or all statement. + + Closes #5008 + * Properly identify parameters with no documentation and add new message called ``missing-any-param-doc`` Closes #3799 diff --git a/doc/whatsnew/2.12.rst b/doc/whatsnew/2.12.rst index 1126ebd53..8b5b5f0c2 100644 --- a/doc/whatsnew/2.12.rst +++ b/doc/whatsnew/2.12.rst @@ -50,7 +50,11 @@ Removed checkers Extensions ========== +* Added an optional extension ``consider-using-any-or-all``: Emitted when a ``for`` loop only + produces a boolean and could be replaced by ``any`` or ``all`` using a generator. Also suggests + a suitable any/all statement if it is concise. + Closes #5008 Other Changes ============= diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 43157d0f9..1d198e87e 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1638,3 +1638,12 @@ def is_empty_str_literal(node: Optional[nodes.NodeNG]) -> bool: return ( isinstance(node, nodes.Const) and isinstance(node.value, str) and not node.value ) + + +def returns_bool(node: nodes.NodeNG) -> bool: + """Returns true if a node is a return that returns a constant boolean""" + return ( + isinstance(node, nodes.Return) + and isinstance(node.value, nodes.Const) + and node.value.value in (True, False) + ) diff --git a/pylint/extensions/for_any_all.py b/pylint/extensions/for_any_all.py new file mode 100644 index 000000000..9c7274095 --- /dev/null +++ b/pylint/extensions/for_any_all.py @@ -0,0 +1,73 @@ +"""Check for use of for loops that only check for a condition.""" +from typing import TYPE_CHECKING + +from astroid import nodes + +from pylint.checkers import BaseChecker +from pylint.checkers.utils import check_messages, returns_bool +from pylint.interfaces import IAstroidChecker + +if TYPE_CHECKING: + from pylint.lint.pylinter import PyLinter + + +class ConsiderUsingAnyOrAllChecker(BaseChecker): + + __implements__ = (IAstroidChecker,) + name = "consider-using-any-or-all" + msgs = { + "C0501": ( + "`for` loop could be '%s'", + "consider-using-any-or-all", + "A for loop that checks for a condition and return a bool can be replaced with any or all.", + ) + } + + @check_messages("consider-using-any-or-all") + def visit_for(self, node: nodes.For) -> None: + if len(node.body) != 1: # Only If node with no Else + return + if not isinstance(node.body[0], nodes.If): + return + + if_children = list(node.body[0].get_children()) + if not len(if_children) == 2: # The If node has only a comparison and return + return + if not returns_bool(if_children[1]): + return + + # Check for terminating boolean return right after the loop + node_after_loop = node.next_sibling() + if returns_bool(node_after_loop): + final_return_bool = node_after_loop.value.value + suggested_string = self._build_suggested_string(node, final_return_bool) + self.add_message( + "consider-using-any-or-all", node=node, args=suggested_string + ) + + @staticmethod + def _build_suggested_string(node: nodes.For, final_return_bool: bool) -> str: + """When a nodes.For node can be rewritten as an any/all statement, return a suggestion for that statement + final_return_bool is the boolean literal returned after the for loop if all conditions fail + """ + loop_var = node.target.as_string() + loop_iter = node.iter.as_string() + test_node = next(node.body[0].get_children()) + + if isinstance(test_node, nodes.UnaryOp) and test_node.op == "not": + # The condition is negated. Advance the node to the operand and modify the suggestion + test_node = test_node.operand + suggested_function = "all" if final_return_bool else "not all" + else: + suggested_function = "not any" if final_return_bool else "any" + + test = test_node.as_string() + return f"{suggested_function}({test} for {loop_var} in {loop_iter})" + + +def register(linter: "PyLinter") -> None: + """Required method to auto register this checker. + + :param linter: Main interface object for Pylint plugins + """ + linter.register_checker(ConsiderUsingAnyOrAllChecker(linter)) diff --git a/tests/extensions/test_for_any_all.py b/tests/extensions/test_for_any_all.py new file mode 100644 index 000000000..d9cf46189 --- /dev/null +++ b/tests/extensions/test_for_any_all.py @@ -0,0 +1,247 @@ +"""Tests for the pylint checker in :mod:`pylint.extensions.for_any_all +""" + +import astroid + +from pylint.extensions.for_any_all import ConsiderUsingAnyOrAllChecker +from pylint.testutils import CheckerTestCase, MessageTest + + +class TestForAnyAll(CheckerTestCase): + + CHECKER_CLASS = ConsiderUsingAnyOrAllChecker + + def test_basic_emit_for_any_all(self) -> None: + """Simple case where we expect the message to be emitted.""" + node = astroid.extract_node( + """ + def is_from_decorator(node): + for parent in node.node_ancestors(): + if isinstance(parent, Decorators): + return True + return False + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="any(isinstance(parent, Decorators) for parent in node.node_ancestors())", + ) + ): + self.checker.visit_for(node) + + def test_emit_not_any_for_any_all(self) -> None: + """Case where we expect the message to be emitted with a not any suggestion.""" + node = astroid.extract_node( + """ + def is_from_decorator(node): + for parent in node.node_ancestors(): + if isinstance(parent, Decorators): + return False + return True + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="not any(isinstance(parent, Decorators) for parent in node.node_ancestors())", + ) + ): + self.checker.visit_for(node) + + def test_emit_not_any_boolop_for_any_all(self) -> None: + """Case where we expect not any statement with a more complicated condition""" + node = astroid.extract_node( + """ + def is_from_decorator(items): + for item in items: + if item % 2 == 0 and (item % 3 == 0 or item > 15): + return False + return True + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="not any(item % 2 == 0 and (item % 3 == 0 or item > 15) for item in items)", + ) + ): + self.checker.visit_for(node) + + def test_emit_long_message_for_any_all(self) -> None: + """Case where we expect a particularly long message to be emitted.""" + node = astroid.extract_node( + """ + def is_from_decorator(node): + for ancestor in itertools.chain([node], ancestors): + if ( + ancestor.name in ("Exception", "BaseException") + and ancestor.root().name == EXCEPTIONS_MODULE + ): + return True + return False + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="any(ancestor.name in ('Exception', 'BaseException') " + "and ancestor.root().name == EXCEPTIONS_MODULE " + "for ancestor in itertools.chain([node], ancestors))", + ) + ): + self.checker.visit_for(node) + + def test_emit_all_for_any_all(self) -> None: + """Case where we expect an all statement because of negation in the condition""" + node = astroid.extract_node( + """ + def is_from_decorator(items): + for item in items: + if not(item % 2 == 0 and (item % 3 == 0 or item > 15)): + return False + return True + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="all(item % 2 == 0 and (item % 3 == 0 or item > 15) for item in items)", + ) + ): + self.checker.visit_for(node) + + def test_emit_not_all_for_any_all(self) -> None: + """Case where we expect a not all statement because of negation in the condition""" + node = astroid.extract_node( + """ + def is_from_decorator(node): + for ancestor in itertools.chain([node], ancestors): + if not ( + ancestor.name in ("Exception", "BaseException") + and ancestor.root().name == EXCEPTIONS_MODULE + ): + return True + return False + """ + ).body[0] + + with self.assertAddsMessages( + MessageTest( + "consider-using-any-or-all", + node=node, + args="not all(ancestor.name in ('Exception', 'BaseException') " + "and ancestor.root().name == EXCEPTIONS_MODULE " + "for ancestor in itertools.chain([node], ancestors))", + ) + ): + self.checker.visit_for(node) + + def test_no_pattern(self) -> None: + """Do not emit if the for loop does not have the pattern we are looking for""" + node = astroid.extract_node( + """ + def no_suggestion_if_not_if(): + for x in range(1): + var = x + return var + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) + + def test_other_return(self) -> None: + """Do not emit if the if-statement does not return a bool""" + node = astroid.extract_node( + """ + def no_suggestion_if_not_bool(item): + for parent in item.parents(): + if isinstance(parent, str): + return "True" + return "False" + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) + + def test_non_conditional_for(self) -> None: + """Do not emit if there is no If condition in the for loop.""" + node = astroid.extract_node( + """ + def print_items(items): + for item in items: + print(item) + return True + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) + + def test_returns_something_else(self) -> None: + """Do not emit if anything besides a boolean is returned.""" + node = astroid.extract_node( + """ + def print_items(items): + for item in items: + return items + return True + + def print_items(items): + for item in items: + return False + return items + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) + + def test_additional_statements(self) -> None: + """Do not emit if there is more logic which can cause side effects + or become less readable in a list comprehension. + """ + node = astroid.extract_node( + """ + def print_items(items): + for item in items: + if isinstance(item, str): + print(item) + return False + return True + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) + + def test_for_if_else(self) -> None: + """Do not emit if the if has an else condition. Generally implies more complicated logic.""" + node = astroid.extract_node( + """ + def is_from_decorator(node): + for parent in node.node_ancestors(): + if isinstance(parent, Decorators): + return True + else: + if parent in Annotations.selected_annotations: + return False + + return False + """ + ).body[0] + + with self.assertNoMessages(): + self.checker.visit_for(node) diff --git a/tests/functional/ext/for_any_all.py b/tests/functional/ext/for_any_all.py new file mode 100644 index 000000000..674bcf68b --- /dev/null +++ b/tests/functional/ext/for_any_all.py @@ -0,0 +1,54 @@ +"""Functional test""" + +def any_even(items): + """Return True if the list contains any even numbers""" + for item in items: # [consider-using-any-or-all] + if item % 2 == 0: + return True + return False + +def all_even(items): + """Return True if the list contains all even numbers""" + for item in items: # [consider-using-any-or-all] + if not item % 2 == 0: + return False + return True + +def any_uneven(items): + """Return True if the list contains any uneven numbers""" + for item in items: # [consider-using-any-or-all] + if not item % 2 == 0: + return True + return False + +def all_uneven(items): + """Return True if the list contains all uneven numbers""" + for item in items: # [consider-using-any-or-all] + if item % 2 == 0: + return False + return True + +def is_from_string(item): + """Return True if one of parents of item is a string""" + for parent in item.parents(): # [consider-using-any-or-all] + if isinstance(parent, str): + return True + return False + +def is_not_from_string(item): + """Return True if one of parents of item isn't a string""" + for parent in item.parents(): # [consider-using-any-or-all] + if not isinstance(parent, str): + return True + return False + +def nested_check(items): + """Tests that for loops at deeper levels are picked up""" + if items and len(items) > 5: + print(items) + for item in items: # [consider-using-any-or-all] + if item in (1, 2, 3): + return False + return True + print(items) + return items[3] > 5 diff --git a/tests/functional/ext/for_any_all.rc b/tests/functional/ext/for_any_all.rc new file mode 100644 index 000000000..41b0eaa71 --- /dev/null +++ b/tests/functional/ext/for_any_all.rc @@ -0,0 +1,2 @@ +[MASTER] +load-plugins=pylint.extensions.for_any_all diff --git a/tests/functional/ext/for_any_all.txt b/tests/functional/ext/for_any_all.txt new file mode 100644 index 000000000..f5ed43799 --- /dev/null +++ b/tests/functional/ext/for_any_all.txt @@ -0,0 +1,7 @@ +consider-using-any-or-all:5:4:any_even:`for` loop could be 'any(item % 2 == 0 for item in items)' +consider-using-any-or-all:12:4:all_even:`for` loop could be 'all(item % 2 == 0 for item in items)' +consider-using-any-or-all:19:4:any_uneven:`for` loop could be 'not all(item % 2 == 0 for item in items)' +consider-using-any-or-all:26:4:all_uneven:`for` loop could be 'not any(item % 2 == 0 for item in items)' +consider-using-any-or-all:33:4:is_from_string:`for` loop could be 'any(isinstance(parent, str) for parent in item.parents())' +consider-using-any-or-all:40:4:is_not_from_string:`for` loop could be 'not all(isinstance(parent, str) for parent in item.parents())' +consider-using-any-or-all:49:8:nested_check:`for` loop could be 'not any(item in (1, 2, 3) for item in items)' -- cgit v1.2.1