Implement PLR2004 (MagicValueComparison) (#1828)

This PR adds [Pylint
`R2004`](https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/magic-value-comparison.html#magic-value-comparison-r2004)

Feel free to suggest changes and additions, I have tried to maintain
parity with the Pylint implementation
[`magic_value.py`](https://github.com/PyCQA/pylint/blob/main/pylint/extensions/magic_value.py)

See #970

README.md

@@ -1096,6 +1096,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,71 @@
+"""Check that magic values are not used in comparisons"""
+import cmath
+
+user_input = 10
+
+if 10 > user_input: # [magic-value-comparison]
+    pass
+
+if 10 == 100: # [comparison-of-constants] R0133
+    pass
+
+if 1 == 3: # [comparison-of-constants] R0133
+    pass
+
+x = 0
+if 4 == 3 == x: # [comparison-of-constants] R0133
+    pass
+
+time_delta = 7224
+ONE_HOUR = 3600
+
+if time_delta > ONE_HOUR: # correct
+    pass
+
+argc = 1
+
+if argc != -1: # correct
+    pass
+
+if argc != 0: # correct
+    pass
+
+if argc != 1: # correct
+    pass
+
+if argc != 2: # [magic-value-comparison]
+    pass
+
+if __name__ == "__main__": # correct
+    pass
+
+ADMIN_PASSWORD = "SUPERSECRET"
+input_password = "password"
+
+if input_password == "": # correct
+    pass
+
+if input_password == ADMIN_PASSWORD: # correct
+    pass
+
+if input_password == "Hunter2": # [magic-value-comparison]
+    pass
+
+PI = 3.141592653589793238
+pi_estimation = 3.14
+
+if pi_estimation == 3.141592653589793238: # [magic-value-comparison]
+    pass
+
+if pi_estimation == PI: # correct
+    pass
+
+HELLO_WORLD = b"Hello, World!"
+user_input = b"Hello, There!"
+
+if user_input == b"something": # [magic-value-comparison]
+    pass
+
+if user_input == HELLO_WORLD: # correct
+    pass
+

ruff.schema.json

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

src/checkers/ast.rs

@@ -2590,6 +2590,10 @@ where
                     );
                 }
 
+                if self.settings.enabled.contains(&RuleCode::PLR2004) {
+                    pylint::rules::magic_value_comparison(self, 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,51 @@
+use itertools::Itertools;
+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) -> bool {
+    match constant {
+        Constant::None => false,
+        // E712 `if True == do_something():`
+        Constant::Bool(_) => false,
+        Constant::Str(value) => !matches!(value.as_str(), "" | "__main__"),
+        Constant::Bytes(_) => true,
+        Constant::Int(value) => !matches!(value.try_into(), Ok(-1 | 0 | 1)),
+        Constant::Tuple(_) => true,
+        Constant::Float(_) => true,
+        Constant::Complex { .. } => true,
+        Constant::Ellipsis => true,
+    }
+}
+
+/// PLR2004
+pub fn magic_value_comparison(checker: &mut Checker, left: &Expr, comparators: &[Expr]) {
+    for (left, right) in std::iter::once(left)
+        .chain(comparators.iter())
+        .tuple_windows()
+    {
+        // If both of the comparators are constant, skip rule for the whole expression.
+        // R0133: comparison-of-constants
+        if matches!(left.node, ExprKind::Constant { .. })
+            && matches!(right.node, ExprKind::Constant { .. })
+        {
+            return;
+        }
+    }
+
+    for comparison_expr in std::iter::once(left).chain(comparators.iter()) {
+        if let ExprKind::Constant { value, .. } = &comparison_expr.node {
+            if is_magic_value(value) {
+                let diagnostic = Diagnostic::new(
+                    violations::MagicValueComparison(value.to_string()),
+                    Range::from_located(comparison_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,55 @@
+---
+source: src/pylint/mod.rs
+expression: diagnostics
+---
+- kind:
+    MagicValueComparison: "10"
+  location:
+    row: 6
+    column: 3
+  end_location:
+    row: 6
+    column: 5
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "2"
+  location:
+    row: 36
+    column: 11
+  end_location:
+    row: 36
+    column: 12
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "'Hunter2'"
+  location:
+    row: 51
+    column: 21
+  end_location:
+    row: 51
+    column: 30
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "3.141592653589793"
+  location:
+    row: 57
+    column: 20
+  end_location:
+    row: 57
+    column: 40
+  fix: ~
+  parent: ~
+- kind:
+    MagicValueComparison: "b'something'"
+  location:
+    row: 66
+    column: 17
+  end_location:
+    row: 66
+    column: 29
+  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;
 );