Add Pylint rule comparison-with-itself (R0124) (#4957)

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

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);
                 }

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),

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}")
+    }
+}

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"))]

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(),

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(),
+                ));
+            }
+        }
+    }
+}

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;

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.
+   |
+
+

ruff.schema.json

@@ -2108,6 +2108,8 @@
         "PLR",
         "PLR0",
         "PLR01",
+        "PLR012",
+        "PLR0124",
         "PLR013",
         "PLR0133",
         "PLR02",