diff options
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/manual_unwrap_or_default.rs | 212 | ||||
| -rw-r--r-- | clippy_lints/src/matches/manual_unwrap_or.rs | 294 | ||||
| -rw-r--r-- | clippy_lints/src/matches/mod.rs | 38 | ||||
| -rw-r--r-- | clippy_utils/src/higher.rs | 6 | ||||
| -rw-r--r-- | clippy_utils/src/lib.rs | 1 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.fixed | 30 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.rs | 15 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or.stderr | 39 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or_default.fixed | 7 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or_default.rs | 12 | ||||
| -rw-r--r-- | tests/ui/manual_unwrap_or_default.stderr | 13 |
13 files changed, 322 insertions, 349 deletions
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index a9b6b369c4c..c2274f0a619 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -335,7 +335,6 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::manual_slice_size_calculation::MANUAL_SLICE_SIZE_CALCULATION_INFO, crate::manual_string_new::MANUAL_STRING_NEW_INFO, crate::manual_strip::MANUAL_STRIP_INFO, - crate::manual_unwrap_or_default::MANUAL_UNWRAP_OR_DEFAULT_INFO, crate::map_unit_fn::OPTION_MAP_UNIT_FN_INFO, crate::map_unit_fn::RESULT_MAP_UNIT_FN_INFO, crate::match_result_ok::MATCH_RESULT_OK_INFO, @@ -345,6 +344,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::matches::MANUAL_MAP_INFO, crate::matches::MANUAL_OK_ERR_INFO, crate::matches::MANUAL_UNWRAP_OR_INFO, + crate::matches::MANUAL_UNWRAP_OR_DEFAULT_INFO, crate::matches::MATCH_AS_REF_INFO, crate::matches::MATCH_BOOL_INFO, crate::matches::MATCH_LIKE_MATCHES_MACRO_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2d390be5248..6f2a4a4c529 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -226,7 +226,6 @@ mod manual_rotate; mod manual_slice_size_calculation; mod manual_string_new; mod manual_strip; -mod manual_unwrap_or_default; mod map_unit_fn; mod match_result_ok; mod matches; @@ -960,7 +959,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_early_pass(|| Box::new(multiple_bound_locations::MultipleBoundLocations)); store.register_late_pass(move |_| Box::new(assigning_clones::AssigningClones::new(conf))); store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); - store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed)); store.register_late_pass(move |_| Box::new(macro_metavars_in_unsafe::ExprMetavarsInUnsafe::new(conf))); store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(conf))); diff --git a/clippy_lints/src/manual_unwrap_or_default.rs b/clippy_lints/src/manual_unwrap_or_default.rs deleted file mode 100644 index 1aa12e72a6d..00000000000 --- a/clippy_lints/src/manual_unwrap_or_default.rs +++ /dev/null @@ -1,212 +0,0 @@ -use rustc_errors::Applicability; -use rustc_hir::def::Res; -use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, MatchSource, Pat, PatExpr, PatExprKind, 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::higher::IfLetOrMatch; -use clippy_utils::sugg::Sugg; -use clippy_utils::ty::{expr_type_is_certain, implements_trait}; -use clippy_utils::{is_default_equivalent, is_in_const_context, path_res, peel_blocks, span_contains_comment}; - -declare_clippy_lint! { - /// ### What it does - /// Checks if a `match` or `if let` expression can be simplified using - /// `.unwrap_or_default()`. - /// - /// ### Why is this bad? - /// It can be done in one call with `.unwrap_or_default()`. - /// - /// ### Example - /// ```no_run - /// let x: Option<String> = Some(String::new()); - /// let y: String = match x { - /// Some(v) => v, - /// None => String::new(), - /// }; - /// - /// let x: Option<Vec<String>> = Some(Vec::new()); - /// let y: Vec<String> = if let Some(v) = x { - /// v - /// } else { - /// Vec::new() - /// }; - /// ``` - /// Use instead: - /// ```no_run - /// let x: Option<String> = Some(String::new()); - /// let y: String = x.unwrap_or_default(); - /// - /// let x: Option<Vec<String>> = Some(Vec::new()); - /// let y: Vec<String> = x.unwrap_or_default(); - /// ``` - #[clippy::version = "1.79.0"] - pub MANUAL_UNWRAP_OR_DEFAULT, - suspicious, - "check if a `match` or `if let` can be simplified with `unwrap_or_default`" -} - -declare_lint_pass!(ManualUnwrapOrDefault => [MANUAL_UNWRAP_OR_DEFAULT]); - -fn get_some<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<HirId> { - if let PatKind::TupleStruct(QPath::Resolved(_, path), &[pat], _) = pat.kind - && let PatKind::Binding(_, pat_id, _, _) = pat.kind - && let Some(def_id) = path.res.opt_def_id() - // Since it comes from a pattern binding, we need to get the parent to actually match - // against it. - && let Some(def_id) = cx.tcx.opt_parent(def_id) - && (cx.tcx.lang_items().get(LangItem::OptionSome) == Some(def_id) - || cx.tcx.lang_items().get(LangItem::ResultOk) == Some(def_id)) - { - Some(pat_id) - } else { - None - } -} - -fn get_none<'tcx>(cx: &LateContext<'tcx>, arm: &Arm<'tcx>) -> Option<&'tcx Expr<'tcx>> { - if let PatKind::Expr(PatExpr { kind: PatExprKind::Path(QPath::Resolved(_, path)), .. }) = arm.pat.kind - && let Some(def_id) = path.res.opt_def_id() - // Since it comes from a pattern binding, we need to get the parent to actually match - // against it. - && let Some(def_id) = cx.tcx.opt_parent(def_id) - && cx.tcx.lang_items().get(LangItem::OptionNone) == Some(def_id) - { - Some(arm.body) - } else if let PatKind::TupleStruct(QPath::Resolved(_, path), _, _)= arm.pat.kind - && let Some(def_id) = path.res.opt_def_id() - // Since it comes from a pattern binding, we need to get the parent to actually match - // against it. - && let Some(def_id) = cx.tcx.opt_parent(def_id) - && cx.tcx.lang_items().get(LangItem::ResultErr) == Some(def_id) - { - Some(arm.body) - } else if let PatKind::Wild = arm.pat.kind { - // We consider that the `Some` check will filter it out if it's not right. - Some(arm.body) - } else { - None - } -} - -fn get_some_and_none_bodies<'tcx>( - cx: &LateContext<'tcx>, - arm1: &'tcx Arm<'tcx>, - arm2: &'tcx Arm<'tcx>, -) -> Option<((&'tcx Expr<'tcx>, HirId), &'tcx Expr<'tcx>)> { - if let Some(binding_id) = get_some(cx, arm1.pat) - && let Some(body_none) = get_none(cx, arm2) - { - Some(((arm1.body, binding_id), body_none)) - } else if let Some(binding_id) = get_some(cx, arm2.pat) - && let Some(body_none) = get_none(cx, arm1) - { - Some(((arm2.body, binding_id), body_none)) - } else { - None - } -} - -#[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 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 - && local_id == binding_id - // 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, condition).map(Sugg::maybe_paren) - { - // Machine applicable only if there are no comments present - let applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { - Applicability::MaybeIncorrect - } else { - Applicability::MachineApplicable - }; - - // We now check if the condition is a None variant, in which case we need to specify the type - if path_res(cx, condition) - .opt_def_id() - .is_some_and(|id| Some(cx.tcx.parent(id)) == cx.tcx.lang_items().option_none_variant()) - { - return span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR_DEFAULT, - expr.span, - format!("{expr_name} can be simplified with `.unwrap_or_default()`"), - "replace it with", - format!("{receiver}::<{expr_type}>.unwrap_or_default()"), - applicability, - ); - } - - // We check if the expression type is still uncertain, in which case we ask the user to specify it - if !expr_type_is_certain(cx, condition) { - return span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR_DEFAULT, - expr.span, - format!("{expr_name} can be simplified with `.unwrap_or_default()`"), - format!("ascribe the type {expr_type} and replace your expression with"), - format!("{receiver}.unwrap_or_default()"), - Applicability::Unspecified, - ); - } - - span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR_DEFAULT, - expr.span, - format!("{expr_name} can be simplified with `.unwrap_or_default()`"), - "replace it with", - format!("{receiver}.unwrap_or_default()"), - applicability, - ); - } -} - -impl<'tcx> LateLintPass<'tcx> for ManualUnwrapOrDefault { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, expr) - && !expr.span.from_expansion() - && !is_in_const_context(cx) - { - handle(cx, if_let_or_match, expr); - } - } -} diff --git a/clippy_lints/src/matches/manual_unwrap_or.rs b/clippy_lints/src/matches/manual_unwrap_or.rs index 5394b7af8ff..b64ae0b24d8 100644 --- a/clippy_lints/src/matches/manual_unwrap_or.rs +++ b/clippy_lints/src/matches/manual_unwrap_or.rs @@ -1,133 +1,219 @@ use clippy_utils::consts::ConstEvalCtxt; -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::{SpanRangeExt, indent_of, reindent_multiline}; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::usage::contains_return_break_continue_macro; -use clippy_utils::{is_res_lang_ctor, path_to_local_id, peel_blocks, sugg}; +use clippy_utils::source::{SpanRangeExt as _, indent_of, reindent_multiline}; use rustc_errors::Applicability; -use rustc_hir::LangItem::{OptionNone, ResultErr}; -use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Arm, Expr, Pat, PatExpr, PatExprKind, PatKind}; -use rustc_lint::LateContext; -use rustc_middle::ty::Ty; +use rustc_hir::def::Res; +use rustc_hir::{Arm, Expr, ExprKind, HirId, LangItem, Pat, PatExpr, PatExprKind, PatKind, QPath}; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::ty::{GenericArgKind, Ty}; use rustc_span::sym; -use super::MANUAL_UNWRAP_OR; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::{expr_type_is_certain, get_type_diagnostic_name, implements_trait}; +use clippy_utils::{is_default_equivalent, is_lint_allowed, path_res, peel_blocks, span_contains_comment}; -pub(super) fn check_match<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'tcx>, - scrutinee: &'tcx Expr<'_>, - arms: &'tcx [Arm<'_>], -) { - let ty = cx.typeck_results().expr_ty(scrutinee); - if let Some((or_arm, unwrap_arm)) = applicable_or_arm(cx, arms) { - check_and_lint(cx, expr, unwrap_arm.pat, scrutinee, unwrap_arm.body, or_arm.body, ty); +use super::{MANUAL_UNWRAP_OR, MANUAL_UNWRAP_OR_DEFAULT}; + +fn get_some(cx: &LateContext<'_>, pat: &Pat<'_>) -> Option<HirId> { + if let PatKind::TupleStruct(QPath::Resolved(_, path), &[pat], _) = pat.kind + && let PatKind::Binding(_, pat_id, _, _) = pat.kind + && let Some(def_id) = path.res.opt_def_id() + // Since it comes from a pattern binding, we need to get the parent to actually match + // against it. + && let Some(def_id) = cx.tcx.opt_parent(def_id) + && let Some(lang_item) = cx.tcx.lang_items().from_def_id(def_id) + && matches!(lang_item, LangItem::OptionSome | LangItem::ResultOk) + { + Some(pat_id) + } else { + None } } -pub(super) fn check_if_let<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - let_pat: &'tcx Pat<'_>, - let_expr: &'tcx Expr<'_>, - then_expr: &'tcx Expr<'_>, - else_expr: &'tcx Expr<'_>, -) { - let ty = cx.typeck_results().expr_ty(let_expr); - let then_ty = cx.typeck_results().expr_ty(then_expr); - // The signature is `fn unwrap_or<T>(self: Option<T>, default: T) -> T`. - // When `expr_adjustments(then_expr).is_empty()`, `T` should equate to `default`'s type. - // Otherwise, type error will occur. - if cx.typeck_results().expr_adjustments(then_expr).is_empty() - && let rustc_middle::ty::Adt(_did, args) = ty.kind() - && let Some(some_ty) = args.first().and_then(|arg| arg.as_type()) - && some_ty != then_ty +fn get_none<'tcx>(cx: &LateContext<'_>, arm: &Arm<'tcx>) -> Option<&'tcx Expr<'tcx>> { + if let PatKind::Expr(PatExpr { kind: PatExprKind::Path(QPath::Resolved(_, path)), .. }) = arm.pat.kind + && let Some(def_id) = path.res.opt_def_id() + // Since it comes from a pattern binding, we need to get the parent to actually match + // against it. + && let Some(def_id) = cx.tcx.opt_parent(def_id) + && cx.tcx.lang_items().get(LangItem::OptionNone) == Some(def_id) { - return; + Some(arm.body) + } else if let PatKind::TupleStruct(QPath::Resolved(_, path), _, _)= arm.pat.kind + && let Some(def_id) = path.res.opt_def_id() + // Since it comes from a pattern binding, we need to get the parent to actually match + // against it. + && let Some(def_id) = cx.tcx.opt_parent(def_id) + && cx.tcx.lang_items().get(LangItem::ResultErr) == Some(def_id) + { + Some(arm.body) + } else if let PatKind::Wild = arm.pat.kind { + // We consider that the `Some` check will filter it out if it's not right. + Some(arm.body) + } else { + None } - check_and_lint(cx, expr, let_pat, let_expr, then_expr, peel_blocks(else_expr), ty); } -fn check_and_lint<'tcx>( +fn get_some_and_none_bodies<'tcx>( cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - let_pat: &'tcx Pat<'_>, - let_expr: &'tcx Expr<'_>, - then_expr: &'tcx Expr<'_>, - else_expr: &'tcx Expr<'_>, - ty: Ty<'tcx>, + arm1: &'tcx Arm<'tcx>, + arm2: &'tcx Arm<'tcx>, +) -> Option<((&'tcx Expr<'tcx>, HirId), &'tcx Expr<'tcx>)> { + if let Some(binding_id) = get_some(cx, arm1.pat) + && let Some(body_none) = get_none(cx, arm2) + { + Some(((arm1.body, binding_id), body_none)) + } else if let Some(binding_id) = get_some(cx, arm2.pat) + && let Some(body_none) = get_none(cx, arm1) + { + Some(((arm2.body, binding_id), body_none)) + } else { + None + } +} + +fn handle( + cx: &LateContext<'_>, + expr: &Expr<'_>, + expr_name: &'static str, + condition: &Expr<'_>, + body_some: &Expr<'_>, + body_none: &Expr<'_>, + binding_id: HirId, ) { - if let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = let_pat.kind - && let Res::Def(DefKind::Ctor(..), ctor_id) = cx.qpath_res(qpath, let_pat.hir_id) - && let Some(variant_id) = cx.tcx.opt_parent(ctor_id) - && (cx.tcx.lang_items().option_some_variant() == Some(variant_id) - || cx.tcx.lang_items().result_ok_variant() == Some(variant_id)) - && let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind - && path_to_local_id(peel_blocks(then_expr), binding_hir_id) - && cx.typeck_results().expr_adjustments(then_expr).is_empty() - && let Some(ty_name) = find_type_name(cx, ty) - && let Some(or_body_snippet) = else_expr.span.get_source_text(cx) - && let Some(indent) = indent_of(cx, expr.span) - && ConstEvalCtxt::new(cx).eval_simple(else_expr).is_some() + // Only deal with situations where both alternatives return the same non-adjusted type. + if cx.typeck_results().expr_ty(body_some) != cx.typeck_results().expr_ty(body_none) { + return; + } + + let expr_type = cx.typeck_results().expr_ty(expr); + // We check that the `Some(x) => x` doesn't do anything apart "returning" the value in `Some`. + if let ExprKind::Path(QPath::Resolved(_, path)) = peel_blocks(body_some).kind + && let Res::Local(local_id) = path.res + && local_id == binding_id { - lint(cx, expr, let_expr, ty_name, &or_body_snippet, indent); + // Machine applicable only if there are no comments present + let mut applicability = if span_contains_comment(cx.sess().source_map(), expr.span) { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }; + let receiver = Sugg::hir_with_applicability(cx, condition, "_", &mut applicability).maybe_paren(); + + // We now check the `None` arm is calling a method equivalent to `Default::default`. + if !is_lint_allowed(cx, MANUAL_UNWRAP_OR_DEFAULT, expr.hir_id) + // We check if the return type of the expression implements Default. + && 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, &[]) + && is_default_equivalent(cx, peel_blocks(body_none)) + { + // We now check if the condition is a None variant, in which case we need to specify the type + if path_res(cx, condition) + .opt_def_id() + .is_some_and(|id| Some(cx.tcx.parent(id)) == cx.tcx.lang_items().option_none_variant()) + { + return span_lint_and_sugg( + cx, + MANUAL_UNWRAP_OR_DEFAULT, + expr.span, + format!("{expr_name} can be simplified with `.unwrap_or_default()`"), + "replace it with", + format!("{receiver}::<{expr_type}>.unwrap_or_default()"), + applicability, + ); + } + + // We check if the expression type is still uncertain, in which case we ask the user to specify it + if !expr_type_is_certain(cx, condition) { + return span_lint_and_sugg( + cx, + MANUAL_UNWRAP_OR_DEFAULT, + expr.span, + format!("{expr_name} can be simplified with `.unwrap_or_default()`"), + format!("ascribe the type {expr_type} and replace your expression with"), + format!("{receiver}.unwrap_or_default()"), + Applicability::Unspecified, + ); + } + + span_lint_and_sugg( + cx, + MANUAL_UNWRAP_OR_DEFAULT, + expr.span, + format!("{expr_name} can be simplified with `.unwrap_or_default()`"), + "replace it with", + format!("{receiver}.unwrap_or_default()"), + applicability, + ); + } else if let Some(ty_name) = find_type_name(cx, cx.typeck_results().expr_ty(condition)) + && cx.typeck_results().expr_adjustments(body_some).is_empty() + && let Some(or_body_snippet) = peel_blocks(body_none).span.get_source_text(cx) + && let Some(indent) = indent_of(cx, expr.span) + && ConstEvalCtxt::new(cx).eval_simple(body_none).is_some() + { + let reindented_or_body = reindent_multiline(&or_body_snippet, true, Some(indent)); + let mut app = Applicability::MachineApplicable; + let suggestion = Sugg::hir_with_context(cx, condition, expr.span.ctxt(), "..", &mut app).maybe_paren(); + span_lint_and_sugg( + cx, + MANUAL_UNWRAP_OR, + expr.span, + format!("this pattern reimplements `{ty_name}::unwrap_or`"), + "replace with", + format!("{suggestion}.unwrap_or({reindented_or_body})",), + app, + ); + } } } fn find_type_name<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<&'static str> { - if is_type_diagnostic_item(cx, ty, sym::Option) { - Some("Option") - } else if is_type_diagnostic_item(cx, ty, sym::Result) { - Some("Result") - } else { - None + match get_type_diagnostic_name(cx, ty)? { + sym::Option => Some("Option"), + sym::Result => Some("Result"), + _ => None, } } -fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<(&'a Arm<'a>, &'a Arm<'a>)> { - if arms.len() == 2 - && arms.iter().all(|arm| arm.guard.is_none()) - && let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| match arm.pat.kind { - PatKind::Expr(PatExpr { - hir_id, - kind: PatExprKind::Path(qpath), - .. - }) => is_res_lang_ctor(cx, cx.qpath_res(qpath, *hir_id), OptionNone), - PatKind::TupleStruct(ref qpath, [pat], _) => { - matches!(pat.kind, PatKind::Wild) - && is_res_lang_ctor(cx, cx.qpath_res(qpath, arm.pat.hir_id), ResultErr) - }, - _ => false, - }) - && let unwrap_arm = &arms[1 - idx] - && !contains_return_break_continue_macro(or_arm.body) +pub fn check_match<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + scrutinee: &'tcx Expr<'tcx>, + arms: &'tcx [Arm<'tcx>], +) { + if let [arm1, arm2] = arms + // Make sure there are no guards to keep things simple + && 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) { - Some((or_arm, unwrap_arm)) - } else { - None + handle(cx, expr, "match", scrutinee, body_some, body_none, binding_id); } } -fn lint<'tcx>( +pub fn check_if_let<'tcx>( cx: &LateContext<'tcx>, - expr: &Expr<'tcx>, - scrutinee: &'tcx Expr<'_>, - ty_name: &str, - or_body_snippet: &str, - indent: usize, + expr: &'tcx Expr<'tcx>, + pat: &'tcx Pat<'tcx>, + scrutinee: &'tcx Expr<'tcx>, + then_expr: &'tcx Expr<'tcx>, + else_expr: &'tcx Expr<'tcx>, ) { - let reindented_or_body = reindent_multiline(or_body_snippet, true, Some(indent)); - - let mut app = Applicability::MachineApplicable; - let suggestion = sugg::Sugg::hir_with_context(cx, scrutinee, expr.span.ctxt(), "..", &mut app).maybe_paren(); - span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR, - expr.span, - format!("this pattern reimplements `{ty_name}::unwrap_or`"), - "replace with", - format!("{suggestion}.unwrap_or({reindented_or_body})",), - app, - ); + if let Some(binding_id) = get_some(cx, pat) { + handle( + cx, + expr, + "if let", + scrutinee, + peel_blocks(then_expr), + peel_blocks(else_expr), + binding_id, + ); + } } diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 2b9173e6f41..ad299ae8f0f 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -724,6 +724,43 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does + /// Checks if a `match` or `if let` expression can be simplified using + /// `.unwrap_or_default()`. + /// + /// ### Why is this bad? + /// It can be done in one call with `.unwrap_or_default()`. + /// + /// ### Example + /// ```no_run + /// let x: Option<String> = Some(String::new()); + /// let y: String = match x { + /// Some(v) => v, + /// None => String::new(), + /// }; + /// + /// let x: Option<Vec<String>> = Some(Vec::new()); + /// let y: Vec<String> = if let Some(v) = x { + /// v + /// } else { + /// Vec::new() + /// }; + /// ``` + /// Use instead: + /// ```no_run + /// let x: Option<String> = Some(String::new()); + /// let y: String = x.unwrap_or_default(); + /// + /// let x: Option<Vec<String>> = Some(Vec::new()); + /// let y: Vec<String> = x.unwrap_or_default(); + /// ``` + #[clippy::version = "1.79.0"] + pub MANUAL_UNWRAP_OR_DEFAULT, + suspicious, + "check if a `match` or `if let` can be simplified with `unwrap_or_default`" +} + +declare_clippy_lint! { + /// ### What it does /// Checks for `match vec[idx]` or `match vec[n..m]`. /// /// ### Why is this bad? @@ -1040,6 +1077,7 @@ impl_lint_pass!(Matches => [ NEEDLESS_MATCH, COLLAPSIBLE_MATCH, MANUAL_UNWRAP_OR, + MANUAL_UNWRAP_OR_DEFAULT, MATCH_ON_VEC_ITEMS, MATCH_STR_CASE_MISMATCH, SIGNIFICANT_DROP_IN_SCRUTINEE, diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index c4d00002292..852378d50e8 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -176,6 +176,12 @@ impl<'hir> IfLetOrMatch<'hir> { ), } } + + pub fn scrutinee(&self) -> &'hir Expr<'hir> { + match self { + Self::Match(scrutinee, _, _) | Self::IfLet(scrutinee, _, _, _, _) => scrutinee, + } + } } /// An `if` or `if let` expression diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 1307ff79bc5..5f39af811cd 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -1027,6 +1027,7 @@ pub fn is_default_equivalent(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { ExprKind::Call(from_func, [arg]) => is_default_equivalent_from(cx, from_func, arg), ExprKind::Path(qpath) => is_res_lang_ctor(cx, cx.qpath_res(qpath, e.hir_id), OptionNone), ExprKind::AddrOf(rustc_hir::BorrowKind::Ref, _, expr) => matches!(expr.kind, ExprKind::Array([])), + ExprKind::Block(Block { stmts: [], expr, .. }, _) => expr.is_some_and(|e| is_default_equivalent(cx, e)), _ => false, } } diff --git a/tests/ui/manual_unwrap_or.fixed b/tests/ui/manual_unwrap_or.fixed index 07e4bdd483a..e12287a7093 100644 --- a/tests/ui/manual_unwrap_or.fixed +++ b/tests/ui/manual_unwrap_or.fixed @@ -18,11 +18,9 @@ fn option_unwrap_or() { // multiline case #[rustfmt::skip] - Some(1).unwrap_or({ - 42 + 42 - + 42 + 42 + 42 - + 42 + 42 + 42 - }); + Some(1).unwrap_or(42 + 42 + + 42 + 42 + 42 + + 42 + 42 + 42); // string case Some("Bob").unwrap_or("Alice"); @@ -125,11 +123,9 @@ fn result_unwrap_or() { // multiline case #[rustfmt::skip] - Ok::<i32, &str>(1).unwrap_or({ - 42 + 42 - + 42 + 42 + 42 - + 42 + 42 + 42 - }); + Ok::<i32, &str>(1).unwrap_or(42 + 42 + + 42 + 42 + 42 + + 42 + 42 + 42); // string case Ok::<&str, &str>("Bob").unwrap_or("Alice"); @@ -159,11 +155,7 @@ fn result_unwrap_or() { Ok(s) => s, Err(s) => s, }; - // could lint, but unused_variables takes care of it - match Ok::<&str, &str>("Alice") { - Ok(s) => s, - Err(s) => "Bob", - }; + Ok::<&str, &str>("Alice").unwrap_or("Bob"); Ok::<i32, i32>(1).unwrap_or(42); @@ -250,4 +242,12 @@ mod issue_13018 { } } +fn implicit_deref(v: Vec<String>) { + let _ = if let Some(s) = v.first() { s } else { "" }; +} + +fn allowed_manual_unwrap_or_zero() -> u32 { + Some(42).unwrap_or(0) +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.rs b/tests/ui/manual_unwrap_or.rs index c88b6f95da6..53cffcab5b5 100644 --- a/tests/ui/manual_unwrap_or.rs +++ b/tests/ui/manual_unwrap_or.rs @@ -216,8 +216,8 @@ fn result_unwrap_or() { Ok(s) => s, Err(s) => s, }; - // could lint, but unused_variables takes care of it match Ok::<&str, &str>("Alice") { + //~^ manual_unwrap_or Ok(s) => s, Err(s) => "Bob", }; @@ -316,4 +316,17 @@ mod issue_13018 { } } +fn implicit_deref(v: Vec<String>) { + let _ = if let Some(s) = v.first() { s } else { "" }; +} + +fn allowed_manual_unwrap_or_zero() -> u32 { + if let Some(x) = Some(42) { + //~^ manual_unwrap_or + x + } else { + 0 + } +} + fn main() {} diff --git a/tests/ui/manual_unwrap_or.stderr b/tests/ui/manual_unwrap_or.stderr index a5deb55786e..320e895fb82 100644 --- a/tests/ui/manual_unwrap_or.stderr +++ b/tests/ui/manual_unwrap_or.stderr @@ -44,11 +44,9 @@ LL | | }; | help: replace with | -LL ~ Some(1).unwrap_or({ -LL + 42 + 42 -LL + + 42 + 42 + 42 -LL + + 42 + 42 + 42 -LL ~ }); +LL ~ Some(1).unwrap_or(42 + 42 +LL + + 42 + 42 + 42 +LL ~ + 42 + 42 + 42); | error: this pattern reimplements `Option::unwrap_or` @@ -145,11 +143,9 @@ LL | | }; | help: replace with | -LL ~ Ok::<i32, &str>(1).unwrap_or({ -LL + 42 + 42 -LL + + 42 + 42 + 42 -LL + + 42 + 42 + 42 -LL ~ }); +LL ~ Ok::<i32, &str>(1).unwrap_or(42 + 42 +LL + + 42 + 42 + 42 +LL ~ + 42 + 42 + 42); | error: this pattern reimplements `Result::unwrap_or` @@ -163,6 +159,16 @@ LL | | }; | |_____^ help: replace with: `Ok::<&str, &str>("Bob").unwrap_or("Alice")` error: this pattern reimplements `Result::unwrap_or` + --> tests/ui/manual_unwrap_or.rs:219:5 + | +LL | / match Ok::<&str, &str>("Alice") { +LL | | +LL | | Ok(s) => s, +LL | | Err(s) => "Bob", +LL | | }; + | |_____^ help: replace with: `Ok::<&str, &str>("Alice").unwrap_or("Bob")` + +error: this pattern reimplements `Result::unwrap_or` --> tests/ui/manual_unwrap_or.rs:225:5 | LL | / if let Ok(x) = Ok::<i32, i32>(1) { @@ -184,5 +190,16 @@ LL | | None => 0, LL | | }; | |_________^ help: replace with: `some_macro!().unwrap_or(0)` -error: aborting due to 16 previous errors +error: this pattern reimplements `Option::unwrap_or` + --> tests/ui/manual_unwrap_or.rs:324:5 + | +LL | / if let Some(x) = Some(42) { +LL | | +LL | | x +LL | | } else { +LL | | 0 +LL | | } + | |_____^ help: replace with: `Some(42).unwrap_or(0)` + +error: aborting due to 18 previous errors diff --git a/tests/ui/manual_unwrap_or_default.fixed b/tests/ui/manual_unwrap_or_default.fixed index 832376fa5af..f4a78ee7619 100644 --- a/tests/ui/manual_unwrap_or_default.fixed +++ b/tests/ui/manual_unwrap_or_default.fixed @@ -1,5 +1,5 @@ #![warn(clippy::manual_unwrap_or_default)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::manual_unwrap_or)] +#![allow(clippy::unnecessary_literal_unwrap)] fn main() { let x: Option<Vec<String>> = None; @@ -99,3 +99,8 @@ fn issue_12928() { let y = if let Some(Y(a, _)) = x { a } else { 0 }; let y = if let Some(Y(a, ..)) = x { a } else { 0 }; } + +// For symetry with `manual_unwrap_or` test +fn allowed_manual_unwrap_or_zero() -> u32 { + Some(42).unwrap_or_default() +} diff --git a/tests/ui/manual_unwrap_or_default.rs b/tests/ui/manual_unwrap_or_default.rs index bedb3f0af0f..60b84b621f6 100644 --- a/tests/ui/manual_unwrap_or_default.rs +++ b/tests/ui/manual_unwrap_or_default.rs @@ -1,5 +1,5 @@ #![warn(clippy::manual_unwrap_or_default)] -#![allow(clippy::unnecessary_literal_unwrap, clippy::manual_unwrap_or)] +#![allow(clippy::unnecessary_literal_unwrap)] fn main() { let x: Option<Vec<String>> = None; @@ -135,3 +135,13 @@ fn issue_12928() { let y = if let Some(Y(a, _)) = x { a } else { 0 }; let y = if let Some(Y(a, ..)) = x { a } else { 0 }; } + +// For symetry with `manual_unwrap_or` test +fn allowed_manual_unwrap_or_zero() -> u32 { + if let Some(x) = Some(42) { + //~^ manual_unwrap_or_default + x + } else { + 0 + } +} diff --git a/tests/ui/manual_unwrap_or_default.stderr b/tests/ui/manual_unwrap_or_default.stderr index ca9aa159152..1e92f20a757 100644 --- a/tests/ui/manual_unwrap_or_default.stderr +++ b/tests/ui/manual_unwrap_or_default.stderr @@ -86,5 +86,16 @@ LL | | _ => 0, LL | | }, | |_________^ help: replace it with: `(*b).unwrap_or_default()` -error: aborting due to 8 previous errors +error: if let can be simplified with `.unwrap_or_default()` + --> tests/ui/manual_unwrap_or_default.rs:141:5 + | +LL | / if let Some(x) = Some(42) { +LL | | +LL | | x +LL | | } else { +LL | | 0 +LL | | } + | |_____^ help: replace it with: `Some(42).unwrap_or_default()` + +error: aborting due to 9 previous errors |