Implement FURB136 (#8664)

## Summary

Implements
[FURB136](https://github.com/dosisod/refurb/blob/master/docs/checks.md#furb136-use-min-max)
that checks for `if` expressions that can be replaced with `min()` or
`max()` calls. See issue #1348 for more information.

This implementation diverges from Refurb's original implementation by
retaining the order of equal values. For example, Refurb suggest that
the following expressions:

```python
highest_score1 = score1 if score1 > score2 else score2
highest_score2 = score1 if score1 >= score2 else score2
```

should be to rewritten as:

```python
highest_score1 = max(score1, score2)
highest_score2 = max(score1, score2)
```

whereas this implementation provides more correct alternatives:

```python
highest_score1 = max(score2, score1)
highest_score2 = max(score1, score2)
```

## Test Plan

Unit test checks all eight possibilities.

crates/ruff_linter/resources/test/fixtures/refurb/FURB136.py

@@ -0,0 +1,25 @@
+x = 1
+y = 2
+
+x if x > y else y  # FURB136
+
+x if x >= y else y  # FURB136
+
+x if x < y else y  # FURB136
+
+x if x <= y else y  # FURB136
+
+y if x > y else x  # FURB136
+
+y if x >= y else x  # FURB136
+
+y if x < y else x  # FURB136
+
+y if x <= y else x  # FURB136
+
+x + y if x > y else y  # OK
+
+x if (
+    x
+    > y
+) else y  # FURB136

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -1281,12 +1281,14 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 }
             }
         }
