Enhancement/add checker consider using min max builtin (#4359)

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Co-authored-by: manderj <joffrey.mander+pro@gmail.com>

6 files changed, 206 insertions, 0 deletions

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"