diff --git a/CHANGELOG.md b/CHANGELOG.md index ef6140152ffc..9f12a6735962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3985,6 +3985,7 @@ Released 2018-09-13 [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits +[`manual_clamp`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_clamp [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map [`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 435411642a75..9ad2a88eb26c 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -125,6 +125,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(main_recursion::MAIN_RECURSION), LintId::of(manual_async_fn::MANUAL_ASYNC_FN), LintId::of(manual_bits::MANUAL_BITS), + LintId::of(manual_clamp::MANUAL_CLAMP), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID), LintId::of(manual_retain::MANUAL_RETAIN), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 185189a6af5b..a58d066fa6b6 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -22,6 +22,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(loops::MANUAL_FLATTEN), LintId::of(loops::SINGLE_ELEMENT_LOOP), LintId::of(loops::WHILE_LET_LOOP), + LintId::of(manual_clamp::MANUAL_CLAMP), LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID), LintId::of(manual_strip::MANUAL_STRIP), LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index ee08d802ccfb..f3ddac49ace1 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -245,6 +245,7 @@ store.register_lints(&[ manual_assert::MANUAL_ASSERT, manual_async_fn::MANUAL_ASYNC_FN, manual_bits::MANUAL_BITS, + manual_clamp::MANUAL_CLAMP, manual_instant_elapsed::MANUAL_INSTANT_ELAPSED, manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, manual_rem_euclid::MANUAL_REM_EUCLID, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fde8aa9f9217..4da3c9ece665 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -268,6 +268,7 @@ mod main_recursion; mod manual_assert; mod manual_async_fn; mod manual_bits; +mod manual_clamp; mod manual_instant_elapsed; mod manual_non_exhaustive; mod manual_rem_euclid; @@ -899,6 +900,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(manual_instant_elapsed::ManualInstantElapsed)); store.register_late_pass(|_| Box::new(partialeq_to_none::PartialeqToNone)); + store.register_late_pass(move |_| Box::new(manual_clamp::ManualClamp::new(msrv))); store.register_late_pass(|_| Box::new(manual_string_new::ManualStringNew)); store.register_late_pass(|_| Box::new(unused_peekable::UnusedPeekable)); store.register_early_pass(|| Box::new(multi_assignments::MultiAssignments)); diff --git a/clippy_lints/src/manual_clamp.rs b/clippy_lints/src/manual_clamp.rs new file mode 100644 index 000000000000..ac5c24ee604b --- /dev/null +++ b/clippy_lints/src/manual_clamp.rs @@ -0,0 +1,715 @@ +use itertools::Itertools; +use rustc_errors::Diagnostic; +use rustc_hir::{ + def::Res, Arm, BinOpKind, Block, Expr, ExprKind, Guard, HirId, PatKind, PathSegment, PrimTy, QPath, StmtKind, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{symbol::sym, Span}; +use std::ops::Deref; + +use clippy_utils::{ + diagnostics::{span_lint_and_then, span_lint_hir_and_then}, + eq_expr_value, get_trait_def_id, + higher::If, + is_diag_trait_item, is_trait_method, meets_msrv, msrvs, path_res, path_to_local_id, paths, peel_blocks, + peel_blocks_with_stmt, + sugg::Sugg, + ty::implements_trait, + visitors::is_const_evaluatable, + MaybePath, +}; +use rustc_errors::Applicability; + +declare_clippy_lint! { + /// ### What it does + /// Identifies good opportunities for a clamp function from std or core, and suggests using it. + /// + /// ### Why is this bad? + /// clamp is much shorter, easier to read, and doesn't use any control flow. + /// + /// ### Known issue(s) + /// If the clamped variable is NaN this suggestion will cause the code to propagate NaN + /// rather than returning either `max` or `min`. + /// + /// `clamp` functions will panic if `max < min`, `max.is_nan()`, or `min.is_nan()`. + /// Some may consider panicking in these situations to be desirable, but it also may + /// introduce panicking where there wasn't any before. + /// + /// ### Examples + /// ```rust + /// # let (input, min, max) = (0, -2, 1); + /// if input > max { + /// max + /// } else if input < min { + /// min + /// } else { + /// input + /// } + /// # ; + /// ``` + /// + /// ```rust + /// # let (input, min, max) = (0, -2, 1); + /// input.max(min).min(max) + /// # ; + /// ``` + /// + /// ```rust + /// # let (input, min, max) = (0, -2, 1); + /// match input { + /// x if x > max => max, + /// x if x < min => min, + /// x => x, + /// } + /// # ; + /// ``` + /// + /// ```rust + /// # let (input, min, max) = (0, -2, 1); + /// let mut x = input; + /// if x < min { x = min; } + /// if x > max { x = max; } + /// ``` + /// Use instead: + /// ```rust + /// # let (input, min, max) = (0, -2, 1); + /// input.clamp(min, max) + /// # ; + /// ``` + #[clippy::version = "1.66.0"] + pub MANUAL_CLAMP, + complexity, + "using a clamp pattern instead of the clamp function" +} +impl_lint_pass!(ManualClamp => [MANUAL_CLAMP]); + +pub struct ManualClamp { + msrv: Option, +} + +impl ManualClamp { + pub fn new(msrv: Option) -> Self { + Self { msrv } + } +} + +#[derive(Debug)] +struct ClampSuggestion<'tcx> { + params: InputMinMax<'tcx>, + span: Span, + make_assignment: Option<&'tcx Expr<'tcx>>, + hir_with_ignore_attr: Option, +} + +#[derive(Debug)] +struct InputMinMax<'tcx> { + input: &'tcx Expr<'tcx>, + min: &'tcx Expr<'tcx>, + max: &'tcx Expr<'tcx>, + is_float: bool, +} + +impl<'tcx> LateLintPass<'tcx> for ManualClamp { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if !meets_msrv(self.msrv, msrvs::CLAMP) { + return; + } + if !expr.span.from_expansion() { + let suggestion = is_if_elseif_else_pattern(cx, expr) + .or_else(|| is_max_min_pattern(cx, expr)) + .or_else(|| is_call_max_min_pattern(cx, expr)) + .or_else(|| is_match_pattern(cx, expr)) + .or_else(|| is_if_elseif_pattern(cx, expr)); + if let Some(suggestion) = suggestion { + emit_suggestion(cx, &suggestion); + } + } + } + + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) { + if !meets_msrv(self.msrv, msrvs::CLAMP) { + return; + } + for suggestion in is_two_if_pattern(cx, block) { + emit_suggestion(cx, &suggestion); + } + } + extract_msrv_attr!(LateContext); +} + +fn emit_suggestion<'tcx>(cx: &LateContext<'tcx>, suggestion: &ClampSuggestion<'tcx>) { + let ClampSuggestion { + params: InputMinMax { + input, + min, + max, + is_float, + }, + span, + make_assignment, + hir_with_ignore_attr, + } = suggestion; + let input = Sugg::hir(cx, input, "..").maybe_par(); + let min = Sugg::hir(cx, min, ".."); + let max = Sugg::hir(cx, max, ".."); + let semicolon = if make_assignment.is_some() { ";" } else { "" }; + let assignment = if let Some(assignment) = make_assignment { + let assignment = Sugg::hir(cx, assignment, ".."); + format!("{assignment} = ") + } else { + String::new() + }; + let suggestion = format!("{assignment}{input}.clamp({min}, {max}){semicolon}"); + let msg = "clamp-like pattern without using clamp function"; + let lint_builder = |d: &mut Diagnostic| { + d.span_suggestion(*span, "replace with clamp", suggestion, Applicability::MaybeIncorrect); + if *is_float { + d.note("clamp will panic if max < min, min.is_nan(), or max.is_nan()") + .note("clamp returns NaN if the input is NaN"); + } else { + d.note("clamp will panic if max < min"); + } + }; + if let Some(hir_id) = hir_with_ignore_attr { + span_lint_hir_and_then(cx, MANUAL_CLAMP, *hir_id, *span, msg, lint_builder); + } else { + span_lint_and_then(cx, MANUAL_CLAMP, *span, msg, lint_builder); + } +} + +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +enum TypeClampability { + Float, + Ord, +} + +impl TypeClampability { + fn is_clampable<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option { + if ty.is_floating_point() { + Some(TypeClampability::Float) + } else if get_trait_def_id(cx, &paths::ORD).map_or(false, |id| implements_trait(cx, ty, id, &[])) { + Some(TypeClampability::Ord) + } else { + None + } + } + + fn is_float(self) -> bool { + matches!(self, TypeClampability::Float) + } +} + +/// Targets patterns like +/// +/// ``` +/// # let (input, min, max) = (0, -3, 12); +/// +/// if input < min { +/// min +/// } else if input > max { +/// max +/// } else { +/// input +/// } +/// # ; +/// ``` +fn is_if_elseif_else_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + if let Some(If { + cond, + then, + r#else: Some(else_if), + }) = If::hir(expr) + && let Some(If { + cond: else_if_cond, + then: else_if_then, + r#else: Some(else_body), + }) = If::hir(peel_blocks(else_if)) + { + let params = is_clamp_meta_pattern( + cx, + &BinaryOp::new(peel_blocks(cond))?, + &BinaryOp::new(peel_blocks(else_if_cond))?, + peel_blocks(then), + peel_blocks(else_if_then), + None, + )?; + // Contents of the else should be the resolved input. + if !eq_expr_value(cx, params.input, peel_blocks(else_body)) { + return None; + } + Some(ClampSuggestion { + params, + span: expr.span, + make_assignment: None, + hir_with_ignore_attr: None, + }) + } else { + None + } +} + +/// Targets patterns like +/// +/// ``` +/// # let (input, min_value, max_value) = (0, -3, 12); +/// +/// input.max(min_value).min(max_value) +/// # ; +/// ``` +fn is_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + if let ExprKind::MethodCall(seg_second, receiver, [arg_second], _) = &expr.kind + && (cx.typeck_results().expr_ty_adjusted(receiver).is_floating_point() || is_trait_method(cx, expr, sym::Ord)) + && let ExprKind::MethodCall(seg_first, input, [arg_first], _) = &receiver.kind + && (cx.typeck_results().expr_ty_adjusted(input).is_floating_point() || is_trait_method(cx, receiver, sym::Ord)) + { + let is_float = cx.typeck_results().expr_ty_adjusted(input).is_floating_point(); + let (min, max) = match (seg_first.ident.as_str(), seg_second.ident.as_str()) { + ("min", "max") => (arg_second, arg_first), + ("max", "min") => (arg_first, arg_second), + _ => return None, + }; + Some(ClampSuggestion { + params: InputMinMax { input, min, max, is_float }, + span: expr.span, + make_assignment: None, + hir_with_ignore_attr: None, + }) + } else { + None + } +} + +/// Targets patterns like +/// +/// ``` +/// # let (input, min_value, max_value) = (0, -3, 12); +/// # use std::cmp::{max, min}; +/// min(max(input, min_value), max_value) +/// # ; +/// ``` +fn is_call_max_min_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + fn segment<'tcx>(cx: &LateContext<'_>, func: &Expr<'tcx>) -> Option> { + match func.kind { + ExprKind::Path(QPath::Resolved(None, path)) => { + let id = path.res.opt_def_id()?; + match cx.tcx.get_diagnostic_name(id) { + Some(sym::cmp_min) => Some(FunctionType::CmpMin), + Some(sym::cmp_max) => Some(FunctionType::CmpMax), + _ if is_diag_trait_item(cx, id, sym::Ord) => { + Some(FunctionType::OrdOrFloat(path.segments.last().expect("infallible"))) + }, + _ => None, + } + }, + ExprKind::Path(QPath::TypeRelative(ty, seg)) => { + matches!(path_res(cx, ty), Res::PrimTy(PrimTy::Float(_))).then(|| FunctionType::OrdOrFloat(seg)) + }, + _ => None, + } + } + + enum FunctionType<'tcx> { + CmpMin, + CmpMax, + OrdOrFloat(&'tcx PathSegment<'tcx>), + } + + fn check<'tcx>( + cx: &LateContext<'tcx>, + outer_fn: &'tcx Expr<'tcx>, + inner_call: &'tcx Expr<'tcx>, + outer_arg: &'tcx Expr<'tcx>, + span: Span, + ) -> Option> { + if let ExprKind::Call(inner_fn, &[ref first, ref second]) = &inner_call.kind + && let Some(inner_seg) = segment(cx, inner_fn) + && let Some(outer_seg) = segment(cx, outer_fn) + { + let (input, inner_arg) = match (is_const_evaluatable(cx, first), is_const_evaluatable(cx, second)) { + (true, false) => (second, first), + (false, true) => (first, second), + _ => return None, + }; + let is_float = cx.typeck_results().expr_ty_adjusted(input).is_floating_point(); + let (min, max) = match (inner_seg, outer_seg) { + (FunctionType::CmpMin, FunctionType::CmpMax) => (outer_arg, inner_arg), + (FunctionType::CmpMax, FunctionType::CmpMin) => (inner_arg, outer_arg), + (FunctionType::OrdOrFloat(first_segment), FunctionType::OrdOrFloat(second_segment)) => { + match (first_segment.ident.as_str(), second_segment.ident.as_str()) { + ("min", "max") => (outer_arg, inner_arg), + ("max", "min") => (inner_arg, outer_arg), + _ => return None, + } + } + _ => return None, + }; + Some(ClampSuggestion { + params: InputMinMax { input, min, max, is_float }, + span, + make_assignment: None, + hir_with_ignore_attr: None, + }) + } else { + None + } + } + + if let ExprKind::Call(outer_fn, [first, second]) = &expr.kind { + check(cx, outer_fn, first, second, expr.span).or_else(|| check(cx, outer_fn, second, first, expr.span)) + } else { + None + } +} + +/// Targets patterns like +/// +/// ``` +/// # let (input, min, max) = (0, -3, 12); +/// +/// match input { +/// input if input > max => max, +/// input if input < min => min, +/// input => input, +/// } +/// # ; +/// ``` +fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + if let ExprKind::Match(value, &[ref first_arm, ref second_arm, ref last_arm], rustc_hir::MatchSource::Normal) = + &expr.kind + { + // Find possible min/max branches + let minmax_values = |a: &'tcx Arm<'tcx>| { + if let PatKind::Binding(_, var_hir_id, _, None) = &a.pat.kind + && let Some(Guard::If(e)) = a.guard { + Some((e, var_hir_id, a.body)) + } else { + None + } + }; + let (first, first_hir_id, first_expr) = minmax_values(first_arm)?; + let (second, second_hir_id, second_expr) = minmax_values(second_arm)?; + let first = BinaryOp::new(first)?; + let second = BinaryOp::new(second)?; + if let PatKind::Binding(_, binding, _, None) = &last_arm.pat.kind + && path_to_local_id(peel_blocks_with_stmt(last_arm.body), *binding) + && last_arm.guard.is_none() + { + // Proceed as normal + } else { + return None; + } + if let Some(params) = is_clamp_meta_pattern( + cx, + &first, + &second, + first_expr, + second_expr, + Some((*first_hir_id, *second_hir_id)), + ) { + return Some(ClampSuggestion { + params: InputMinMax { + input: value, + min: params.min, + max: params.max, + is_float: params.is_float, + }, + span: expr.span, + make_assignment: None, + hir_with_ignore_attr: None, + }); + } + } + None +} + +/// Targets patterns like +/// +/// ``` +/// # let (input, min, max) = (0, -3, 12); +/// +/// let mut x = input; +/// if x < min { x = min; } +/// if x > max { x = max; } +/// ``` +fn is_two_if_pattern<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) -> Vec> { + block_stmt_with_last(block) + .tuple_windows() + .filter_map(|(maybe_set_first, maybe_set_second)| { + if let StmtKind::Expr(first_expr) = *maybe_set_first + && let StmtKind::Expr(second_expr) = *maybe_set_second + && let Some(If { cond: first_cond, then: first_then, r#else: None }) = If::hir(first_expr) + && let Some(If { cond: second_cond, then: second_then, r#else: None }) = If::hir(second_expr) + && let ExprKind::Assign( + maybe_input_first_path, + maybe_min_max_first, + _ + ) = peel_blocks_with_stmt(first_then).kind + && let ExprKind::Assign( + maybe_input_second_path, + maybe_min_max_second, + _ + ) = peel_blocks_with_stmt(second_then).kind + && eq_expr_value(cx, maybe_input_first_path, maybe_input_second_path) + && let Some(first_bin) = BinaryOp::new(first_cond) + && let Some(second_bin) = BinaryOp::new(second_cond) + && let Some(input_min_max) = is_clamp_meta_pattern( + cx, + &first_bin, + &second_bin, + maybe_min_max_first, + maybe_min_max_second, + None + ) + { + Some(ClampSuggestion { + params: InputMinMax { + input: maybe_input_first_path, + min: input_min_max.min, + max: input_min_max.max, + is_float: input_min_max.is_float, + }, + span: first_expr.span.to(second_expr.span), + make_assignment: Some(maybe_input_first_path), + hir_with_ignore_attr: Some(first_expr.hir_id()), + }) + } else { + None + } + }) + .collect() +} + +/// Targets patterns like +/// +/// ``` +/// # let (mut input, min, max) = (0, -3, 12); +/// +/// if input < min { +/// input = min; +/// } else if input > max { +/// input = max; +/// } +/// ``` +fn is_if_elseif_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option> { + if let Some(If { + cond, + then, + r#else: Some(else_if), + }) = If::hir(expr) + && let Some(If { + cond: else_if_cond, + then: else_if_then, + r#else: None, + }) = If::hir(peel_blocks(else_if)) + && let ExprKind::Assign( + maybe_input_first_path, + maybe_min_max_first, + _ + ) = peel_blocks_with_stmt(then).kind + && let ExprKind::Assign( + maybe_input_second_path, + maybe_min_max_second, + _ + ) = peel_blocks_with_stmt(else_if_then).kind + { + let params = is_clamp_meta_pattern( + cx, + &BinaryOp::new(peel_blocks(cond))?, + &BinaryOp::new(peel_blocks(else_if_cond))?, + peel_blocks(maybe_min_max_first), + peel_blocks(maybe_min_max_second), + None, + )?; + if !eq_expr_value(cx, maybe_input_first_path, maybe_input_second_path) { + return None; + } + Some(ClampSuggestion { + params, + span: expr.span, + make_assignment: Some(maybe_input_first_path), + hir_with_ignore_attr: None, + }) + } else { + None + } +} + +/// `ExprKind::Binary` but more narrowly typed +#[derive(Debug, Clone, Copy)] +struct BinaryOp<'tcx> { + op: BinOpKind, + left: &'tcx Expr<'tcx>, + right: &'tcx Expr<'tcx>, +} + +impl<'tcx> BinaryOp<'tcx> { + fn new(e: &'tcx Expr<'tcx>) -> Option> { + match &e.kind { + ExprKind::Binary(op, left, right) => Some(BinaryOp { + op: op.node, + left, + right, + }), + _ => None, + } + } + + fn flip(&self) -> Self { + Self { + op: match self.op { + BinOpKind::Le => BinOpKind::Ge, + BinOpKind::Lt => BinOpKind::Gt, + BinOpKind::Ge => BinOpKind::Le, + BinOpKind::Gt => BinOpKind::Lt, + other => other, + }, + left: self.right, + right: self.left, + } + } +} + +/// The clamp meta pattern is a pattern shared between many (but not all) patterns. +/// In summary, this pattern consists of two if statements that meet many criteria, +/// - binary operators that are one of [`>`, `<`, `>=`, `<=`]. +/// - Both binary statements must have a shared argument +/// - Which can appear on the left or right side of either statement +/// - The binary operators must define a finite range for the shared argument. To put this in +/// the terms of Rust `std` library, the following ranges are acceptable +/// - `Range` +/// - `RangeInclusive` +/// And all other range types are not accepted. For the purposes of `clamp` it's irrelevant +/// whether the range is inclusive or not, the output is the same. +/// - The result of each if statement must be equal to the argument unique to that if statement. The +/// result can not be the shared argument in either case. +fn is_clamp_meta_pattern<'tcx>( + cx: &LateContext<'tcx>, + first_bin: &BinaryOp<'tcx>, + second_bin: &BinaryOp<'tcx>, + first_expr: &'tcx Expr<'tcx>, + second_expr: &'tcx Expr<'tcx>, + // This parameters is exclusively for the match pattern. + // It exists because the variable bindings used in that pattern + // refer to the variable bound in the match arm, not the variable + // bound outside of it. Fortunately due to context we know this has to + // be the input variable, not the min or max. + input_hir_ids: Option<(HirId, HirId)>, +) -> Option> { + fn check<'tcx>( + cx: &LateContext<'tcx>, + first_bin: &BinaryOp<'tcx>, + second_bin: &BinaryOp<'tcx>, + first_expr: &'tcx Expr<'tcx>, + second_expr: &'tcx Expr<'tcx>, + input_hir_ids: Option<(HirId, HirId)>, + is_float: bool, + ) -> Option> { + match (&first_bin.op, &second_bin.op) { + (BinOpKind::Ge | BinOpKind::Gt, BinOpKind::Le | BinOpKind::Lt) => { + let (min, max) = (second_expr, first_expr); + let refers_to_input = match input_hir_ids { + Some((first_hir_id, second_hir_id)) => { + path_to_local_id(peel_blocks(first_bin.left), first_hir_id) + && path_to_local_id(peel_blocks(second_bin.left), second_hir_id) + }, + None => eq_expr_value(cx, first_bin.left, second_bin.left), + }; + (refers_to_input + && eq_expr_value(cx, first_bin.right, first_expr) + && eq_expr_value(cx, second_bin.right, second_expr)) + .then_some(InputMinMax { + input: first_bin.left, + min, + max, + is_float, + }) + }, + _ => None, + } + } + // First filter out any expressions with side effects + let exprs = [ + first_bin.left, + first_bin.right, + second_bin.left, + second_bin.right, + first_expr, + second_expr, + ]; + let clampability = TypeClampability::is_clampable(cx, cx.typeck_results().expr_ty(first_expr))?; + let is_float = clampability.is_float(); + if exprs.iter().any(|e| peel_blocks(e).can_have_side_effects()) { + return None; + } + if !(is_ord_op(first_bin.op) && is_ord_op(second_bin.op)) { + return None; + } + let cases = [ + (*first_bin, *second_bin), + (first_bin.flip(), second_bin.flip()), + (first_bin.flip(), *second_bin), + (*first_bin, second_bin.flip()), + ]; + + cases.into_iter().find_map(|(first, second)| { + check(cx, &first, &second, first_expr, second_expr, input_hir_ids, is_float).or_else(|| { + check( + cx, + &second, + &first, + second_expr, + first_expr, + input_hir_ids.map(|(l, r)| (r, l)), + is_float, + ) + }) + }) +} + +fn block_stmt_with_last<'tcx>(block: &'tcx Block<'tcx>) -> impl Iterator> { + block + .stmts + .iter() + .map(|s| MaybeBorrowedStmtKind::Borrowed(&s.kind)) + .chain( + block + .expr + .as_ref() + .map(|e| MaybeBorrowedStmtKind::Owned(StmtKind::Expr(e))), + ) +} + +fn is_ord_op(op: BinOpKind) -> bool { + matches!(op, BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt) +} + +/// Really similar to Cow, but doesn't have a `Clone` requirement. +#[derive(Debug)] +enum MaybeBorrowedStmtKind<'a> { + Borrowed(&'a StmtKind<'a>), + Owned(StmtKind<'a>), +} + +impl<'a> Clone for MaybeBorrowedStmtKind<'a> { + fn clone(&self) -> Self { + match self { + Self::Borrowed(t) => Self::Borrowed(t), + Self::Owned(StmtKind::Expr(e)) => Self::Owned(StmtKind::Expr(e)), + Self::Owned(_) => unreachable!("Owned should only ever contain a StmtKind::Expr."), + } + } +} + +impl<'a> Deref for MaybeBorrowedStmtKind<'a> { + type Target = StmtKind<'a>; + + fn deref(&self) -> &Self::Target { + match self { + Self::Borrowed(t) => t, + Self::Owned(t) => t, + } + } +} diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index a8265b50f273..2b336e87ef76 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -213,7 +213,7 @@ define_Conf! { /// /// Suppress lints whenever the suggested change would cause breakage for other crates. (avoid_breaking_exported_api: bool = true), - /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS. + /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP. /// /// The minimum rust version that the project supports (msrv: Option = None), diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 904091c57e83..8b843732a236 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -17,7 +17,7 @@ msrv_aliases! { 1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR } 1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST } 1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS } - 1,50,0 { BOOL_THEN } + 1,50,0 { BOOL_THEN, CLAMP } 1,47,0 { TAU } 1,46,0 { CONST_IF_MATCH } 1,45,0 { STR_STRIP_PREFIX } diff --git a/src/docs.rs b/src/docs.rs index 166be0618ffd..39540e4b0489 100644 --- a/src/docs.rs +++ b/src/docs.rs @@ -255,6 +255,7 @@ docs! { "manual_assert", "manual_async_fn", "manual_bits", + "manual_clamp", "manual_filter_map", "manual_find", "manual_find_map", diff --git a/src/docs/manual_clamp.txt b/src/docs/manual_clamp.txt new file mode 100644 index 000000000000..8993f6683adf --- /dev/null +++ b/src/docs/manual_clamp.txt @@ -0,0 +1,46 @@ +### What it does +Identifies good opportunities for a clamp function from std or core, and suggests using it. + +### Why is this bad? +clamp is much shorter, easier to read, and doesn't use any control flow. + +### Known issue(s) +If the clamped variable is NaN this suggestion will cause the code to propagate NaN +rather than returning either `max` or `min`. + +`clamp` functions will panic if `max < min`, `max.is_nan()`, or `min.is_nan()`. +Some may consider panicking in these situations to be desirable, but it also may +introduce panicking where there wasn't any before. + +### Examples +``` +if input > max { + max +} else if input < min { + min +} else { + input +} +``` + +``` +input.max(min).min(max) +``` + +``` +match input { + x if x > max => max, + x if x < min => min, + x => x, +} +``` + +``` +let mut x = input; +if x < min { x = min; } +if x > max { x = max; } +``` +Use instead: +``` +input.clamp(min, max) +``` \ No newline at end of file diff --git a/tests/ui/manual_clamp.rs b/tests/ui/manual_clamp.rs new file mode 100644 index 000000000000..54fd888af99f --- /dev/null +++ b/tests/ui/manual_clamp.rs @@ -0,0 +1,304 @@ +#![warn(clippy::manual_clamp)] +#![allow( + unused, + dead_code, + clippy::unnecessary_operation, + clippy::no_effect, + clippy::if_same_then_else +)] + +use std::cmp::{max as cmp_max, min as cmp_min}; + +const CONST_MAX: i32 = 10; +const CONST_MIN: i32 = 4; + +const CONST_F64_MAX: f64 = 10.0; +const CONST_F64_MIN: f64 = 4.0; + +fn main() { + let (input, min, max) = (0, -2, 3); + // Lint + let x0 = if max < input { + max + } else if min > input { + min + } else { + input + }; + + let x1 = if input > max { + max + } else if input < min { + min + } else { + input + }; + + let x2 = if input < min { + min + } else if input > max { + max + } else { + input + }; + + let x3 = if min > input { + min + } else if max < input { + max + } else { + input + }; + + let x4 = input.max(min).min(max); + + let x5 = input.min(max).max(min); + + let x6 = match input { + x if x > max => max, + x if x < min => min, + x => x, + }; + + let x7 = match input { + x if x < min => min, + x if x > max => max, + x => x, + }; + + let x8 = match input { + x if max < x => max, + x if min > x => min, + x => x, + }; + + let mut x9 = input; + if x9 < min { + x9 = min; + } + if x9 > max { + x9 = max; + } + + let x10 = match input { + x if min > x => min, + x if max < x => max, + x => x, + }; + + let mut x11 = input; + let _ = 1; + if x11 > max { + x11 = max; + } + if x11 < min { + x11 = min; + } + + let mut x12 = input; + if min > x12 { + x12 = min; + } + if max < x12 { + x12 = max; + } + + let mut x13 = input; + if max < x13 { + x13 = max; + } + if min > x13 { + x13 = min; + } + + let x14 = if input > CONST_MAX { + CONST_MAX + } else if input < CONST_MIN { + CONST_MIN + } else { + input + }; + { + let (input, min, max) = (0.0f64, -2.0, 3.0); + let x15 = if input > max { + max + } else if input < min { + min + } else { + input + }; + } + { + let input: i32 = cmp_min_max(1); + // These can only be detected if exactly one of the arguments to the inner function is const. + let x16 = cmp_max(cmp_min(input, CONST_MAX), CONST_MIN); + let x17 = cmp_min(cmp_max(input, CONST_MIN), CONST_MAX); + let x18 = cmp_max(CONST_MIN, cmp_min(input, CONST_MAX)); + let x19 = cmp_min(CONST_MAX, cmp_max(input, CONST_MIN)); + let x20 = cmp_max(cmp_min(CONST_MAX, input), CONST_MIN); + let x21 = cmp_min(cmp_max(CONST_MIN, input), CONST_MAX); + let x22 = cmp_max(CONST_MIN, cmp_min(CONST_MAX, input)); + let x23 = cmp_min(CONST_MAX, cmp_max(CONST_MIN, input)); + let input: f64 = cmp_min_max(1) as f64; + let x24 = f64::max(f64::min(input, CONST_F64_MAX), CONST_F64_MIN); + let x25 = f64::min(f64::max(input, CONST_F64_MIN), CONST_F64_MAX); + let x26 = f64::max(CONST_F64_MIN, f64::min(input, CONST_F64_MAX)); + let x27 = f64::min(CONST_F64_MAX, f64::max(input, CONST_F64_MIN)); + let x28 = f64::max(f64::min(CONST_F64_MAX, input), CONST_F64_MIN); + let x29 = f64::min(f64::max(CONST_F64_MIN, input), CONST_F64_MAX); + let x30 = f64::max(CONST_F64_MIN, f64::min(CONST_F64_MAX, input)); + let x31 = f64::min(CONST_F64_MAX, f64::max(CONST_F64_MIN, input)); + } + let mut x32 = input; + if x32 < min { + x32 = min; + } else if x32 > max { + x32 = max; + } + + // It's important this be the last set of statements + let mut x33 = input; + if max < x33 { + x33 = max; + } + if min > x33 { + x33 = min; + } +} + +// This code intentionally nonsense. +fn no_lint() { + let (input, min, max) = (0, -2, 3); + let x0 = if max < input { + max + } else if min > input { + max + } else { + min + }; + + let x1 = if input > max { + max + } else if input > min { + min + } else { + max + }; + + let x2 = if max < min { + min + } else if input > max { + input + } else { + input + }; + + let x3 = if min > input { + input + } else if max < input { + max + } else { + max + }; + + let x6 = match input { + x if x < max => x, + x if x < min => x, + x => x, + }; + + let x7 = match input { + x if x < min => max, + x if x > max => min, + x => x, + }; + + let x8 = match input { + x if max > x => max, + x if min > x => min, + x => x, + }; + + let mut x9 = input; + if x9 > min { + x9 = min; + } + if x9 > max { + x9 = max; + } + + let x10 = match input { + x if min > x => min, + x if max < x => max, + x => min, + }; + + let mut x11 = input; + if x11 > max { + x11 = min; + } + if x11 < min { + x11 = max; + } + + let mut x12 = input; + if min > x12 { + x12 = max * 3; + } + if max < x12 { + x12 = min; + } + + let mut x13 = input; + if max < x13 { + let x13 = max; + } + if min > x13 { + x13 = min; + } + let mut x14 = input; + if x14 < min { + x14 = 3; + } else if x14 > max { + x14 = max; + } + { + let input: i32 = cmp_min_max(1); + // These can only be detected if exactly one of the arguments to the inner function is const. + let x16 = cmp_max(cmp_max(input, CONST_MAX), CONST_MIN); + let x17 = cmp_min(cmp_min(input, CONST_MIN), CONST_MAX); + let x18 = cmp_max(CONST_MIN, cmp_max(input, CONST_MAX)); + let x19 = cmp_min(CONST_MAX, cmp_min(input, CONST_MIN)); + let x20 = cmp_max(cmp_max(CONST_MAX, input), CONST_MIN); + let x21 = cmp_min(cmp_min(CONST_MIN, input), CONST_MAX); + let x22 = cmp_max(CONST_MIN, cmp_max(CONST_MAX, input)); + let x23 = cmp_min(CONST_MAX, cmp_min(CONST_MIN, input)); + let input: f64 = cmp_min_max(1) as f64; + let x24 = f64::max(f64::max(input, CONST_F64_MAX), CONST_F64_MIN); + let x25 = f64::min(f64::min(input, CONST_F64_MIN), CONST_F64_MAX); + let x26 = f64::max(CONST_F64_MIN, f64::max(input, CONST_F64_MAX)); + let x27 = f64::min(CONST_F64_MAX, f64::min(input, CONST_F64_MIN)); + let x28 = f64::max(f64::max(CONST_F64_MAX, input), CONST_F64_MIN); + let x29 = f64::min(f64::min(CONST_F64_MIN, input), CONST_F64_MAX); + let x30 = f64::max(CONST_F64_MIN, f64::max(CONST_F64_MAX, input)); + let x31 = f64::min(CONST_F64_MAX, f64::min(CONST_F64_MIN, input)); + let x32 = f64::min(CONST_F64_MAX, f64::min(CONST_F64_MIN, CONST_F64_MAX)); + } +} + +fn dont_tell_me_what_to_do() { + let (input, min, max) = (0, -2, 3); + let mut x_never = input; + #[allow(clippy::manual_clamp)] + if x_never < min { + x_never = min; + } + if x_never > max { + x_never = max; + } +} + +/// Just to ensure this isn't const evaled +fn cmp_min_max(input: i32) -> i32 { + input * 3 +} diff --git a/tests/ui/manual_clamp.stderr b/tests/ui/manual_clamp.stderr new file mode 100644 index 000000000000..25650483595d --- /dev/null +++ b/tests/ui/manual_clamp.stderr @@ -0,0 +1,375 @@ +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:76:5 + | +LL | / if x9 < min { +LL | | x9 = min; +LL | | } +LL | | if x9 > max { +LL | | x9 = max; +LL | | } + | |_____^ help: replace with clamp: `x9 = x9.clamp(min, max);` + | + = note: `-D clippy::manual-clamp` implied by `-D warnings` + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:91:5 + | +LL | / if x11 > max { +LL | | x11 = max; +LL | | } +LL | | if x11 < min { +LL | | x11 = min; +LL | | } + | |_____^ help: replace with clamp: `x11 = x11.clamp(min, max);` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:99:5 + | +LL | / if min > x12 { +LL | | x12 = min; +LL | | } +LL | | if max < x12 { +LL | | x12 = max; +LL | | } + | |_____^ help: replace with clamp: `x12 = x12.clamp(min, max);` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:107:5 + | +LL | / if max < x13 { +LL | | x13 = max; +LL | | } +LL | | if min > x13 { +LL | | x13 = min; +LL | | } + | |_____^ help: replace with clamp: `x13 = x13.clamp(min, max);` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:161:5 + | +LL | / if max < x33 { +LL | | x33 = max; +LL | | } +LL | | if min > x33 { +LL | | x33 = min; +LL | | } + | |_____^ help: replace with clamp: `x33 = x33.clamp(min, max);` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:21:14 + | +LL | let x0 = if max < input { + | ______________^ +LL | | max +LL | | } else if min > input { +LL | | min +LL | | } else { +LL | | input +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:29:14 + | +LL | let x1 = if input > max { + | ______________^ +LL | | max +LL | | } else if input < min { +LL | | min +LL | | } else { +LL | | input +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:37:14 + | +LL | let x2 = if input < min { + | ______________^ +LL | | min +LL | | } else if input > max { +LL | | max +LL | | } else { +LL | | input +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:45:14 + | +LL | let x3 = if min > input { + | ______________^ +LL | | min +LL | | } else if max < input { +LL | | max +LL | | } else { +LL | | input +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:53:14 + | +LL | let x4 = input.max(min).min(max); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:55:14 + | +LL | let x5 = input.min(max).max(min); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:57:14 + | +LL | let x6 = match input { + | ______________^ +LL | | x if x > max => max, +LL | | x if x < min => min, +LL | | x => x, +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:63:14 + | +LL | let x7 = match input { + | ______________^ +LL | | x if x < min => min, +LL | | x if x > max => max, +LL | | x => x, +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:69:14 + | +LL | let x8 = match input { + | ______________^ +LL | | x if max < x => max, +LL | | x if min > x => min, +LL | | x => x, +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:83:15 + | +LL | let x10 = match input { + | _______________^ +LL | | x if min > x => min, +LL | | x if max < x => max, +LL | | x => x, +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:114:15 + | +LL | let x14 = if input > CONST_MAX { + | _______________^ +LL | | CONST_MAX +LL | | } else if input < CONST_MIN { +LL | | CONST_MIN +LL | | } else { +LL | | input +LL | | }; + | |_____^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:123:19 + | +LL | let x15 = if input > max { + | ___________________^ +LL | | max +LL | | } else if input < min { +LL | | min +LL | | } else { +LL | | input +LL | | }; + | |_________^ help: replace with clamp: `input.clamp(min, max)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:134:19 + | +LL | let x16 = cmp_max(cmp_min(input, CONST_MAX), CONST_MIN); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:135:19 + | +LL | let x17 = cmp_min(cmp_max(input, CONST_MIN), CONST_MAX); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:136:19 + | +LL | let x18 = cmp_max(CONST_MIN, cmp_min(input, CONST_MAX)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:137:19 + | +LL | let x19 = cmp_min(CONST_MAX, cmp_max(input, CONST_MIN)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:138:19 + | +LL | let x20 = cmp_max(cmp_min(CONST_MAX, input), CONST_MIN); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:139:19 + | +LL | let x21 = cmp_min(cmp_max(CONST_MIN, input), CONST_MAX); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:140:19 + | +LL | let x22 = cmp_max(CONST_MIN, cmp_min(CONST_MAX, input)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:141:19 + | +LL | let x23 = cmp_min(CONST_MAX, cmp_max(CONST_MIN, input)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_MIN, CONST_MAX)` + | + = note: clamp will panic if max < min + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:143:19 + | +LL | let x24 = f64::max(f64::min(input, CONST_F64_MAX), CONST_F64_MIN); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:144:19 + | +LL | let x25 = f64::min(f64::max(input, CONST_F64_MIN), CONST_F64_MAX); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:145:19 + | +LL | let x26 = f64::max(CONST_F64_MIN, f64::min(input, CONST_F64_MAX)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:146:19 + | +LL | let x27 = f64::min(CONST_F64_MAX, f64::max(input, CONST_F64_MIN)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:147:19 + | +LL | let x28 = f64::max(f64::min(CONST_F64_MAX, input), CONST_F64_MIN); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:148:19 + | +LL | let x29 = f64::min(f64::max(CONST_F64_MIN, input), CONST_F64_MAX); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:149:19 + | +LL | let x30 = f64::max(CONST_F64_MIN, f64::min(CONST_F64_MAX, input)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:150:19 + | +LL | let x31 = f64::min(CONST_F64_MAX, f64::max(CONST_F64_MIN, input)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with clamp: `input.clamp(CONST_F64_MIN, CONST_F64_MAX)` + | + = note: clamp will panic if max < min, min.is_nan(), or max.is_nan() + = note: clamp returns NaN if the input is NaN + +error: clamp-like pattern without using clamp function + --> $DIR/manual_clamp.rs:153:5 + | +LL | / if x32 < min { +LL | | x32 = min; +LL | | } else if x32 > max { +LL | | x32 = max; +LL | | } + | |_____^ help: replace with clamp: `x32 = x32.clamp(min, max);` + | + = note: clamp will panic if max < min + +error: aborting due to 34 previous errors + diff --git a/tests/ui/min_max.rs b/tests/ui/min_max.rs index b2bc97f4744a..24e52afd6917 100644 --- a/tests/ui/min_max.rs +++ b/tests/ui/min_max.rs @@ -1,4 +1,5 @@ #![warn(clippy::all)] +#![allow(clippy::manual_clamp)] use std::cmp::max as my_max; use std::cmp::min as my_min; diff --git a/tests/ui/min_max.stderr b/tests/ui/min_max.stderr index c70b77eabbd8..069d9068657e 100644 --- a/tests/ui/min_max.stderr +++ b/tests/ui/min_max.stderr @@ -1,5 +1,5 @@ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:23:5 + --> $DIR/min_max.rs:24:5 | LL | min(1, max(3, x)); | ^^^^^^^^^^^^^^^^^ @@ -7,73 +7,73 @@ LL | min(1, max(3, x)); = note: `-D clippy::min-max` implied by `-D warnings` error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:24:5 + --> $DIR/min_max.rs:25:5 | LL | min(max(3, x), 1); | ^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:25:5 + --> $DIR/min_max.rs:26:5 | LL | max(min(x, 1), 3); | ^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:26:5 + --> $DIR/min_max.rs:27:5 | LL | max(3, min(x, 1)); | ^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:28:5 + --> $DIR/min_max.rs:29:5 | LL | my_max(3, my_min(x, 1)); | ^^^^^^^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:38:5 + --> $DIR/min_max.rs:39:5 | LL | min("Apple", max("Zoo", s)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:39:5 + --> $DIR/min_max.rs:40:5 | LL | max(min(s, "Apple"), "Zoo"); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:44:5 + --> $DIR/min_max.rs:45:5 | LL | x.min(1).max(3); | ^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:45:5 + --> $DIR/min_max.rs:46:5 | LL | x.max(3).min(1); | ^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:46:5 + --> $DIR/min_max.rs:47:5 | LL | f.max(3f32).min(1f32); | ^^^^^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:52:5 + --> $DIR/min_max.rs:53:5 | LL | max(x.min(1), 3); | ^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:55:5 + --> $DIR/min_max.rs:56:5 | LL | s.max("Zoo").min("Apple"); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `min`/`max` combination leads to constant result - --> $DIR/min_max.rs:56:5 + --> $DIR/min_max.rs:57:5 | LL | s.min("Apple").max("Zoo"); | ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs index 44e407bd1ab2..c4c6391bb4c1 100644 --- a/tests/ui/min_rust_version_attr.rs +++ b/tests/ui/min_rust_version_attr.rs @@ -160,6 +160,17 @@ fn manual_rem_euclid() { let _: i32 = ((x % 4) + 4) % 4; } +fn manual_clamp() { + let (input, min, max) = (0, -1, 2); + let _ = if input < min { + min + } else if input > max { + max + } else { + input + }; +} + fn main() { filter_map_next(); checked_conversion(); @@ -180,6 +191,7 @@ fn main() { err_expect(); cast_abs_to_unsigned(); manual_rem_euclid(); + manual_clamp(); } mod just_under_msrv { diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr index b1c23b539ffd..faabb0e4386d 100644 --- a/tests/ui/min_rust_version_attr.stderr +++ b/tests/ui/min_rust_version_attr.stderr @@ -1,12 +1,12 @@ error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:204:24 + --> $DIR/min_rust_version_attr.rs:216:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::manual-strip` implied by `-D warnings` note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:203:9 + --> $DIR/min_rust_version_attr.rs:215:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -17,13 +17,13 @@ LL ~ assert_eq!(.to_uppercase(), "WORLD!"); | error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:216:24 + --> $DIR/min_rust_version_attr.rs:228:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:215:9 + --> $DIR/min_rust_version_attr.rs:227:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^