Implement PLR2004 (MagicValueComparison)

README.md

@@ -1095,6 +1095,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/2.15.7/) on PyPI.
 | PLR0402 | ConsiderUsingFromImport | Use `from ... import ...` in lieu of alias |  |
 | PLR1701 | ConsiderMergingIsinstance | Merge these isinstance calls: `isinstance(..., (...))` |  |
 | PLR1722 | UseSysExit | Use `sys.exit()` instead of `exit` | 🛠 |
+| PLR2004 | MagicValueComparison | Magic number used in comparison, consider replacing magic with a constant variable |  |
 | PLW0120 | UselessElseOnLoop | Else clause on loop without a break statement, remove the else and de-indent all the code inside it |  |
 | PLW0602 | GlobalVariableNotAssigned | Using global for `...` but no assignment is done |  |
 

resources/test/fixtures/pylint/magic_value_comparison.py

@@ -0,0 +1,21 @@
+"""Check that magic values are not used in comparisons"""
+
+user_input = 10
+
+if 10 > user_input: # [magic-value-comparison]
+    pass
+
+if 10 == 100: # [magic-value-comparison]
+    pass
+
+time_delta = 7224
+one_hour = 3600
+
+if time_delta > one_hour: # correct
+    pass
+
+argc = 1
+
+if argc != 1: # correct
+    pass
+

ruff.schema.json

@@ -1416,6 +1416,10 @@
         "PLR1701",
         "PLR172",
         "PLR1722",
+        "PLR2",
+        "PLR20",
+        "PLR200",
+        "PLR2004",
         "PLW",
         "PLW0",
         "PLW01",

src/checkers/ast.rs

@@ -2583,6 +2583,10 @@ where
                     );
                 }
 
+                if self.settings.enabled.contains(&RuleCode::PLR2004) {
+                    pylint::rules::magic_value_comparison(self, expr, left, comparators);
+                }
+
                 if self.settings.enabled.contains(&RuleCode::SIM118) {
                     flake8_simplify::rules::key_in_dict_compare(self, expr, left, ops, comparators);
                 }

src/pylint/mod.rs

@@ -27,6 +27,7 @@ mod tests {
     #[test_case(RuleCode::PLR1722, Path::new("consider_using_sys_exit_4.py"); "PLR1722_4")]
     #[test_case(RuleCode::PLR1722, Path::new("consider_using_sys_exit_5.py"); "PLR1722_5")]
     #[test_case(RuleCode::PLR1722, Path::new("consider_using_sys_exit_6.py"); "PLR1722_6")]
+    #[test_case(RuleCode::PLR2004, Path::new("magic_value_comparison.py"); "PLR2004")]
     #[test_case(RuleCode::PLW0120, Path::new("useless_else_on_loop.py"); "PLW0120")]
     #[test_case(RuleCode::PLW0602, Path::new("global_variable_not_assigned.py"); "PLW0602")]
     fn rules(rule_code: RuleCode, path: &Path) -> Result<()> {

src/pylint/rules/magic_value_comparison.rs

@@ -0,0 +1,41 @@
+use rustpython_ast::{Constant, Expr, ExprKind};
+
+use crate::ast::types::Range;
+use crate::checkers::ast::Checker;
+use crate::registry::Diagnostic;
+use crate::violations;
+
+fn is_magic_value(constant: &Constant) -> Option<String> {
+    match constant {
+        Constant::Int(value) => {
+            if matches!(value.try_into(), Ok(-1 | 0 | 1)) {
+                None
+            } else {
+                Some(value.to_string())
+            }
+        }
+        Constant::Float(value) => Some(value.to_string()),
+        _ => None,
+    }
+}
+
+/// PLR2004
+pub fn magic_value_comparison(
+    checker: &mut Checker,
+    expr: &Expr,
+    left: &Expr,
+    comparators: &[Expr],
+) {
+    for comparison_expr in comparators.iter().chain([left]) {
+        if let ExprKind::Constant { value, .. } = &comparison_expr.node {
+            if let Some(value) = is_magic_value(value) {
+                let diagnostic = Diagnostic::new(
+                    violations::MagicValueComparison(value),
+                    Range::from_located(expr),
+                );
+
+                checker.diagnostics.push(diagnostic);
+            }
+        }
+    }
+}

src/pylint/rules/mod.rs

@@ -1,4 +1,5 @@
 pub use await_outside_async::await_outside_async;
+pub use magic_value_comparison::magic_value_comparison;
 pub use merge_isinstance::merge_isinstance;
 pub use misplaced_comparison_constant::misplaced_comparison_constant;
 pub use property_with_parameters::property_with_parameters;
@@ -10,6 +11,7 @@ pub use useless_else_on_loop::useless_else_on_loop;
 pub use useless_import_alias::useless_import_alias;
 
 mod await_outside_async;
+mod magic_value_comparison;
 mod merge_isinstance;
 mod misplaced_comparison_constant;
 mod property_with_parameters;

src/pylint/snapshots/ruff__pylint__tests__PLR2004_magic_value_comparison.py.snap

@@ -0,0 +1,35 @@
+---
+source: src/pylint/mod.rs
+expression: diagnostics
+---
+- kind:
+    MagicValueComparison: "10"
+  location:
+    row: 5
+    column: 3
+  end_location:
+    row: 5
+    column: 18
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "100"
+  location:
+    row: 8
+    column: 3
+  end_location:
+    row: 8
+    column: 12
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "10"
+  location:
+    row: 8
+    column: 3
+  end_location:
+    row: 8
+    column: 12
+  fix: ~
+  parent: ~
+

src/registry.rs

@@ -189,6 +189,7 @@ define_rule_mapping!(
     PLR0402 => violations::ConsiderUsingFromImport,
     PLR1701 => violations::ConsiderMergingIsinstance,
     PLR1722 => violations::UseSysExit,
+    PLR2004 => violations::MagicValueComparison,
     PLW0120 => violations::UselessElseOnLoop,
     PLW0602 => violations::GlobalVariableNotAssigned,
     // flake8-builtins

src/violations.rs

@@ -1191,6 +1191,22 @@ impl AlwaysAutofixableViolation for UseSysExit {
     }
 }
 
+define_violation!(
+    pub struct MagicValueComparison(pub String);
+);
+impl Violation for MagicValueComparison {
+    fn message(&self) -> String {
+        let MagicValueComparison(value) = self;
+        format!(
+            "Magic number used in comparison, consider replacing {value} with a constant variable"
+        )
+    }
+
+    fn placeholder() -> Self {
+        MagicValueComparison("magic".to_string())
+    }
+}
+
 define_violation!(
     pub struct UselessElseOnLoop;
 );