From 812b0976a9e04c6bc5256911419daf4f82de404b Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 13 Apr 2024 18:57:20 -0400 Subject: [PATCH] [`pylint`] Support inverted comparisons (`PLR1730`) (#10920) ## Summary Adds more aggressive logic to PLR1730, `if-stmt-min-max` Closes #10907 ## Test Plan `cargo test` --------- Co-authored-by: Charlie Marsh --- .../test/fixtures/pylint/if_stmt_min_max.py | 25 +++ .../src/rules/pylint/rules/if_stmt_min_max.rs | 47 +++-- ...nt__tests__PLR1730_if_stmt_min_max.py.snap | 190 ++++++++++++++++++ 3 files changed, 248 insertions(+), 14 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 e27adfda6db49..5fdeba431ed26 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 @@ -134,3 +134,28 @@ def __le__(self, b): value. attr ) = 3 + +class Foo: + _min = 0 + _max = 0 + + def foo(self, value) -> None: + if value < self._min: + self._min = value + if value > self._max: + self._max = value + + if self._min < value: + self._min = value + if self._max > value: + self._max = value + + if value <= self._min: + self._min = value + if value >= self._max: + self._max = value + + if self._min <= value: + self._min = value + if self._max >= value: + self._max = value 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 af8be5d15845a..a31965273c61d 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 @@ -107,27 +107,46 @@ pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf) { 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::Min, true), - CmpOp::GtE => (MinMax::Min, false), - CmpOp::Lt => (MinMax::Max, true), - CmpOp::LtE => (MinMax::Max, false), - _ => return, - }; - let [right] = &**comparators else { return; }; let left_cmp = ComparableExpr::from(left); let body_target_cmp = ComparableExpr::from(body_target); - let right_statement_cmp = ComparableExpr::from(right); + let right_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 left_is_target = left_cmp == body_target_cmp; + let right_is_target = right_cmp == body_target_cmp; + let left_is_value = left_cmp == body_value_cmp; + let right_is_value = right_cmp == body_value_cmp; + + // Determine whether to use `min()` or `max()`, and whether to flip the + // order of the arguments, which is relevant for breaking ties. + // Also ensure that we understand the operation we're trying to do, + // by checking both sides of the comparison and assignment. + let (min_max, flip_args) = match ( + left_is_target, + right_is_target, + left_is_value, + right_is_value, + ) { + (true, false, false, true) => match op { + CmpOp::Lt => (MinMax::Max, true), + CmpOp::LtE => (MinMax::Max, false), + CmpOp::Gt => (MinMax::Min, true), + CmpOp::GtE => (MinMax::Min, false), + _ => return, + }, + (false, true, true, false) => match op { + CmpOp::Lt => (MinMax::Min, true), + CmpOp::LtE => (MinMax::Min, false), + CmpOp::Gt => (MinMax::Max, true), + CmpOp::GtE => (MinMax::Max, false), + _ => return, + }, + _ => return, + }; let (arg1, arg2) = if flip_args { (left.as_ref(), right) 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 19c71e7f7823d..52baee302a31d 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 @@ -280,6 +280,8 @@ if_stmt_min_max.py:132:1: PLR1730 [*] Replace `if` statement with `min` call 135 | | attr 136 | | ) = 3 | |_________^ PLR1730 +137 | +138 | class Foo: | = help: Replace with `min` call @@ -294,3 +296,191 @@ if_stmt_min_max.py:132:1: PLR1730 [*] Replace `if` statement with `min` call 135 134 | attr 136 |- ) = 3 135 |+ ) = min(value.attr, 3) +137 136 | +138 137 | class Foo: +139 138 | _min = 0 + +if_stmt_min_max.py:143:9: PLR1730 [*] Replace `if` statement with `self._min = min(value, self._min)` + | +142 | def foo(self, value) -> None: +143 | if value < self._min: + | _________^ +144 | | self._min = value + | |_____________________________^ PLR1730 +145 | if value > self._max: +146 | self._max = value + | + = help: Replace with `self._min = min(value, self._min)` + +ℹ Safe fix +140 140 | _max = 0 +141 141 | +142 142 | def foo(self, value) -> None: +143 |- if value < self._min: +144 |- self._min = value + 143 |+ self._min = min(value, self._min) +145 144 | if value > self._max: +146 145 | self._max = value +147 146 | + +if_stmt_min_max.py:145:9: PLR1730 [*] Replace `if` statement with `self._max = max(value, self._max)` + | +143 | if value < self._min: +144 | self._min = value +145 | if value > self._max: + | _________^ +146 | | self._max = value + | |_____________________________^ PLR1730 +147 | +148 | if self._min < value: + | + = help: Replace with `self._max = max(value, self._max)` + +ℹ Safe fix +142 142 | def foo(self, value) -> None: +143 143 | if value < self._min: +144 144 | self._min = value +145 |- if value > self._max: +146 |- self._max = value + 145 |+ self._max = max(value, self._max) +147 146 | +148 147 | if self._min < value: +149 148 | self._min = value + +if_stmt_min_max.py:148:9: PLR1730 [*] Replace `if` statement with `self._min = max(self._min, value)` + | +146 | self._max = value +147 | +148 | if self._min < value: + | _________^ +149 | | self._min = value + | |_____________________________^ PLR1730 +150 | if self._max > value: +151 | self._max = value + | + = help: Replace with `self._min = max(self._min, value)` + +ℹ Safe fix +145 145 | if value > self._max: +146 146 | self._max = value +147 147 | +148 |- if self._min < value: +149 |- self._min = value + 148 |+ self._min = max(self._min, value) +150 149 | if self._max > value: +151 150 | self._max = value +152 151 | + +if_stmt_min_max.py:150:9: PLR1730 [*] Replace `if` statement with `self._max = min(self._max, value)` + | +148 | if self._min < value: +149 | self._min = value +150 | if self._max > value: + | _________^ +151 | | self._max = value + | |_____________________________^ PLR1730 +152 | +153 | if value <= self._min: + | + = help: Replace with `self._max = min(self._max, value)` + +ℹ Safe fix +147 147 | +148 148 | if self._min < value: +149 149 | self._min = value +150 |- if self._max > value: +151 |- self._max = value + 150 |+ self._max = min(self._max, value) +152 151 | +153 152 | if value <= self._min: +154 153 | self._min = value + +if_stmt_min_max.py:153:9: PLR1730 [*] Replace `if` statement with `self._min = min(self._min, value)` + | +151 | self._max = value +152 | +153 | if value <= self._min: + | _________^ +154 | | self._min = value + | |_____________________________^ PLR1730 +155 | if value >= self._max: +156 | self._max = value + | + = help: Replace with `self._min = min(self._min, value)` + +ℹ Safe fix +150 150 | if self._max > value: +151 151 | self._max = value +152 152 | +153 |- if value <= self._min: +154 |- self._min = value + 153 |+ self._min = min(self._min, value) +155 154 | if value >= self._max: +156 155 | self._max = value +157 156 | + +if_stmt_min_max.py:155:9: PLR1730 [*] Replace `if` statement with `self._max = max(self._max, value)` + | +153 | if value <= self._min: +154 | self._min = value +155 | if value >= self._max: + | _________^ +156 | | self._max = value + | |_____________________________^ PLR1730 +157 | +158 | if self._min <= value: + | + = help: Replace with `self._max = max(self._max, value)` + +ℹ Safe fix +152 152 | +153 153 | if value <= self._min: +154 154 | self._min = value +155 |- if value >= self._max: +156 |- self._max = value + 155 |+ self._max = max(self._max, value) +157 156 | +158 157 | if self._min <= value: +159 158 | self._min = value + +if_stmt_min_max.py:158:9: PLR1730 [*] Replace `if` statement with `self._min = max(value, self._min)` + | +156 | self._max = value +157 | +158 | if self._min <= value: + | _________^ +159 | | self._min = value + | |_____________________________^ PLR1730 +160 | if self._max >= value: +161 | self._max = value + | + = help: Replace with `self._min = max(value, self._min)` + +ℹ Safe fix +155 155 | if value >= self._max: +156 156 | self._max = value +157 157 | +158 |- if self._min <= value: +159 |- self._min = value + 158 |+ self._min = max(value, self._min) +160 159 | if self._max >= value: +161 160 | self._max = value + +if_stmt_min_max.py:160:9: PLR1730 [*] Replace `if` statement with `self._max = min(value, self._max)` + | +158 | if self._min <= value: +159 | self._min = value +160 | if self._max >= value: + | _________^ +161 | | self._max = value + | |_____________________________^ PLR1730 + | + = help: Replace with `self._max = min(value, self._max)` + +ℹ Safe fix +157 157 | +158 158 | if self._min <= value: +159 159 | self._min = value +160 |- if self._max >= value: +161 |- self._max = value + 160 |+ self._max = min(value, self._max)