[false-negative] Fix 8947 for consider-using-min/max-builtin

doc/whatsnew/fragments/8947.false_negative

@@ -0,0 +1,3 @@
+Fix a false-negative for unnecessary if blocks using a different than expected ordering of arguments.
+
+Closes #8947.

pylint/checkers/refactoring/refactoring_checker.py

@@ -851,6 +851,11 @@ def visit_if(self, node: nodes.If) -> None:
     # pylint: disable = too-many-branches
     def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None:
         """Check if the given if node can be refactored as a min/max python builtin."""
+        # This function is written expecting a test condition of form:
+        #  if a < b: # [consider-using-max-builtin]
+        #    a = b
+        #  if a > b: # [consider-using-min-builtin]
+        #    a = b
         if self._is_actual_elif(node) or node.orelse:
             # Not interested in if statements with multiple branches.
             return
@@ -872,35 +877,31 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None:
         ):
             return
 
-        # Check that the assignation is on the same variable.
-        if hasattr(node.test.left, "name"):
-            left_operand = node.test.left.name
-        elif hasattr(node.test.left, "attrname"):
-            left_operand = node.test.left.attrname
-        else:
-            return
-
         if hasattr(target, "name"):
             target_assignation = target.name
         elif hasattr(target, "attrname"):
             target_assignation = target.attrname
         else:
             return
 
-        if not (left_operand == target_assignation):
+        if isinstance(body.value, nodes.Name):
+            body_value = body.value.name
+        elif isinstance(body.value, nodes.Const):
+            body_value = body.value.value
+        else:
             return
 
-        if len(node.test.ops) > 1:
+        if hasattr(node.test.left, "name"):
+            left_operand = node.test.left.name
+        elif hasattr(node.test.left, "attrname"):
+            left_operand = node.test.left.attrname
+        else:
             return
 
-        if not isinstance(body.value, (nodes.Name, nodes.Const)):
+        if len(node.test.ops) > 1:
             return
 
         operator, right_statement = node.test.ops[0]
-        if isinstance(body.value, nodes.Name):
-            body_value = body.value.name
-        else:
-            body_value = body.value.value
 
         if isinstance(right_statement, nodes.Name):
             right_statement_value = right_statement.name
@@ -909,8 +910,16 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None:
         else:
             return
 
-        # Verify the right part of the statement is the same.
-        if right_statement_value != body_value:
+        if left_operand == target_assignation:
+            # statement is in expected form
+            pass
+        elif right_statement_value == target_assignation:
+            # statement is in reverse form
+            operator = utils.get_inverse_comparator(operator)
+        else:
+            return
+
+        if not (right_statement_value == body_value or left_operand == body_value):
             return
 
         if operator in {"<", "<="}:

tests/functional/c/consider/consider_using_min_max_builtin.py

@@ -23,6 +23,11 @@
 if value > value2:  # [consider-using-min-builtin]
     value = value2
 
+if value2 > value3:  # [consider-using-max-builtin]
+    value3 = value2
+
+if value < value2:  # [consider-using-min-builtin]
+    value2 = value
 
 class A:
     def __init__(self):
@@ -70,6 +75,12 @@ def __le__(self, b):
 if value > 10:
     value = 2
 
+if 10 < value:
+    value = 2
+
+if 10 > value:
+    value = 2
+
 if value > 10:
     value = 2
     value2 = 3

tests/functional/c/consider/consider_using_min_max_builtin.txt

@@ -4,8 +4,10 @@ consider-using-max-builtin:14:0:15:14::Consider using 'value = max(value, 10)' i
 consider-using-min-builtin:17:0:18:14::Consider using 'value = min(value, 10)' instead of unnecessary if block:UNDEFINED
 consider-using-max-builtin:20:0:21:18::Consider using 'value = max(value, value2)' instead of unnecessary if block:UNDEFINED
 consider-using-min-builtin:23:0:24:18::Consider using 'value = min(value, value2)' instead of unnecessary if block:UNDEFINED
-consider-using-min-builtin:33:0:34:17::Consider using 'value = min(value, 10)' instead of unnecessary if block:UNDEFINED
-consider-using-min-builtin:57:0:58:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
-consider-using-max-builtin:60:0:61:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED
-consider-using-min-builtin:63:0:64:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
-consider-using-max-builtin:66:0:67:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED
+consider-using-max-builtin:26:0:27:19::Consider using 'value3 = max(value3, value2)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:29:0:30:18::Consider using 'value2 = min(value2, value)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:38:0:39:17::Consider using 'value = min(value, 10)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:62:0:63:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
+consider-using-max-builtin:65:0:66:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:68:0:69:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
+consider-using-max-builtin:71:0:72:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED