New ConfusingConsecutiveElifCheckr (for: #3920) (#4318)

7 files changed, 300 insertions, 0 deletions

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 <andi.finkler@gmail.com>
+
+# 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 <andi.finkler@gmail.com>
+
+# 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