diff --git a/crates/ruff/resources/test/fixtures/pylint/comparison_with_itself.py b/crates/ruff/resources/test/fixtures/pylint/comparison_with_itself.py new file mode 100644 index 0000000000..e6505608a0 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/comparison_with_itself.py @@ -0,0 +1,45 @@ +# Errors. +foo == foo + +foo != foo + +foo > foo + +foo >= foo + +foo < foo + +foo <= foo + +foo is foo + +foo is not foo + +foo in foo + +foo not in foo + +# Non-errors. +"foo" == "foo" # This is flagged by `comparison-of-constant` instead. + +foo == "foo" + +foo == bar + +foo != bar + +foo > bar + +foo >= bar + +foo < bar + +foo <= bar + +foo is bar + +foo is not bar + +foo in bar + +foo not in bar diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index b41b485357..6ceb0a490d 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -3357,6 +3357,10 @@ where ); } + if self.enabled(Rule::ComparisonWithItself) { + pylint::rules::comparison_with_itself(self, left, ops, comparators); + } + if self.enabled(Rule::ComparisonOfConstant) { pylint::rules::comparison_of_constant(self, left, ops, comparators); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 641755699e..79f465aad3 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -183,6 +183,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "E2513") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidCharacterEsc), (Pylint, "E2514") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidCharacterNul), (Pylint, "E2515") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidCharacterZeroWidthSpace), + (Pylint, "R0124") => (RuleGroup::Unspecified, rules::pylint::rules::ComparisonWithItself), (Pylint, "R0133") => (RuleGroup::Unspecified, rules::pylint::rules::ComparisonOfConstant), (Pylint, "R0206") => (RuleGroup::Unspecified, rules::pylint::rules::PropertyWithParameters), (Pylint, "R0402") => (RuleGroup::Unspecified, rules::pylint::rules::ManualFromImport), diff --git a/crates/ruff/src/rules/pylint/helpers.rs b/crates/ruff/src/rules/pylint/helpers.rs index 8c27fb7367..d7d6c95b45 100644 --- a/crates/ruff/src/rules/pylint/helpers.rs +++ b/crates/ruff/src/rules/pylint/helpers.rs @@ -3,6 +3,8 @@ use ruff_python_semantic::analyze::function_type::FunctionType; use ruff_python_semantic::model::SemanticModel; use ruff_python_semantic::scope::ScopeKind; use rustpython_parser::ast; +use rustpython_parser::ast::Cmpop; +use std::fmt; use crate::settings::Settings; @@ -44,3 +46,31 @@ pub(crate) fn in_dunder_init(model: &SemanticModel, settings: &Settings) -> bool } true } + +/// A wrapper around [`Cmpop`] that implements `Display`. +#[derive(Debug)] +pub(crate) struct CmpopExt(Cmpop); + +impl From<&Cmpop> for CmpopExt { + fn from(cmpop: &Cmpop) -> Self { + CmpopExt(*cmpop) + } +} + +impl fmt::Display for CmpopExt { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let representation = match self.0 { + Cmpop::Eq => "==", + Cmpop::NotEq => "!=", + Cmpop::Lt => "<", + Cmpop::LtE => "<=", + Cmpop::Gt => ">", + Cmpop::GtE => ">=", + Cmpop::Is => "is", + Cmpop::IsNot => "is not", + Cmpop::In => "in", + Cmpop::NotIn => "not in", + }; + write!(f, "{representation}") + } +} diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index c7536d1922..c99c843d06 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -31,6 +31,7 @@ mod tests { Rule::RepeatedIsinstanceCalls, Path::new("repeated_isinstance_calls.py") )] + #[test_case(Rule::ComparisonWithItself, Path::new("comparison_with_itself.py"))] #[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_1.py"))] diff --git a/crates/ruff/src/rules/pylint/rules/comparison_of_constant.rs b/crates/ruff/src/rules/pylint/rules/comparison_of_constant.rs index 76d4468f7f..b38f72ad2d 100644 --- a/crates/ruff/src/rules/pylint/rules/comparison_of_constant.rs +++ b/crates/ruff/src/rules/pylint/rules/comparison_of_constant.rs @@ -1,5 +1,3 @@ -use std::fmt; - use itertools::Itertools; use rustpython_parser::ast::{self, Cmpop, Expr, Ranged}; @@ -7,55 +5,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; - -#[derive(Debug, PartialEq, Eq, Copy, Clone)] -pub(crate) enum ViolationsCmpop { - Eq, - NotEq, - Lt, - LtE, - Gt, - GtE, - Is, - IsNot, - In, - NotIn, -} - -impl From<&Cmpop> for ViolationsCmpop { - fn from(cmpop: &Cmpop) -> Self { - match cmpop { - Cmpop::Eq => Self::Eq, - Cmpop::NotEq => Self::NotEq, - Cmpop::Lt => Self::Lt, - Cmpop::LtE => Self::LtE, - Cmpop::Gt => Self::Gt, - Cmpop::GtE => Self::GtE, - Cmpop::Is => Self::Is, - Cmpop::IsNot => Self::IsNot, - Cmpop::In => Self::In, - Cmpop::NotIn => Self::NotIn, - } - } -} - -impl fmt::Display for ViolationsCmpop { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let representation = match self { - Self::Eq => "==", - Self::NotEq => "!=", - Self::Lt => "<", - Self::LtE => "<=", - Self::Gt => ">", - Self::GtE => ">=", - Self::Is => "is", - Self::IsNot => "is not", - Self::In => "in", - Self::NotIn => "not in", - }; - write!(f, "{representation}") - } -} +use crate::rules::pylint::helpers::CmpopExt; /// ## What it does /// Checks for comparisons between constants. @@ -80,7 +30,7 @@ impl fmt::Display for ViolationsCmpop { #[violation] pub struct ComparisonOfConstant { left_constant: String, - op: ViolationsCmpop, + op: Cmpop, right_constant: String, } @@ -94,8 +44,8 @@ impl Violation for ComparisonOfConstant { } = self; format!( - "Two constants compared in a comparison, consider replacing `{left_constant} {op} \ - {right_constant}`" + "Two constants compared in a comparison, consider replacing `{left_constant} {} {right_constant}`", + CmpopExt::from(op) ) } } @@ -126,7 +76,7 @@ pub(crate) fn comparison_of_constant( let diagnostic = Diagnostic::new( ComparisonOfConstant { left_constant: checker.generator().constant(left_constant), - op: op.into(), + op: *op, right_constant: checker.generator().constant(right_constant), }, left.range(), diff --git a/crates/ruff/src/rules/pylint/rules/comparison_with_itself.rs b/crates/ruff/src/rules/pylint/rules/comparison_with_itself.rs new file mode 100644 index 0000000000..12824054cd --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/comparison_with_itself.rs @@ -0,0 +1,67 @@ +use itertools::Itertools; +use rustpython_parser::ast::{Cmpop, Expr, Ranged}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::CmpopExt; + +/// ## What it does +/// Checks for operations that compare a name to itself. +/// +/// ## Why is this bad? +/// Comparing a name to itself always results in the same value, and is likely +/// a mistake. +/// +/// ## Example +/// ```python +/// foo == foo +/// ``` +/// +/// ## References +/// - [Python documentation: Comparisons](https://docs.python.org/3/reference/expressions.html#comparisons) +#[violation] +pub struct ComparisonWithItself { + left: String, + op: Cmpop, + right: String, +} + +impl Violation for ComparisonWithItself { + #[derive_message_formats] + fn message(&self) -> String { + let ComparisonWithItself { left, op, right } = self; + format!( + "Name compared with itself, consider replacing `{left} {} {right}`", + CmpopExt::from(op) + ) + } +} + +/// PLR0124 +pub(crate) fn comparison_with_itself( + checker: &mut Checker, + left: &Expr, + ops: &[Cmpop], + comparators: &[Expr], +) { + for ((left, right), op) in std::iter::once(left) + .chain(comparators.iter()) + .tuple_windows() + .zip(ops) + { + if let (Expr::Name(left), Expr::Name(right)) = (left, right) { + if left.id == right.id { + checker.diagnostics.push(Diagnostic::new( + ComparisonWithItself { + left: left.id.to_string(), + op: *op, + right: right.id.to_string(), + }, + left.range(), + )); + } + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 70f088b313..b254b79541 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -7,6 +7,7 @@ pub(crate) use binary_op_exception::{binary_op_exception, BinaryOpException}; pub(crate) use collapsible_else_if::{collapsible_else_if, CollapsibleElseIf}; pub(crate) use compare_to_empty_string::{compare_to_empty_string, CompareToEmptyString}; pub(crate) use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant}; +pub(crate) use comparison_with_itself::{comparison_with_itself, ComparisonWithItself}; pub(crate) use continue_in_finally::{continue_in_finally, ContinueInFinally}; pub(crate) use duplicate_bases::{duplicate_bases, DuplicateBases}; pub(crate) use global_statement::{global_statement, GlobalStatement}; @@ -63,6 +64,7 @@ mod binary_op_exception; mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; +mod comparison_with_itself; mod continue_in_finally; mod duplicate_bases; mod global_statement; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR0124_comparison_with_itself.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR0124_comparison_with_itself.py.snap new file mode 100644 index 0000000000..5571f0606b --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR0124_comparison_with_itself.py.snap @@ -0,0 +1,103 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +comparison_with_itself.py:2:1: PLR0124 Name compared with itself, consider replacing `foo == foo` + | +2 | # Errors. +3 | foo == foo + | ^^^ PLR0124 +4 | +5 | foo != foo + | + +comparison_with_itself.py:4:1: PLR0124 Name compared with itself, consider replacing `foo != foo` + | +4 | foo == foo +5 | +6 | foo != foo + | ^^^ PLR0124 +7 | +8 | foo > foo + | + +comparison_with_itself.py:6:1: PLR0124 Name compared with itself, consider replacing `foo > foo` + | + 6 | foo != foo + 7 | + 8 | foo > foo + | ^^^ PLR0124 + 9 | +10 | foo >= foo + | + +comparison_with_itself.py:8:1: PLR0124 Name compared with itself, consider replacing `foo >= foo` + | + 8 | foo > foo + 9 | +10 | foo >= foo + | ^^^ PLR0124 +11 | +12 | foo < foo + | + +comparison_with_itself.py:10:1: PLR0124 Name compared with itself, consider replacing `foo < foo` + | +10 | foo >= foo +11 | +12 | foo < foo + | ^^^ PLR0124 +13 | +14 | foo <= foo + | + +comparison_with_itself.py:12:1: PLR0124 Name compared with itself, consider replacing `foo <= foo` + | +12 | foo < foo +13 | +14 | foo <= foo + | ^^^ PLR0124 +15 | +16 | foo is foo + | + +comparison_with_itself.py:14:1: PLR0124 Name compared with itself, consider replacing `foo is foo` + | +14 | foo <= foo +15 | +16 | foo is foo + | ^^^ PLR0124 +17 | +18 | foo is not foo + | + +comparison_with_itself.py:16:1: PLR0124 Name compared with itself, consider replacing `foo is not foo` + | +16 | foo is foo +17 | +18 | foo is not foo + | ^^^ PLR0124 +19 | +20 | foo in foo + | + +comparison_with_itself.py:18:1: PLR0124 Name compared with itself, consider replacing `foo in foo` + | +18 | foo is not foo +19 | +20 | foo in foo + | ^^^ PLR0124 +21 | +22 | foo not in foo + | + +comparison_with_itself.py:20:1: PLR0124 Name compared with itself, consider replacing `foo not in foo` + | +20 | foo in foo +21 | +22 | foo not in foo + | ^^^ PLR0124 +23 | +24 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index ec762028df..ac06622620 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2108,6 +2108,8 @@ "PLR", "PLR0", "PLR01", + "PLR012", + "PLR0124", "PLR013", "PLR0133", "PLR02",