From bafb1497b0b46d6e7d9db5c3716c8638e3686944 Mon Sep 17 00:00:00 2001 From: Qwiddle13 <32040075+Qwiddle13@users.noreply.github.com> Date: Sat, 17 Apr 2021 13:28:21 -0400 Subject: Enhancement/add checker consider using min max builtin (#4359) Co-authored-by: Pierre Sassoulas Co-authored-by: manderj --- CONTRIBUTORS.txt | 4 + ChangeLog | 4 + doc/whatsnew/2.8.rst | 2 + pylint/checkers/refactoring/refactoring_checker.py | 88 ++++++++++++++++++++ .../c/consider/consider_using_min_max_builtin.py | 97 ++++++++++++++++++++++ .../c/consider/consider_using_min_max_builtin.txt | 11 +++ 6 files changed, 206 insertions(+) create mode 100644 tests/functional/c/consider/consider_using_min_max_builtin.py create mode 100644 tests/functional/c/consider/consider_using_min_max_builtin.txt diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 2160884db..0252731b5 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -472,3 +472,7 @@ contributors: * Sebastian Müller: contributor * Ramiro Leal-Cavazos (ramiro050): Fixed bug preventing pylint from working with emacs tramp + +* manderj: contributor + +* qwiddle: contributor diff --git a/ChangeLog b/ChangeLog index 9019de244..a1dd0606a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,10 @@ Release date: Undefined Closes #2822, #4206, #4284 +* Add ``consider-using-min-max-builtin`` check for if statement which could be replaced by Python builtin min or max + + Closes #3406 + What's New in Pylint 2.7.5? =========================== diff --git a/doc/whatsnew/2.8.rst b/doc/whatsnew/2.8.rst index 74beb7de4..11bce8c36 100644 --- a/doc/whatsnew/2.8.rst +++ b/doc/whatsnew/2.8.rst @@ -17,6 +17,8 @@ New checkers * Add new extension ``ConfusingConsecutiveElifChecker``. This optional checker emits a refactoring message (R5601 ``confusing-consecutive-elif``) if if/elif statements with different indentation levels follow directly one after the other. +* Add ``consider-using-min-max-builtin`` check for if statement which could be replaced by Python builtin min or max. + Other Changes ============= diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index a95bed97b..bab86b4bf 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -291,6 +291,16 @@ class RefactoringChecker(checkers.BaseTokenChecker): "Comprehension inside of 'any' or 'all' is unnecessary. " "A generator would be sufficient and faster.", ), + "R1730": ( + "Consider using '%s' instead of unnecessary if block", + "consider-using-min-builtin", + "Using the min builtin instead of a conditional improves readability and conciseness.", + ), + "R1731": ( + "Consider using '%s' instead of unnecessary if block", + "consider-using-max-builtin", + "Using the max builtin instead of a conditional improves readability and conciseness.", + ), } options = ( ( @@ -609,6 +619,84 @@ class RefactoringChecker(checkers.BaseTokenChecker): self._check_superfluous_else_break(node) self._check_superfluous_else_continue(node) self._check_consider_get(node) + self._check_consider_using_min_max_builtin(node) + + def _check_consider_using_min_max_builtin(self, node: astroid.If): + """Check if the given if node can be refactored as an min/max python builtin.""" + if self._is_actual_elif(node) or node.orelse: + # Not interested in if statements with multiple branches. + return + + if len(node.body) != 1: + return + + body = node.body[0] + # Check if condition can be reduced. + if not hasattr(body, "targets") or len(body.targets) != 1: + return + + target = body.targets[0] + if not ( + isinstance(node.test, astroid.Compare) + and not isinstance(target, astroid.Subscript) + and not isinstance(node.test.left, astroid.Subscript) + and isinstance(body, astroid.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 + + if hasattr(target, "name"): + target_assignation = target.name + elif hasattr(target, "attrname"): + target_assignation = target.attrname + else: + return + + if not (left_operand == target_assignation): + return + + if len(node.test.ops) > 1: + return + + if not isinstance(body.value, (astroid.Name, astroid.Const)): + return + + operator, right_statement = node.test.ops[0] + if isinstance(body.value, astroid.Name): + body_value = body.value.name + else: + body_value = body.value.value + + if isinstance(right_statement, astroid.Name): + right_statement_value = right_statement.name + else: + right_statement_value = right_statement.value + + # Verify the right part of the statement is the same. + if right_statement_value != body_value: + return + + if operator in ("<", "<="): + reduced_to = "{target} = max({target}, {item})".format( + target=target_assignation, item=body_value + ) + self.add_message( + "consider-using-max-builtin", node=node, args=(reduced_to,) + ) + elif operator in (">", ">="): + reduced_to = "{target} = min({target}, {item})".format( + target=target_assignation, item=body_value + ) + self.add_message( + "consider-using-min-builtin", node=node, args=(reduced_to,) + ) @utils.check_messages("simplifiable-if-expression") def visit_ifexp(self, node): diff --git a/tests/functional/c/consider/consider_using_min_max_builtin.py b/tests/functional/c/consider/consider_using_min_max_builtin.py new file mode 100644 index 000000000..e12e4992f --- /dev/null +++ b/tests/functional/c/consider/consider_using_min_max_builtin.py @@ -0,0 +1,97 @@ +# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, redefined-outer-name + +value = 10 +value2 = 0 +value3 = 3 + +# Positive +if value < 10: # [consider-using-max-builtin] + value = 10 + +if value >= 10: # [consider-using-min-builtin] + value = 10 + +if value <= 10: # [consider-using-max-builtin] + value = 10 + +if value > 10: # [consider-using-min-builtin] + value = 10 + +if value < value2: # [consider-using-max-builtin] + value = value2 + +if value > value2: # [consider-using-min-builtin] + value = value2 + + +class A: + def __init__(self): + self.value = 13 + + +A1 = A() +if A1.value > 10: # [consider-using-min-builtin] + A1.value = 10 + + +class AA: + def __init__(self, value): + self.value = value + + def __gt__(self, b): + return self.value > b + + def __ge__(self, b): + return self.value >= b + + def __lt__(self, b): + return self.value < b + + def __le__(self, b): + return self.value <= b + + +A1 = AA(0) +A2 = AA(3) + +if A1 > A2: # [consider-using-min-builtin] + A1 = A2 + +if A2 < A1: # [consider-using-max-builtin] + A2 = A1 + +if A1 >= A2: # [consider-using-min-builtin] + A1 = A2 + +if A2 <= A1: # [consider-using-max-builtin] + A2 = A1 + +# Negative +if value > 10: + value = 2 + +if value > 10: + value = 2 + value2 = 3 + +if value > value2: + value = value3 + +if value > 5: + value = value3 + +if 2 < value <= 3: + value = 1 + +if value <= 3: + value = 5 + +if value <= 3: + value = 5 +elif value == 3: + value = 2 + +if value > 10: + value = 10 +else: + value = 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 new file mode 100644 index 000000000..9ed5e0559 --- /dev/null +++ b/tests/functional/c/consider/consider_using_min_max_builtin.txt @@ -0,0 +1,11 @@ +consider-using-max-builtin:8:0::"Consider using 'value = max(value, 10)' instead of unnecessary if block" +consider-using-min-builtin:11:0::"Consider using 'value = min(value, 10)' instead of unnecessary if block" +consider-using-max-builtin:14:0::"Consider using 'value = max(value, 10)' instead of unnecessary if block" +consider-using-min-builtin:17:0::"Consider using 'value = min(value, 10)' instead of unnecessary if block" +consider-using-max-builtin:20:0::"Consider using 'value = max(value, value2)' instead of unnecessary if block" +consider-using-min-builtin:23:0::"Consider using 'value = min(value, value2)' instead of unnecessary if block" +consider-using-min-builtin:33:0::"Consider using 'value = min(value, 10)' instead of unnecessary if block" +consider-using-min-builtin:57:0::"Consider using 'A1 = min(A1, A2)' instead of unnecessary if block" +consider-using-max-builtin:60:0::"Consider using 'A2 = max(A2, A1)' instead of unnecessary if block" +consider-using-min-builtin:63:0::"Consider using 'A1 = min(A1, A2)' instead of unnecessary if block" +consider-using-max-builtin:66:0::"Consider using 'A2 = max(A2, A1)' instead of unnecessary if block" -- cgit v1.2.1