From b47e8e6770d89775d31082dde4f0653330b6de3c Mon Sep 17 00:00:00 2001 From: max0x53 <110969456+max0x53@users.noreply.github.com> Date: Fri, 13 Jan 2023 00:44:18 +0000 Subject: [PATCH] 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 | 1 + .../fixtures/pylint/magic_value_comparison.py | 71 +++++++++++++++++++ ruff.schema.json | 4 ++ src/checkers/ast.rs | 4 ++ src/pylint/mod.rs | 1 + src/pylint/rules/magic_value_comparison.rs | 51 +++++++++++++ src/pylint/rules/mod.rs | 2 + ...ts__PLR2004_magic_value_comparison.py.snap | 55 ++++++++++++++ src/registry.rs | 1 + src/violations.rs | 16 +++++ 10 files changed, 206 insertions(+) create mode 100644 resources/test/fixtures/pylint/magic_value_comparison.py create mode 100644 src/pylint/rules/magic_value_comparison.rs create mode 100644 src/pylint/snapshots/ruff__pylint__tests__PLR2004_magic_value_comparison.py.snap diff --git a/README.md b/README.md index 9366602bc475b..02ba0e53d6305 100644 --- a/README.md +++ b/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 | | diff --git a/resources/test/fixtures/pylint/magic_value_comparison.py b/resources/test/fixtures/pylint/magic_value_comparison.py new file mode 100644 index 0000000000000..c6a57de11d661 --- /dev/null +++ b/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 + diff --git a/ruff.schema.json b/ruff.schema.json index c382e8081374e..ba4d8a73aa426 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1445,6 +1445,10 @@ "PLR1701", "PLR172", "PLR1722", + "PLR2", + "PLR20", + "PLR200", + "PLR2004", "PLW", "PLW0", "PLW01", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 5eaddc4bad76c..337f815d7f971 100644 --- a/src/checkers/ast.rs +++ b/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); } diff --git a/src/pylint/mod.rs b/src/pylint/mod.rs index 6d154d380e80e..55e11bcb17e4a 100644 --- a/src/pylint/mod.rs +++ b/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<()> { diff --git a/src/pylint/rules/magic_value_comparison.rs b/src/pylint/rules/magic_value_comparison.rs new file mode 100644 index 0000000000000..839a51df065ea --- /dev/null +++ b/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); + } + } + } +} diff --git a/src/pylint/rules/mod.rs b/src/pylint/rules/mod.rs index 8b645038353de..b9d3f87919df1 100644 --- a/src/pylint/rules/mod.rs +++ b/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; diff --git a/src/pylint/snapshots/ruff__pylint__tests__PLR2004_magic_value_comparison.py.snap b/src/pylint/snapshots/ruff__pylint__tests__PLR2004_magic_value_comparison.py.snap new file mode 100644 index 0000000000000..b6b01e29631f8 --- /dev/null +++ b/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: ~ + diff --git a/src/registry.rs b/src/registry.rs index c52b7e09462ae..4c8f3bd6e794e 100644 --- a/src/registry.rs +++ b/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 diff --git a/src/violations.rs b/src/violations.rs index 789472169ffe3..c920cb1ff70c8 100644 --- a/src/violations.rs +++ b/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; );