Fixup

crates/ruff/resources/test/fixtures/pylint/compare_to_empty_string.py

@@ -1,13 +1,19 @@
 x = "a string"
 y = "another string"
+z = ""
 
-def should_raise_lint_warning():
+
+def errors():
     if x is "" or x == "":
         print("x is an empty string")
 
     if y is not "" or y != "":
         print("y is not an empty string")
 
-def should_not_raise_lint_warning():
+    if "" != z:
+        print("z is an empty string")
+
+
+def ok():
     if x and not y:
         print("x is not an empty string, but y is an empty string")

crates/ruff/src/rules/pylint/rules/compare_to_empty_string.rs

@@ -1,20 +1,23 @@
-use anyhow::anyhow;
+use anyhow::bail;
 use itertools::Itertools;
+use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind};
+
+use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::{unparse_constant, unparse_expr};
-use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind, Located};
+use ruff_python_ast::types::Range;
 
-use crate::{checkers::ast::Checker, registry::Diagnostic, violation::Violation};
+use crate::checkers::ast::Checker;
 
-#[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
-pub enum EmptyStringViolationsCmpop {
+pub enum EmptyStringCmpop {
     Is,
     IsNot,
     Eq,
     NotEq,
 }
 
-impl TryFrom<&Cmpop> for EmptyStringViolationsCmpop {
+impl TryFrom<&Cmpop> for EmptyStringCmpop {
     type Error = anyhow::Error;
 
     fn try_from(value: &Cmpop) -> Result<Self, Self::Error> {
@@ -23,14 +26,21 @@ impl TryFrom<&Cmpop> for EmptyStringViolationsCmpop {
             Cmpop::IsNot => Ok(Self::IsNot),
             Cmpop::Eq => Ok(Self::Eq),
             Cmpop::NotEq => Ok(Self::NotEq),
-            _ => Err(anyhow!(
+            _ => bail!("{value:?} cannot be converted to EmptyStringCmpop"),
-                "{value:?} cannot be converted to EmptyStringViolationsCmpop"
-            )),
         }
     }
 }
 
-impl std::fmt::Display for EmptyStringViolationsCmpop {
+impl EmptyStringCmpop {
+    pub fn into_unary(self) -> &'static str {
+        match self {
+            Self::Is | Self::Eq => "",
+            Self::IsNot | Self::NotEq => "not ",
+        }
+    }
+}
+
+impl std::fmt::Display for EmptyStringCmpop {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let repr = match self {
             Self::Is => "is",
@@ -44,21 +54,16 @@ impl std::fmt::Display for EmptyStringViolationsCmpop {
 
 #[violation]
 pub struct CompareToEmptyString {
-    pub lhs: String,
+    pub existing: String,
-    pub op: EmptyStringViolationsCmpop,
+    pub replacement: String,
-    pub rhs: String,
 }
 
 impl Violation for CompareToEmptyString {
     #[derive_message_formats]
     fn message(&self) -> String {
-        let prefix = match self.op {
-            EmptyStringViolationsCmpop::Is | EmptyStringViolationsCmpop::Eq => "",
-            EmptyStringViolationsCmpop::IsNot | EmptyStringViolationsCmpop::NotEq => "not ",
-        };
         format!(
-            "`{} {} {}` can be simplified to `{}{}` as an empty string is falsey",
+            "`{}` can be simplified to `{}` as an empty string is falsey",
-            self.lhs, self.op, self.rhs, prefix, self.lhs
+            self.existing, self.replacement,
         )
     }
 }
@@ -69,27 +74,60 @@ pub fn compare_to_empty_string(
     ops: &[Cmpop],
     comparators: &[Expr],
 ) {
+    let mut first = true;
     for ((lhs, rhs), op) in std::iter::once(left)
         .chain(comparators.iter())
-        .tuple_windows::<(&Located<_>, &Located<_>)>()
+        .tuple_windows::<(&Expr<_>, &Expr<_>)>()
         .zip(ops)
     {
-        if matches!(op, Cmpop::Eq | Cmpop::NotEq | Cmpop::Is | Cmpop::IsNot) {
+        if let Ok(op) = EmptyStringCmpop::try_from(op) {
+            if std::mem::take(&mut first) {
+                // Check the left-most expression.
+                if let ExprKind::Constant { value, .. } = &lhs.node {
+                    if let Constant::Str(s) = value {
+                        if s.is_empty() {
+                            let existing = format!(
+                                "{} {} {}",
+                                unparse_constant(value, checker.stylist),
+                                op,
+                                unparse_expr(rhs, checker.stylist)
+                            );
+                            let replacement = format!(
+                                "{}{}",
+                                op.into_unary(),
+                                unparse_expr(rhs, checker.stylist)
+                            );
+                            checker.diagnostics.push(Diagnostic::new(
+                                CompareToEmptyString {
+                                    existing,
+                                    replacement,
+                                },
+                                Range::from(lhs),
+                            ));
+                        }
+                    }
+                }
+            }
+
+            // Check all right-hand expressions.
             if let ExprKind::Constant { value, .. } = &rhs.node {
                 if let Constant::Str(s) = value {
                     if s.is_empty() {
-                        let diag = Diagnostic::new(
+                        let existing = format!(
-                            CompareToEmptyString {
+                            "{} {} {}",
-                                lhs: unparse_expr(lhs, checker.stylist),
+                            unparse_expr(lhs, checker.stylist),
-                                // we know `op` can be safely converted into a
+                            op,
-                                // EmptyStringViolationCmpop due to the first if statement being
+                            unparse_constant(value, checker.stylist),
-                                // true in this branch
-                                op: op.try_into().unwrap(),
-                                rhs: unparse_constant(value, checker.stylist),
-                            },
-                            lhs.into(),
                         );
-                        checker.diagnostics.push(diag);
+                        let replacement =
+                            format!("{}{}", op.into_unary(), unparse_expr(lhs, checker.stylist));
+                        checker.diagnostics.push(Diagnostic::new(
+                            CompareToEmptyString {
+                                existing,
+                                replacement,
+                            },
+                            Range::from(rhs),
+                        ));
                     }
                 }
             }

crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC1901_compare_to_empty_string.py.snap

@@ -8,11 +8,11 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 5
+    row: 7
-    column: 7
+    column: 12
   end_location:
-    row: 5
+    row: 7
-    column: 8
+    column: 14
   fix: ~
   parent: ~
 - kind:
@@ -21,11 +21,11 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 5
+    row: 7
-    column: 18
+    column: 23
   end_location:
-    row: 5
+    row: 7
-    column: 19
+    column: 25
   fix: ~
   parent: ~
 - kind:
@@ -34,11 +34,11 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 8
+    row: 10
-    column: 7
+    column: 16
   end_location:
-    row: 8
+    row: 10
-    column: 8
+    column: 18
   fix: ~
   parent: ~
 - kind:
@@ -47,11 +47,24 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 8
+    row: 10
-    column: 22
+    column: 27
   end_location:
-    row: 8
+    row: 10
-    column: 23
+    column: 29
+  fix: ~
+  parent: ~
+- kind:
+    name: CompareToEmptyString
+    body: "`\"\" != z` can be simplified to `not z` as an empty string is falsey"
+    suggestion: ~
+    fixable: false
+  location:
+    row: 13
+    column: 7
+  end_location:
+    row: 13
+    column: 9
   fix: ~
   parent: ~