Move ``misplaced-comparison-constant`` to optional extension (#5298)

* Move ``misplaced-comparison-constant`` to optional extension

* Update functional tests to increase coverage

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

19 files changed, 112 insertions, 44 deletions

diff --git a/ChangeLog b/ChangeLog
index 96773582c..a4998baf8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -162,6 +162,12 @@ Release date: TBA
 
   Closes #4580
 
+* Moved ``misplaced-comparison-constant`` to its own extension ``comparison_placement``.
+  This checker was opinionated and now no longer a default. It can be reactived by adding
+  ``pylint.extensions.comparison_placement`` to ``load-plugins`` in your config.
+
+  Closes #1064
+
 * A new ``bad-configuration-section`` checker was added that will emit for misplaced option
   in pylint's top level namespace for toml configuration. Top-level dictionaries or option defined
   in the wrong section will still silently not be taken into account, which is tracked in a
diff --git a/doc/whatsnew/2.12.rst b/doc/whatsnew/2.12.rst
index 59c89ff22..b69af3289 100644
--- a/doc/whatsnew/2.12.rst
+++ b/doc/whatsnew/2.12.rst
@@ -75,6 +75,12 @@ Extensions
 
   Closes #5008
 
+* Moved ``misplaced-comparison-constant`` to its own extension ``comparison_placement``.
+  This checker was opinionated and now no longer a default. It can be reactived by adding
+  ``pylint.extensions.comparison_placement`` to ``load-plugins`` in your config.
+
+  Closes #1064
+
 Other Changes
 =============
 
diff --git a/pylint/checkers/base.py b/pylint/checkers/base.py
index 510b2cdcf..fd18fb867 100644
--- a/pylint/checkers/base.py
+++ b/pylint/checkers/base.py
@@ -214,7 +214,7 @@ DEFAULT_ARGUMENT_SYMBOLS = dict(
         )
     },
 )
-REVERSED_COMPS = {"<": ">", "<=": ">=", ">": "<", ">=": "<="}
+
 COMPARISON_OPERATORS = frozenset(("==", "!=", "<", ">", "<=", ">="))
 # List of methods which can be redefined
 REDEFINABLE_METHODS = frozenset(("__module__",))
@@ -2323,13 +2323,6 @@ class ComparisonChecker(_BasicChecker):
             "Used when an expression is compared to singleton "
             "values like True, False or None.",
         ),
