Replace misplaced-comparison-constant with SIM300 (#1980)

Closes: #1954.

BREAKING_CHANGES.md

@@ -1,5 +1,12 @@
 # Breaking Changes
 
+## 0.0.226
+
+### `misplaced-comparison-constant` (`PLC2201`) was deprecated in favor of `SIM300` ([#1980](https://github.com/charliermarsh/ruff/pull/1980))
+
+These two rules contain (nearly) identical logic. To deduplicate the rule set, we've upgraded
+`SIM300` to handle a few more cases, and deprecated `PLC2201` in favor of `SIM300`.
+
 ## 0.0.225
 
 ### `@functools.cache` rewrites have been moved to a standalone rule (`UP033`) ([#1938](https://github.com/charliermarsh/ruff/pull/1938))

README.md

@@ -1112,7 +1112,6 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI.
 | Code | Name | Message | Fix |
 | ---- | ---- | ------- | --- |
 | PLC0414 | UselessImportAlias | Import alias does not rename original package | 🛠 |
-| PLC2201 | MisplacedComparisonConstant | Comparison should be ... | 🛠 |
 | PLC3002 | UnnecessaryDirectLambdaCall | Lambda expression called directly. Execute the expression inline instead. |  |
 
 #### Error (PLE)

resources/test/fixtures/flake8_simplify/SIM300.py

@@ -2,6 +2,9 @@
 "yoda" == compare  # SIM300
 'yoda' == compare  # SIM300
 42 == age  # SIM300
+"yoda" <= compare  # SIM300
+'yoda' < compare  # SIM300
+42 > age  # SIM300
 
 # OK
 compare == "yoda"

ruff.schema.json

@@ -1484,10 +1484,6 @@
         "PLC04",
         "PLC041",
         "PLC0414",
-        "PLC2",
-        "PLC22",
-        "PLC220",
-        "PLC2201",
         "PLC3",
         "PLC30",
         "PLC300",

src/checkers/ast.rs

@@ -2552,16 +2552,6 @@ where
                     );
                 }
 
-                if self.settings.rules.enabled(&RuleCode::PLC2201) {
-                    pylint::rules::misplaced_comparison_constant(
-                        self,
-                        expr,
-                        left,
-                        ops,
-                        comparators,
-                    );
-                }
-
                 if self.settings.rules.enabled(&RuleCode::PLR0133) {
                     pylint::rules::constant_comparison(self, left, ops, comparators);
                 }

src/registry.rs

