PLR1730 autofixes max/mins in the wrong order

[TylerYep]

Description

Ruff v0.9.4

Output:

PLR1730 [*] Replace `if` statement with `self.top = min(self.top, curr)`
    |
352 | /             if curr <= self.top:
353 | |                 self.top = curr
    | |_______________________________^ PLR1730

Ruff autofixes this to self.top = min(self.top, curr), but this is incorrect.

The correct autofix is self.top = min(curr, self.top), because if curr and self.top are equivalent, self.top should not be updated.

This autofix works when equivalent values are interchangeable, but when we use custom overrides for the __eq__ function, values may be equal but not interchangeable.

[AlexWaygood]

Yes, agreed that this is buggy. Here's a minimal repro for how this can change the order of elements that compare equal but are not equivalent:

>>> import itertools
>>> COUNT = itertools.count()
>>> from functools import total_ordering
>>> @total_ordering
... class Foo:
...     def __init__(self, value):
...         self.value = value
...         self.count = next(COUNT)
...     def __eq__(self, other):
...         isinstance(other, Foo) and other.value == self.value
...     def __lt__(self, other):
...         if not isinstance(other, Foo):
...             return NotImplemented
...         self.value < other.value
...         
>>> current = Foo(1)
>>> top = Foo(1)
>>> current.count
0
>>> top.count
1
>>> current = min(top, current)
>>> current.count
1

[VascoSch92]

Can I give a try? :-)

[AlexWaygood]

Can I give a try? :-)

Go for it! Let us know if you need any help or clarification :-)

[VascoSch92]

Just a clarification. I was looking into the code and I saw this snippet

/// R1730, R1731
pub(crate) fn if_stmt_min_max(checker: &mut Checker, stmt_if: &ast::StmtIf)

Two small questions:

• So I suppose there are two rules merged in one, i.e., PLR1730 and PLR1731 (one for min and one for max), correct?
  But in the docs we have just 1730: https://docs.astral.sh/ruff/rules/if-stmt-min-max/

• The example above was just for min but I suppose the same bug persist also for max, correct?

[MichaReiser]

Yes, Ruff has a single rule that corresponds to two pylint rules. I'd expect that the same bug is present for max. The best way to verify it is to add a test for both min and max before fixing the bug.

[VascoSch92]

Looking at the python documentation:

If multiple items are maximal, the function returns the first one encountered. max doc

If multiple items are minimal, the function returns the first one encountered. min doc

As a general rule, the variable that we want to update must always be in the first position in the min/max method. (To be consistent with python implementation).

Therefore, the autofix discussed at the beginning of this issue was indeed correct. But the problem mentioned is real and present. In fact, we have at line 452 of if_stmt_min_max.py.snap

ℹ 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

which is wrong as it should be self._min = max(self._min, value).

I will check if we can enforce the above mentioned rule in the suggestion and in the auto fix ;-)

Or am I understanding something wrong?

[MichaReiser]

That all sounds correct to me. Thanks for the detailed write up!