From 68c411ff943c71cd86d18a0b50f23b8da9d78181 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 19:01:44 -0400 Subject: [PATCH] Move `ManualMap` into `Matches` lint pass --- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.register_style.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/manual_map.rs | 316 ------------------------- clippy_lints/src/matches/manual_map.rs | 306 ++++++++++++++++++++++++ clippy_lints/src/matches/mod.rs | 30 +++ 7 files changed, 339 insertions(+), 321 deletions(-) delete mode 100644 clippy_lints/src/manual_map.rs create mode 100644 clippy_lints/src/matches/manual_map.rs diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 435c0829a..d4ec046d0 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -133,7 +133,6 @@ 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_map::MANUAL_MAP), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(manual_strip::MANUAL_STRIP), LintId::of(map_clone::MAP_CLONE), @@ -142,6 +141,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), + LintId::of(matches::MANUAL_MAP), LintId::of(matches::MANUAL_UNWRAP_OR), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c8cebca89..4cf38e8ab 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -250,7 +250,6 @@ store.register_lints(&[ manual_assert::MANUAL_ASSERT, manual_async_fn::MANUAL_ASYNC_FN, manual_bits::MANUAL_BITS, - manual_map::MANUAL_MAP, manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, manual_ok_or::MANUAL_OK_OR, manual_strip::MANUAL_STRIP, @@ -261,6 +260,7 @@ store.register_lints(&[ match_result_ok::MATCH_RESULT_OK, matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, + matches::MANUAL_MAP, matches::MANUAL_UNWRAP_OR, matches::MATCH_AS_REF, matches::MATCH_BOOL, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 07e90b8b6..355753517 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -45,12 +45,12 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(main_recursion::MAIN_RECURSION), LintId::of(manual_async_fn::MANUAL_ASYNC_FN), LintId::of(manual_bits::MANUAL_BITS), - LintId::of(manual_map::MANUAL_MAP), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(map_clone::MAP_CLONE), LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), + LintId::of(matches::MANUAL_MAP), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(matches::MATCH_OVERLAPPING_ARM), LintId::of(matches::MATCH_REF_PATS), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d9941d75f..c924bfe2a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -279,7 +279,6 @@ mod main_recursion; mod manual_assert; mod manual_async_fn; mod manual_bits; -mod manual_map; mod manual_non_exhaustive; mod manual_ok_or; mod manual_strip; @@ -845,7 +844,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(|| Box::new(redundant_slicing::RedundantSlicing)); store.register_late_pass(|| Box::new(from_str_radix_10::FromStrRadix10)); - store.register_late_pass(|| Box::new(manual_map::ManualMap)); store.register_late_pass(move || Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv))); store.register_late_pass(|| Box::new(bool_assert_comparison::BoolAssertComparison)); store.register_early_pass(move || Box::new(module_style::ModStyle)); diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs deleted file mode 100644 index 230ae029e..000000000 --- a/clippy_lints/src/manual_map.rs +++ /dev/null @@ -1,316 +0,0 @@ -use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF}; -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::higher::IfLetOrMatch; -use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; -use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function}; -use clippy_utils::{ - can_move_expr_to_closure, in_constant, is_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id, - peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, CaptureKind, -}; -use rustc_ast::util::parser::PREC_POSTFIX; -use rustc_errors::Applicability; -use rustc_hir::LangItem::{OptionNone, OptionSome}; -use rustc_hir::{ - def::Res, Arm, BindingAnnotation, Block, BlockCheckMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, - QPath, UnsafeSource, -}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_middle::lint::in_external_macro; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{sym, SyntaxContext}; - -declare_clippy_lint! { - /// ### What it does - /// Checks for usages of `match` which could be implemented using `map` - /// - /// ### Why is this bad? - /// Using the `map` method is clearer and more concise. - /// - /// ### Example - /// ```rust - /// match Some(0) { - /// Some(x) => Some(x + 1), - /// None => None, - /// }; - /// ``` - /// Use instead: - /// ```rust - /// Some(0).map(|x| x + 1); - /// ``` - #[clippy::version = "1.52.0"] - pub MANUAL_MAP, - style, - "reimplementation of `map`" -} - -declare_lint_pass!(ManualMap => [MANUAL_MAP]); - -impl<'tcx> LateLintPass<'tcx> for ManualMap { - #[expect(clippy::too_many_lines)] - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - let (scrutinee, then_pat, then_body, else_pat, else_body) = match IfLetOrMatch::parse(cx, expr) { - Some(IfLetOrMatch::IfLet(scrutinee, pat, body, Some(r#else))) => (scrutinee, pat, body, None, r#else), - Some(IfLetOrMatch::Match( - scrutinee, - [arm1 @ Arm { guard: None, .. }, arm2 @ Arm { guard: None, .. }], - _, - )) => (scrutinee, arm1.pat, arm1.body, Some(arm2.pat), arm2.body), - _ => return, - }; - if in_external_macro(cx.sess(), expr.span) || in_constant(cx, expr.hir_id) { - return; - } - - let (scrutinee_ty, ty_ref_count, ty_mutability) = - peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee)); - if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::Option) - && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Option)) - { - return; - } - - let expr_ctxt = expr.span.ctxt(); - let (some_expr, some_pat, pat_ref_count, is_wild_none) = match ( - try_parse_pattern(cx, then_pat, expr_ctxt), - else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)), - ) { - (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { - (else_body, pattern, ref_count, true) - }, - (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { - (else_body, pattern, ref_count, false) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => { - (then_body, pattern, ref_count, true) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => { - (then_body, pattern, ref_count, false) - }, - _ => return, - }; - - // Top level or patterns aren't allowed in closures. - if matches!(some_pat.kind, PatKind::Or(_)) { - return; - } - - let some_expr = match get_some_expr(cx, some_expr, false, expr_ctxt) { - Some(expr) => expr, - None => return, - }; - - // These two lints will go back and forth with each other. - if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit - && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) - { - return; - } - - // `map` won't perform any adjustments. - if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { - return; - } - - // Determine which binding mode to use. - let explicit_ref = some_pat.contains_explicit_ref_binding(); - let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability)); - - let as_ref_str = match binding_ref { - Some(Mutability::Mut) => ".as_mut()", - Some(Mutability::Not) => ".as_ref()", - None => "", - }; - - match can_move_expr_to_closure(cx, some_expr.expr) { - Some(captures) => { - // Check if captures the closure will need conflict with borrows made in the scrutinee. - // TODO: check all the references made in the scrutinee expression. This will require interacting - // with the borrow checker. Currently only `[.]*` is checked for. - if let Some(binding_ref_mutability) = binding_ref { - let e = peel_hir_expr_while(scrutinee, |e| match e.kind { - ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), - _ => None, - }); - if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind { - match captures.get(l) { - Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return, - Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { - return; - }, - Some(CaptureKind::Ref(Mutability::Not)) | None => (), - } - } - } - }, - None => return, - }; - - let mut app = Applicability::MachineApplicable; - - // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or - // it's being passed by value. - let scrutinee = peel_hir_expr_refs(scrutinee).0; - let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app); - let scrutinee_str = - if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX { - format!("({})", scrutinee_str) - } else { - scrutinee_str.into() - }; - - let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { - if_chain! { - if !some_expr.needs_unsafe_block; - if let Some(func) = can_pass_as_func(cx, id, some_expr.expr); - if func.span.ctxt() == some_expr.expr.span.ctxt(); - then { - snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() - } else { - if path_to_local_id(some_expr.expr, id) - && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) - && binding_ref.is_some() - { - return; - } - - // `ref` and `ref mut` annotations were handled earlier. - let annotation = if matches!(annotation, BindingAnnotation::Mutable) { - "mut " - } else { - "" - }; - let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; - if some_expr.needs_unsafe_block { - format!("|{}{}| unsafe {{ {} }}", annotation, some_binding, expr_snip) - } else { - format!("|{}{}| {}", annotation, some_binding, expr_snip) - } - } - } - } else if !is_wild_none && explicit_ref.is_none() { - // TODO: handle explicit reference annotations. - let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0; - let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; - if some_expr.needs_unsafe_block { - format!("|{}| unsafe {{ {} }}", pat_snip, expr_snip) - } else { - format!("|{}| {}", pat_snip, expr_snip) - } - } else { - // Refutable bindings and mixed reference annotations can't be handled by `map`. - return; - }; - - span_lint_and_sugg( - cx, - MANUAL_MAP, - expr.span, - "manual implementation of `Option::map`", - "try this", - if else_pat.is_none() && is_else_clause(cx.tcx, expr) { - format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str) - } else { - format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str) - }, - app, - ); - } -} - -// Checks whether the expression could be passed as a function, or whether a closure is needed. -// Returns the function to be passed to `map` if it exists. -fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { - match expr.kind { - ExprKind::Call(func, [arg]) - if path_to_local_id(arg, binding) - && cx.typeck_results().expr_adjustments(arg).is_empty() - && !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) => - { - Some(func) - }, - _ => None, - } -} - -enum OptionPat<'a> { - Wild, - None, - Some { - // The pattern contained in the `Some` tuple. - pattern: &'a Pat<'a>, - // The number of references before the `Some` tuple. - // e.g. `&&Some(_)` has a ref count of 2. - ref_count: usize, - }, -} - -struct SomeExpr<'tcx> { - expr: &'tcx Expr<'tcx>, - needs_unsafe_block: bool, -} - -// Try to parse into a recognized `Option` pattern. -// i.e. `_`, `None`, `Some(..)`, or a reference to any of those. -fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option> { - fn f<'tcx>( - cx: &LateContext<'tcx>, - pat: &'tcx Pat<'_>, - ref_count: usize, - ctxt: SyntaxContext, - ) -> Option> { - match pat.kind { - PatKind::Wild => Some(OptionPat::Wild), - PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt), - PatKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone) => Some(OptionPat::None), - PatKind::TupleStruct(ref qpath, [pattern], _) - if is_lang_ctor(cx, qpath, OptionSome) && pat.span.ctxt() == ctxt => - { - Some(OptionPat::Some { pattern, ref_count }) - }, - _ => None, - } - } - f(cx, pat, 0, ctxt) -} - -// Checks for an expression wrapped by the `Some` constructor. Returns the contained expression. -fn get_some_expr<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - needs_unsafe_block: bool, - ctxt: SyntaxContext, -) -> Option> { - // TODO: Allow more complex expressions. - match expr.kind { - ExprKind::Call( - Expr { - kind: ExprKind::Path(ref qpath), - .. - }, - [arg], - ) if ctxt == expr.span.ctxt() && is_lang_ctor(cx, qpath, OptionSome) => Some(SomeExpr { - expr: arg, - needs_unsafe_block, - }), - ExprKind::Block( - Block { - stmts: [], - expr: Some(expr), - rules, - .. - }, - _, - ) => get_some_expr( - cx, - expr, - needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), - ctxt, - ), - _ => None, - } -} - -// Checks for the `None` value. -fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - matches!(peel_blocks(expr).kind, ExprKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone)) -} diff --git a/clippy_lints/src/matches/manual_map.rs b/clippy_lints/src/matches/manual_map.rs new file mode 100644 index 000000000..542905a2d --- /dev/null +++ b/clippy_lints/src/matches/manual_map.rs @@ -0,0 +1,306 @@ +use crate::{map_unit_fn::OPTION_MAP_UNIT_FN, matches::MATCH_AS_REF}; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; +use clippy_utils::ty::{is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function}; +use clippy_utils::{ + can_move_expr_to_closure, is_else_clause, is_lang_ctor, is_lint_allowed, path_to_local_id, peel_blocks, + peel_hir_expr_refs, peel_hir_expr_while, CaptureKind, +}; +use rustc_ast::util::parser::PREC_POSTFIX; +use rustc_errors::Applicability; +use rustc_hir::LangItem::{OptionNone, OptionSome}; +use rustc_hir::{ + def::Res, Arm, BindingAnnotation, Block, BlockCheckMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, + QPath, UnsafeSource, +}; +use rustc_lint::LateContext; +use rustc_span::{sym, SyntaxContext}; + +use super::MANUAL_MAP; + +pub(super) fn check_match<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + scrutinee: &'tcx Expr<'_>, + arms: &'tcx [Arm<'_>], +) { + if let [arm1, arm2] = arms + && arm1.guard.is_none() + && arm2.guard.is_none() + { + check(cx, expr, scrutinee, arm1.pat, arm1.body, Some(arm2.pat), arm2.body); + } +} + +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<'_>, +) { + check(cx, expr, let_expr, let_pat, then_expr, None, else_expr); +} + +#[expect(clippy::too_many_lines)] +fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + scrutinee: &'tcx Expr<'_>, + then_pat: &'tcx Pat<'_>, + then_body: &'tcx Expr<'_>, + else_pat: Option<&'tcx Pat<'_>>, + else_body: &'tcx Expr<'_>, +) { + let (scrutinee_ty, ty_ref_count, ty_mutability) = + peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee)); + if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::Option) + && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::Option)) + { + return; + } + + let expr_ctxt = expr.span.ctxt(); + let (some_expr, some_pat, pat_ref_count, is_wild_none) = match ( + try_parse_pattern(cx, then_pat, expr_ctxt), + else_pat.map_or(Some(OptionPat::Wild), |p| try_parse_pattern(cx, p, expr_ctxt)), + ) { + (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { + (else_body, pattern, ref_count, true) + }, + (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_body) => { + (else_body, pattern, ref_count, false) + }, + (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_body) => { + (then_body, pattern, ref_count, true) + }, + (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_body) => { + (then_body, pattern, ref_count, false) + }, + _ => return, + }; + + // Top level or patterns aren't allowed in closures. + if matches!(some_pat.kind, PatKind::Or(_)) { + return; + } + + let some_expr = match get_some_expr(cx, some_expr, false, expr_ctxt) { + Some(expr) => expr, + None => return, + }; + + // These two lints will go back and forth with each other. + if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit + && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) + { + return; + } + + // `map` won't perform any adjustments. + if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() { + return; + } + + // Determine which binding mode to use. + let explicit_ref = some_pat.contains_explicit_ref_binding(); + let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability)); + + let as_ref_str = match binding_ref { + Some(Mutability::Mut) => ".as_mut()", + Some(Mutability::Not) => ".as_ref()", + None => "", + }; + + match can_move_expr_to_closure(cx, some_expr.expr) { + Some(captures) => { + // Check if captures the closure will need conflict with borrows made in the scrutinee. + // TODO: check all the references made in the scrutinee expression. This will require interacting + // with the borrow checker. Currently only `[.]*` is checked for. + if let Some(binding_ref_mutability) = binding_ref { + let e = peel_hir_expr_while(scrutinee, |e| match e.kind { + ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) => Some(e), + _ => None, + }); + if let ExprKind::Path(QPath::Resolved(None, Path { res: Res::Local(l), .. })) = e.kind { + match captures.get(l) { + Some(CaptureKind::Value | CaptureKind::Ref(Mutability::Mut)) => return, + Some(CaptureKind::Ref(Mutability::Not)) if binding_ref_mutability == Mutability::Mut => { + return; + }, + Some(CaptureKind::Ref(Mutability::Not)) | None => (), + } + } + } + }, + None => return, + }; + + let mut app = Applicability::MachineApplicable; + + // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or + // it's being passed by value. + let scrutinee = peel_hir_expr_refs(scrutinee).0; + let (scrutinee_str, _) = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app); + let scrutinee_str = if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX { + format!("({})", scrutinee_str) + } else { + scrutinee_str.into() + }; + + let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { + if_chain! { + if !some_expr.needs_unsafe_block; + if let Some(func) = can_pass_as_func(cx, id, some_expr.expr); + if func.span.ctxt() == some_expr.expr.span.ctxt(); + then { + snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() + } else { + if path_to_local_id(some_expr.expr, id) + && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) + && binding_ref.is_some() + { + return; + } + + // `ref` and `ref mut` annotations were handled earlier. + let annotation = if matches!(annotation, BindingAnnotation::Mutable) { + "mut " + } else { + "" + }; + let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; + if some_expr.needs_unsafe_block { + format!("|{}{}| unsafe {{ {} }}", annotation, some_binding, expr_snip) + } else { + format!("|{}{}| {}", annotation, some_binding, expr_snip) + } + } + } + } else if !is_wild_none && explicit_ref.is_none() { + // TODO: handle explicit reference annotations. + let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0; + let expr_snip = snippet_with_context(cx, some_expr.expr.span, expr_ctxt, "..", &mut app).0; + if some_expr.needs_unsafe_block { + format!("|{}| unsafe {{ {} }}", pat_snip, expr_snip) + } else { + format!("|{}| {}", pat_snip, expr_snip) + } + } else { + // Refutable bindings and mixed reference annotations can't be handled by `map`. + return; + }; + + span_lint_and_sugg( + cx, + MANUAL_MAP, + expr.span, + "manual implementation of `Option::map`", + "try this", + if else_pat.is_none() && is_else_clause(cx.tcx, expr) { + format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str) + } else { + format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str) + }, + app, + ); +} + +// Checks whether the expression could be passed as a function, or whether a closure is needed. +// Returns the function to be passed to `map` if it exists. +fn can_pass_as_func<'tcx>(cx: &LateContext<'tcx>, binding: HirId, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { + match expr.kind { + ExprKind::Call(func, [arg]) + if path_to_local_id(arg, binding) + && cx.typeck_results().expr_adjustments(arg).is_empty() + && !type_is_unsafe_function(cx, cx.typeck_results().expr_ty(func).peel_refs()) => + { + Some(func) + }, + _ => None, + } +} + +enum OptionPat<'a> { + Wild, + None, + Some { + // The pattern contained in the `Some` tuple. + pattern: &'a Pat<'a>, + // The number of references before the `Some` tuple. + // e.g. `&&Some(_)` has a ref count of 2. + ref_count: usize, + }, +} + +struct SomeExpr<'tcx> { + expr: &'tcx Expr<'tcx>, + needs_unsafe_block: bool, +} + +// Try to parse into a recognized `Option` pattern. +// i.e. `_`, `None`, `Some(..)`, or a reference to any of those. +fn try_parse_pattern<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option> { + fn f<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + ref_count: usize, + ctxt: SyntaxContext, + ) -> Option> { + match pat.kind { + PatKind::Wild => Some(OptionPat::Wild), + PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt), + PatKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone) => Some(OptionPat::None), + PatKind::TupleStruct(ref qpath, [pattern], _) + if is_lang_ctor(cx, qpath, OptionSome) && pat.span.ctxt() == ctxt => + { + Some(OptionPat::Some { pattern, ref_count }) + }, + _ => None, + } + } + f(cx, pat, 0, ctxt) +} + +// Checks for an expression wrapped by the `Some` constructor. Returns the contained expression. +fn get_some_expr<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + needs_unsafe_block: bool, + ctxt: SyntaxContext, +) -> Option> { + // TODO: Allow more complex expressions. + match expr.kind { + ExprKind::Call( + Expr { + kind: ExprKind::Path(ref qpath), + .. + }, + [arg], + ) if ctxt == expr.span.ctxt() && is_lang_ctor(cx, qpath, OptionSome) => Some(SomeExpr { + expr: arg, + needs_unsafe_block, + }), + ExprKind::Block( + Block { + stmts: [], + expr: Some(expr), + rules, + .. + }, + _, + ) => get_some_expr( + cx, + expr, + needs_unsafe_block || *rules == BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided), + ctxt, + ), + _ => None, + } +} + +// Checks for the `None` value. +fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + matches!(peel_blocks(expr).kind, ExprKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone)) +} diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index fee7471f1..d1e42f39e 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -10,6 +10,7 @@ use rustc_span::{Span, SpanData, SyntaxContext}; mod collapsible_match; mod infallible_destructuring_match; +mod manual_map; mod manual_unwrap_or; mod match_as_ref; mod match_bool; @@ -861,6 +862,30 @@ declare_clippy_lint! { "return errors explicitly rather than hiding them behind a `?`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usages of `match` which could be implemented using `map` + /// + /// ### Why is this bad? + /// Using the `map` method is clearer and more concise. + /// + /// ### Example + /// ```rust + /// match Some(0) { + /// Some(x) => Some(x + 1), + /// None => None, + /// }; + /// ``` + /// Use instead: + /// ```rust + /// Some(0).map(|x| x + 1); + /// ``` + #[clippy::version = "1.52.0"] + pub MANUAL_MAP, + style, + "reimplementation of `map`" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -901,6 +926,7 @@ impl_lint_pass!(Matches => [ MATCH_STR_CASE_MISMATCH, SIGNIFICANT_DROP_IN_SCRUTINEE, TRY_ERR, + MANUAL_MAP, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -949,6 +975,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { if !in_constant(cx, expr.hir_id) { manual_unwrap_or::check(cx, expr, ex, arms); + manual_map::check_match(cx, expr, ex, arms); } if self.infallible_destructuring_match_linted { @@ -973,6 +1000,9 @@ impl<'tcx> LateLintPass<'tcx> for Matches { else_expr, ); } + if !in_constant(cx, expr.hir_id) { + manual_map::check_if_let(cx, expr, if_let.let_pat, if_let.let_expr, if_let.if_then, else_expr); + } } redundant_pattern_match::check_if_let( cx,