feat: add manual_max and manual_min lints

CHANGELOG.md

@@ -7006,8 +7006,10 @@ Released 2018-09-13
 [`manual_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else
 [`manual_main_separator_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_main_separator_str
 [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map
+[`manual_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_max
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_midpoint`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint
+[`manual_min`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_min
 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
 [`manual_noop_waker`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_noop_waker

clippy_lints/src/declared_lints.rs

@@ -314,6 +314,8 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::manual_is_power_of_two::MANUAL_IS_POWER_OF_TWO_INFO,
     crate::manual_let_else::MANUAL_LET_ELSE_INFO,
     crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO,
+    crate::manual_max::MANUAL_MAX_INFO,
+    crate::manual_max::MANUAL_MIN_INFO,
     crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO,
     crate::manual_noop_waker::MANUAL_NOOP_WAKER_INFO,
     crate::manual_option_as_slice::MANUAL_OPTION_AS_SLICE_INFO,

clippy_lints/src/lib.rs

@@ -213,6 +213,7 @@ mod manual_is_ascii_check;
 mod manual_is_power_of_two;
 mod manual_let_else;
 mod manual_main_separator_str;
+mod manual_max;
 mod manual_non_exhaustive;
 mod manual_noop_waker;
 mod manual_option_as_slice;
@@ -760,6 +761,7 @@ rustc_lint::late_lint_methods!(
         PartialeqToNone: partialeq_to_none::PartialeqToNone = partialeq_to_none::PartialeqToNone,
         ManualAbsDiff: manual_abs_diff::ManualAbsDiff = manual_abs_diff::ManualAbsDiff::new(conf),
         ManualClamp: manual_clamp::ManualClamp = manual_clamp::ManualClamp::new(conf),
+        ManualMax: manual_max::ManualMax = manual_max::ManualMax::new(conf),
         ManualStringNew: manual_string_new::ManualStringNew = manual_string_new::ManualStringNew,
         UnusedPeekable: unused_peekable::UnusedPeekable = unused_peekable::UnusedPeekable,
         BoolToIntWithIf: bool_to_int_with_if::BoolToIntWithIf = bool_to_int_with_if::BoolToIntWithIf,

clippy_lints/src/manual_max.rs

@@ -0,0 +1,279 @@
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::higher::If;
+use clippy_utils::msrvs::{self, Msrv};
+use clippy_utils::sugg::Sugg;
+use clippy_utils::ty::implements_trait;
+use clippy_utils::{eq_expr_value, is_in_const_context, peel_blocks, peel_blocks_with_stmt, sym};
+use rustc_errors::Applicability;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for a single `if`/`else` (or guarding `if`) that picks the greater of
+    /// two values, where [`Ord::max`] would be clearer.
+    ///
+    /// ### Why is this bad?
+    /// `a.max(b)` is shorter, has no branch, and states the intent directly. Unlike
+    /// [`MANUAL_CLAMP`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp),
+    /// which only triggers when both a lower *and* an upper bound are applied, this lint
+    /// catches the common single-sided "floor" case.
+    ///
+    /// ### Known problems
+    /// On a tie the two forms can return different operands: `Ord::max` returns the
+    /// *second* argument when the operands compare equal, whereas `if a < b { b } else { a }`
+    /// returns the *first* (`a`). For types whose `Eq`-equal values are observationally
+    /// distinct (e.g. ordered by a key field), the rewrite changes which value is selected,
+    /// so the suggestion is `MaybeIncorrect`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # let (a, b) = (1, 2);
+    /// let _ = if a < b { b } else { a };
+    ///
+    /// let mut cores = a;
+    /// if cores < b {
+    ///     cores = b;
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # let (a, b) = (1, 2);
+    /// let _ = a.max(b);
+    ///
+    /// let mut cores = a;
+    /// cores = cores.max(b);
+    /// ```
+    #[clippy::version = "1.98.0"]
+    pub MANUAL_MAX,
+    complexity,
+    "an `if`/`else` that could be written as a call to `Ord::max`"
+}
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for a single `if`/`else` (or guarding `if`) that picks the lesser of
+    /// two values, where [`Ord::min`] would be clearer.
+    ///
+    /// ### Why is this bad?
+    /// `a.min(b)` is shorter, has no branch, and states the intent directly. Unlike
+    /// [`MANUAL_CLAMP`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp),
+    /// which only triggers when both a lower *and* an upper bound are applied, this lint
+    /// catches the common single-sided "ceiling" case.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # let (a, b) = (1, 2);
+    /// let _ = if a > b { b } else { a };
+    ///
+    /// let mut cores = a;
+    /// if cores > b {
+    ///     cores = b;
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # let (a, b) = (1, 2);
+    /// let _ = a.min(b);
+    ///
+    /// let mut cores = a;
+    /// cores = cores.min(b);
+    /// ```
+    #[clippy::version = "1.98.0"]
+    pub MANUAL_MIN,
+    complexity,
+    "an `if`/`else` that could be written as a call to `Ord::min`"
+}
+
+impl_lint_pass!(ManualMax => [MANUAL_MAX, MANUAL_MIN]);
+
+pub struct ManualMax {
+    msrv: Msrv,
+}
+
+impl ManualMax {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self { msrv: conf.msrv }
+    }
+}
+
+#[derive(Clone, Copy)]
+enum MinMax {
+    Max,
+    Min,
+}
+
+impl MinMax {
+    fn method(self) -> &'static str {
+        match self {
+            MinMax::Max => "max",
+            MinMax::Min => "min",
+        }
+    }
+
+    fn lint(self) -> &'static rustc_lint::Lint {
+        match self {
+            MinMax::Max => MANUAL_MAX,
+            MinMax::Min => MANUAL_MIN,
+        }
+    }
+}
+
+/// `lhs OP rhs` where `OP` is a comparison; used to reason about which operand the
+/// condition selects when it holds.
+struct Cmp<'tcx> {
+    op: BinOpKind,
+    lhs: &'tcx Expr<'tcx>,
+    rhs: &'tcx Expr<'tcx>,
+}
+
+impl<'tcx> Cmp<'tcx> {
+    fn new(cond: &'tcx Expr<'tcx>) -> Option<Self> {
+        if let ExprKind::Binary(op, lhs, rhs) = peel_blocks(cond).kind
+            && matches!(op.node, BinOpKind::Lt | BinOpKind::Le | BinOpKind::Gt | BinOpKind::Ge)
+        {
+            Some(Self { op: op.node, lhs, rhs })
+        } else {
+            None
+        }
+    }
+
+    /// Returns the operator rewritten so that `var` sits on the left-hand side, i.e. the
+    /// direction of the comparison as seen from `var`. Returns `None` if `var` is not one
+    /// of the operands.
+    fn orient(&self, cx: &LateContext<'tcx>, var: &Expr<'tcx>, ctxt: rustc_span::SyntaxContext) -> Option<BinOpKind> {
+        if eq_expr_value(cx, ctxt, var, self.lhs) {
+            Some(self.op)
+        } else if eq_expr_value(cx, ctxt, var, self.rhs) {
+            Some(flip(self.op))
+        } else {
+            None
+        }
+    }
+}
+
+fn flip(op: BinOpKind) -> BinOpKind {
+    match op {
+        BinOpKind::Lt => BinOpKind::Gt,
+        BinOpKind::Le => BinOpKind::Ge,
+        BinOpKind::Gt => BinOpKind::Lt,
+        BinOpKind::Ge => BinOpKind::Le,
+        other => other,
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for ManualMax {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        // `Ord::max`/`Ord::min` were stabilized in 1.21.0.
+        if expr.span.from_expansion() || is_in_const_context(cx) || !self.msrv.meets(cx, msrvs::ORD_MAX_MIN) {
+            return;
+        }
+        if let Some(If { cond, then, r#else }) = If::hir(expr)
+            && let Some(cmp) = Cmp::new(cond)
+            // Only `Ord` types: `f32`/`f64` implement `PartialOrd` only, and `a.max(b)`
+            // differs from the branching form when an operand is `NaN`. Both operands must be
+            // `Ord` (and hence the same type) for `lhs.max(rhs)` to type-check.
+            && is_ord(cx, cmp.lhs)
+            && is_ord(cx, cmp.rhs)
+        {
+            let ctxt = expr.span.ctxt();
+            let found = match r#else {
+                Some(r#else) => match_select(cx, &cmp, peel_blocks(then), peel_blocks(r#else), ctxt),
+                None => match_guard(cx, &cmp, peel_blocks_with_stmt(then), ctxt),
+            };
+            if let Some((kind, recv, arg, assign_to)) = found {
+                emit(cx, kind, expr, recv, arg, assign_to);
+            }
+        }
+    }
+}
+
+/// Matches the value-returning form `if lhs OP rhs { x } else { y }`, where `{x, y}` are
+/// the two operands in either order.
+fn match_select<'tcx>(
+    cx: &LateContext<'tcx>,
+    cmp: &Cmp<'tcx>,
+    then: &'tcx Expr<'tcx>,
+    r#else: &'tcx Expr<'tcx>,
+    ctxt: rustc_span::SyntaxContext,
+) -> Option<(MinMax, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Option<&'tcx Expr<'tcx>>)> {
+    let picks_lhs = eq_expr_value(cx, ctxt, then, cmp.lhs) && eq_expr_value(cx, ctxt, r#else, cmp.rhs);
+    let picks_rhs = eq_expr_value(cx, ctxt, then, cmp.rhs) && eq_expr_value(cx, ctxt, r#else, cmp.lhs);
+    // When the condition holds: `Lt`/`Le` means `lhs` is the smaller operand, `Gt`/`Ge`
+    // means `lhs` is the larger one. Picking the larger operand => `max`, else `min`.
+    let lhs_is_greater_when_true = matches!(cmp.op, BinOpKind::Gt | BinOpKind::Ge);
+    let kind = match (picks_lhs, picks_rhs) {
+        (true, false) if lhs_is_greater_when_true => MinMax::Max,
+        (true, false) => MinMax::Min,
+        (false, true) if lhs_is_greater_when_true => MinMax::Min,
+        (false, true) => MinMax::Max,
+        _ => return None,
+    };
+    Some((kind, cmp.lhs, cmp.rhs, None))
+}
+
+/// Matches the guarding form `if x OP bound { x = bound; }` (no `else`), equivalent to
+/// `x = x.max(bound)` / `x = x.min(bound)`.
+fn match_guard<'tcx>(
+    cx: &LateContext<'tcx>,
+    cmp: &Cmp<'tcx>,
+    then: &'tcx Expr<'tcx>,
+    ctxt: rustc_span::SyntaxContext,
+) -> Option<(MinMax, &'tcx Expr<'tcx>, &'tcx Expr<'tcx>, Option<&'tcx Expr<'tcx>>)> {
+    if let ExprKind::Assign(target, value, _) = then.kind
+        // The assigned value must be the bound that is compared against, and the assignment
+        // target the clamped variable.
+        && let Some(oriented) = cmp.orient(cx, target, ctxt)
+        && (eq_expr_value(cx, ctxt, value, cmp.lhs) || eq_expr_value(cx, ctxt, value, cmp.rhs))
+        && !eq_expr_value(cx, ctxt, value, target)
+    {
+        // `if x < bound { x = bound }` raises `x` to a floor => `max`;
+        // `if x > bound { x = bound }` lowers `x` to a ceiling => `min`.
+        let kind = match oriented {
+            BinOpKind::Lt | BinOpKind::Le => MinMax::Max,
+            BinOpKind::Gt | BinOpKind::Ge => MinMax::Min,
+            _ => return None,
+        };
+        return Some((kind, target, value, Some(target)));
+    }
+    None
+}
+
+fn is_ord<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
+    let ty = cx.typeck_results().expr_ty(expr);
+    cx.tcx
+        .get_diagnostic_item(sym::Ord)
+        .is_some_and(|ord| implements_trait(cx, ty, ord, &[]))
+}
+
+fn emit<'tcx>(
+    cx: &LateContext<'tcx>,
+    kind: MinMax,
+    expr: &Expr<'tcx>,
+    recv: &Expr<'tcx>,
+    arg: &Expr<'tcx>,
+    assign_to: Option<&Expr<'tcx>>,
+) {
+    let recv = Sugg::hir(cx, recv, "..").maybe_paren();
+    let arg = Sugg::hir(cx, arg, "..");
+    let call = format!("{recv}.{}({arg})", kind.method());
+    let sugg = match assign_to {
+        // The guard form replaces a whole `if` *statement*, so the assignment needs its
+        // own terminating `;`; the value-returning form is an expression and must not.
+        Some(target) => format!("{} = {call};", Sugg::hir(cx, target, "..")),
+        None => call,
+    };
+    span_lint_and_sugg(
+        cx,
+        kind.lint(),
+        expr.span,
+        format!("this `if` expression is a manual `{}`", kind.method()),
+        "replace with",
+        sugg,
+        // The branching form re-evaluates the selected operand, so operands with side
+        // effects could behave differently.
+        Applicability::MaybeIncorrect,
+    );
+}

clippy_utils/src/msrvs.rs

@@ -81,6 +81,7 @@ msrv_aliases! {
     1,27,0 { ITERATOR_TRY_FOLD, DOUBLE_ENDED_ITERATOR_RFIND, DURATION_FROM_NANOS_MICROS }
     1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN, POINTER_ADD_SUB_METHODS }
     1,24,0 { IS_ASCII_DIGIT, PTR_NULL }
+    1,21,0 { ORD_MAX_MIN }
     1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN }
     1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR }
     1,16,0 { STR_REPEAT, RESULT_UNWRAP_OR_DEFAULT }

tests/ui/manual_clamp.fixed

@@ -1,4 +1,5 @@
 #![warn(clippy::manual_clamp)]
+#![allow(clippy::manual_max, clippy::manual_min)]
 #![expect(clippy::if_same_then_else, clippy::needless_match)]
 
 use std::cmp::{max as cmp_max, min as cmp_min};

tests/ui/manual_clamp.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::manual_clamp)]
+#![allow(clippy::manual_max, clippy::manual_min)]
 #![expect(clippy::if_same_then_else, clippy::needless_match)]
 
 use std::cmp::{max as cmp_max, min as cmp_min};