From c4a80a8cd636b2d04e33c09d1d94fc5144bf39e8 Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Fri, 6 Oct 2023 13:47:10 -0700 Subject: [PATCH 1/6] add: failing test cases and error messages issue opened with 2 failing positive cases. added 2 negative cases of similar form --- .../c/consider/consider_using_min_max_builtin.py | 11 +++++++++++ .../c/consider/consider_using_min_max_builtin.txt | 12 +++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) 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..dfb2ca219a 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,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 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..34bf92f525 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,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 From d7dec7b24120beb63cecf9a44daafec7386a8b1b Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Fri, 6 Oct 2023 14:01:11 -0700 Subject: [PATCH 2/6] fix: consider-using-min-max-builtin passes tests Altered order of checks to be more understanding towards statements written in the reverse order of expected. --- doc/whatsnew/fragments/8947.false_negative | 3 ++ .../refactoring/refactoring_checker.py | 43 +++++++++++-------- 2 files changed, 29 insertions(+), 17 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 d29326693a..6d2beedd95 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/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,14 +877,6 @@ 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"): @@ -887,20 +884,24 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: 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 {"<", "<="}: From da20c018ea74be85b962093086b1f2a6e69f6d2f Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Sat, 7 Oct 2023 12:08:51 -0700 Subject: [PATCH 3/6] add: consider-using-min/max-builtin match cases Matches on Call nodes Matches on BinaryOp nodes Update test cases to include positive matches and negatives --- .../refactoring/refactoring_checker.py | 21 ++++++++++++++++++- .../access_attr_before_def_false_positive.py | 3 ++- .../consider_using_min_max_builtin.py | 14 +++++++++++++ .../consider_using_min_max_builtin.txt | 12 ++++++----- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 6d2beedd95..c4a5d0d57e 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -876,6 +876,9 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: and isinstance(body, nodes.Assign) ): 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 @@ -884,29 +887,45 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: else: return + # Then let's take the actual code version (string) of the body value + # and the test left operand + # Since a test could be of form float(x) < value, we just need to call + # left_operand = node.test.left.as_string() + # right_statement_value = right_statement.as_string() + # and body_value = body.value.as_string() + if isinstance(body.value, nodes.Name): body_value = body.value.name elif isinstance(body.value, nodes.Const): body_value = body.value.value + elif isinstance(body.value, (nodes.Call, nodes.BinOp)): + body_value = body.value.as_string() else: return + # Originally this block stops all Call nodes from being checked if hasattr(node.test.left, "name"): left_operand = node.test.left.name elif hasattr(node.test.left, "attrname"): left_operand = node.test.left.attrname + elif isinstance(node.test.left, (nodes.Call, nodes.BinOp)): + left_operand = node.test.left.as_string() else: return + # left_operand = node.test.left.as_string() + if len(node.test.ops) > 1: return operator, right_statement = node.test.ops[0] - + # right_statement_value = right_statement.as_string() if isinstance(right_statement, nodes.Name): right_statement_value = right_statement.name elif isinstance(right_statement, nodes.Const): right_statement_value = right_statement.value + elif isinstance(right_statement, (nodes.Call, nodes.BinOp)): + right_statement_value = right_statement.as_string() else: return 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..5fbf736129 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 @@ -33,6 +33,7 @@ def readUntilArray(self, matches, _=None): self.process_rawq() maxLength = 0 for match in matches: + # newly matched consider-using-max-builtin if len(match) > maxLength: maxLength = len(match) 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 dfb2ca219a..10194f213c 100644 --- a/tests/functional/c/consider/consider_using_min_max_builtin.py +++ b/tests/functional/c/consider/consider_using_min_max_builtin.py @@ -29,6 +29,13 @@ 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): self.value = 13 @@ -107,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 34bf92f525..ce1e98bd09 100644 --- a/tests/functional/c/consider/consider_using_min_max_builtin.txt +++ b/tests/functional/c/consider/consider_using_min_max_builtin.txt @@ -6,8 +6,10 @@ consider-using-max-builtin:20:0:21:18::Consider using 'value = max(value, value2 consider-using-min-builtin:23:0:24:18::Consider using 'value = min(value, value2)' 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 +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 From a760faacbd26901bce1fc19585be08fe95cf6c7f Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Sat, 7 Oct 2023 12:31:19 -0700 Subject: [PATCH 4/6] change: refactor min-max checker for conciseness fix linting errors introduced by other changes on this branch --- .../refactoring/refactoring_checker.py | 59 +++++++------------ 1 file changed, 20 insertions(+), 39 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index c4a5d0d57e..40e89061bf 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -848,7 +848,6 @@ 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: @@ -863,6 +862,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: @@ -887,46 +898,16 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: else: return - # Then let's take the actual code version (string) of the body value - # and the test left operand - # Since a test could be of form float(x) < value, we just need to call - # left_operand = node.test.left.as_string() - # right_statement_value = right_statement.as_string() - # and body_value = body.value.as_string() - - if isinstance(body.value, nodes.Name): - body_value = body.value.name - elif isinstance(body.value, nodes.Const): - body_value = body.value.value - elif isinstance(body.value, (nodes.Call, nodes.BinOp)): - body_value = body.value.as_string() - else: - return - - # Originally this block stops all Call nodes from being checked - if hasattr(node.test.left, "name"): - left_operand = node.test.left.name - elif hasattr(node.test.left, "attrname"): - left_operand = node.test.left.attrname - elif isinstance(node.test.left, (nodes.Call, nodes.BinOp)): - left_operand = node.test.left.as_string() - else: - return - - # left_operand = node.test.left.as_string() - if len(node.test.ops) > 1: return - operator, right_statement = node.test.ops[0] - # right_statement_value = right_statement.as_string() - if isinstance(right_statement, nodes.Name): - right_statement_value = right_statement.name - elif isinstance(right_statement, nodes.Const): - right_statement_value = right_statement.value - elif isinstance(right_statement, (nodes.Call, nodes.BinOp)): - right_statement_value = right_statement.as_string() - else: + + body_value = get_node_name(body.value) + left_operand = get_node_name(node.test.left) + right_statement_value = get_node_name(right_statement) + + if not right_statement_value or not body_value or not left_operand: + # nodes in test or Assign are not of a handle-able type return if left_operand == target_assignation: @@ -938,7 +919,7 @@ def _check_consider_using_min_max_builtin(self, node: nodes.If) -> None: else: return - if not (right_statement_value == body_value or left_operand == body_value): + if body_value not in (right_statement_value, left_operand): return if operator in {"<", "<="}: From f05d3531ce16d4ee757e919ea34e5a5a6d717c3b Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Sat, 7 Oct 2023 13:36:21 -0700 Subject: [PATCH 5/6] fix: resolve suggested changes Remove extra comment Install extra dependencies, pre-commit passing locally --- .../functional/a/access/access_attr_before_def_false_positive.py | 1 - 1 file changed, 1 deletion(-) 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 5fbf736129..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 @@ -33,7 +33,6 @@ def readUntilArray(self, matches, _=None): self.process_rawq() maxLength = 0 for match in matches: - # newly matched consider-using-max-builtin if len(match) > maxLength: maxLength = len(match) From aa77f4bc8693201fed5921a89bbea41c56ca950e Mon Sep 17 00:00:00 2001 From: Ryan Ozawa Date: Sat, 7 Oct 2023 23:01:27 -0700 Subject: [PATCH 6/6] change: remove unused guard clause --- pylint/checkers/refactoring/refactoring_checker.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 40e89061bf..3a917a0367 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -906,10 +906,6 @@ def get_node_name(node: nodes.NodeNG) -> str: left_operand = get_node_name(node.test.left) right_statement_value = get_node_name(right_statement) - if not right_statement_value or not body_value or not left_operand: - # nodes in test or Assign are not of a handle-able type - return - if left_operand == target_assignation: # statement is in expected form pass