cleanup plc1901 impl

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

@@ -1,11 +1,13 @@
 x = "a string"
 y = "another string"
 
+def should_raise_lint_warning():
     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 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,3 +1,4 @@
+use anyhow::anyhow;
 use itertools::Itertools;
 use ruff_macros::{derive_message_formats, violation};
 use ruff_python_ast::helpers::{unparse_constant, unparse_expr};
@@ -5,27 +6,59 @@ use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind, Located};
 
 use crate::{checkers::ast::Checker, registry::Diagnostic, violation::Violation};
 
-use super::comparison_of_constant::ViolationsCmpop;
+#[derive(Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
+pub enum EmptyStringViolationsCmpop {
+    Is,
+    IsNot,
+    Eq,
+    NotEq,
+}
+
+impl TryFrom<&Cmpop> for EmptyStringViolationsCmpop {
+    type Error = anyhow::Error;
+
+    fn try_from(value: &Cmpop) -> Result<Self, Self::Error> {
+        match value {
+            Cmpop::Is => Ok(Self::Is),
+            Cmpop::IsNot => Ok(Self::IsNot),
+            Cmpop::Eq => Ok(Self::Eq),
+            Cmpop::NotEq => Ok(Self::NotEq),
+            _ => Err(anyhow!(
+                "{value:?} cannot be converted to EmptyStringViolationsCmpop"
+            )),
+        }
+    }
+}
+
+impl std::fmt::Display for EmptyStringViolationsCmpop {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let repr = match self {
+            Self::Is => "is",
+            Self::IsNot => "is not",
+            Self::Eq => "==",
+            Self::NotEq => "!=",
+        };
+        write!(f, "{repr}")
+    }
+}
 
 #[violation]
 pub struct CompareToEmptyString {
     pub lhs: String,
-    pub op: ViolationsCmpop,
+    pub op: EmptyStringViolationsCmpop,
     pub rhs: String,
 }
 
 impl Violation for CompareToEmptyString {
     #[derive_message_formats]
     fn message(&self) -> String {
-        let cond = match self.op {
+        let prefix = match self.op {
-            ViolationsCmpop::Is | ViolationsCmpop::Eq => "",
+            EmptyStringViolationsCmpop::Is | EmptyStringViolationsCmpop::Eq => "",
-            // the assumption is that this message will only ever be shown
+            EmptyStringViolationsCmpop::IsNot | EmptyStringViolationsCmpop::NotEq => "not ",
-            // if op is `in`, `=`, `not in`, `!=`
-            _ => "not",
         };
         format!(
-            "{} {} {} can be simplified to {} {} as strings are falsey",
+            "`{} {} {}` can be simplified to `{}{}` as an empty string is falsey",
-            self.lhs, self.op, self.rhs, cond, self.lhs
+            self.lhs, self.op, self.rhs, prefix, self.lhs
         )
     }
 }
@@ -48,7 +81,10 @@ pub fn compare_to_empty_string(
                         let diag = Diagnostic::new(
                             CompareToEmptyString {
                                 lhs: unparse_expr(lhs, checker.stylist),
-                                op: op.into(),
+                                // 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),
                             },
                             lhs.into(),

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

@@ -3,55 +3,55 @@ source: crates/ruff/src/rules/pylint/mod.rs
 expression: diagnostics
 ---
 - kind:
-    CompareToEmptyString:
+    name: CompareToEmptyString
-      lhs: x
+    body: "`x is \"\"` can be simplified to `x` as an empty string is falsey"
-      op: Is
+    suggestion: ~
-      rhs: "\"\""
+    fixable: false
   location:
-    row: 4
+    row: 5
-    column: 3
+    column: 7
   end_location:
-    row: 4
+    row: 5
-    column: 4
+    column: 8
   fix: ~
   parent: ~
 - kind:
-    CompareToEmptyString:
+    name: CompareToEmptyString
-      lhs: x
+    body: "`x == \"\"` can be simplified to `x` as an empty string is falsey"
-      op: Eq
+    suggestion: ~
-      rhs: "\"\""
+    fixable: false
   location:
-    row: 4
+    row: 5
-    column: 14
-  end_location:
-    row: 4
-    column: 15
-  fix: ~
-  parent: ~
-- kind:
-    CompareToEmptyString:
-      lhs: y
-      op: IsNot
-      rhs: "\"\""
-  location:
-    row: 7
-    column: 3
-  end_location:
-    row: 7
-    column: 4
-  fix: ~
-  parent: ~
-- kind:
-    CompareToEmptyString:
-      lhs: y
-      op: NotEq
-      rhs: "\"\""
-  location:
-    row: 7
     column: 18
   end_location:
-    row: 7
+    row: 5
     column: 19
   fix: ~
   parent: ~
+- kind:
+    name: CompareToEmptyString
+    body: "`y is not \"\"` can be simplified to `not y` as an empty string is falsey"
+    suggestion: ~
+    fixable: false
+  location:
+    row: 8
+    column: 7
+  end_location:
+    row: 8
+    column: 8
+  fix: ~
+  parent: ~
+- kind:
+    name: CompareToEmptyString
+    body: "`y != \"\"` can be simplified to `not y` as an empty string is falsey"
+    suggestion: ~
+    fixable: false
+  location:
+    row: 8
+    column: 22
+  end_location:
+    row: 8
+    column: 23
+  fix: ~
+  parent: ~