diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs index c562ceb5bce..84fb183e3f7 100644 --- a/clippy_lints/src/manual_unwrap_or_default.rs +++ b/clippy_lints/src/manual_unwrap_or_default.rs @@ -1,13 +1,14 @@ -use clippy_utils::sugg::Sugg; use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatKind, QPath}; use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::ty::GenericArgKind; use rustc_session::declare_lint_pass; use rustc_span::sym; use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_opt; +use clippy_utils::higher::IfLetOrMatch; +use clippy_utils::sugg::Sugg; use clippy_utils::ty::implements_trait; use clippy_utils::{in_constant, is_default_equivalent, peel_blocks, span_contains_comment}; @@ -105,19 +106,39 @@ fn get_some_and_none_bodies<'tcx>( } } -fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { - let ExprKind::Match(match_expr, [arm1, arm2], MatchSource::Normal | MatchSource::ForLoopDesugar) = expr.kind else { - return false; +#[allow(clippy::needless_pass_by_value)] +fn handle<'tcx>(cx: &LateContext<'tcx>, if_let_or_match: IfLetOrMatch<'tcx>, expr: &'tcx Expr<'tcx>) { + // Get expr_name ("if let" or "match" depending on kind of expression), the condition, the body for + // the some arm, the body for the none arm and the binding id of the some arm + let (expr_name, condition, body_some, body_none, binding_id) = match if_let_or_match { + IfLetOrMatch::Match(condition, [arm1, arm2], MatchSource::Normal | MatchSource::ForLoopDesugar) + // Make sure there are no guards to keep things simple + if arm1.guard.is_none() + && arm2.guard.is_none() + // Get the some and none bodies and the binding id of the some arm + && let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2) => + { + ("match", condition, body_some, body_none, binding_id) + }, + IfLetOrMatch::IfLet(condition, pat, if_expr, Some(else_expr), _) + if let Some(binding_id) = get_some(cx, pat) => + { + ("if let", condition, if_expr, else_expr, binding_id) + }, + _ => { + // All other cases (match with number of arms != 2, if let without else, etc.) + return; + }, }; - // We don't want conditions on the arms to simplify things. - if arm1.guard.is_none() - && arm2.guard.is_none() - // We check that the returned type implements the `Default` trait. - && let match_ty = cx.typeck_results().expr_ty(expr) - && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) - && implements_trait(cx, match_ty, default_trait_id, &[]) - // We now get the bodies for both the `Some` and `None` arms. - && let Some(((body_some, binding_id), body_none)) = get_some_and_none_bodies(cx, arm1, arm2) + + // We check if the return type of the expression implements Default. + let expr_type = cx.typeck_results().expr_ty(expr); + if let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) + && implements_trait(cx, expr_type, default_trait_id, &[]) + // We check if the initial condition implements Default. + && let Some(condition_ty) = cx.typeck_results().expr_ty(condition).walk().nth(1) + && let GenericArgKind::Type(condition_ty) = condition_ty.unpack() + && implements_trait(cx, condition_ty, default_trait_id, &[]) // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(body_some).kind && let Res::Local(local_id) = path.res @@ -125,8 +146,9 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { // We now check the `None` arm is calling a method equivalent to `Default::default`. && let body_none = peel_blocks(body_none) && is_default_equivalent(cx, body_none) - && let Some(receiver) = Sugg::hir_opt(cx, match_expr).map(Sugg::maybe_par) + && let Some(receiver) = Sugg::hir_opt(cx, condition).map(Sugg::maybe_par) { + // Machine applicable only if there are no comments present let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { Applicability::MaybeIncorrect } else { @@ -136,48 +158,12 @@ fn handle_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { cx, MANUAL_UNWRAP_OR_DEFAULT, expr.span, - "match can be simplified with `.unwrap_or_default()`", + format!("{expr_name} can be simplified with `.unwrap_or_default()`"), "replace it with", format!("{receiver}.unwrap_or_default()"), applicability, ); } - true -} - -fn handle_if_let<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if let ExprKind::If(cond, if_block, Some(else_expr)) = expr.kind - && let ExprKind::Let(let_) = cond.kind - && let ExprKind::Block(_, _) = else_expr.kind - // We check that the returned type implements the `Default` trait. - && let match_ty = cx.typeck_results().expr_ty(expr) - && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) - && implements_trait(cx, match_ty, default_trait_id, &[]) - && let Some(binding_id) = get_some(cx, let_.pat) - // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. - && let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(if_block).kind - && let Res::Local(local_id) = path.res - && local_id == binding_id - // We now check the `None` arm is calling a method equivalent to `Default::default`. - && let body_else = peel_blocks(else_expr) - && is_default_equivalent(cx, body_else) - && let Some(if_let_expr_snippet) = snippet_opt(cx, let_.init.span) - { - let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { - Applicability::MaybeIncorrect - } else { - Applicability::MachineApplicable - }; - span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR_DEFAULT, - expr.span, - "if let can be simplified with `.unwrap_or_default()`", - "replace it with", - format!("{if_let_expr_snippet}.unwrap_or_default()"), - applicability, - ); - } } impl<'tcx> LateLintPass<'tcx> for ManualUnwrapOrDefault { @@ -185,8 +171,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { if expr.span.from_expansion() || in_constant(cx, expr.hir_id) { return; } - if !handle_match(cx, expr) { - handle_if_let(cx, expr); + // Call handle only if the expression is `if let` or `match` + if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, expr) { + handle(cx, if_let_or_match, expr); } } } diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index a0b707628a8..d6e736ba9cc 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -16,6 +16,16 @@ fn main() { let x: Option> = None; x.unwrap_or_default(); + + // Issue #12564 + // No error as &Vec<_> doesn't implement std::default::Default + let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]); + let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] }; + // Same code as above written using match. + let x: &[_] = match map.get(&0) { + Some(x) => x, + None => &[], + }; } // Issue #12531 diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index 1d4cca12f6c..462d5d90ee7 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -37,6 +37,16 @@ fn main() { } else { Vec::default() }; + + // Issue #12564 + // No error as &Vec<_> doesn't implement std::default::Default + let mut map = std::collections::HashMap::from([(0, vec![0; 3]), (1, vec![1; 3]), (2, vec![2])]); + let x: &[_] = if let Some(x) = map.get(&0) { x } else { &[] }; + // Same code as above written using match. + let x: &[_] = match map.get(&0) { + Some(x) => x, + None => &[], + }; } // Issue #12531 diff --git a/tests/ui/manual_unwrap_or_default.stderr b/tests/ui/manual_unwrap_or_default.stderr index d89212e6045..3f1da444301 100644 --- a/tests/ui/manual_unwrap_or_default.stderr +++ b/tests/ui/manual_unwrap_or_default.stderr @@ -53,7 +53,7 @@ LL | | }; | |_____^ help: replace it with: `x.unwrap_or_default()` error: match can be simplified with `.unwrap_or_default()` - --> tests/ui/manual_unwrap_or_default.rs:46:20 + --> tests/ui/manual_unwrap_or_default.rs:56:20 | LL | Some(_) => match *b { | ____________________^