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/codes.rs

@@ -161,35 +161,35 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
         (Pyflakes, "901") => Rule::RaiseNotImplemented,
 
         // pylint
+        (Pylint, "C0414") => Rule::UselessImportAlias,
+        (Pylint, "C1901") => Rule::CompareToEmptyString,
+        (Pylint, "C3002") => Rule::UnnecessaryDirectLambdaCall,
         (Pylint, "E0100") => Rule::YieldInInit,
         (Pylint, "E0101") => Rule::ReturnInInit,
+        (Pylint, "E0117") => Rule::NonlocalWithoutBinding,
+        (Pylint, "E0118") => Rule::UsedPriorGlobalDeclaration,
         (Pylint, "E0604") => Rule::InvalidAllObject,
         (Pylint, "E0605") => Rule::InvalidAllFormat,
+        (Pylint, "E1142") => Rule::AwaitOutsideAsync,
         (Pylint, "E1205") => Rule::LoggingTooManyArgs,
         (Pylint, "E1206") => Rule::LoggingTooFewArgs,
         (Pylint, "E1307") => Rule::BadStringFormatType,
-        (Pylint, "E2502") => Rule::BidirectionalUnicode,
         (Pylint, "E1310") => Rule::BadStrStripCall,
-        (Pylint, "C0414") => Rule::UselessImportAlias,
-        (Pylint, "C3002") => Rule::UnnecessaryDirectLambdaCall,
-        (Pylint, "E0117") => Rule::NonlocalWithoutBinding,
-        (Pylint, "E0118") => Rule::UsedPriorGlobalDeclaration,
-        (Pylint, "E1142") => Rule::AwaitOutsideAsync,
+        (Pylint, "E2502") => Rule::BidirectionalUnicode,
+        (Pylint, "R0133") => Rule::ComparisonOfConstant,
         (Pylint, "R0206") => Rule::PropertyWithParameters,
         (Pylint, "R0402") => Rule::ConsiderUsingFromImport,
-        (Pylint, "C1901") => Rule::CompareToEmptyString,
-        (Pylint, "R0133") => Rule::ComparisonOfConstant,
+        (Pylint, "R0911") => Rule::TooManyReturnStatements,
+        (Pylint, "R0912") => Rule::TooManyBranches,
+        (Pylint, "R0913") => Rule::TooManyArguments,
+        (Pylint, "R0915") => Rule::TooManyStatements,
         (Pylint, "R1701") => Rule::ConsiderMergingIsinstance,
         (Pylint, "R1722") => Rule::ConsiderUsingSysExit,
-        (Pylint, "R5501") => Rule::CollapsibleElseIf,
         (Pylint, "R2004") => Rule::MagicValueComparison,
+        (Pylint, "R5501") => Rule::CollapsibleElseIf,
         (Pylint, "W0120") => Rule::UselessElseOnLoop,
         (Pylint, "W0602") => Rule::GlobalVariableNotAssigned,
         (Pylint, "W0603") => Rule::GlobalStatement,
-        (Pylint, "R0911") => Rule::TooManyReturnStatements,
-        (Pylint, "R0913") => Rule::TooManyArguments,
-        (Pylint, "R0912") => Rule::TooManyBranches,
-        (Pylint, "R0915") => Rule::TooManyStatements,
         (Pylint, "W2901") => Rule::RedefinedLoopName,
 
         // flake8-builtins

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)]
-pub enum EmptyStringViolationsCmpop {
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
+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!(
-                "{value:?} cannot be converted to EmptyStringViolationsCmpop"
-            )),
+            _ => bail!("{value:?} cannot be converted to EmptyStringCmpop"),
+        }
+    }
+}
+
+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 EmptyStringViolationsCmpop {
+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 op: EmptyStringViolationsCmpop,
-    pub rhs: String,
+    pub existing: String,
+    pub replacement: 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",
-            self.lhs, self.op, self.rhs, prefix, self.lhs
+            "`{}` can be simplified to `{}` as an empty string is falsey",
+            self.existing, self.replacement,
         )
     }
 }
@@ -69,27 +74,49 @@ 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 constant = unparse_constant(value, checker.stylist);
+                            let expr = unparse_expr(rhs, checker.stylist);
+                            let existing = format!("{constant} {op} {expr}");
+                            let replacement = format!("{}{expr}", op.into_unary());
+                            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 expr = unparse_expr(lhs, checker.stylist);
+                        let constant = unparse_constant(value, checker.stylist);
+                        let existing = format!("{expr} {op} {constant}");
+                        let replacement = format!("{}{expr}", op.into_unary());
+                        checker.diagnostics.push(Diagnostic::new(
                             CompareToEmptyString {
-                                lhs: unparse_expr(lhs, checker.stylist),
-                                // we know `op` can be safely converted into a
-                                // EmptyStringViolationCmpop due to the first if statement being
-                                // true in this branch
-                                op: op.try_into().unwrap(),
-                                rhs: unparse_constant(value, checker.stylist),
+                                existing,
+                                replacement,
                             },
-                            lhs.into(),
-                        );
-                        checker.diagnostics.push(diag);
+                            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
-    column: 7
+    row: 7
+    column: 12
   end_location:
-    row: 5
-    column: 8
+    row: 7
+    column: 14
   fix: ~
   parent: ~
 - kind:
@@ -21,11 +21,11 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 5
-    column: 18
+    row: 7
+    column: 23
   end_location:
-    row: 5
-    column: 19
+    row: 7
+    column: 25
   fix: ~
   parent: ~
 - kind:
@@ -34,11 +34,11 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 8
-    column: 7
+    row: 10
+    column: 16
   end_location:
-    row: 8
-    column: 8
+    row: 10
+    column: 18
   fix: ~
   parent: ~
 - kind:
@@ -47,11 +47,24 @@ expression: diagnostics
     suggestion: ~
     fixable: false
   location:
-    row: 8
-    column: 22
+    row: 10
+    column: 27
   end_location:
-    row: 8
-    column: 23
+    row: 10
+    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: ~