Fixup
diff --git a/crates/ruff/resources/test/fixtures/pylint/compare_to_empty_string.py b/crates/ruff/resources/test/fixtures/pylint/compare_to_empty_string.py index 59f2bbc..91b2926 100644 --- a/crates/ruff/resources/test/fixtures/pylint/compare_to_empty_string.py +++ b/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")
diff --git a/crates/ruff/src/rules/pylint/rules/compare_to_empty_string.rs b/crates/ruff/src/rules/pylint/rules/compare_to_empty_string.rs index 13cbb68..74e69e0 100644 --- a/crates/ruff/src/rules/pylint/rules/compare_to_empty_string.rs +++ b/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 @@ 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 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 @@ #[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,60 @@ 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( - 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), - }, - lhs.into(), + let existing = format!( + "{} {} {}", + unparse_expr(lhs, checker.stylist), + op, + unparse_constant(value, checker.stylist), ); - 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), + )); } } }
diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC1901_compare_to_empty_string.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC1901_compare_to_empty_string.py.snap index 7cb120b..d26a454 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC1901_compare_to_empty_string.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC1901_compare_to_empty_string.py.snap
@@ -8,11 +8,11 @@ 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 @@ 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 @@ 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 @@ 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: ~