diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py b/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py new file mode 100644 index 0000000000000..e27adfda6db49 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py @@ -0,0 +1,136 @@ +# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, redefined-outer-name + +value = 10 +value2 = 0 +value3 = 3 + +# Positive +if value < 10: # [max-instead-of-if] + value = 10 + +if value <= 10: # [max-instead-of-if] + value = 10 + +if value < value2: # [max-instead-of-if] + value = value2 + +if value > 10: # [min-instead-of-if] + value = 10 + +if value >= 10: # [min-instead-of-if] + value = 10 + +if value > value2: # [min-instead-of-if] + value = value2 + + +class A: + def __init__(self): + self.value = 13 + + +A1 = A() +if A1.value < 10: # [max-instead-of-if] + A1.value = 10 + +if A1.value > 10: # [min-instead-of-if] + A1.value = 10 + + +class AA: + def __init__(self, value): + self.value = value + + def __gt__(self, b): + return self.value > b + + def __ge__(self, b): + return self.value >= b + + def __lt__(self, b): + return self.value < b + + def __le__(self, b): + return self.value <= b + + +A1 = AA(0) +A2 = AA(3) + +if A2 < A1: # [max-instead-of-if] + A2 = A1 + +if A2 <= A1: # [max-instead-of-if] + A2 = A1 + +if A2 > A1: # [min-instead-of-if] + A2 = A1 + +if A2 >= A1: # [min-instead-of-if] + A2 = A1 + +# Negative +if value < 10: + value = 2 + +if value <= 3: + value = 5 + +if value < 10: + value = 2 + value2 = 3 + +if value < value2: + value = value3 + +if value < 5: + value = value3 + +if 2 < value <= 3: + value = 1 + +if value < 10: + value = 10 +else: + value = 3 + +if value <= 3: + value = 5 +elif value == 3: + value = 2 + +if value > 10: + value = 2 + +if value >= 3: + value = 5 + +if value > 10: + value = 2 + value2 = 3 + +if value > value2: + value = value3 + +if value > 5: + value = value3 + +if 2 > value >= 3: + value = 1 + +if value > 10: + value = 10 +else: + value = 3 + +if value >= 3: + value = 5 +elif value == 3: + value = 2 + +# Parenthesized expressions +if value.attr > 3: + ( + value. + attr + ) = 3 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index cc027dc208dc0..43bbf14b4bbb1 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1117,6 +1117,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::TooManyBooleanExpressions) { pylint::rules::too_many_boolean_expressions(checker, if_); } + if checker.enabled(Rule::IfStmtMinMax) { + pylint::rules::if_stmt_min_max(checker, if_); + } if checker.source_type.is_stub() { if checker.any_enabled(&[ Rule::UnrecognizedVersionInfoCheck, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5e6a0ead3d765..efe2ed63c6057 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -287,6 +287,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), + (Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::IfStmtMinMax), (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), (Pylint, "R2004") => (RuleGroup::Stable, rules::pylint::rules::MagicValueComparison), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 867e6dfc9596b..ed77947eb8b62 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -47,6 +47,7 @@ mod tests { #[test_case(Rule::EqWithoutHash, Path::new("eq_without_hash.py"))] #[test_case(Rule::EmptyComment, Path::new("empty_comment.py"))] #[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))] + #[test_case(Rule::IfStmtMinMax, Path::new("if_stmt_min_max.py"))] #[test_case(Rule::SingleStringSlots, Path::new("single_string_slots.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_0.py"))] #[test_case(Rule::SysExitAlias, Path::new("sys_exit_alias_1.py"))] diff --git a/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs new file mode 100644 index 0000000000000..dfbca11d10aff --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs @@ -0,0 +1,196 @@ +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::parenthesize::parenthesized_range; +use ruff_python_ast::{self as ast, CmpOp, Stmt}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::fix::snippet::SourceCodeSnippet; + +/// ## What it does +/// Checks for `if` statements that can be replaced with `min()` or `max()` +/// calls. +/// +/// ## Why is this bad? +/// An `if` statement that selects the lesser or greater of two sub-expressions +/// can be replaced with a `min()` or `max()` call respectively. When possible, +/// prefer `min()` and `max()`, as they're more concise and readable than the +/// equivalent `if` statements. +/// +/// ## Example +/// ```python +/// if score > highest_score: +/// highest_score = score +/// ``` +/// +/// Use instead: +/// ```python +/// highest_score = max(highest_score, score) +/// ``` +/// +/// ## References +/// - [Python documentation: max function](https://docs.python.org/3/library/functions.html#max) +/// - [Python documentation: min function](https://docs.python.org/3/library/functions.html#min) +#[violation] +pub struct IfStmtMinMax { + min_max: MinMax, + replacement: SourceCodeSnippet, +} + +impl Violation for IfStmtMinMax { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let Self { + min_max, + replacement, + } = self; + if let Some(replacement) = replacement.full_display() { + format!("Replace `if` statement with `{replacement}`") + } else { + format!("Replace `if` statement with `{min_max}` call") + } + } + + fn fix_title(&self) -> Option { + let Self { + min_max, + replacement, + } = self; + if let Some(replacement) = replacement.full_display() { + Some(format!("Replace with `{replacement}`")) + } else { + Some(format!("Replace with `{min_max}` call")) + } + } +} + +/// R1730, R1731 +pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { + let ast::StmtIf { + test, + body, + elif_else_clauses, + range: _, + } = stmt_if; + + if !elif_else_clauses.is_empty() { + return; + } + + let [body @ Stmt::Assign(ast::StmtAssign { + targets: body_targets, + value: body_value, + .. + })] = body.as_slice() + else { + return; + }; + let [body_target] = body_targets.as_slice() else { + return; + }; + + let Some(ast::ExprCompare { + ops, + left, + comparators, + .. + }) = test.as_compare_expr() + else { + return; + }; + + // Ignore, e.g., `foo < bar < baz`. + let [op] = &**ops else { + return; + }; + + // Determine whether to use `min()` or `max()`, and whether to flip the + // order of the arguments, which is relevant for breaking ties. + let (min_max, flip_args) = match op { + CmpOp::Gt => (MinMax::Max, true), + CmpOp::GtE => (MinMax::Max, false), + CmpOp::Lt => (MinMax::Min, true), + CmpOp::LtE => (MinMax::Min, false), + _ => return, + }; + + let [right] = &**comparators else { + return; + }; + + let _min_or_max = match op { + CmpOp::Gt | CmpOp::GtE => MinMax::Min, + CmpOp::Lt | CmpOp::LtE => MinMax::Max, + _ => return, + }; + + let left_cmp = ComparableExpr::from(left); + let body_target_cmp = ComparableExpr::from(body_target); + let right_statement_cmp = ComparableExpr::from(right); + let body_value_cmp = ComparableExpr::from(body_value); + if left_cmp != body_target_cmp || right_statement_cmp != body_value_cmp { + return; + } + + let (arg1, arg2) = if flip_args { + (left.as_ref(), right) + } else { + (right, left.as_ref()) + }; + + let replacement = format!( + "{} = {min_max}({}, {})", + checker.locator().slice( + parenthesized_range( + body_target.into(), + body.into(), + checker.indexer().comment_ranges(), + checker.locator().contents() + ) + .unwrap_or(body_target.range()) + ), + checker.locator().slice(arg1), + checker.locator().slice(arg2), + ); + + let mut diagnostic = Diagnostic::new( + IfStmtMinMax { + min_max, + replacement: SourceCodeSnippet::from_str(replacement.as_str()), + }, + stmt_if.range(), + ); + + if checker.semantic().is_builtin(min_max.as_str()) { + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + replacement, + stmt_if.range(), + ))); + } + + checker.diagnostics.push(diagnostic); +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum MinMax { + Min, + Max, +} + +impl MinMax { + fn as_str(self) -> &'static str { + match self { + Self::Min => "min", + Self::Max => "max", + } + } +} + +impl std::fmt::Display for MinMax { + fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(fmt, "{}", self.as_str()) + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 93f89e1f6cec0..56cb0edd97f9b 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -21,6 +21,7 @@ pub(crate) use eq_without_hash::*; pub(crate) use global_at_module_level::*; pub(crate) use global_statement::*; pub(crate) use global_variable_not_assigned::*; +pub(crate) use if_stmt_min_max::*; pub(crate) use import_outside_top_level::*; pub(crate) use import_private_name::*; pub(crate) use import_self::*; @@ -116,6 +117,7 @@ mod eq_without_hash; mod global_at_module_level; mod global_statement; mod global_variable_not_assigned; +mod if_stmt_min_max; mod import_outside_top_level; mod import_private_name; mod import_self; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap new file mode 100644 index 0000000000000..70ff72b378ae6 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap @@ -0,0 +1,296 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +if_stmt_min_max.py:8:1: PLR1730 [*] Replace `if` statement with `value = min(value, 10)` + | + 7 | # Positive + 8 | / if value < 10: # [max-instead-of-if] + 9 | | value = 10 + | |______________^ PLR1730 +10 | +11 | if value <= 10: # [max-instead-of-if] + | + = help: Replace with `value = min(value, 10)` + +ℹ Safe fix +5 5 | value3 = 3 +6 6 | +7 7 | # Positive +8 |-if value < 10: # [max-instead-of-if] +9 |- value = 10 + 8 |+value = min(value, 10) +10 9 | +11 10 | if value <= 10: # [max-instead-of-if] +12 11 | value = 10 + +if_stmt_min_max.py:11:1: PLR1730 [*] Replace `if` statement with `value = min(10, value)` + | + 9 | value = 10 +10 | +11 | / if value <= 10: # [max-instead-of-if] +12 | | value = 10 + | |______________^ PLR1730 +13 | +14 | if value < value2: # [max-instead-of-if] + | + = help: Replace with `value = min(10, value)` + +ℹ Safe fix +8 8 | if value < 10: # [max-instead-of-if] +9 9 | value = 10 +10 10 | +11 |-if value <= 10: # [max-instead-of-if] +12 |- value = 10 + 11 |+value = min(10, value) +13 12 | +14 13 | if value < value2: # [max-instead-of-if] +15 14 | value = value2 + +if_stmt_min_max.py:14:1: PLR1730 [*] Replace `if` statement with `value = min(value, value2)` + | +12 | value = 10 +13 | +14 | / if value < value2: # [max-instead-of-if] +15 | | value = value2 + | |__________________^ PLR1730 +16 | +17 | if value > 10: # [min-instead-of-if] + | + = help: Replace with `value = min(value, value2)` + +ℹ Safe fix +11 11 | if value <= 10: # [max-instead-of-if] +12 12 | value = 10 +13 13 | +14 |-if value < value2: # [max-instead-of-if] +15 |- value = value2 + 14 |+value = min(value, value2) +16 15 | +17 16 | if value > 10: # [min-instead-of-if] +18 17 | value = 10 + +if_stmt_min_max.py:17:1: PLR1730 [*] Replace `if` statement with `value = max(value, 10)` + | +15 | value = value2 +16 | +17 | / if value > 10: # [min-instead-of-if] +18 | | value = 10 + | |______________^ PLR1730 +19 | +20 | if value >= 10: # [min-instead-of-if] + | + = help: Replace with `value = max(value, 10)` + +ℹ Safe fix +14 14 | if value < value2: # [max-instead-of-if] +15 15 | value = value2 +16 16 | +17 |-if value > 10: # [min-instead-of-if] +18 |- value = 10 + 17 |+value = max(value, 10) +19 18 | +20 19 | if value >= 10: # [min-instead-of-if] +21 20 | value = 10 + +if_stmt_min_max.py:20:1: PLR1730 [*] Replace `if` statement with `value = max(10, value)` + | +18 | value = 10 +19 | +20 | / if value >= 10: # [min-instead-of-if] +21 | | value = 10 + | |______________^ PLR1730 +22 | +23 | if value > value2: # [min-instead-of-if] + | + = help: Replace with `value = max(10, value)` + +ℹ Safe fix +17 17 | if value > 10: # [min-instead-of-if] +18 18 | value = 10 +19 19 | +20 |-if value >= 10: # [min-instead-of-if] +21 |- value = 10 + 20 |+value = max(10, value) +22 21 | +23 22 | if value > value2: # [min-instead-of-if] +24 23 | value = value2 + +if_stmt_min_max.py:23:1: PLR1730 [*] Replace `if` statement with `value = max(value, value2)` + | +21 | value = 10 +22 | +23 | / if value > value2: # [min-instead-of-if] +24 | | value = value2 + | |__________________^ PLR1730 + | + = help: Replace with `value = max(value, value2)` + +ℹ Safe fix +20 20 | if value >= 10: # [min-instead-of-if] +21 21 | value = 10 +22 22 | +23 |-if value > value2: # [min-instead-of-if] +24 |- value = value2 + 23 |+value = max(value, value2) +25 24 | +26 25 | +27 26 | class A: + +if_stmt_min_max.py:33:1: PLR1730 [*] Replace `if` statement with `A1.value = min(A1.value, 10)` + | +32 | A1 = A() +33 | / if A1.value < 10: # [max-instead-of-if] +34 | | A1.value = 10 + | |_________________^ PLR1730 +35 | +36 | if A1.value > 10: # [min-instead-of-if] + | + = help: Replace with `A1.value = min(A1.value, 10)` + +ℹ Safe fix +30 30 | +31 31 | +32 32 | A1 = A() +33 |-if A1.value < 10: # [max-instead-of-if] +34 |- A1.value = 10 + 33 |+A1.value = min(A1.value, 10) +35 34 | +36 35 | if A1.value > 10: # [min-instead-of-if] +37 36 | A1.value = 10 + +if_stmt_min_max.py:36:1: PLR1730 [*] Replace `if` statement with `A1.value = max(A1.value, 10)` + | +34 | A1.value = 10 +35 | +36 | / if A1.value > 10: # [min-instead-of-if] +37 | | A1.value = 10 + | |_________________^ PLR1730 + | + = help: Replace with `A1.value = max(A1.value, 10)` + +ℹ Safe fix +33 33 | if A1.value < 10: # [max-instead-of-if] +34 34 | A1.value = 10 +35 35 | +36 |-if A1.value > 10: # [min-instead-of-if] +37 |- A1.value = 10 + 36 |+A1.value = max(A1.value, 10) +38 37 | +39 38 | +40 39 | class AA: + +if_stmt_min_max.py:60:1: PLR1730 [*] Replace `if` statement with `A2 = min(A2, A1)` + | +58 | A2 = AA(3) +59 | +60 | / if A2 < A1: # [max-instead-of-if] +61 | | A2 = A1 + | |___________^ PLR1730 +62 | +63 | if A2 <= A1: # [max-instead-of-if] + | + = help: Replace with `A2 = min(A2, A1)` + +ℹ Safe fix +57 57 | A1 = AA(0) +58 58 | A2 = AA(3) +59 59 | +60 |-if A2 < A1: # [max-instead-of-if] +61 |- A2 = A1 + 60 |+A2 = min(A2, A1) +62 61 | +63 62 | if A2 <= A1: # [max-instead-of-if] +64 63 | A2 = A1 + +if_stmt_min_max.py:63:1: PLR1730 [*] Replace `if` statement with `A2 = min(A1, A2)` + | +61 | A2 = A1 +62 | +63 | / if A2 <= A1: # [max-instead-of-if] +64 | | A2 = A1 + | |___________^ PLR1730 +65 | +66 | if A2 > A1: # [min-instead-of-if] + | + = help: Replace with `A2 = min(A1, A2)` + +ℹ Safe fix +60 60 | if A2 < A1: # [max-instead-of-if] +61 61 | A2 = A1 +62 62 | +63 |-if A2 <= A1: # [max-instead-of-if] +64 |- A2 = A1 + 63 |+A2 = min(A1, A2) +65 64 | +66 65 | if A2 > A1: # [min-instead-of-if] +67 66 | A2 = A1 + +if_stmt_min_max.py:66:1: PLR1730 [*] Replace `if` statement with `A2 = max(A2, A1)` + | +64 | A2 = A1 +65 | +66 | / if A2 > A1: # [min-instead-of-if] +67 | | A2 = A1 + | |___________^ PLR1730 +68 | +69 | if A2 >= A1: # [min-instead-of-if] + | + = help: Replace with `A2 = max(A2, A1)` + +ℹ Safe fix +63 63 | if A2 <= A1: # [max-instead-of-if] +64 64 | A2 = A1 +65 65 | +66 |-if A2 > A1: # [min-instead-of-if] +67 |- A2 = A1 + 66 |+A2 = max(A2, A1) +68 67 | +69 68 | if A2 >= A1: # [min-instead-of-if] +70 69 | A2 = A1 + +if_stmt_min_max.py:69:1: PLR1730 [*] Replace `if` statement with `A2 = max(A1, A2)` + | +67 | A2 = A1 +68 | +69 | / if A2 >= A1: # [min-instead-of-if] +70 | | A2 = A1 + | |___________^ PLR1730 +71 | +72 | # Negative + | + = help: Replace with `A2 = max(A1, A2)` + +ℹ Safe fix +66 66 | if A2 > A1: # [min-instead-of-if] +67 67 | A2 = A1 +68 68 | +69 |-if A2 >= A1: # [min-instead-of-if] +70 |- A2 = A1 + 69 |+A2 = max(A1, A2) +71 70 | +72 71 | # Negative +73 72 | if value < 10: + +if_stmt_min_max.py:132:1: PLR1730 [*] Replace `if` statement with `max` call + | +131 | # Parenthesized expressions +132 | / if value.attr > 3: +133 | | ( +134 | | value. +135 | | attr +136 | | ) = 3 + | |_________^ PLR1730 + | + = help: Replace with `max` call + +ℹ Safe fix +129 129 | value = 2 +130 130 | +131 131 | # Parenthesized expressions +132 |-if value.attr > 3: +133 |- ( + 132 |+( +134 133 | value. +135 134 | attr +136 |- ) = 3 + 135 |+ ) = max(value.attr, 3) diff --git a/ruff.schema.json b/ruff.schema.json index ea85c0ce15c40..bc00be8a327bb 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3355,6 +3355,7 @@ "PLR172", "PLR1722", "PLR173", + "PLR1730", "PLR1733", "PLR1736", "PLR2",