From 56db200f856a2d913ae4dc6316dce0013f853733 Mon Sep 17 00:00:00 2001 From: DudeNr33 <3929834+DudeNr33@users.noreply.github.com> Date: Sun, 11 Apr 2021 11:57:15 +0200 Subject: New ConfusingConsecutiveElifCheckr (for: #3920) (#4318) --- ChangeLog | 3 + doc/whatsnew/2.8.rst | 3 + pylint/extensions/confusing_elif.py | 55 ++++++ tests/extensions/test_confusing_elif.py | 195 ++++++++++++++++++++++ tests/functional/c/confusing_consecutive_elif.py | 41 +++++ tests/functional/c/confusing_consecutive_elif.rc | 2 + tests/functional/c/confusing_consecutive_elif.txt | 1 + 7 files changed, 300 insertions(+) create mode 100644 pylint/extensions/confusing_elif.py create mode 100644 tests/extensions/test_confusing_elif.py create mode 100644 tests/functional/c/confusing_consecutive_elif.py create mode 100644 tests/functional/c/confusing_consecutive_elif.rc create mode 100644 tests/functional/c/confusing_consecutive_elif.txt diff --git a/ChangeLog b/ChangeLog index 4a64d928d..a1fdaf851 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,9 @@ Release date: Undefined .. Put new features here and also in 'doc/whatsnew/2.8.rst' +* Add new extension ``ConfusingConsecutiveElifChecker``. This optional checker emits a refactoring message (R5601 ``confusing-consecutive-elif``) + if if/elif statements with different indentation levels follow directly one after the other. + * Use a prescriptive message for ``unidiomatic-typecheck`` Closes #3891 diff --git a/doc/whatsnew/2.8.rst b/doc/whatsnew/2.8.rst index 12c4661cb..9b1c036cf 100644 --- a/doc/whatsnew/2.8.rst +++ b/doc/whatsnew/2.8.rst @@ -14,6 +14,9 @@ New checkers * Add ``deprecated-argument`` check for deprecated arguments. +* Add new extension ``ConfusingConsecutiveElifChecker``. This optional checker emits a refactoring message (R5601 ``confusing-consecutive-elif``) + if if/elif statements with different indentation levels follow directly one after the other. + Other Changes ============= diff --git a/pylint/extensions/confusing_elif.py b/pylint/extensions/confusing_elif.py new file mode 100644 index 000000000..f61be02f9 --- /dev/null +++ b/pylint/extensions/confusing_elif.py @@ -0,0 +1,55 @@ +# Copyright (c) 2021 Andreas Finkler + +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/PyCQA/pylint/blob/master/COPYING + +import astroid + +from pylint.checkers import BaseChecker +from pylint.checkers.utils import check_messages +from pylint.interfaces import IAstroidChecker +from pylint.lint import PyLinter + + +class ConfusingConsecutiveElifChecker(BaseChecker): + """Checks if "elif" is used right after an indented block that finishes with "if" or "elif" itself.""" + + __implements__ = IAstroidChecker + + name = "confusing-elif-checker" + priority = -1 + msgs = { + "R5601": ( + "Consecutive elif with differing indentation level, consider creating a function to separate the inner elif", + "confusing-consecutive-elif", + "Used when an elif statement follows right after an indented block which itself ends with if or elif. " + "It may not be ovious if the elif statement was willingly or mistakenly unindented. " + "Extracting the indented if statement into a separate function might avoid confusion and prevent errors.", + ) + } + + @check_messages("confusing-consecutive-elif") + def visit_if(self, node: astroid.If): + body_ends_with_if = isinstance( + node.body[-1], astroid.If + ) and self._has_no_else_clause(node.body[-1]) + if node.has_elif_block() and body_ends_with_if: + self.add_message("confusing-consecutive-elif", node=node.orelse[0]) + + @staticmethod + def _has_no_else_clause(node: astroid.If): + orelse = node.orelse + while orelse and isinstance(orelse[0], astroid.If): + orelse = orelse[0].orelse + if not orelse or isinstance(orelse[0], astroid.If): + return True + return False + + +def register(linter: PyLinter): + """This required method auto registers the checker. + + :param linter: The linter to register the checker to. + :type linter: pylint.lint.PyLinter + """ + linter.register_checker(ConfusingConsecutiveElifChecker(linter)) diff --git a/tests/extensions/test_confusing_elif.py b/tests/extensions/test_confusing_elif.py new file mode 100644 index 000000000..3392b4401 --- /dev/null +++ b/tests/extensions/test_confusing_elif.py @@ -0,0 +1,195 @@ +# Copyright (c) 2021 Andreas Finkler + +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/PyCQA/pylint/blob/master/COPYING + +"""Tests for the pylint checker in :mod:`pylint.extensions.confusing_elif +""" + +import astroid + +from pylint.extensions.confusing_elif import ConfusingConsecutiveElifChecker +from pylint.testutils import CheckerTestCase, Message + +MSG_ID_CONFUSING_CONSECUTIVE_ELIF = "confusing-consecutive-elif" + + +class TestConfusingConsecutiveElifChecker(CheckerTestCase): + """Tests for pylint.extensions.confusing_elif.ConfusingConsecutiveElifChecker""" + + CHECKER_CLASS = ConfusingConsecutiveElifChecker + + def test_triggered_if_if_block_ends_with_elif(self): + """ + Given an if-elif construct + When the body of the if ends with an elif + Then the message confusing-consecutive-elif must be triggered. + """ + example_code = """ + def foo(a, b, c): + if a > b: #@ + if a > 0: + return a + elif a < 0: + return 0 + elif a > c: #@ + return c + """ + if_node_to_test, elif_node = astroid.extract_node(example_code) + with self.assertAddsMessages( + Message(msg_id=MSG_ID_CONFUSING_CONSECUTIVE_ELIF, node=elif_node) + ): + self.checker.visit_if(if_node_to_test) + + def test_triggered_if_elif_block_ends_with_elif(self): + """ + Given an if-elif-elif construct + When the body of the first elif ends with an elif + Then the message confusing-consecutive-elif must be triggered. + """ + example_code = """ + def foo(a, b, c): + if a < b: + return b + elif a > b: #@ + if a > 0: + return a + elif a < 0: + return 0 + elif a > c: #@ + return c + """ + if_node_to_test, elif_node = astroid.extract_node(example_code) + with self.assertAddsMessages( + Message(msg_id=MSG_ID_CONFUSING_CONSECUTIVE_ELIF, node=elif_node) + ): + self.checker.visit_if(if_node_to_test) + + def test_triggered_if_block_ends_with_if(self): + """ + Given an if-elif construct + When the body of the if ends with an if + Then the message confusing-consecutive-elif must be triggered. + """ + example_code = """ + def foo(a, b, c): + if a > b: #@ + if a > 0: + return a + elif a > c: #@ + return c + """ + if_node_to_test, elif_node = astroid.extract_node(example_code) + with self.assertAddsMessages( + Message(msg_id=MSG_ID_CONFUSING_CONSECUTIVE_ELIF, node=elif_node) + ): + self.checker.visit_if(if_node_to_test) + + def test_not_triggered_if_indented_block_ends_with_else(self): + """ + Given an if-elif construct + When the body of the if ends with an else + Then no message shall be triggered. + """ + example_code = """ + def foo(a, b, c): + if a > b: #@ + if a > 0: + return a + else: + return 0 + elif a > c: + return c + """ + if_node_to_test = astroid.extract_node(example_code) + with self.assertNoMessages(): + self.checker.visit_if(if_node_to_test) + + def test_not_triggered_if_indentend_block_ends_with_call(self): + """ + Given an if-elif construct + When the body of the if ends with a function call + Then no message shall be triggered. + + Note: There is nothing special about the body ending with a function call. This is just taken as a + representative value for the equivalence class of "every node class unrelated to if/elif/else". + """ + example_code = """ + def foo(a, b, c): + result = None + if a > b: #@ + if a > 0: + result = a + print("result is", result) + elif a > c: + result = c + return result + """ + if_node_to_test = astroid.extract_node(example_code) + with self.assertNoMessages(): + self.checker.visit_if(if_node_to_test) + + def test_not_triggered_if_indented_block_ends_with_ifexp(self): + """ + Given an if-elif construct + When the body of the if ends with an if expression + Then no message shall be triggered. + """ + example_code = """ + def foo(a, b, c): + result = None + if a > b: #@ + if a > 0: + result = a + result = b if b > c else c + elif a > c: + result = c + return result + """ + if_node_to_test = astroid.extract_node(example_code) + with self.assertNoMessages(): + self.checker.visit_if(if_node_to_test) + + def test_not_triggered_if_outer_block_does_not_have_elif(self): + """ + Given an if construct without an elif + When the body of the if ends with an if + Then no message shall be triggered. + """ + example_code = """ + def foo(a, b, c): + result = None + if a > b: #@ + if a > 0: + result = a + elif a < 0: + result = 0 + else: + result = c + return result + """ + if_node_to_test = astroid.extract_node(example_code) + with self.assertNoMessages(): + self.checker.visit_if(if_node_to_test) + + def test_not_triggered_if_outer_block_continues_with_if(self): + """ + Given an if construct which continues with a new if construct + When the body of the first if ends with an if expression + Then no message shall be triggered. + """ + example_code = """ + def foo(a, b, c): + result = None + if a > b: #@ + if a > 0: + result = a + elif a < 0: + result = 0 + if b > c: + result = c + return result + """ + if_node_to_test = astroid.extract_node(example_code) + with self.assertNoMessages(): + self.checker.visit_if(if_node_to_test) diff --git a/tests/functional/c/confusing_consecutive_elif.py b/tests/functional/c/confusing_consecutive_elif.py new file mode 100644 index 000000000..9f1d1ffe9 --- /dev/null +++ b/tests/functional/c/confusing_consecutive_elif.py @@ -0,0 +1,41 @@ +# pylint: disable=missing-module-docstring, missing-function-docstring + +def check_config(machine, old_conf, new_conf): + """Example code that will trigger the message""" + if old_conf: + if not new_conf: + machine.disable() + elif old_conf.value != new_conf.value: + machine.disable() + machine.enable(new_conf.value) + elif new_conf: # [confusing-consecutive-elif] + machine.enable(new_conf.value) + + +def check_config_2(machine, old_conf, new_conf): + """Example code must not trigger the message, because the inner block ends with else.""" + if old_conf: + if not new_conf: + machine.disable() + elif old_conf.value != new_conf.value: + machine.disable() + machine.enable(new_conf.value) + else: + pass + elif new_conf: + machine.enable(new_conf.value) + +def check_config_3(machine, old_conf, new_conf): + """ + Example code must not trigger the message, + because the inner if is not the final node of the body. + """ + if old_conf: + if not new_conf: + machine.disable() + elif old_conf.value != new_conf.value: + machine.disable() + machine.enable(new_conf.value) + print("Processed old configuration...") + elif new_conf: + machine.enable(new_conf.value) diff --git a/tests/functional/c/confusing_consecutive_elif.rc b/tests/functional/c/confusing_consecutive_elif.rc new file mode 100644 index 000000000..6a11b2c09 --- /dev/null +++ b/tests/functional/c/confusing_consecutive_elif.rc @@ -0,0 +1,2 @@ +[MASTER] +load-plugins=pylint.extensions.confusing_elif diff --git a/tests/functional/c/confusing_consecutive_elif.txt b/tests/functional/c/confusing_consecutive_elif.txt new file mode 100644 index 000000000..55cc0d13f --- /dev/null +++ b/tests/functional/c/confusing_consecutive_elif.txt @@ -0,0 +1 @@ +confusing-consecutive-elif:11:4:check_config:Consecutive elif with differing indentation level, consider creating a function to separate the inner elif -- cgit v1.2.1