Skip to content

Commit

Permalink
[pylint] Support inverted comparisons (PLR1730) (#10920)
Browse files Browse the repository at this point in the history 
## Summary

Adds more aggressive logic to PLR1730, `if-stmt-min-max`

Closes #10907 

## Test Plan

`cargo test`

---------

Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
@diceroll123 @charliermarsh
diceroll123 and charliermarsh authored Apr 13, 2024
1 parent b356c43 commit 812b097
Show file tree
Hide file tree
Showing 3 changed files with 248 additions and 14 deletions.
25 changes: 25 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pylint/if_stmt_min_max.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
47 changes: 33 additions & 14 deletions crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
190 changes: 190 additions & 0 deletions ...rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR1730_if_stmt_min_max.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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)

0 comments on commit 812b097

Please sign in to comment.