-        "C0122": (
-            "Comparison should be %s",
-            "misplaced-comparison-constant",
-            "Used when the constant is placed on the left side "
-            "of a comparison. It is usually clearer in intent to "
-            "place it in the right hand side of the comparison.",
-        ),
         "C0123": (
             "Use isinstance() rather than type() for a typecheck.",
             "unidiomatic-typecheck",
@@ -2478,13 +2471,6 @@ class ComparisonChecker(_BasicChecker):
         if is_const or is_other_literal:
             self.add_message("literal-comparison", node=node)
 
-    def _check_misplaced_constant(self, node, left, right, operator):
-        if isinstance(right, nodes.Const):
-            return
-        operator = REVERSED_COMPS.get(operator, operator)
-        suggestion = f"{right.as_string()} {operator} {left.value!r}"
-        self.add_message("misplaced-comparison-constant", node=node, args=(suggestion,))
-
     def _check_logical_tautology(self, node: nodes.Compare):
         """Check if identifier is compared against itself.
         :param node: Compare node
@@ -2532,7 +2518,6 @@ class ComparisonChecker(_BasicChecker):
 
     @utils.check_messages(
         "singleton-comparison",
-        "misplaced-comparison-constant",
         "unidiomatic-typecheck",
         "literal-comparison",
         "comparison-with-itself",
@@ -2549,8 +2534,6 @@ class ComparisonChecker(_BasicChecker):
 
         left = node.left
         operator, right = node.ops[0]
-        if operator in COMPARISON_OPERATORS and isinstance(left, nodes.Const):
-            self._check_misplaced_constant(node, left, right, operator)
 
         if operator in ("==", "!="):
             self._check_singleton_comparison(
diff --git a/pylint/extensions/comparison_placement.py b/pylint/extensions/comparison_placement.py
new file mode 100644
index 000000000..64c8dee5e
--- /dev/null
+++ b/pylint/extensions/comparison_placement.py
@@ -0,0 +1,67 @@
+# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
+
+"""
+Checks for yoda comparisons (variable before constant)
+See https://en.wikipedia.org/wiki/Yoda_conditions
+"""
+
+
+from astroid import nodes
+
+from pylint.checkers import BaseChecker, utils
+from pylint.interfaces import IAstroidChecker
+
+REVERSED_COMPS = {"<": ">", "<=": ">=", ">": "<", ">=": "<="}
+COMPARISON_OPERATORS = frozenset(("==", "!=", "<", ">", "<=", ">="))
+
+
+class MisplacedComparisonConstantChecker(BaseChecker):
+    """Checks the placement of constants in comparisons"""
+
+    __implements__ = (IAstroidChecker,)
+
+    # configuration section name
+    name = "comparison-placement"
+    msgs = {
+        "C2201": (
+            "Comparison should be %s",
+            "misplaced-comparison-constant",
+            "Used when the constant is placed on the left side "
+            "of a comparison. It is usually clearer in intent to "
+            "place it in the right hand side of the comparison.",
+            {"old_names": [("C0122", "old-misplaced-comparison-constant")]},
+        )
+    }
+
+    options = ()
+
+    def _check_misplaced_constant(
+        self,
+        node: nodes.Compare,
+        left: nodes.NodeNG,
+        right: nodes.NodeNG,
+        operator: str,
+    ):
+        if isinstance(right, nodes.Const):
+            return
+        operator = REVERSED_COMPS.get(operator, operator)
+        suggestion = f"{right.as_string()} {operator} {left.value!r}"
+        self.add_message("misplaced-comparison-constant", node=node, args=(suggestion,))
+
+    @utils.check_messages("misplaced-comparison-constant")
+    def visit_compare(self, node: nodes.Compare) -> None:
+        # NOTE: this checker only works with binary comparisons like 'x == 42'
+        # but not 'x == y == 42'
+        if len(node.ops) != 1:
+            return
+
+        left = node.left
+        operator, right = node.ops[0]
+        if operator in COMPARISON_OPERATORS and isinstance(left, nodes.Const):
+            self._check_misplaced_constant(node, left, right, operator)
+
+
+def register(linter):
+    """Required method to auto register this checker."""
+    linter.register_checker(MisplacedComparisonConstantChecker(linter))
diff --git a/pylintrc b/pylintrc
index 8df15f713..95e796939 100644
--- a/pylintrc
+++ b/pylintrc
@@ -24,6 +24,7 @@ load-plugins=
     pylint.extensions.overlapping_exceptions,
     pylint.extensions.typing,
     pylint.extensions.redefined_variable_type,
+    pylint.extensions.comparison_placement,
 
 # Use multiple processes to speed up Pylint.
 jobs=1
diff --git a/tests/checkers/unittest_base.py b/tests/checkers/unittest_base.py
index 9f0d5fd4f..46a9913a4 100644
--- a/tests/checkers/unittest_base.py
+++ b/tests/checkers/unittest_base.py
@@ -548,9 +548,6 @@ class TestComparison(CheckerTestCase):
         node = astroid.extract_node("True == foo")
         messages = (
             MessageTest(
-                "misplaced-comparison-constant", node=node, args=("foo == True",)
-            ),
-            MessageTest(
                 "singleton-comparison",
                 node=node,
                 args=(
@@ -565,9 +562,6 @@ class TestComparison(CheckerTestCase):
         node = astroid.extract_node("False == foo")
         messages = (
             MessageTest(
-                "misplaced-comparison-constant", node=node, args=("foo == False",)
-            ),
-            MessageTest(
                 "singleton-comparison",
                 node=node,
                 args=(
@@ -582,9 +576,6 @@ class TestComparison(CheckerTestCase):
         node = astroid.extract_node("None == foo")
         messages = (
             MessageTest(
-                "misplaced-comparison-constant", node=node, args=("foo == None",)
-            ),
-            MessageTest(
                 "singleton-comparison",
                 node=node,
                 args=("'None == foo'", "'None is foo'"),
diff --git a/tests/extensions/data/compare_to_zero.py b/tests/extensions/data/compare_to_zero.py
index dfe6bbcb5..29fd13994 100644
--- a/tests/extensions/data/compare_to_zero.py
+++ b/tests/extensions/data/compare_to_zero.py
@@ -1,4 +1,4 @@
-# pylint: disable=literal-comparison,missing-docstring,misplaced-comparison-constant
+# pylint: disable=literal-comparison,missing-docstring
 
 X = 123
 Y = len('test')
diff --git a/tests/functional/a/assert_on_tuple.py b/tests/functional/a/assert_on_tuple.py
index e360737e6..57dbd0907 100644
--- a/tests/functional/a/assert_on_tuple.py
+++ b/tests/functional/a/assert_on_tuple.py
@@ -1,6 +1,6 @@
 '''Assert check example'''
 
-# pylint: disable=misplaced-comparison-constant, comparison-with-itself
+# pylint: disable=comparison-with-itself
 assert (1 == 1, 2 == 2), "no error"
 assert (1 == 1, 2 == 2) # [assert-on-tuple]
 assert 1 == 1, "no error"
diff --git a/tests/functional/c/comparison_with_callable.py b/tests/functional/c/comparison_with_callable.py
index b676844ae..fb02729d8 100644
--- a/tests/functional/c/comparison_with_callable.py
+++ b/tests/functional/c/comparison_with_callable.py
@@ -1,4 +1,4 @@
-# pylint: disable = disallowed-name, missing-docstring, useless-return, misplaced-comparison-constant, invalid-name, no-self-use, line-too-long, useless-object-inheritance
+# pylint: disable = disallowed-name, missing-docstring, useless-return, invalid-name, no-self-use, line-too-long, useless-object-inheritance
 def foo():
     return None
 
diff --git a/tests/functional/c/consider/consider_using_in.py b/tests/functional/c/consider/consider_using_in.py
index 7ce5dd0b6..5bc06e438 100644
--- a/tests/functional/c/consider/consider_using_in.py
+++ b/tests/functional/c/consider/consider_using_in.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, invalid-name, pointless-statement, misplaced-comparison-constant, undefined-variable, literal-comparison, line-too-long, unneeded-not, too-few-public-methods, use-implicit-booleaness-not-comparison
+# pylint: disable=missing-docstring, invalid-name, pointless-statement, undefined-variable, literal-comparison, line-too-long, unneeded-not, too-few-public-methods, use-implicit-booleaness-not-comparison
 
 value = value1 = 1
 value2 = 2
diff --git a/tests/functional/e/excess_escapes.py b/tests/functional/e/excess_escapes.py
index 19fd5cb4d..a81dc7b9a 100644
--- a/tests/functional/e/excess_escapes.py
+++ b/tests/functional/e/excess_escapes.py
@@ -1,4 +1,4 @@
-# pylint:disable=pointless-string-statement, fixme, misplaced-comparison-constant, comparison-with-itself
+# pylint:disable=pointless-string-statement, fixme, comparison-with-itself
 """Stray backslash escapes may be missing a raw-string prefix."""
 # pylint: disable=redundant-u-string-prefix
 
diff --git a/tests/functional/m/misplaced_bare_raise.py b/tests/functional/m/misplaced_bare_raise.py
index 6148733e2..c704f0122 100644
--- a/tests/functional/m/misplaced_bare_raise.py
+++ b/tests/functional/m/misplaced_bare_raise.py
@@ -11,7 +11,6 @@ try:
 except Exception:

     raise

 

-# pylint: disable=misplaced-comparison-constant

 try:

     pass

 except Exception:

diff --git a/tests/functional/m/misplaced_bare_raise.txt b/tests/functional/m/misplaced_bare_raise.txt
index 38e04f988..d64ff43cb 100644
--- a/tests/functional/m/misplaced_bare_raise.txt
+++ b/tests/functional/m/misplaced_bare_raise.txt
@@ -1,7 +1,7 @@
-misplaced-bare-raise:5:4::The raise statement is not inside an except clause
-misplaced-bare-raise:36:16:test1.best:The raise statement is not inside an except clause
-misplaced-bare-raise:39:4:test1:The raise statement is not inside an except clause
-misplaced-bare-raise:40:0::The raise statement is not inside an except clause
-misplaced-bare-raise:49:4::The raise statement is not inside an except clause
-misplaced-bare-raise:57:4:A:The raise statement is not inside an except clause
-misplaced-bare-raise:68:4::The raise statement is not inside an except clause
+misplaced-bare-raise:5:4::The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:35:16:test1.best:The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:38:4:test1:The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:39:0::The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:48:4::The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:56:4:A:The raise statement is not inside an except clause:HIGH
+misplaced-bare-raise:67:4::The raise statement is not inside an except clause:HIGH
diff --git a/tests/functional/m/misplaced_comparison_constant.py b/tests/functional/m/misplaced_comparison_constant.py
index 29f2b1ed8..0162187bf 100644
--- a/tests/functional/m/misplaced_comparison_constant.py
+++ b/tests/functional/m/misplaced_comparison_constant.py
@@ -34,3 +34,16 @@ def good_comparison():
     for i in range(10):
         if i == 5:
             pass
+
+def double_comparison():
+    """Check that we return early for non-binary comparison"""
+    for i in range(10):
+        if i == 1 == 2:
+            pass
+        if 2 <= i <= 8:
+            print("Between 2 and 8 inclusive")
+
+def const_comparison():
+    """Check that we return early for comparison of two constants"""
+    if 1 == 2:
+        pass
diff --git a/tests/functional/m/misplaced_comparison_constant.rc b/tests/functional/m/misplaced_comparison_constant.rc
new file mode 100644
index 000000000..abe6b9f27
--- /dev/null
+++ b/tests/functional/m/misplaced_comparison_constant.rc
@@ -0,0 +1,3 @@
+[MASTER]
+load-plugins=
+    pylint.extensions.comparison_placement,
diff --git a/tests/functional/n/nan_comparison_check.py b/tests/functional/n/nan_comparison_check.py
index b01cf2636..b9a720b9a 100644
--- a/tests/functional/n/nan_comparison_check.py
+++ b/tests/functional/n/nan_comparison_check.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, invalid-name, misplaced-comparison-constant
+# pylint: disable=missing-docstring, invalid-name
 # pylint: disable=literal-comparison,comparison-with-itself, import-error
 """Test detection of NaN value comparison."""
 import numpy
diff --git a/tests/functional/s/singleton_comparison.py b/tests/functional/s/singleton_comparison.py
index d7ebdaf24..771a1d3cb 100644
--- a/tests/functional/s/singleton_comparison.py
+++ b/tests/functional/s/singleton_comparison.py
@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, invalid-name, misplaced-comparison-constant,literal-comparison, comparison-with-itself
+# pylint: disable=missing-docstring, invalid-name, literal-comparison, comparison-with-itself
 x = 42
 a = x is None
 b = x == None  # [singleton-comparison]
diff --git a/tests/functional/u/use/use_implicit_booleaness_not_len.py b/tests/functional/u/use/use_implicit_booleaness_not_len.py
index b4107ac2d..fa410a81b 100644
--- a/tests/functional/u/use/use_implicit_booleaness_not_len.py
+++ b/tests/functional/u/use/use_implicit_booleaness_not_len.py
@@ -1,4 +1,4 @@
-# pylint: disable=too-few-public-methods,import-error, missing-docstring, misplaced-comparison-constant
+# pylint: disable=too-few-public-methods,import-error, missing-docstring
 # pylint: disable=useless-super-delegation,wrong-import-position,invalid-name, wrong-import-order, condition-evals-to-constant
 
 if len('TEST'):  # [use-implicit-booleaness-not-len]
diff --git a/tests/functional/u/use/using_constant_test.py b/tests/functional/u/use/using_constant_test.py
index 74b8a9c1a..ca1ce013c 100644
--- a/tests/functional/u/use/using_constant_test.py
+++ b/tests/functional/u/use/using_constant_test.py
@@ -114,7 +114,6 @@ if not 3:
 if instance.method():
     pass
 
-# pylint: disable=misplaced-comparison-constant
 if 2 < 3:
     pass