Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement PLR2004 (MagicValueComparison) #1828

Merged
merged 4 commits into from
Jan 13, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -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 | |

71 changes: 71 additions & 0 deletions resources/test/fixtures/pylint/magic_value_comparison.py
Original file line number Diff line number Diff line change
@@ -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

4 changes: 4 additions & 0 deletions ruff.schema.json
Original file line number Diff line number Diff line change
@@ -1416,6 +1416,10 @@
"PLR1701",
"PLR172",
"PLR1722",
"PLR2",
"PLR20",
"PLR200",
"PLR2004",
"PLW",
"PLW0",
"PLW01",
4 changes: 4 additions & 0 deletions src/checkers/ast.rs
Original file line number Diff line number Diff line change
@@ -2583,6 +2583,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);
}
1 change: 1 addition & 0 deletions src/pylint/mod.rs
Original file line number Diff line number Diff line change
@@ -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<()> {
51 changes: 51 additions & 0 deletions src/pylint/rules/magic_value_comparison.rs
Original file line number Diff line number Diff line change
@@ -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 {
max0x53 marked this conversation as resolved.
Show resolved Hide resolved
if is_magic_value(value) {
let diagnostic = Diagnostic::new(
violations::MagicValueComparison(value.to_string()),
Range::from_located(comparison_expr),
);

checker.diagnostics.push(diagnostic);
}
}
}
}
2 changes: 2 additions & 0 deletions src/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
@@ -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: ~

1 change: 1 addition & 0 deletions src/registry.rs
Original file line number Diff line number Diff line change
@@ -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
16 changes: 16 additions & 0 deletions src/violations.rs
Original file line number Diff line number Diff line change
@@ -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;
);