-        Expr::IfExp(ast::ExprIfExp {
-            test,
-            body,
-            orelse,
-            range: _,
-        }) => {
+        Expr::IfExp(
+            if_exp @ ast::ExprIfExp {
+                test,
+                body,
+                orelse,
+                range: _,
+            },
+        ) => {
             if checker.enabled(Rule::IfElseBlockInsteadOfDictGet) {
                 flake8_simplify::rules::if_exp_instead_of_dict_get(
                     checker, expr, test, body, orelse,
@@ -1301,6 +1303,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::IfExprWithTwistedArms) {
                 flake8_simplify::rules::twisted_arms_in_ifexpr(checker, expr, test, body, orelse);
             }
+            if checker.enabled(Rule::IfExprMinMax) {
+                refurb::rules::if_expr_min_max(checker, if_exp);
+            }
         }
         Expr::ListComp(
             comp @ ast::ExprListComp {

crates/ruff_linter/src/codes.rs

@@ -947,6 +947,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Refurb, "131") => (RuleGroup::Nursery, rules::refurb::rules::DeleteFullSlice),
         #[allow(deprecated)]
         (Refurb, "132") => (RuleGroup::Nursery, rules::refurb::rules::CheckAndRemoveFromSet),
+        (Refurb, "136") => (RuleGroup::Preview, rules::refurb::rules::IfExprMinMax),
         (Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
         (Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
         (Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),

crates/ruff_linter/src/rules/refurb/mod.rs

@@ -18,6 +18,7 @@ mod tests {
     #[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
     #[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
     #[test_case(Rule::CheckAndRemoveFromSet, Path::new("FURB132.py"))]
+    #[test_case(Rule::IfExprMinMax, Path::new("FURB136.py"))]
     #[test_case(Rule::ReimplementedStarmap, Path::new("FURB140.py"))]
     #[test_case(Rule::SliceCopy, Path::new("FURB145.py"))]
     #[test_case(Rule::UnnecessaryEnumerate, Path::new("FURB148.py"))]

crates/ruff_linter/src/rules/refurb/rules/if_expr_min_max.rs

@@ -0,0 +1,178 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::comparable::ComparableExpr;
+use ruff_python_ast::{self as ast, CmpOp, Expr};
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+use crate::fix::snippet::SourceCodeSnippet;
+
+/// ## What it does
+/// Checks for `if` expressions that can be replaced with `min()` or `max()`
+/// calls.
+///
+/// ## Why is this bad?
+/// An `if` expression that selects the lesser or greater of two
+/// sub-expressions can be replaced with a `min()` or `max()` call
+/// respectively. When possible, prefer `min()` and `max()`, as they're more
+/// concise and readable than the equivalent `if` expression.
+///
+/// ## Example
+/// ```python
+/// highest_score = score1 if score1 > score2 else score2
+/// ```
+///
+/// Use instead:
+/// ```python
+/// highest_score = max(score2, score1)
+/// ```
+///
+/// ## References
+/// - [Python documentation: `min`](https://docs.python.org/3.11/library/functions.html#min)
+/// - [Python documentation: `max`](https://docs.python.org/3.11/library/functions.html#max)
+#[violation]
+pub struct IfExprMinMax {
+    min_max: MinMax,
+    expression: SourceCodeSnippet,
+    replacement: SourceCodeSnippet,
+}
+
+impl Violation for IfExprMinMax {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        let Self {
+            min_max,
+            expression,
+            replacement,
+        } = self;
+
+        match (expression.full_display(), replacement.full_display()) {
+            (_, None) => {
+                format!("Replace `if` expression with `{min_max}` call")
+            }
+            (None, Some(replacement)) => {
+                format!("Replace `if` expression with `{replacement}`")
+            }
+            (Some(expression), Some(replacement)) => {
+                format!("Replace `{expression}` with `{replacement}`")
+            }
+        }
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        let Self {
+            replacement,
+            min_max,
+            ..
+        } = self;
+        if let Some(replacement) = replacement.full_display() {
+            Some(format!("Replace with `{replacement}`"))
+        } else {
+            Some(format!("Replace with `{min_max}` call"))
+        }
+    }
+}
+
+/// FURB136
+pub(crate) fn if_expr_min_max(checker: &mut Checker, if_exp: &ast::ExprIfExp) {
+    let Expr::Compare(ast::ExprCompare {
+        left,
+        ops,
+        comparators,
+        ..
+    }) = if_exp.test.as_ref()
+    else {
+        return;
+    };
+
+    // Ignore, e.g., `foo < bar < baz`.
+    let [op] = ops.as_slice() else {
+        return;
+    };
+
+    // Determine whether to use `min()` or `max()`, and whether to flip the
+    // order of the arguments, which is relevant for breaking ties.
+    let (mut min_max, mut flip_args) = match op {
+        CmpOp::Gt => (MinMax::Max, true),
+        CmpOp::GtE => (MinMax::Max, false),
+        CmpOp::Lt => (MinMax::Min, true),
+        CmpOp::LtE => (MinMax::Min, false),
+        _ => return,
+    };
+
+    let [right] = comparators.as_slice() else {
+        return;
+    };
+
+    let body_cmp = ComparableExpr::from(if_exp.body.as_ref());
+    let orelse_cmp = ComparableExpr::from(if_exp.orelse.as_ref());
+    let left_cmp = ComparableExpr::from(left);
+    let right_cmp = ComparableExpr::from(right);
+
+    if body_cmp == right_cmp && orelse_cmp == left_cmp {
+        min_max = min_max.reverse();
+        flip_args = !flip_args;
+    } else if body_cmp != left_cmp || orelse_cmp != right_cmp {
+        return;
+    }
+
+    let (arg1, arg2) = if flip_args {
+        (right, left.as_ref())
+    } else {
+        (left.as_ref(), right)
+    };
+
+    let replacement = format!(
+        "{min_max}({}, {})",
+        checker.generator().expr(arg1),
+        checker.generator().expr(arg2),
+    );
+
+    let mut diagnostic = Diagnostic::new(
+        IfExprMinMax {
+            min_max,
+            expression: SourceCodeSnippet::from_str(checker.locator().slice(if_exp)),
+            replacement: SourceCodeSnippet::from_str(replacement.as_str()),
+        },
+        if_exp.range(),
+    );
+
+    if checker.semantic().is_builtin(min_max.as_str()) {
+        diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
+            replacement,
+            if_exp.range(),
+        )));
+    }
+
+    checker.diagnostics.push(diagnostic);
+}
+
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+enum MinMax {
+    Min,
+    Max,
+}
+
+impl MinMax {
+    fn reverse(self) -> Self {
+        match self {
+            Self::Min => Self::Max,
+            Self::Max => Self::Min,
+        }
+    }
+
+    fn as_str(self) -> &'static str {
+        match self {
+            Self::Min => "min",
+            Self::Max => "max",
+        }
+    }
+}
+
+impl std::fmt::Display for MinMax {
+    fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
+        write!(fmt, "{}", self.as_str())
+    }
+}

crates/ruff_linter/src/rules/refurb/rules/mod.rs

@@ -1,5 +1,6 @@
 pub(crate) use check_and_remove_from_set::*;
 pub(crate) use delete_full_slice::*;
+pub(crate) use if_expr_min_max::*;
 pub(crate) use implicit_cwd::*;
 pub(crate) use isinstance_type_none::*;
 pub(crate) use print_empty_string::*;
@@ -13,6 +14,7 @@ pub(crate) use unnecessary_enumerate::*;
 
 mod check_and_remove_from_set;
 mod delete_full_slice;
+mod if_expr_min_max;
 mod implicit_cwd;
 mod isinstance_type_none;
 mod print_empty_string;