@@ -80,7 +80,6 @@ ruff_macros::define_rule_mapping!(
     F901 => violations::RaiseNotImplemented,
     // pylint
     PLC0414 => violations::UselessImportAlias,
-    PLC2201 => violations::MisplacedComparisonConstant,
     PLC3002 => violations::UnnecessaryDirectLambdaCall,
     PLE0117 => violations::NonlocalWithoutBinding,
     PLE0118 => violations::UsedPriorGlobalDeclaration,

src/rules/flake8_simplify/rules/yoda_conditions.rs

@@ -14,17 +14,20 @@ pub fn yoda_conditions(
     ops: &[Cmpop],
     comparators: &[Expr],
 ) {
-    if !matches!(ops[..], [Cmpop::Eq]) {
+    let ([op], [right]) = (ops, comparators) else {
+        return;
+    };
+
+    if !matches!(
+        op,
+        Cmpop::Eq | Cmpop::NotEq | Cmpop::Lt | Cmpop::LtE | Cmpop::Gt | Cmpop::GtE,
+    ) {
         return;
     }
-    if comparators.len() != 1 {
+    if !matches!(&left.node, &ExprKind::Constant { .. }) {
         return;
     }
-    if !matches!(left.node, ExprKind::Constant { .. }) {
-        return;
-    }
-    let right = comparators.first().unwrap();
-    if matches!(right.node, ExprKind::Constant { .. }) {
+    if matches!(&right.node, &ExprKind::Constant { .. }) {
         return;
     }
 
@@ -36,16 +39,27 @@ pub fn yoda_conditions(
         .locator
         .slice_source_code_range(&Range::from_located(right));
 
+    // Reverse the operation.
+    let reversed_op = match op {
+        Cmpop::Eq => "==",
+        Cmpop::NotEq => "!=",
+        Cmpop::Lt => ">",
+        Cmpop::LtE => ">=",
+        Cmpop::Gt => "<",
+        Cmpop::GtE => "<=",
+        _ => unreachable!("Expected comparison operator"),
+    };
+
+    let suggestion = format!("{variable} {reversed_op} {constant}");
     let mut diagnostic = Diagnostic::new(
         violations::YodaConditions {
-            constant: constant.to_string(),
-            variable: variable.to_string(),
+            suggestion: suggestion.to_string(),
         },
         Range::from_located(expr),
     );
     if checker.patch(diagnostic.kind.code()) {
         diagnostic.amend(Fix::replacement(
-            format!("{variable} == {constant}"),
+            suggestion,
             left.location,
             right.end_location.unwrap(),
         ));

src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap

@@ -4,8 +4,7 @@ expression: diagnostics
 ---
 - kind:
     YodaConditions:
-      variable: compare
-      constant: "\"yoda\""
+      suggestion: "compare == \"yoda\""
   location:
     row: 2
     column: 0
@@ -23,8 +22,7 @@ expression: diagnostics
   parent: ~
 - kind:
     YodaConditions:
-      variable: compare
-      constant: "'yoda'"
+      suggestion: "compare == 'yoda'"
   location:
     row: 3
     column: 0
@@ -42,8 +40,7 @@ expression: diagnostics
   parent: ~
 - kind:
     YodaConditions:
-      variable: age
-      constant: "42"
+      suggestion: age == 42
   location:
     row: 4
     column: 0
@@ -59,4 +56,58 @@ expression: diagnostics
       row: 4
       column: 9
   parent: ~
+- kind:
+    YodaConditions:
+      suggestion: "compare >= \"yoda\""
+  location:
+    row: 5
+    column: 0
+  end_location:
+    row: 5
+    column: 17
+  fix:
+    content: "compare >= \"yoda\""
+    location:
+      row: 5
+      column: 0
+    end_location:
+      row: 5
+      column: 17
+  parent: ~
+- kind:
+    YodaConditions:
+      suggestion: "compare > 'yoda'"
+  location:
+    row: 6
+    column: 0
+  end_location:
+    row: 6
+    column: 16
+  fix:
+    content: "compare > 'yoda'"
+    location:
+      row: 6
+      column: 0
+    end_location:
+      row: 6
+      column: 16
+  parent: ~
+- kind:
+    YodaConditions:
+      suggestion: age < 42
+  location:
+    row: 7
+    column: 0
+  end_location:
+    row: 7
+    column: 8
+  fix:
+    content: age < 42
+    location:
+      row: 7
+      column: 0
+    end_location:
+      row: 7
+      column: 8
+  parent: ~
 

src/rules/pylint/mod.rs

@@ -13,7 +13,6 @@ mod tests {
     use crate::settings::Settings;
 
     #[test_case(RuleCode::PLC0414, Path::new("import_aliasing.py"); "PLC0414")]
-    #[test_case(RuleCode::PLC2201, Path::new("misplaced_comparison_constant.py"); "PLC2201")]
     #[test_case(RuleCode::PLC3002, Path::new("unnecessary_direct_lambda_call.py"); "PLC3002")]
     #[test_case(RuleCode::PLE0117, Path::new("nonlocal_without_binding.py"); "PLE0117")]
     #[test_case(RuleCode::PLE0118, Path::new("used_prior_global_declaration.py"); "PLE0118")]

src/rules/pylint/rules/misplaced_comparison_constant.rs

@@ -1,56 +0,0 @@
-use rustpython_ast::{Cmpop, Expr, ExprKind};
-
-use crate::ast::types::Range;
-use crate::checkers::ast::Checker;
-use crate::fix::Fix;
-use crate::registry::Diagnostic;
-use crate::violations;
-
-/// PLC2201
-pub fn misplaced_comparison_constant(
-    checker: &mut Checker,
-    expr: &Expr,
-    left: &Expr,
-    ops: &[Cmpop],
-    comparators: &[Expr],
-) {
-    let ([op], [right]) = (ops, comparators) else {
-        return;
-    };
-
-    if !matches!(
-        op,
-        Cmpop::Eq | Cmpop::NotEq | Cmpop::Lt | Cmpop::LtE | Cmpop::Gt | Cmpop::GtE,
-    ) {
-        return;
-    }
-    if !matches!(&left.node, &ExprKind::Constant { .. }) {
-        return;
-    }
-    if matches!(&right.node, &ExprKind::Constant { .. }) {
-        return;
-    }
-
-    let reversed_op = match op {
-        Cmpop::Eq => "==",
-        Cmpop::NotEq => "!=",
-        Cmpop::Lt => ">",
-        Cmpop::LtE => ">=",
-        Cmpop::Gt => "<",
-        Cmpop::GtE => "<=",
-        _ => unreachable!("Expected comparison operator"),
-    };
-    let suggestion = format!("{right} {reversed_op} {left}");
-    let mut diagnostic = Diagnostic::new(
-        violations::MisplacedComparisonConstant(suggestion.clone()),
-        Range::from_located(expr),
-    );
-    if checker.patch(diagnostic.kind.code()) {
-        diagnostic.amend(Fix::replacement(
-            suggestion,
-            expr.location,
-            expr.end_location.unwrap(),
-        ));
-    }
-    checker.diagnostics.push(diagnostic);
-}

src/rules/pylint/rules/mod.rs

@@ -2,7 +2,6 @@ pub use await_outside_async::await_outside_async;
 pub use constant_comparison::constant_comparison;
 pub use magic_value_comparison::magic_value_comparison;
 pub use merge_isinstance::merge_isinstance;
-pub use misplaced_comparison_constant::misplaced_comparison_constant;
 pub use property_with_parameters::property_with_parameters;
 pub use unnecessary_direct_lambda_call::unnecessary_direct_lambda_call;
 pub use use_from_import::use_from_import;
@@ -15,7 +14,6 @@ mod await_outside_async;
 mod constant_comparison;
 mod magic_value_comparison;
 mod merge_isinstance;
-mod misplaced_comparison_constant;
 mod property_with_parameters;
 mod unnecessary_direct_lambda_call;
 mod use_from_import;

src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC2201_misplaced_comparison_constant.py.snap

@@ -1,107 +0,0 @@
----
-source: src/rules/pylint/mod.rs
-expression: diagnostics
----
-- kind:
-    MisplacedComparisonConstant: i >= 5
-  location:
-    row: 20
-    column: 11
-  end_location:
-    row: 20
-    column: 17
-  fix:
-    content: i >= 5
-    location:
-      row: 20
-      column: 11
-    end_location:
-      row: 20
-      column: 17
-  parent: ~
-- kind:
-    MisplacedComparisonConstant: i == 1
-  location:
-    row: 22
-    column: 11
-  end_location:
-    row: 22
-    column: 17
-  fix:
-    content: i == 1
-    location:
-      row: 22
-      column: 11
-    end_location:
-      row: 22
-      column: 17
-  parent: ~
-- kind:
-    MisplacedComparisonConstant: dummy_return() > 3
-  location:
-    row: 24
-    column: 11
-  end_location:
-    row: 24
-    column: 29
-  fix:
-    content: dummy_return() > 3
-    location:
-      row: 24
-      column: 11
-    end_location:
-      row: 24
-      column: 29
-  parent: ~
-- kind:
-    MisplacedComparisonConstant: instance.dummy_return() != 4
-  location:
-    row: 26
-    column: 11
-  end_location:
-    row: 26
-    column: 39
-  fix:
-    content: instance.dummy_return() != 4
-    location:
-      row: 26
-      column: 11
-    end_location:
-      row: 26
-      column: 39
-  parent: ~
-- kind:
-    MisplacedComparisonConstant: instance.attr == 1
-  location:
-    row: 28
-    column: 11
-  end_location:
-    row: 28
-    column: 29
-  fix:
-    content: instance.attr == 1
-    location:
-      row: 28
-      column: 11
-    end_location:
-      row: 28
-      column: 29
-  parent: ~
-- kind:
-    MisplacedComparisonConstant: "instance.attr == 'aaa'"
-  location:
-    row: 30
-    column: 11
-  end_location:
-    row: 30
-    column: 33
-  fix:
-    content: "instance.attr == 'aaa'"
-    location:
-      row: 30
-      column: 11
-    end_location:
-      row: 30
-      column: 33
-  parent: ~
-

src/violations.rs

@@ -1056,25 +1056,6 @@ impl AlwaysAutofixableViolation for UselessImportAlias {
     }
 }
 
-define_violation!(
-    pub struct MisplacedComparisonConstant(pub String);
-);
-impl AlwaysAutofixableViolation for MisplacedComparisonConstant {
-    fn message(&self) -> String {
-        let MisplacedComparisonConstant(comparison) = self;
-        format!("Comparison should be {comparison}")
-    }
-
-    fn autofix_title(&self) -> String {
-        let MisplacedComparisonConstant(comparison) = self;
-        format!("Replace with {comparison}")
-    }
-
-    fn placeholder() -> Self {
-        MisplacedComparisonConstant("...".to_string())
-    }
-}
-
 define_violation!(
     pub struct UnnecessaryDirectLambdaCall;
 );
@@ -3135,25 +3116,23 @@ impl AlwaysAutofixableViolation for AndFalse {
 
 define_violation!(
     pub struct YodaConditions {
-        pub variable: String,
-        pub constant: String,
+        pub suggestion: String,
     }
 );
 impl AlwaysAutofixableViolation for YodaConditions {
     fn message(&self) -> String {
-        let YodaConditions { variable, constant } = self;
-        format!("Yoda conditions are discouraged, use `{variable} == {constant}` instead")
+        let YodaConditions { suggestion } = self;
+        format!("Yoda conditions are discouraged, use `{suggestion}` instead")
     }
 
     fn autofix_title(&self) -> String {
-        let YodaConditions { variable, constant } = self;
-        format!("Replace Yoda condition with `{variable} == {constant}`")
+        let YodaConditions { suggestion } = self;
+        format!("Replace Yoda condition with `{suggestion}`")
     }
 
     fn placeholder() -> Self {
         YodaConditions {
-            variable: "x".to_string(),
-            constant: "1".to_string(),
+            suggestion: "x == 1".to_string(),
         }
     }
 }