diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM300.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM300.py index ac327ca6ca..25735ca70c 100644 --- a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM300.py +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM300.py @@ -6,6 +6,8 @@ "yoda" <= compare # SIM300 "yoda" < compare # SIM300 42 > age # SIM300 +-42 > age # SIM300 ++42 > age # SIM300 YODA == age # SIM300 YODA > age # SIM300 YODA >= age # SIM300 diff --git a/crates/ruff/resources/test/fixtures/pylint/magic_value_comparison.py b/crates/ruff/resources/test/fixtures/pylint/magic_value_comparison.py index 122887388d..c292546b52 100644 --- a/crates/ruff/resources/test/fixtures/pylint/magic_value_comparison.py +++ b/crates/ruff/resources/test/fixtures/pylint/magic_value_comparison.py @@ -11,6 +11,9 @@ if 10 == 100: # [comparison-of-constants] R0133 if 1 == 3: # [comparison-of-constants] R0133 pass +if 1 == -3: # [comparison-of-constants] R0133 + pass + x = 0 if 4 == 3 == x: # [comparison-of-constants] R0133 pass @@ -35,6 +38,12 @@ if argc != 1: # correct if argc != 2: # [magic-value-comparison] pass +if argc != -2: # [magic-value-comparison] + pass + +if argc != +2: # [magic-value-comparison] + pass + if __name__ == "__main__": # correct pass diff --git a/crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs b/crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs index 2c1d28c224..fc56e232bf 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs @@ -2,7 +2,7 @@ use anyhow::Result; use libcst_native::{Codegen, CodegenState, CompOp}; use ruff_macros::{define_violation, derive_message_formats}; use ruff_python::str::{self}; -use rustpython_parser::ast::{Cmpop, Expr, ExprKind}; +use rustpython_parser::ast::{Cmpop, Expr, ExprKind, Unaryop}; use crate::ast::types::Range; use crate::checkers::ast::Checker; @@ -50,6 +50,10 @@ fn is_constant_like(expr: &Expr) -> bool { ExprKind::Constant { .. } => true, ExprKind::Tuple { elts, .. } => elts.iter().all(is_constant_like), ExprKind::Name { id, .. } => str::is_upper(id), + ExprKind::UnaryOp { + op: Unaryop::UAdd | Unaryop::USub | Unaryop::Invert, + operand, + } => matches!(operand.node, ExprKind::Constant { .. }), _ => false, } } diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap index e0799923ee..6eea1acc2d 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM300_SIM300.py.snap @@ -130,110 +130,146 @@ expression: diagnostics parent: ~ - kind: YodaConditions: - suggestion: age == YODA + suggestion: age < -42 location: row: 9 column: 0 end_location: row: 9 - column: 11 + column: 9 fix: - content: age == YODA + content: age < -42 location: row: 9 column: 0 end_location: row: 9 + column: 9 + parent: ~ +- kind: + YodaConditions: + suggestion: age < +42 + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 9 + fix: + content: age < +42 + location: + row: 10 + column: 0 + end_location: + row: 10 + column: 9 + parent: ~ +- kind: + YodaConditions: + suggestion: age == YODA + location: + row: 11 + column: 0 + end_location: + row: 11 + column: 11 + fix: + content: age == YODA + location: + row: 11 + column: 0 + end_location: + row: 11 column: 11 parent: ~ - kind: YodaConditions: suggestion: age < YODA location: - row: 10 + row: 12 column: 0 end_location: - row: 10 + row: 12 column: 10 fix: content: age < YODA location: - row: 10 + row: 12 column: 0 end_location: - row: 10 + row: 12 column: 10 parent: ~ - kind: YodaConditions: suggestion: age <= YODA location: - row: 11 + row: 13 column: 0 end_location: - row: 11 + row: 13 column: 11 fix: content: age <= YODA location: - row: 11 + row: 13 column: 0 end_location: - row: 11 + row: 13 column: 11 parent: ~ - kind: YodaConditions: suggestion: age == JediOrder.YODA location: - row: 12 + row: 14 column: 0 end_location: - row: 12 + row: 14 column: 21 fix: content: age == JediOrder.YODA location: - row: 12 + row: 14 column: 0 end_location: - row: 12 + row: 14 column: 21 parent: ~ - kind: YodaConditions: suggestion: (number - 100) > 0 location: - row: 13 + row: 15 column: 0 end_location: - row: 13 + row: 15 column: 18 fix: content: (number - 100) > 0 location: - row: 13 + row: 15 column: 0 end_location: - row: 13 + row: 15 column: 18 parent: ~ - kind: YodaConditions: suggestion: (60 * 60) < SomeClass().settings.SOME_CONSTANT_VALUE location: - row: 14 + row: 16 column: 0 end_location: - row: 14 + row: 16 column: 52 fix: content: (60 * 60) < SomeClass().settings.SOME_CONSTANT_VALUE location: - row: 14 + row: 16 column: 0 end_location: - row: 14 + row: 16 column: 52 parent: ~ diff --git a/crates/ruff/src/rules/pylint/rules/magic_value_comparison.rs b/crates/ruff/src/rules/pylint/rules/magic_value_comparison.rs index 0245e2cab7..c1a0f490e7 100644 --- a/crates/ruff/src/rules/pylint/rules/magic_value_comparison.rs +++ b/crates/ruff/src/rules/pylint/rules/magic_value_comparison.rs @@ -1,8 +1,9 @@ use itertools::Itertools; -use ruff_macros::{define_violation, derive_message_formats}; -use rustpython_parser::ast::{Constant, Expr, ExprKind}; +use rustpython_parser::ast::{Constant, Expr, ExprKind, Unaryop}; -use crate::ast::helpers::unparse_constant; +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::helpers::unparse_expr; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::registry::Diagnostic; @@ -24,6 +25,22 @@ impl Violation for MagicValueComparison { } } +/// If an [`Expr`] is a constant (or unary operation on a constant), return the [`Constant`]. +fn as_constant(expr: &Expr) -> Option<&Constant> { + match &expr.node { + ExprKind::Constant { value, .. } => Some(value), + ExprKind::UnaryOp { + op: Unaryop::UAdd | Unaryop::USub | Unaryop::Invert, + operand, + } => match &operand.node { + ExprKind::Constant { value, .. } => Some(value), + _ => None, + }, + _ => None, + } +} + +/// Return `true` if a [`Constant`] is a magic value. fn is_magic_value(constant: &Constant, allowed_types: &[ConstantType]) -> bool { if let Ok(constant_type) = ConstantType::try_from(constant) { if allowed_types.contains(&constant_type) { @@ -37,7 +54,7 @@ fn is_magic_value(constant: &Constant, allowed_types: &[ConstantType]) -> bool { Constant::Ellipsis => false, // Otherwise, special-case some common string and integer types. Constant::Str(value) => !matches!(value.as_str(), "" | "__main__"), - Constant::Int(value) => !matches!(value.try_into(), Ok(-1 | 0 | 1)), + Constant::Int(value) => !matches!(value.try_into(), Ok(0 | 1)), Constant::Bytes(_) => true, Constant::Tuple(_) => true, Constant::Float(_) => true, @@ -53,19 +70,17 @@ pub fn magic_value_comparison(checker: &mut Checker, left: &Expr, comparators: & { // 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 { .. }) - { + if as_constant(left).is_some() && as_constant(right).is_some() { return; } } for comparison_expr in std::iter::once(left).chain(comparators.iter()) { - if let ExprKind::Constant { value, .. } = &comparison_expr.node { + if let Some(value) = as_constant(comparison_expr) { if is_magic_value(value, &checker.settings.pylint.allow_magic_value_types) { checker.diagnostics.push(Diagnostic::new( MagicValueComparison { - value: unparse_constant(value, checker.stylist), + value: unparse_expr(comparison_expr, checker.stylist), }, Range::from_located(comparison_expr), )); diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap index 0e1ee6cb3f..544da52f34 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap @@ -1,5 +1,5 @@ --- -source: src/rules/pylint/mod.rs +source: crates/ruff/src/rules/pylint/mod.rs expression: diagnostics --- - kind: @@ -17,21 +17,43 @@ expression: diagnostics MagicValueComparison: value: "2" location: - row: 35 + row: 38 column: 11 end_location: - row: 35 + row: 38 column: 12 fix: ~ parent: ~ +- kind: + MagicValueComparison: + value: "-2" + location: + row: 41 + column: 11 + end_location: + row: 41 + column: 13 + fix: ~ + parent: ~ +- kind: + MagicValueComparison: + value: "+2" + location: + row: 44 + column: 11 + end_location: + row: 44 + column: 13 + fix: ~ + parent: ~ - kind: MagicValueComparison: value: "3.141592653589793" location: - row: 56 + row: 65 column: 20 end_location: - row: 56 + row: 65 column: 40 fix: ~ parent: ~ diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap index 35b9e39e05..c29d17304c 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap @@ -6,10 +6,10 @@ expression: diagnostics MagicValueComparison: value: "\"Hunter2\"" location: - row: 50 + row: 59 column: 21 end_location: - row: 50 + row: 59 column: 30 fix: ~ parent: ~ @@ -17,10 +17,10 @@ expression: diagnostics MagicValueComparison: value: "3.141592653589793" location: - row: 56 + row: 65 column: 20 end_location: - row: 56 + row: 65 column: 40 fix: ~ parent: ~ @@ -28,10 +28,10 @@ expression: diagnostics MagicValueComparison: value: "b\"something\"" location: - row: 65 + row: 74 column: 17 end_location: - row: 65 + row: 74 column: 29 fix: ~ parent: ~