[false-negative] Fix for consider-using-min/max-builtin (#9127)

Author: rhyn0

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

@@ -871,16 +871,32 @@ def visit_if(self, node: nodes.If) -> None:
         self._check_consider_get(node)
         self._check_consider_using_min_max_builtin(node)
 
-    # 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
 
         if len(node.body) != 1:
             return
 
+        def get_node_name(node: nodes.NodeNG) -> str:
+            """Obtain simplest representation of a node as a string."""
+            if isinstance(node, nodes.Name):
+                return node.name  # type: ignore[no-any-return]
+            if isinstance(node, nodes.Attribute):
+                return node.attrname  # type: ignore[no-any-return]
+            if isinstance(node, nodes.Const):
+                return str(node.value)
+            # this is a catch-all for nodes that are not of type Name or Attribute
+            # extremely helpful for Call or BinOp
+            return node.as_string()  # type: ignore[no-any-return]
+
         body = node.body[0]
         # Check if condition can be reduced.
         if not hasattr(body, "targets") or len(body.targets) != 1:
@@ -894,14 +910,9 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None:
             and isinstance(body, nodes.Assign)
         ):
             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
+        # Assign body line has one requirement and that is the assign target
+        # is of type name or attribute. Attribute referring to NamedTuple.x perse.
+        # So we have to check that target is of these types
 
         if hasattr(target, "name"):
             target_assignation = target.name
@@ -910,30 +921,24 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None:
         else:
             return
 
-        if not (left_operand == target_assignation):
-            return
-
         if len(node.test.ops) > 1:
             return
-
-        if not isinstance(body.value, (nodes.Name, nodes.Const)):
-            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
-        elif isinstance(right_statement, nodes.Const):
-            right_statement_value = right_statement.value
+        body_value = get_node_name(body.value)
+        left_operand = get_node_name(node.test.left)
+        right_statement_value = get_node_name(right_statement)
+
+        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
 
-        # Verify the right part of the statement is the same.
-        if right_statement_value != body_value:
+        if body_value not in (right_statement_value, left_operand):
             return
 
         if operator in {"<", "<="}:

tests/functional/a/access/access_attr_before_def_false_positive.py

@@ -1,5 +1,5 @@
 # pylint: disable=invalid-name,too-many-public-methods,attribute-defined-outside-init
-# pylint: disable=too-few-public-methods,deprecated-module
+# pylint: disable=too-few-public-methods,deprecated-module,consider-using-max-builtin
 """This module demonstrates a possible problem of pyLint with calling __init__ s
 from inherited classes.
 Initializations done there are not considered, which results in Error E0203 for

tests/functional/c/consider/consider_using_min_max_builtin.py

@@ -23,6 +23,18 @@
 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
+
+if value > float(value3):  # [consider-using-min-builtin]
+    value = float(value3)
+
+offset = 1
+if offset + value < value2:  # [consider-using-min-builtin]
+    value2 = offset + value
 
 class A:
     def __init__(self):
@@ -70,6 +82,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
@@ -96,6 +114,13 @@ def __le__(self, b):
 else:
     value = 3
 
+if value > float(value3):
+    value = float(value2)
+
+offset = 1
+if offset + value < value2:
+    value2 = offset
+
 
 # https://github.com/pylint-dev/pylint/issues/4379
 var = 1

tests/functional/c/consider/consider_using_min_max_builtin.txt

@@ -4,8 +4,12 @@ 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:32:0:33:25::Consider using 'value = min(value, float(value3))' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:36:0:37:27::Consider using 'value2 = min(value2, offset + value)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:45:0:46:17::Consider using 'value = min(value, 10)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:69:0:70:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
+consider-using-max-builtin:72:0:73:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED
+consider-using-min-builtin:75:0:76:11::Consider using 'A1 = min(A1, A2)' instead of unnecessary if block:UNDEFINED
+consider-using-max-builtin:78:0:79:11::Consider using 'A2 = max(A2, A1)' instead of unnecessary if block:UNDEFINED