Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement/add checker consider using min max builtin #4359

4 changes: 4 additions & 0 deletions CONTRIBUTORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -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?
===========================
Expand Down
2 changes: 2 additions & 0 deletions doc/whatsnew/2.8.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
=============

Expand Down
88 changes: 88 additions & 0 deletions pylint/checkers/refactoring/refactoring_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
(
Expand Down Expand Up @@ -609,6 +619,84 @@ def visit_if(self, node):
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):
Expand Down
97 changes: 97 additions & 0 deletions tests/functional/c/consider/consider_using_min_max_builtin.py
Original file line number Diff line number Diff line change
@@ -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
11 changes: 11 additions & 0 deletions tests/functional/c/consider/consider_using_min_max_builtin.txt
Original file line number Diff line number Diff line change
@@ -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"