From 4bf3524e84682a924e7115d3aaa6942c3d459ad5 Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Fri, 23 Feb 2024 12:35:20 -0800 Subject: [PATCH] [false-negative] Fix for consider-using-min/max-builtin (#9127) --- doc/whatsnew/fragments/8947.false_negative | 3 + .../refactoring/refactoring_checker.py | 57 ++++++++++--------- .../access_attr_before_def_false_positive.py | 2 +- .../consider_using_min_max_builtin.py | 25 ++++++++ .../consider_using_min_max_builtin.txt | 14 +++-- 5 files changed, 69 insertions(+), 32 deletions(-) create mode 100644 doc/whatsnew/fragments/8947.false_negative diff --git a/doc/whatsnew/fragments/8947.false_negative b/doc/whatsnew/fragments/8947.false_negative new file mode 100644 index 0000000000..7ecacfabbb --- /dev/null +++ b/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. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 14b8adbc27..a9acf47748 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -871,9 +871,13 @@ 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 @@ -881,6 +885,18 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: 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 {"<", "<="}: diff --git a/tests/functional/a/access/access_attr_before_def_false_positive.py b/tests/functional/a/access/access_attr_before_def_false_positive.py index ebdb76c6af..3d74b302f9 100644 --- a/tests/functional/a/access/access_attr_before_def_false_positive.py +++ b/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 diff --git a/tests/functional/c/consider/consider_using_min_max_builtin.py b/tests/functional/c/consider/consider_using_min_max_builtin.py index e33394c476..10194f213c 100644 --- a/tests/functional/c/consider/consider_using_min_max_builtin.py +++ b/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 diff --git a/tests/functional/c/consider/consider_using_min_max_builtin.txt b/tests/functional/c/consider/consider_using_min_max_builtin.txt index cd8f661c59..ce1e98bd09 100644 --- a/tests/functional/c/consider/consider_using_min_max_builtin.txt +++ b/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