Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] Support inverted comparisons (PLR1730) #10920

Merged
merged 2 commits into from
Apr 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,18 @@ 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
47 changes: 33 additions & 14 deletions crates/ruff_linter/src/rules/pylint/rules/if_stmt_min_max.rs
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
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,95 @@ 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

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
|
= 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)