From cf2b7d81f385215c85800827c2f08b36cf380436 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Sun, 11 Feb 2024 20:43:27 +0100 Subject: [PATCH 1/9] Implement max part --- .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../pylint/rules/max_min_instead_of_if.rs | 214 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + 4 files changed, 220 insertions(+) create mode 100644 crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 37bc90fcaca9d..759c19192e1cf 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1130,6 +1130,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::MaxInsteadOfIf) { + pylint::rules::max_instead_of_if(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 8e275cc3712e0..467f52608ab21 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -278,6 +278,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), + (Pylint, "R1731") => (RuleGroup::Preview, rules::pylint::rules::MaxInsteadOfIf), (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/rules/max_min_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs new file mode 100644 index 0000000000000..aeef48d164a7f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs @@ -0,0 +1,214 @@ +use std::ops::Deref; + +use ast::LiteralExpressionRef; +use ruff_diagnostics::{Diagnostic, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Arguments, CmpOp, Expr, ExprAttribute, ExprContext, Stmt}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Check for `if`-`else`-blocks that can be replaced with a ternary operator. +/// +/// ## Why is this bad? +/// `if`-`else`-blocks that assign a value to a variable in both branches can +/// be expressed more concisely by using a ternary operator. +/// +/// ## Example +/// ```python +/// if foo: +/// bar = x +/// else: +/// bar = y +/// ``` +/// +/// Use instead: +/// ```python +/// bar = x if foo else y +/// ``` +/// +/// ## References +/// - [Python documentation: Conditional expressions](https://docs.python.org/3/reference/expressions.html#conditional-expressions) +#[violation] +pub struct MaxInsteadOfIf { + contents: String, +} + +impl Violation for MaxInsteadOfIf { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let MaxInsteadOfIf { contents } = self; + format!("Consider using `{contents}` instead of unnecessary if block") + } +} + +#[violation] +pub struct MinInsteadOfIf { + contents: String, +} + +impl Violation for MinInsteadOfIf { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + let MinInsteadOfIf { contents } = self; + format!("Use min instead of if `{contents}`") + } +} + +/// R1731 +pub(crate) fn max_instead_of_if(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 [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; + }; + + if !( + !body_target.is_subscript_expr() + && !left.is_subscript_expr() + ) {return;} + + let ([op], [right_statement]) = (ops.as_slice(), comparators.as_slice()) else { + return; + }; + + if !matches!(op, CmpOp::Lt | CmpOp::LtE) {return;} + if !match_left(left, body_target) {return;} + if !match_right(right_statement, body_value) {return;} + + let func_node = ast::ExprName { + id: "max".into(), + ctx: ExprContext::Load, + range: TextRange::default(), + }; + let value_node = ast::ExprCall { + func: Box::new(func_node.into()), + arguments: Arguments { + args: vec![body_target.clone(), body_value.deref().clone()], + keywords: vec![], + range: TextRange::default(), + }, + range: TextRange::default(), + }; + /*let expr_node = ast::StmtExpr { + value: Box::new(value_node.into()), + range: TextRange::default(), + };*/ + let assign_node = ast::StmtAssign { + targets: vec![body_target.clone()], + value: Box::new(value_node.into()), + range: TextRange::default(), + }; + + let diagnostic = Diagnostic::new( + MaxInsteadOfIf { + contents: checker.generator().stmt(&assign_node.into()), + }, + stmt_if.range(), + ); + checker.diagnostics.push(diagnostic); +} + +fn match_left(left: &Expr, body_target: &Expr) -> bool { + // Check that assignment is on the same variable + if left.is_name_expr() && body_target.is_name_expr() { + let Some(left_operand) = left.as_name_expr() else {return false}; + let Some(target_assignation) = body_target.as_name_expr() else {return false}; + return left_operand.id == target_assignation.id + } + + if left.is_attribute_expr() && body_target.is_attribute_expr() { + let Some(left_operand) = left.as_attribute_expr() else {return false}; + let Some(target_assignation) = body_target.as_attribute_expr() else {return false}; + return match_attributes(left_operand, target_assignation) + } + + false +} + +fn match_right(right_statement: &Expr, body_value: &Box) -> bool { + // Verify the right part of the statement is the same. + if right_statement.is_name_expr() && body_value.is_name_expr() { + let Some(right_statement_value) = right_statement.as_name_expr() else {return false}; + let Some(body_value_value) = body_value.as_name_expr() else {return false}; + return right_statement_value.id == body_value_value.id + } + if right_statement.is_literal_expr() && body_value.is_literal_expr() { + let Some(right_statement_value) = right_statement.as_literal_expr() else {return false}; + let Some(body_value_value) = body_value.as_literal_expr() else {return false}; + match (right_statement_value, body_value_value) { + ( + LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value1, ..}), + LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value2, ..}) + ) => { + return value1.iter().map(|b| b.value.as_slice()).eq(value2.iter().map(|b| b.value.as_slice())) + }, + ( + LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral{value: value1, ..}), + LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral{value: value2, ..}) + ) => { + return value1.to_str() == value2.to_str() + }, + ( + LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral{value: value1, ..}), + LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral{value: value2, ..}) + ) => { + return value1 == value2 + }, + (_, _) => {return false} + } + } + false +} + +fn match_attributes(expr1: &ExprAttribute, expr2: &ExprAttribute) -> bool { + if expr1.attr.as_str() != expr2.attr.as_str() {return false} + + if expr1.value.is_name_expr() && expr2.value.is_name_expr() { + let Some(ast::ExprName { + id: id1, + .. + }) = expr1.value.as_name_expr() else {return false}; + let Some(ast::ExprName { + id: id2, + .. + }) = expr2.value.as_name_expr() else {return false}; + return id1 == id2 + } + + if expr1.value.is_attribute_expr() && expr2.value.is_attribute_expr() { + let Some(expr1) = expr1.value.as_attribute_expr() else {return false;}; + let Some(expr2) = expr2.value.as_attribute_expr() else {return false;}; + return match_attributes(expr1, expr2) + } + + false +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 3ba5d1061b7d2..6ff01a55087fc 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -34,6 +34,7 @@ pub(crate) use load_before_global_declaration::*; pub(crate) use logging::*; pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; +pub(crate) use max_min_instead_of_if::*; pub(crate) use misplaced_bare_raise::*; pub(crate) use named_expr_without_context::*; pub(crate) use nested_min_max::*; @@ -97,6 +98,7 @@ mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; +mod max_min_instead_of_if; mod continue_in_finally; mod duplicate_bases; mod empty_comment; From 16f897182f2f849fc43639b39fc14a720dd059c4 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Tue, 13 Feb 2024 21:45:29 +0100 Subject: [PATCH 2/9] Add tests --- .../test/fixtures/pylint/max_instead_of_if.py | 82 +++++++++++++++++++ crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../pylint/rules/max_min_instead_of_if.rs | 8 +- ...__tests__PLR1731_max_instead_of_if.py.snap | 64 +++++++++++++++ ruff.schema.json | 1 + 5 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py new file mode 100644 index 0000000000000..189c8d6284324 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py @@ -0,0 +1,82 @@ +# 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 + + +class A: + def __init__(self): + self.value = 13 + + +A1 = A() +if A1.value < 10: # [max-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 + +# 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 diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 518dd781459d5..619d0beb3040b 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -42,6 +42,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::MaxInsteadOfIf, Path::new("max_instead_of_if.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/max_min_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs index aeef48d164a7f..eafbef9533e52 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs @@ -96,7 +96,7 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { && !left.is_subscript_expr() ) {return;} - let ([op], [right_statement]) = (ops.as_slice(), comparators.as_slice()) else { + let ([op], [right_statement]) = (&**ops, &**comparators) else { return; }; @@ -112,8 +112,8 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { let value_node = ast::ExprCall { func: Box::new(func_node.into()), arguments: Arguments { - args: vec![body_target.clone(), body_value.deref().clone()], - keywords: vec![], + args: Box::from([body_target.clone(), body_value.deref().clone()]), + keywords: Box::from([]), range: TextRange::default(), }, range: TextRange::default(), @@ -169,7 +169,7 @@ fn match_right(right_statement: &Expr, body_value: &Box) -> bool { LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value1, ..}), LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value2, ..}) ) => { - return value1.iter().map(|b| b.value.as_slice()).eq(value2.iter().map(|b| b.value.as_slice())) + return value1.iter().map(|b| b.as_slice()).eq(value2.iter().map(|b| b.as_slice())) }, ( LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral{value: value1, ..}), diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap new file mode 100644 index 0000000000000..9a0719165fee3 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap @@ -0,0 +1,64 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +max_instead_of_if.py:8:1: PLR1731 Consider using `value = max(value, 10)` instead of unnecessary if block + | + 7 | # Positive + 8 | / if value < 10: # [max-instead-of-if] + 9 | | value = 10 + | |______________^ PLR1731 +10 | +11 | if value <= 10: # [max-instead-of-if] + | + +max_instead_of_if.py:11:1: PLR1731 Consider using `value = max(value, 10)` instead of unnecessary if block + | + 9 | value = 10 +10 | +11 | / if value <= 10: # [max-instead-of-if] +12 | | value = 10 + | |______________^ PLR1731 +13 | +14 | if value < value2: # [max-instead-of-if] + | + +max_instead_of_if.py:14:1: PLR1731 Consider using `value = max(value, value2)` instead of unnecessary if block + | +12 | value = 10 +13 | +14 | / if value < value2: # [max-instead-of-if] +15 | | value = value2 + | |__________________^ PLR1731 + | + +max_instead_of_if.py:24:1: PLR1731 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block + | +23 | A1 = A() +24 | / if A1.value < 10: # [max-instead-of-if] +25 | | A1.value = 10 + | |_________________^ PLR1731 + | + +max_instead_of_if.py:48:1: PLR1731 Consider using `A2 = max(A2, A1)` instead of unnecessary if block + | +46 | A2 = AA(3) +47 | +48 | / if A2 < A1: # [max-instead-of-if] +49 | | A2 = A1 + | |___________^ PLR1731 +50 | +51 | if A2 <= A1: # [max-instead-of-if] + | + +max_instead_of_if.py:51:1: PLR1731 Consider using `A2 = max(A2, A1)` instead of unnecessary if block + | +49 | A2 = A1 +50 | +51 | / if A2 <= A1: # [max-instead-of-if] +52 | | A2 = A1 + | |___________^ PLR1731 +53 | +54 | # Negative + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 369c973256cc5..75796544a51eb 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3281,6 +3281,7 @@ "PLR172", "PLR1722", "PLR173", + "PLR1731", "PLR1733", "PLR1736", "PLR2", From df8f755d00652fd5b73237fe23cfdd89a546dcb5 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Thu, 15 Feb 2024 09:47:08 +0100 Subject: [PATCH 3/9] Add min part --- .../test/fixtures/pylint/min_instead_of_if.py | 82 ++++++++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../pylint/rules/max_min_instead_of_if.rs | 125 +++++++++++++++--- ...__tests__PLR1730_min_instead_of_if.py.snap | 64 +++++++++ ruff.schema.json | 1 + 7 files changed, 255 insertions(+), 22 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py new file mode 100644 index 0000000000000..5bb9e54c07391 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py @@ -0,0 +1,82 @@ +# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, redefined-outer-name + +value = 10 +value2 = 0 +value3 = 3 + +# Positive +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: # [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: # [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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 759c19192e1cf..ca2b49bbb555d 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1133,6 +1133,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::MaxInsteadOfIf) { pylint::rules::max_instead_of_if(checker, if_); } + if checker.enabled(Rule::MinInsteadOfIf) { + pylint::rules::min_instead_of_if(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 467f52608ab21..b872e84ae4daa 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -278,6 +278,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), + (Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::MinInsteadOfIf), (Pylint, "R1731") => (RuleGroup::Preview, rules::pylint::rules::MaxInsteadOfIf), (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 619d0beb3040b..2db07973247d3 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -43,6 +43,7 @@ mod tests { #[test_case(Rule::EmptyComment, Path::new("empty_comment.py"))] #[test_case(Rule::ManualFromImport, Path::new("import_aliasing.py"))] #[test_case(Rule::MaxInsteadOfIf, Path::new("max_instead_of_if.py"))] + #[test_case(Rule::MinInsteadOfIf, Path::new("min_instead_of_if.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/max_min_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs index eafbef9533e52..fcd1957e11a0a 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs @@ -1,7 +1,7 @@ use std::ops::Deref; use ast::LiteralExpressionRef; -use ruff_diagnostics::{Diagnostic, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{self as ast, Arguments, CmpOp, Expr, ExprAttribute, ExprContext, Stmt}; use ruff_text_size::{Ranged, TextRange}; @@ -9,35 +9,31 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; /// ## What it does -/// Check for `if`-`else`-blocks that can be replaced with a ternary operator. +/// Check for an if node that can be refactored as a max python builtin. /// /// ## Why is this bad? -/// `if`-`else`-blocks that assign a value to a variable in both branches can -/// be expressed more concisely by using a ternary operator. +/// An if block where the test and assignment have the same structure can +/// be expressed more concisely by using the python builtin max function. /// /// ## Example /// ```python -/// if foo: -/// bar = x -/// else: -/// bar = y +/// if value < 10: +/// value = 10 /// ``` /// /// Use instead: /// ```python -/// bar = x if foo else y +/// value = max(value, 10) /// ``` /// /// ## References -/// - [Python documentation: Conditional expressions](https://docs.python.org/3/reference/expressions.html#conditional-expressions) +/// - [Python documentation: max function](https://docs.python.org/3/library/functions.html#max) #[violation] pub struct MaxInsteadOfIf { contents: String, } impl Violation for MaxInsteadOfIf { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; - #[derive_message_formats] fn message(&self) -> String { let MaxInsteadOfIf { contents } = self; @@ -45,21 +41,111 @@ impl Violation for MaxInsteadOfIf { } } +/// ## What it does +/// Check for an if node that can be refactored as a min python builtin. +/// +/// ## Why is this bad? +/// An if block where the test and assignment have the same structure can +/// be expressed more concisely by using the python builtin min function. +/// +/// ## Example +/// ```python +/// if value > 10: +/// value = 10 +/// ``` +/// +/// Use instead: +/// ```python +/// value = min(value, 10) +/// ``` +/// +/// ## References +/// - [Python documentation: min function](https://docs.python.org/3/library/functions.html#min) #[violation] pub struct MinInsteadOfIf { contents: String, } impl Violation for MinInsteadOfIf { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; - #[derive_message_formats] fn message(&self) -> String { let MinInsteadOfIf { contents } = self; - format!("Use min instead of if `{contents}`") + format!("Consider using `{contents}` instead of unnecessary if block") } } +/// R1730 +pub(crate) fn min_instead_of_if(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 [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; + }; + + if !( + !body_target.is_subscript_expr() + && !left.is_subscript_expr() + ) {return;} + + let ([op], [right_statement]) = (&**ops, &**comparators) else { + return; + }; + + if !matches!(op, CmpOp::Gt | CmpOp::GtE) {return;} + if !match_left(left, body_target) {return;} + if !match_right(right_statement, body_value) {return;} + + let func_node = ast::ExprName { + id: "min".into(), + ctx: ExprContext::Load, + range: TextRange::default(), + }; + let value_node = ast::ExprCall { + func: Box::new(func_node.into()), + arguments: Arguments { + args: Box::from([body_target.clone(), body_value.deref().clone()]), + keywords: Box::from([]), + range: TextRange::default(), + }, + range: TextRange::default(), + }; + let assign_node = ast::StmtAssign { + targets: vec![body_target.clone()], + value: Box::new(value_node.into()), + range: TextRange::default(), + }; + let diagnostic = Diagnostic::new( + MinInsteadOfIf { + contents: checker.generator().stmt(&assign_node.into()), + }, + stmt_if.range(), + ); + checker.diagnostics.push(diagnostic); +} + /// R1731 pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { let ast::StmtIf { @@ -118,16 +204,11 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { }, range: TextRange::default(), }; - /*let expr_node = ast::StmtExpr { - value: Box::new(value_node.into()), - range: TextRange::default(), - };*/ let assign_node = ast::StmtAssign { targets: vec![body_target.clone()], value: Box::new(value_node.into()), range: TextRange::default(), }; - let diagnostic = Diagnostic::new( MaxInsteadOfIf { contents: checker.generator().stmt(&assign_node.into()), @@ -138,7 +219,7 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { } fn match_left(left: &Expr, body_target: &Expr) -> bool { - // Check that assignment is on the same variable + // Check that the assignments are on the same variable if left.is_name_expr() && body_target.is_name_expr() { let Some(left_operand) = left.as_name_expr() else {return false}; let Some(target_assignation) = body_target.as_name_expr() else {return false}; @@ -155,7 +236,7 @@ fn match_left(left: &Expr, body_target: &Expr) -> bool { } fn match_right(right_statement: &Expr, body_value: &Box) -> bool { - // Verify the right part of the statement is the same. + // Verify that the right part of the statements are the same. if right_statement.is_name_expr() && body_value.is_name_expr() { let Some(right_statement_value) = right_statement.as_name_expr() else {return false}; let Some(body_value_value) = body_value.as_name_expr() else {return false}; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap new file mode 100644 index 0000000000000..acbe38a32c434 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap @@ -0,0 +1,64 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +min_instead_of_if.py:8:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block + | + 7 | # Positive + 8 | / if value > 10: # [min-instead-of-if] + 9 | | value = 10 + | |______________^ PLR1730 +10 | +11 | if value >= 10: # [min-instead-of-if] + | + +min_instead_of_if.py:11:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block + | + 9 | value = 10 +10 | +11 | / if value >= 10: # [min-instead-of-if] +12 | | value = 10 + | |______________^ PLR1730 +13 | +14 | if value > value2: # [min-instead-of-if] + | + +min_instead_of_if.py:14:1: PLR1730 Consider using `value = min(value, value2)` instead of unnecessary if block + | +12 | value = 10 +13 | +14 | / if value > value2: # [min-instead-of-if] +15 | | value = value2 + | |__________________^ PLR1730 + | + +min_instead_of_if.py:24:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block + | +23 | A1 = A() +24 | / if A1.value > 10: # [min-instead-of-if] +25 | | A1.value = 10 + | |_________________^ PLR1730 + | + +min_instead_of_if.py:48:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block + | +46 | A2 = AA(3) +47 | +48 | / if A2 > A1: # [min-instead-of-if] +49 | | A2 = A1 + | |___________^ PLR1730 +50 | +51 | if A2 >= A1: # [min-instead-of-if] + | + +min_instead_of_if.py:51:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block + | +49 | A2 = A1 +50 | +51 | / if A2 >= A1: # [min-instead-of-if] +52 | | A2 = A1 + | |___________^ PLR1730 +53 | +54 | # Negative + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 75796544a51eb..584e4d0064592 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3281,6 +3281,7 @@ "PLR172", "PLR1722", "PLR173", + "PLR1730", "PLR1731", "PLR1733", "PLR1736", From fdd51a47049f910c560ee4b2c726cbc77dfd8e57 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Thu, 15 Feb 2024 16:38:52 +0100 Subject: [PATCH 4/9] Format and lint --- .../pylint/rules/max_min_instead_of_if.rs | 187 +++++++++++------- .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 +- 2 files changed, 117 insertions(+), 72 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs index fcd1957e11a0a..5f1ba3dfd4e11 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs @@ -82,41 +82,50 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { elif_else_clauses, range: _, } = stmt_if; - - if !elif_else_clauses.is_empty() {return;} - + + if !elif_else_clauses.is_empty() { + return; + } + let [Stmt::Assign(ast::StmtAssign { targets: body_targets, value: body_value, .. - })] = body.as_slice() else { + })] = 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 { + }) = test.as_compare_expr() + else { return; }; - - if !( - !body_target.is_subscript_expr() - && !left.is_subscript_expr() - ) {return;} + + if !(!body_target.is_subscript_expr() && !left.is_subscript_expr()) { + return; + } let ([op], [right_statement]) = (&**ops, &**comparators) else { return; }; - if !matches!(op, CmpOp::Gt | CmpOp::GtE) {return;} - if !match_left(left, body_target) {return;} - if !match_right(right_statement, body_value) {return;} + if !matches!(op, CmpOp::Gt | CmpOp::GtE) { + return; + } + if !match_left(left, body_target) { + return; + } + if !match_right(right_statement, body_value) { + return; + } let func_node = ast::ExprName { id: "min".into(), @@ -154,41 +163,50 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { elif_else_clauses, range: _, } = stmt_if; - - if !elif_else_clauses.is_empty() {return;} - + + if !elif_else_clauses.is_empty() { + return; + } + let [Stmt::Assign(ast::StmtAssign { targets: body_targets, value: body_value, .. - })] = body.as_slice() else { + })] = 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 { + }) = test.as_compare_expr() + else { return; }; - - if !( - !body_target.is_subscript_expr() - && !left.is_subscript_expr() - ) {return;} + + if !(!body_target.is_subscript_expr() && !left.is_subscript_expr()) { + return; + } let ([op], [right_statement]) = (&**ops, &**comparators) else { return; }; - if !matches!(op, CmpOp::Lt | CmpOp::LtE) {return;} - if !match_left(left, body_target) {return;} - if !match_right(right_statement, body_value) {return;} + if !matches!(op, CmpOp::Lt | CmpOp::LtE) { + return; + } + if !match_left(left, body_target) { + return; + } + if !match_right(right_statement, body_value) { + return; + } let func_node = ast::ExprName { id: "max".into(), @@ -221,75 +239,102 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { fn match_left(left: &Expr, body_target: &Expr) -> bool { // Check that the assignments are on the same variable if left.is_name_expr() && body_target.is_name_expr() { - let Some(left_operand) = left.as_name_expr() else {return false}; - let Some(target_assignation) = body_target.as_name_expr() else {return false}; - return left_operand.id == target_assignation.id + let Some(left_operand) = left.as_name_expr() else { + return false; + }; + let Some(target_assignation) = body_target.as_name_expr() else { + return false; + }; + return left_operand.id == target_assignation.id; } - + if left.is_attribute_expr() && body_target.is_attribute_expr() { - let Some(left_operand) = left.as_attribute_expr() else {return false}; - let Some(target_assignation) = body_target.as_attribute_expr() else {return false}; - return match_attributes(left_operand, target_assignation) + let Some(left_operand) = left.as_attribute_expr() else { + return false; + }; + let Some(target_assignation) = body_target.as_attribute_expr() else { + return false; + }; + return match_attributes(left_operand, target_assignation); } false } -fn match_right(right_statement: &Expr, body_value: &Box) -> bool { +fn match_right(right_statement: &Expr, body_value: &Expr) -> bool { // Verify that the right part of the statements are the same. if right_statement.is_name_expr() && body_value.is_name_expr() { - let Some(right_statement_value) = right_statement.as_name_expr() else {return false}; - let Some(body_value_value) = body_value.as_name_expr() else {return false}; - return right_statement_value.id == body_value_value.id + let Some(right_statement_value) = right_statement.as_name_expr() else { + return false; + }; + let Some(body_value_value) = body_value.as_name_expr() else { + return false; + }; + return right_statement_value.id == body_value_value.id; } if right_statement.is_literal_expr() && body_value.is_literal_expr() { - let Some(right_statement_value) = right_statement.as_literal_expr() else {return false}; - let Some(body_value_value) = body_value.as_literal_expr() else {return false}; + let Some(right_statement_value) = right_statement.as_literal_expr() else { + return false; + }; + let Some(body_value_value) = body_value.as_literal_expr() else { + return false; + }; match (right_statement_value, body_value_value) { ( - LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value1, ..}), - LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral{value: value2, ..}) + LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral { value: value1, .. }), + LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral { value: value2, .. }), ) => { - return value1.iter().map(|b| b.as_slice()).eq(value2.iter().map(|b| b.as_slice())) - }, + return value1 + .iter() + .map(ruff_python_ast::BytesLiteral::as_slice) + .eq(value2.iter().map(ruff_python_ast::BytesLiteral::as_slice)) + } ( - LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral{value: value1, ..}), - LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral{value: value2, ..}) - ) => { - return value1.to_str() == value2.to_str() - }, + LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral { + value: value1, .. + }), + LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral { + value: value2, .. + }), + ) => return value1.to_str() == value2.to_str(), ( - LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral{value: value1, ..}), - LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral{value: value2, ..}) - ) => { - return value1 == value2 - }, - (_, _) => {return false} + LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { + value: value1, .. + }), + LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { + value: value2, .. + }), + ) => return value1 == value2, + (_, _) => return false, } } false } fn match_attributes(expr1: &ExprAttribute, expr2: &ExprAttribute) -> bool { - if expr1.attr.as_str() != expr2.attr.as_str() {return false} - + if expr1.attr.as_str() != expr2.attr.as_str() { + return false; + } + if expr1.value.is_name_expr() && expr2.value.is_name_expr() { - let Some(ast::ExprName { - id: id1, - .. - }) = expr1.value.as_name_expr() else {return false}; - let Some(ast::ExprName { - id: id2, - .. - }) = expr2.value.as_name_expr() else {return false}; - return id1 == id2 + let Some(ast::ExprName { id: id1, .. }) = expr1.value.as_name_expr() else { + return false; + }; + let Some(ast::ExprName { id: id2, .. }) = expr2.value.as_name_expr() else { + return false; + }; + return id1 == id2; } if expr1.value.is_attribute_expr() && expr2.value.is_attribute_expr() { - let Some(expr1) = expr1.value.as_attribute_expr() else {return false;}; - let Some(expr2) = expr2.value.as_attribute_expr() else {return false;}; - return match_attributes(expr1, expr2) + let Some(expr1) = expr1.value.as_attribute_expr() else { + return false; + }; + let Some(expr2) = expr2.value.as_attribute_expr() else { + return false; + }; + return match_attributes(expr1, expr2); } - + false } diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 6ff01a55087fc..d3d0e086462a3 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -98,7 +98,6 @@ mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; -mod max_min_instead_of_if; mod continue_in_finally; mod duplicate_bases; mod empty_comment; @@ -121,6 +120,7 @@ mod load_before_global_declaration; mod logging; mod magic_value_comparison; mod manual_import_from; +mod max_min_instead_of_if; mod misplaced_bare_raise; mod named_expr_without_context; mod nested_min_max; From 595ec4ca1177d2ca49ece928c1956ef79b7dbb34 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Sat, 17 Feb 2024 21:21:00 +0100 Subject: [PATCH 5/9] Use ComparableExpr to get rid of unnecessary code --- ...tead_of_if.rs => min_max_instead_of_if.rs} | 127 ++---------------- .../ruff_linter/src/rules/pylint/rules/mod.rs | 4 +- 2 files changed, 16 insertions(+), 115 deletions(-) rename crates/ruff_linter/src/rules/pylint/rules/{max_min_instead_of_if.rs => min_max_instead_of_if.rs} (57%) diff --git a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs similarity index 57% rename from crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs rename to crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs index 5f1ba3dfd4e11..d420f11c62590 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/max_min_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs @@ -1,9 +1,9 @@ use std::ops::Deref; -use ast::LiteralExpressionRef; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, Arguments, CmpOp, Expr, ExprAttribute, ExprContext, Stmt}; +use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::{self as ast, Arguments, CmpOp, ExprContext, Stmt}; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -120,10 +120,12 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { if !matches!(op, CmpOp::Gt | CmpOp::GtE) { return; } - if !match_left(left, body_target) { - return; - } - if !match_right(right_statement, body_value) { + + let left_cmp = ComparableExpr::from(left); + let body_target_cmp = ComparableExpr::from(body_target); + let right_statement_cmp = ComparableExpr::from(right_statement); + let body_value_cmp = ComparableExpr::from(body_value); + if left_cmp != body_target_cmp || right_statement_cmp != body_value_cmp { return; } @@ -201,10 +203,12 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { if !matches!(op, CmpOp::Lt | CmpOp::LtE) { return; } - if !match_left(left, body_target) { - return; - } - if !match_right(right_statement, body_value) { + + let left_cmp = ComparableExpr::from(left); + let body_target_cmp = ComparableExpr::from(body_target); + let right_statement_cmp = ComparableExpr::from(right_statement); + let body_value_cmp = ComparableExpr::from(body_value); + if left_cmp != body_target_cmp || right_statement_cmp != body_value_cmp { return; } @@ -235,106 +239,3 @@ pub(crate) fn max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { ); checker.diagnostics.push(diagnostic); } - -fn match_left(left: &Expr, body_target: &Expr) -> bool { - // Check that the assignments are on the same variable - if left.is_name_expr() && body_target.is_name_expr() { - let Some(left_operand) = left.as_name_expr() else { - return false; - }; - let Some(target_assignation) = body_target.as_name_expr() else { - return false; - }; - return left_operand.id == target_assignation.id; - } - - if left.is_attribute_expr() && body_target.is_attribute_expr() { - let Some(left_operand) = left.as_attribute_expr() else { - return false; - }; - let Some(target_assignation) = body_target.as_attribute_expr() else { - return false; - }; - return match_attributes(left_operand, target_assignation); - } - - false -} - -fn match_right(right_statement: &Expr, body_value: &Expr) -> bool { - // Verify that the right part of the statements are the same. - if right_statement.is_name_expr() && body_value.is_name_expr() { - let Some(right_statement_value) = right_statement.as_name_expr() else { - return false; - }; - let Some(body_value_value) = body_value.as_name_expr() else { - return false; - }; - return right_statement_value.id == body_value_value.id; - } - if right_statement.is_literal_expr() && body_value.is_literal_expr() { - let Some(right_statement_value) = right_statement.as_literal_expr() else { - return false; - }; - let Some(body_value_value) = body_value.as_literal_expr() else { - return false; - }; - match (right_statement_value, body_value_value) { - ( - LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral { value: value1, .. }), - LiteralExpressionRef::BytesLiteral(ast::ExprBytesLiteral { value: value2, .. }), - ) => { - return value1 - .iter() - .map(ruff_python_ast::BytesLiteral::as_slice) - .eq(value2.iter().map(ruff_python_ast::BytesLiteral::as_slice)) - } - ( - LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral { - value: value1, .. - }), - LiteralExpressionRef::StringLiteral(ast::ExprStringLiteral { - value: value2, .. - }), - ) => return value1.to_str() == value2.to_str(), - ( - LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { - value: value1, .. - }), - LiteralExpressionRef::NumberLiteral(ast::ExprNumberLiteral { - value: value2, .. - }), - ) => return value1 == value2, - (_, _) => return false, - } - } - false -} - -fn match_attributes(expr1: &ExprAttribute, expr2: &ExprAttribute) -> bool { - if expr1.attr.as_str() != expr2.attr.as_str() { - return false; - } - - if expr1.value.is_name_expr() && expr2.value.is_name_expr() { - let Some(ast::ExprName { id: id1, .. }) = expr1.value.as_name_expr() else { - return false; - }; - let Some(ast::ExprName { id: id2, .. }) = expr2.value.as_name_expr() else { - return false; - }; - return id1 == id2; - } - - if expr1.value.is_attribute_expr() && expr2.value.is_attribute_expr() { - let Some(expr1) = expr1.value.as_attribute_expr() else { - return false; - }; - let Some(expr2) = expr2.value.as_attribute_expr() else { - return false; - }; - return match_attributes(expr1, expr2); - } - - false -} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index d3d0e086462a3..b6a55c5cfa840 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -34,7 +34,7 @@ pub(crate) use load_before_global_declaration::*; pub(crate) use logging::*; pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; -pub(crate) use max_min_instead_of_if::*; +pub(crate) use min_max_instead_of_if::*; pub(crate) use misplaced_bare_raise::*; pub(crate) use named_expr_without_context::*; pub(crate) use nested_min_max::*; @@ -120,7 +120,7 @@ mod load_before_global_declaration; mod logging; mod magic_value_comparison; mod manual_import_from; -mod max_min_instead_of_if; +mod min_max_instead_of_if; mod misplaced_bare_raise; mod named_expr_without_context; mod nested_min_max; From 320aea4ec7edc6c189e1d1c73c8f9f7ce42df824 Mon Sep 17 00:00:00 2001 From: Tibor Reiss Date: Sat, 17 Feb 2024 21:57:38 +0100 Subject: [PATCH 6/9] Merge rules --- .../test/fixtures/pylint/min_instead_of_if.py | 82 ---------- ...tead_of_if.py => min_max_instead_of_if.py} | 47 ++++++ .../src/checkers/ast/analyze/statement.rs | 7 +- crates/ruff_linter/src/codes.rs | 3 +- crates/ruff_linter/src/rules/pylint/mod.rs | 3 +- .../pylint/rules/min_max_instead_of_if.rs | 147 +++--------------- ...__tests__PLR1730_min_instead_of_if.py.snap | 64 -------- ...sts__PLR1730_min_max_instead_of_if.py.snap | 130 ++++++++++++++++ ...__tests__PLR1731_max_instead_of_if.py.snap | 64 -------- ruff.schema.json | 1 - 10 files changed, 205 insertions(+), 343 deletions(-) delete mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py rename crates/ruff_linter/resources/test/fixtures/pylint/{max_instead_of_if.py => min_max_instead_of_if.py} (64%) delete mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap delete mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py deleted file mode 100644 index 5bb9e54c07391..0000000000000 --- a/crates/ruff_linter/resources/test/fixtures/pylint/min_instead_of_if.py +++ /dev/null @@ -1,82 +0,0 @@ -# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, redefined-outer-name - -value = 10 -value2 = 0 -value3 = 3 - -# Positive -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: # [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: # [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 diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/min_max_instead_of_if.py similarity index 64% rename from crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py rename to crates/ruff_linter/resources/test/fixtures/pylint/min_max_instead_of_if.py index 189c8d6284324..aac2204f51a41 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/max_instead_of_if.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/min_max_instead_of_if.py @@ -14,6 +14,15 @@ 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): @@ -24,6 +33,9 @@ def __init__(self): 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): @@ -51,6 +63,12 @@ def __le__(self, b): 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 @@ -80,3 +98,32 @@ def __le__(self, b): 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 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index ca2b49bbb555d..313380fb53a21 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1130,11 +1130,8 @@ 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::MaxInsteadOfIf) { - pylint::rules::max_instead_of_if(checker, if_); - } - if checker.enabled(Rule::MinInsteadOfIf) { - pylint::rules::min_instead_of_if(checker, if_); + if checker.enabled(Rule::MinMaxInsteadOfIf) { + pylint::rules::min_max_instead_of_if(checker, if_); } if checker.source_type.is_stub() { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index b872e84ae4daa..61caca54dd264 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -278,8 +278,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), - (Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::MinInsteadOfIf), - (Pylint, "R1731") => (RuleGroup::Preview, rules::pylint::rules::MaxInsteadOfIf), + (Pylint, "R1730") => (RuleGroup::Preview, rules::pylint::rules::MinMaxInsteadOfIf), (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 2db07973247d3..c0e4581daa9ce 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -42,8 +42,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::MaxInsteadOfIf, Path::new("max_instead_of_if.py"))] - #[test_case(Rule::MinInsteadOfIf, Path::new("min_instead_of_if.py"))] + #[test_case(Rule::MinMaxInsteadOfIf, Path::new("min_max_instead_of_if.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/min_max_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs index d420f11c62590..d6319fb0444fe 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs @@ -9,11 +9,11 @@ use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; /// ## What it does -/// Check for an if node that can be refactored as a max python builtin. +/// Check for an if node that can be refactored as a min/max python builtin. /// /// ## Why is this bad? /// An if block where the test and assignment have the same structure can -/// be expressed more concisely by using the python builtin max function. +/// be expressed more concisely by using the python builtin min/max function. /// /// ## Example /// ```python @@ -28,54 +28,22 @@ use crate::checkers::ast::Checker; /// /// ## References /// - [Python documentation: max function](https://docs.python.org/3/library/functions.html#max) -#[violation] -pub struct MaxInsteadOfIf { - contents: String, -} - -impl Violation for MaxInsteadOfIf { - #[derive_message_formats] - fn message(&self) -> String { - let MaxInsteadOfIf { contents } = self; - format!("Consider using `{contents}` instead of unnecessary if block") - } -} - -/// ## What it does -/// Check for an if node that can be refactored as a min python builtin. -/// -/// ## Why is this bad? -/// An if block where the test and assignment have the same structure can -/// be expressed more concisely by using the python builtin min function. -/// -/// ## Example -/// ```python -/// if value > 10: -/// value = 10 -/// ``` -/// -/// Use instead: -/// ```python -/// value = min(value, 10) -/// ``` -/// -/// ## References /// - [Python documentation: min function](https://docs.python.org/3/library/functions.html#min) #[violation] -pub struct MinInsteadOfIf { +pub struct MinMaxInsteadOfIf { contents: String, } -impl Violation for MinInsteadOfIf { +impl Violation for MinMaxInsteadOfIf { #[derive_message_formats] fn message(&self) -> String { - let MinInsteadOfIf { contents } = self; + let MinMaxInsteadOfIf { contents } = self; format!("Consider using `{contents}` instead of unnecessary if block") } } -/// R1730 -pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { +/// R1730 (and also R1731) +pub(crate) fn min_max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { let ast::StmtIf { test, body, @@ -117,9 +85,11 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { return; }; - if !matches!(op, CmpOp::Gt | CmpOp::GtE) { - 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); @@ -130,7 +100,7 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { } let func_node = ast::ExprName { - id: "min".into(), + id: min_or_max.as_str().into(), ctx: ExprContext::Load, range: TextRange::default(), }; @@ -149,7 +119,7 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { range: TextRange::default(), }; let diagnostic = Diagnostic::new( - MinInsteadOfIf { + MinMaxInsteadOfIf { contents: checker.generator().stmt(&assign_node.into()), }, stmt_if.range(), @@ -157,85 +127,16 @@ pub(crate) fn min_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { checker.diagnostics.push(diagnostic); } -/// R1731 -pub(crate) fn max_instead_of_if(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 [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; - }; - - if !(!body_target.is_subscript_expr() && !left.is_subscript_expr()) { - return; - } - - let ([op], [right_statement]) = (&**ops, &**comparators) else { - return; - }; - - if !matches!(op, CmpOp::Lt | CmpOp::LtE) { - return; - } +enum MinMax { + Min, + Max, +} - let left_cmp = ComparableExpr::from(left); - let body_target_cmp = ComparableExpr::from(body_target); - let right_statement_cmp = ComparableExpr::from(right_statement); - let body_value_cmp = ComparableExpr::from(body_value); - if left_cmp != body_target_cmp || right_statement_cmp != body_value_cmp { - return; +impl MinMax { + fn as_str(&self) -> &'static str { + match self { + Self::Min => "min", + Self::Max => "max", + } } - - let func_node = ast::ExprName { - id: "max".into(), - ctx: ExprContext::Load, - range: TextRange::default(), - }; - let value_node = ast::ExprCall { - func: Box::new(func_node.into()), - arguments: Arguments { - args: Box::from([body_target.clone(), body_value.deref().clone()]), - keywords: Box::from([]), - range: TextRange::default(), - }, - range: TextRange::default(), - }; - let assign_node = ast::StmtAssign { - targets: vec![body_target.clone()], - value: Box::new(value_node.into()), - range: TextRange::default(), - }; - let diagnostic = Diagnostic::new( - MaxInsteadOfIf { - contents: checker.generator().stmt(&assign_node.into()), - }, - stmt_if.range(), - ); - checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap deleted file mode 100644 index acbe38a32c434..0000000000000 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_instead_of_if.py.snap +++ /dev/null @@ -1,64 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/pylint/mod.rs ---- -min_instead_of_if.py:8:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block - | - 7 | # Positive - 8 | / if value > 10: # [min-instead-of-if] - 9 | | value = 10 - | |______________^ PLR1730 -10 | -11 | if value >= 10: # [min-instead-of-if] - | - -min_instead_of_if.py:11:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block - | - 9 | value = 10 -10 | -11 | / if value >= 10: # [min-instead-of-if] -12 | | value = 10 - | |______________^ PLR1730 -13 | -14 | if value > value2: # [min-instead-of-if] - | - -min_instead_of_if.py:14:1: PLR1730 Consider using `value = min(value, value2)` instead of unnecessary if block - | -12 | value = 10 -13 | -14 | / if value > value2: # [min-instead-of-if] -15 | | value = value2 - | |__________________^ PLR1730 - | - -min_instead_of_if.py:24:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block - | -23 | A1 = A() -24 | / if A1.value > 10: # [min-instead-of-if] -25 | | A1.value = 10 - | |_________________^ PLR1730 - | - -min_instead_of_if.py:48:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block - | -46 | A2 = AA(3) -47 | -48 | / if A2 > A1: # [min-instead-of-if] -49 | | A2 = A1 - | |___________^ PLR1730 -50 | -51 | if A2 >= A1: # [min-instead-of-if] - | - -min_instead_of_if.py:51:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block - | -49 | A2 = A1 -50 | -51 | / if A2 >= A1: # [min-instead-of-if] -52 | | A2 = A1 - | |___________^ PLR1730 -53 | -54 | # Negative - | - - diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap new file mode 100644 index 0000000000000..013a2dad641ef --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap @@ -0,0 +1,130 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +min_max_instead_of_if.py:8:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block + | + 7 | # Positive + 8 | / if value < 10: # [max-instead-of-if] + 9 | | value = 10 + | |______________^ PLR1730 +10 | +11 | if value <= 10: # [max-instead-of-if] + | + +min_max_instead_of_if.py:11:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block + | + 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] + | + +min_max_instead_of_if.py:14:1: PLR1730 Consider using `value = max(value, value2)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:17:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:20:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:23:1: PLR1730 Consider using `value = min(value, value2)` instead of unnecessary if block + | +21 | value = 10 +22 | +23 | / if value > value2: # [min-instead-of-if] +24 | | value = value2 + | |__________________^ PLR1730 + | + +min_max_instead_of_if.py:33:1: PLR1730 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block + | +34 | A1.value = 10 +35 | +36 | / if A1.value > 10: # [min-instead-of-if] +37 | | A1.value = 10 + | |_________________^ PLR1730 + | + +min_max_instead_of_if.py:60:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:63:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block + | +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] + | + +min_max_instead_of_if.py:69:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block + | +67 | A2 = A1 +68 | +69 | / if A2 >= A1: # [min-instead-of-if] +70 | | A2 = A1 + | |___________^ PLR1730 +71 | +72 | # Negative + | + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap deleted file mode 100644 index 9a0719165fee3..0000000000000 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1731_max_instead_of_if.py.snap +++ /dev/null @@ -1,64 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/pylint/mod.rs ---- -max_instead_of_if.py:8:1: PLR1731 Consider using `value = max(value, 10)` instead of unnecessary if block - | - 7 | # Positive - 8 | / if value < 10: # [max-instead-of-if] - 9 | | value = 10 - | |______________^ PLR1731 -10 | -11 | if value <= 10: # [max-instead-of-if] - | - -max_instead_of_if.py:11:1: PLR1731 Consider using `value = max(value, 10)` instead of unnecessary if block - | - 9 | value = 10 -10 | -11 | / if value <= 10: # [max-instead-of-if] -12 | | value = 10 - | |______________^ PLR1731 -13 | -14 | if value < value2: # [max-instead-of-if] - | - -max_instead_of_if.py:14:1: PLR1731 Consider using `value = max(value, value2)` instead of unnecessary if block - | -12 | value = 10 -13 | -14 | / if value < value2: # [max-instead-of-if] -15 | | value = value2 - | |__________________^ PLR1731 - | - -max_instead_of_if.py:24:1: PLR1731 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block - | -23 | A1 = A() -24 | / if A1.value < 10: # [max-instead-of-if] -25 | | A1.value = 10 - | |_________________^ PLR1731 - | - -max_instead_of_if.py:48:1: PLR1731 Consider using `A2 = max(A2, A1)` instead of unnecessary if block - | -46 | A2 = AA(3) -47 | -48 | / if A2 < A1: # [max-instead-of-if] -49 | | A2 = A1 - | |___________^ PLR1731 -50 | -51 | if A2 <= A1: # [max-instead-of-if] - | - -max_instead_of_if.py:51:1: PLR1731 Consider using `A2 = max(A2, A1)` instead of unnecessary if block - | -49 | A2 = A1 -50 | -51 | / if A2 <= A1: # [max-instead-of-if] -52 | | A2 = A1 - | |___________^ PLR1731 -53 | -54 | # Negative - | - - diff --git a/ruff.schema.json b/ruff.schema.json index 584e4d0064592..abe030c0eaef5 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3282,7 +3282,6 @@ "PLR1722", "PLR173", "PLR1730", - "PLR1731", "PLR1733", "PLR1736", "PLR2", From 292eb262219bbc2a7c98f55c123bb92f9c3c4c1d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 6 Apr 2024 13:04:11 -0400 Subject: [PATCH 7/9] Rename rule --- ...ax_instead_of_if.py => if_stmt_min_max.py} | 0 .../src/checkers/ast/analyze/statement.rs | 4 +-- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/pylint/mod.rs | 2 +- ...ax_instead_of_if.rs => if_stmt_min_max.rs} | 12 ++++----- .../ruff_linter/src/rules/pylint/rules/mod.rs | 4 +-- ...t__tests__PLR1730_if_stmt_min_max.py.snap} | 26 +++++++++---------- 7 files changed, 24 insertions(+), 26 deletions(-) rename crates/ruff_linter/resources/test/fixtures/pylint/{min_max_instead_of_if.py => if_stmt_min_max.py} (100%) rename crates/ruff_linter/src/rules/pylint/rules/{min_max_instead_of_if.rs => if_stmt_min_max.rs} (92%) rename crates/ruff_linter/src/rules/pylint/snapshots/{ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap => ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap} (62%) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/min_max_instead_of_if.py b/crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/pylint/min_max_instead_of_if.py rename to crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 3bc6ff3cc3a4c..43bbf14b4bbb1 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1117,8 +1117,8 @@ 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::MinMaxInsteadOfIf) { - pylint::rules::min_max_instead_of_if(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(&[ diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 995a3679aeaef..efe2ed63c6057 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -287,7 +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::MinMaxInsteadOfIf), + (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 024e9b48138e4..ed77947eb8b62 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -47,7 +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::MinMaxInsteadOfIf, Path::new("min_max_instead_of_if.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/min_max_instead_of_if.rs b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs similarity index 92% rename from crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs rename to crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs index d6319fb0444fe..1b18597a73d9e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/min_max_instead_of_if.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs @@ -30,20 +30,20 @@ use crate::checkers::ast::Checker; /// - [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 MinMaxInsteadOfIf { +pub struct IfStmtMinMax { contents: String, } -impl Violation for MinMaxInsteadOfIf { +impl Violation for IfStmtMinMax { #[derive_message_formats] fn message(&self) -> String { - let MinMaxInsteadOfIf { contents } = self; + let IfStmtMinMax { contents } = self; format!("Consider using `{contents}` instead of unnecessary if block") } } -/// R1730 (and also R1731) -pub(crate) fn min_max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf) { +/// R1730, R1731 +pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { let ast::StmtIf { test, body, @@ -119,7 +119,7 @@ pub(crate) fn min_max_instead_of_if(checker: &mut Checker, stmt_if: &ast::StmtIf range: TextRange::default(), }; let diagnostic = Diagnostic::new( - MinMaxInsteadOfIf { + IfStmtMinMax { contents: checker.generator().stmt(&assign_node.into()), }, stmt_if.range(), diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index bf3ac1fef8851..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::*; @@ -37,7 +38,6 @@ pub(crate) use load_before_global_declaration::*; pub(crate) use logging::*; pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; -pub(crate) use min_max_instead_of_if::*; pub(crate) use misplaced_bare_raise::*; pub(crate) use modified_iterating_set::*; pub(crate) use named_expr_without_context::*; @@ -117,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; @@ -133,7 +134,6 @@ mod load_before_global_declaration; mod logging; mod magic_value_comparison; mod manual_import_from; -mod min_max_instead_of_if; mod misplaced_bare_raise; mod modified_iterating_set; mod named_expr_without_context; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap similarity index 62% rename from crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap rename to crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap index 013a2dad641ef..3773c5ca517c1 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_min_max_instead_of_if.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -min_max_instead_of_if.py:8:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block +if_stmt_min_max.py:8:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block | 7 | # Positive 8 | / if value < 10: # [max-instead-of-if] @@ -11,7 +11,7 @@ min_max_instead_of_if.py:8:1: PLR1730 Consider using `value = max(value, 10)` in 11 | if value <= 10: # [max-instead-of-if] | -min_max_instead_of_if.py:11:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block +if_stmt_min_max.py:11:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block | 9 | value = 10 10 | @@ -22,7 +22,7 @@ min_max_instead_of_if.py:11:1: PLR1730 Consider using `value = max(value, 10)` i 14 | if value < value2: # [max-instead-of-if] | -min_max_instead_of_if.py:14:1: PLR1730 Consider using `value = max(value, value2)` instead of unnecessary if block +if_stmt_min_max.py:14:1: PLR1730 Consider using `value = max(value, value2)` instead of unnecessary if block | 12 | value = 10 13 | @@ -33,7 +33,7 @@ min_max_instead_of_if.py:14:1: PLR1730 Consider using `value = max(value, value2 17 | if value > 10: # [min-instead-of-if] | -min_max_instead_of_if.py:17:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block +if_stmt_min_max.py:17:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block | 15 | value = value2 16 | @@ -44,7 +44,7 @@ min_max_instead_of_if.py:17:1: PLR1730 Consider using `value = min(value, 10)` i 20 | if value >= 10: # [min-instead-of-if] | -min_max_instead_of_if.py:20:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block +if_stmt_min_max.py:20:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block | 18 | value = 10 19 | @@ -55,7 +55,7 @@ min_max_instead_of_if.py:20:1: PLR1730 Consider using `value = min(value, 10)` i 23 | if value > value2: # [min-instead-of-if] | -min_max_instead_of_if.py:23:1: PLR1730 Consider using `value = min(value, value2)` instead of unnecessary if block +if_stmt_min_max.py:23:1: PLR1730 Consider using `value = min(value, value2)` instead of unnecessary if block | 21 | value = 10 22 | @@ -64,7 +64,7 @@ min_max_instead_of_if.py:23:1: PLR1730 Consider using `value = min(value, value2 | |__________________^ PLR1730 | -min_max_instead_of_if.py:33:1: PLR1730 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block +if_stmt_min_max.py:33:1: PLR1730 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block | 32 | A1 = A() 33 | / if A1.value < 10: # [max-instead-of-if] @@ -74,7 +74,7 @@ min_max_instead_of_if.py:33:1: PLR1730 Consider using `A1.value = max(A1.value, 36 | if A1.value > 10: # [min-instead-of-if] | -min_max_instead_of_if.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block +if_stmt_min_max.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block | 34 | A1.value = 10 35 | @@ -83,7 +83,7 @@ min_max_instead_of_if.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, | |_________________^ PLR1730 | -min_max_instead_of_if.py:60:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:60:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block | 58 | A2 = AA(3) 59 | @@ -94,7 +94,7 @@ min_max_instead_of_if.py:60:1: PLR1730 Consider using `A2 = max(A2, A1)` instead 63 | if A2 <= A1: # [max-instead-of-if] | -min_max_instead_of_if.py:63:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:63:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of unnecessary if block | 61 | A2 = A1 62 | @@ -105,7 +105,7 @@ min_max_instead_of_if.py:63:1: PLR1730 Consider using `A2 = max(A2, A1)` instead 66 | if A2 > A1: # [min-instead-of-if] | -min_max_instead_of_if.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block | 64 | A2 = A1 65 | @@ -116,7 +116,7 @@ min_max_instead_of_if.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead 69 | if A2 >= A1: # [min-instead-of-if] | -min_max_instead_of_if.py:69:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:69:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block | 67 | A2 = A1 68 | @@ -126,5 +126,3 @@ min_max_instead_of_if.py:69:1: PLR1730 Consider using `A2 = min(A2, A1)` instead 71 | 72 | # Negative | - - From 163fb69b7db1e78f99acb2130110461460b14d0c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 6 Apr 2024 13:19:41 -0400 Subject: [PATCH 8/9] Tweak docs; make more consistent with if-expr-min-max --- .../src/rules/pylint/rules/if_stmt_min_max.rs | 123 +++++++++---- ...nt__tests__PLR1730_if_stmt_min_max.py.snap | 168 ++++++++++++++++-- 2 files changed, 240 insertions(+), 51 deletions(-) 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 index 1b18597a73d9e..c6e7f7d8e74ef 100644 --- 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 @@ -1,29 +1,31 @@ -use std::ops::Deref; - -use ruff_diagnostics::{Diagnostic, Violation}; +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::{self as ast, Arguments, CmpOp, ExprContext, Stmt}; -use ruff_text_size::{Ranged, TextRange}; +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 -/// Check for an if node that can be refactored as a min/max python builtin. +/// Checks for `if` statements that can be replaced with `min()` or `max()` +/// calls. /// /// ## Why is this bad? -/// An if block where the test and assignment have the same structure can -/// be expressed more concisely by using the python builtin min/max function. +/// 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 value < 10: -/// value = 10 +/// if score > highest_score: +/// highest_score = score /// ``` /// /// Use instead: /// ```python -/// value = max(value, 10) +/// highest_score = max(highest_score, score) /// ``` /// /// ## References @@ -31,14 +33,36 @@ use crate::checkers::ast::Checker; /// - [Python documentation: min function](https://docs.python.org/3/library/functions.html#min) #[violation] pub struct IfStmtMinMax { - contents: String, + min_max: MinMax, + replacement: SourceCodeSnippet, } impl Violation for IfStmtMinMax { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { - let IfStmtMinMax { contents } = self; - format!("Consider using `{contents}` instead of unnecessary if block") + 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")) + } } } @@ -77,11 +101,22 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { return; }; - if !(!body_target.is_subscript_expr() && !left.is_subscript_expr()) { + // 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 ([op], [right_statement]) = (&**ops, &**comparators) else { + let [right] = &**comparators else { return; }; @@ -93,50 +128,60 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { let left_cmp = ComparableExpr::from(left); let body_target_cmp = ComparableExpr::from(body_target); - let right_statement_cmp = ComparableExpr::from(right_statement); + 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 func_node = ast::ExprName { - id: min_or_max.as_str().into(), - ctx: ExprContext::Load, - range: TextRange::default(), + let (arg1, arg2) = if flip_args { + (left.as_ref(), right) + } else { + (right, left.as_ref()) }; - let value_node = ast::ExprCall { - func: Box::new(func_node.into()), - arguments: Arguments { - args: Box::from([body_target.clone(), body_value.deref().clone()]), - keywords: Box::from([]), - range: TextRange::default(), - }, - range: TextRange::default(), - }; - let assign_node = ast::StmtAssign { - targets: vec![body_target.clone()], - value: Box::new(value_node.into()), - range: TextRange::default(), - }; - let diagnostic = Diagnostic::new( + + let replacement = format!( + "{} = {min_max}({}, {})", + checker.locator().slice(body_target), + checker.locator().slice(arg1), + checker.locator().slice(arg2), + ); + + let mut diagnostic = Diagnostic::new( IfStmtMinMax { - contents: checker.generator().stmt(&assign_node.into()), + 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 { + 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/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 index 3773c5ca517c1..03d19c4e5098b 100644 --- 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 @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -if_stmt_min_max.py:8:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block +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] @@ -10,8 +10,20 @@ if_stmt_min_max.py:8:1: PLR1730 Consider using `value = max(value, 10)` instead 10 | 11 | if value <= 10: # [max-instead-of-if] | + = help: Replace with `value = min(value, 10)` -if_stmt_min_max.py:11:1: PLR1730 Consider using `value = max(value, 10)` instead of unnecessary if block +ℹ 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 | @@ -21,8 +33,20 @@ if_stmt_min_max.py:11:1: PLR1730 Consider using `value = max(value, 10)` instead 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 Consider using `value = max(value, value2)` instead of unnecessary if block +if_stmt_min_max.py:14:1: PLR1730 [*] Replace `if` statement with `value = min(value, value2)` | 12 | value = 10 13 | @@ -32,8 +56,20 @@ if_stmt_min_max.py:14:1: PLR1730 Consider using `value = max(value, value2)` ins 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 Consider using `value = min(value, 10)` instead of unnecessary if block +if_stmt_min_max.py:17:1: PLR1730 [*] Replace `if` statement with `value = max(value, 10)` | 15 | value = value2 16 | @@ -43,8 +79,20 @@ if_stmt_min_max.py:17:1: PLR1730 Consider using `value = min(value, 10)` instead 19 | 20 | if value >= 10: # [min-instead-of-if] | + = help: Replace with `value = max(value, 10)` -if_stmt_min_max.py:20:1: PLR1730 Consider using `value = min(value, 10)` instead of unnecessary if block +ℹ 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 | @@ -54,8 +102,20 @@ if_stmt_min_max.py:20:1: PLR1730 Consider using `value = min(value, 10)` instead 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 Consider using `value = min(value, value2)` instead of unnecessary if block +if_stmt_min_max.py:23:1: PLR1730 [*] Replace `if` statement with `value = max(value, value2)` | 21 | value = 10 22 | @@ -63,8 +123,20 @@ if_stmt_min_max.py:23:1: PLR1730 Consider using `value = min(value, value2)` ins 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 Consider using `A1.value = max(A1.value, 10)` instead of unnecessary if block +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] @@ -73,8 +145,20 @@ if_stmt_min_max.py:33:1: PLR1730 Consider using `A1.value = max(A1.value, 10)` i 35 | 36 | if A1.value > 10: # [min-instead-of-if] | + = help: Replace with `A1.value = min(A1.value, 10)` -if_stmt_min_max.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` instead of unnecessary if block +ℹ 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 | @@ -82,8 +166,20 @@ if_stmt_min_max.py:36:1: PLR1730 Consider using `A1.value = min(A1.value, 10)` i 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 Consider using `A2 = max(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:60:1: PLR1730 [*] Replace `if` statement with `A2 = min(A2, A1)` | 58 | A2 = AA(3) 59 | @@ -93,8 +189,20 @@ if_stmt_min_max.py:60:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of un 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 Consider using `A2 = max(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:63:1: PLR1730 [*] Replace `if` statement with `A2 = min(A1, A2)` | 61 | A2 = A1 62 | @@ -104,8 +212,20 @@ if_stmt_min_max.py:63:1: PLR1730 Consider using `A2 = max(A2, A1)` instead of un 65 | 66 | if A2 > A1: # [min-instead-of-if] | + = help: Replace with `A2 = min(A1, A2)` -if_stmt_min_max.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of unnecessary if block +ℹ 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 | @@ -115,8 +235,20 @@ if_stmt_min_max.py:66:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of un 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 Consider using `A2 = min(A2, A1)` instead of unnecessary if block +if_stmt_min_max.py:69:1: PLR1730 [*] Replace `if` statement with `A2 = max(A1, A2)` | 67 | A2 = A1 68 | @@ -126,3 +258,15 @@ if_stmt_min_max.py:69:1: PLR1730 Consider using `A2 = min(A2, A1)` instead of un 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: From ae46f90fb1c5ebac424caf1dc2696465f2dd3c24 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 6 Apr 2024 13:24:44 -0400 Subject: [PATCH 9/9] Handle parenthesized expressions --- .../test/fixtures/pylint/if_stmt_min_max.py | 7 ++++++ .../src/rules/pylint/rules/if_stmt_min_max.rs | 15 +++++++++--- ...nt__tests__PLR1730_if_stmt_min_max.py.snap | 24 +++++++++++++++++++ 3 files changed, 43 insertions(+), 3 deletions(-) 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 index aac2204f51a41..e27adfda6db49 100644 --- 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 @@ -127,3 +127,10 @@ def __le__(self, b): value = 5 elif value == 3: value = 2 + +# Parenthesized expressions +if value.attr > 3: + ( + value. + attr + ) = 3 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 index c6e7f7d8e74ef..dfbca11d10aff 100644 --- 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 @@ -1,6 +1,7 @@ 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; @@ -79,7 +80,7 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { return; } - let [Stmt::Assign(ast::StmtAssign { + let [body @ Stmt::Assign(ast::StmtAssign { targets: body_targets, value: body_value, .. @@ -120,7 +121,7 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { return; }; - let min_or_max = match op { + let _min_or_max = match op { CmpOp::Gt | CmpOp::GtE => MinMax::Min, CmpOp::Lt | CmpOp::LtE => MinMax::Max, _ => return, @@ -142,7 +143,15 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { let replacement = format!( "{} = {min_max}({}, {})", - checker.locator().slice(body_target), + 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), ); 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 index 03d19c4e5098b..70ff72b378ae6 100644 --- 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 @@ -270,3 +270,27 @@ if_stmt_min_max.py:69:1: PLR1730 [*] Replace `if` statement with `A2 = max(A1, A 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)