diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/collapsible_match.rs index f5683856889f2..86226be715d3f 100644 --- a/clippy_lints/src/collapsible_match.rs +++ b/clippy_lints/src/collapsible_match.rs @@ -1,9 +1,10 @@ use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::higher::IfLetOrMatch; use clippy_utils::visitors::is_local_used; -use clippy_utils::{higher, is_lang_ctor, is_unit_expr, path_to_local, peel_ref_operators, SpanlessEq}; +use clippy_utils::{is_lang_ctor, is_unit_expr, path_to_local, peel_ref_operators, SpanlessEq}; use if_chain::if_chain; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, MatchSource, Pat, PatKind, StmtKind}; +use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::{MultiSpan, Span}; @@ -149,33 +150,6 @@ fn strip_singleton_blocks<'hir>(mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> expr } -enum IfLetOrMatch<'hir> { - Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource), - /// scrutinee, pattern, then block, else block - IfLet( - &'hir Expr<'hir>, - &'hir Pat<'hir>, - &'hir Expr<'hir>, - Option<&'hir Expr<'hir>>, - ), -} - -impl<'hir> IfLetOrMatch<'hir> { - fn parse(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option { - match expr.kind { - ExprKind::Match(expr, arms, source) => Some(Self::Match(expr, arms, source)), - _ => higher::IfLet::hir(cx, expr).map( - |higher::IfLet { - let_expr, - let_pat, - if_then, - if_else, - }| { Self::IfLet(let_expr, let_pat, if_then, if_else) }, - ), - } - } -} - /// A "wild-like" arm has a wild (`_`) or `None` pattern and no guard. Such arms can be "collapsed" /// into a single wild arm without any significant loss in semantics or readability. fn arm_is_wild_like(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool { diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index 385a3f546b9d0..b5f573cb104e9 100644 --- a/clippy_lints/src/manual_map.rs +++ b/clippy_lints/src/manual_map.rs @@ -1,6 +1,6 @@ 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; +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}; use clippy_utils::{ @@ -46,190 +46,169 @@ declare_lint_pass!(ManualMap => [MANUAL_MAP]); impl LateLintPass<'_> for ManualMap { #[allow(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(higher::IfLet { - let_pat, - let_expr, - if_then, - if_else: Some(if_else), - }) = higher::IfLet::hir(cx, expr) - { - manage_lint(cx, expr, (&let_pat.kind, if_then), (&PatKind::Wild, if_else), let_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; } - if let ExprKind::Match(scrutinee, [then @ Arm { guard: None, .. }, r#else @ Arm { guard: None, .. }], _) = - expr.kind + 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_type) + && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type)) { - manage_lint( - cx, - expr, - (&then.pat.kind, then.body), - (&r#else.pat.kind, r#else.body), - scrutinee, - ); + return; } - } -} - -fn manage_lint<'tcx>( - cx: &LateContext<'tcx>, - expr: &'tcx Expr<'_>, - then: (&'tcx PatKind<'_>, &'tcx Expr<'_>), - r#else: (&'tcx PatKind<'_>, &'tcx Expr<'_>), - scrut: &'tcx Expr<'_>, -) { - 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(scrut)); - if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type) - && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type)) - { - return; - } - - let (then_pat, then_expr) = then; - let (else_pat, else_expr) = r#else; - 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), - try_parse_pattern(cx, else_pat, expr_ctxt), - ) { - (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_expr) => { - (else_expr, pattern, ref_count, true) - }, - (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count })) if is_none_expr(cx, then_expr) => { - (else_expr, pattern, ref_count, false) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild)) if is_none_expr(cx, else_expr) => { - (then_expr, pattern, ref_count, true) - }, - (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None)) if is_none_expr(cx, else_expr) => { - (then_expr, pattern, ref_count, false) - }, - _ => 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; - } + // 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, expr_ctxt) { - Some(expr) => expr, - None => return, - }; + let some_expr = match get_some_expr(cx, some_expr, expr_ctxt) { + Some(expr) => expr, + None => return, + }; - if cx.typeck_results().expr_ty(some_expr) == cx.tcx.types.unit - && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id) - { - return; - } + // These two lints will go back and forth with each other. + if cx.typeck_results().expr_ty(some_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).is_empty() { - return; - } + // `map` won't perform any adjustments. + if !cx.typeck_results().expr_adjustments(some_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)); + // 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 => "", - }; + 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) { - 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(scrut, |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 => (), + match can_move_expr_to_closure(cx, some_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; + }, + None => return, + }; - // 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(scrut).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 mut app = Applicability::MachineApplicable; - let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { - match can_pass_as_func(cx, id, some_expr) { - Some(func) if func.span.ctxt() == some_expr.span.ctxt() => { - snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() - }, - _ => { - if path_to_local_id(some_expr, id) - && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) - && binding_ref.is_some() - { - return; - } + // 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() + }; - // `ref` and `ref mut` annotations were handled earlier. - let annotation = if matches!(annotation, BindingAnnotation::Mutable) { - "mut " - } else { - "" - }; - format!( - "|{}{}| {}", - annotation, - some_binding, - snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0 - ) - }, - } - } else if !is_wild_none && explicit_ref.is_none() { - // TODO: handle explicit reference annotations. - format!( - "|{}| {}", - snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0, - snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0 - ) - } else { - // Refutable bindings and mixed reference annotations can't be handled by `map`. - return; - }; + let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind { + match can_pass_as_func(cx, id, some_expr) { + Some(func) if func.span.ctxt() == some_expr.span.ctxt() => { + snippet_with_applicability(cx, func.span, "..", &mut app).into_owned() + }, + _ => { + if path_to_local_id(some_expr, id) + && !is_lint_allowed(cx, MATCH_AS_REF, expr.hir_id) + && binding_ref.is_some() + { + return; + } - span_lint_and_sugg( - cx, - MANUAL_MAP, - expr.span, - "manual implementation of `Option::map`", - "try this", - if is_else_clause(cx.tcx, expr) { - format!("{{ {}{}.map({}) }}", scrutinee_str, as_ref_str, body_str) + // `ref` and `ref mut` annotations were handled earlier. + let annotation = if matches!(annotation, BindingAnnotation::Mutable) { + "mut " + } else { + "" + }; + format!( + "|{}{}| {}", + annotation, + some_binding, + snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0 + ) + }, + } + } else if !is_wild_none && explicit_ref.is_none() { + // TODO: handle explicit reference annotations. + format!( + "|{}| {}", + snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0, + snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app).0 + ) } else { - format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str) - }, - app, - ); + // 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. @@ -259,28 +238,21 @@ enum OptionPat<'a> { // Try to parse into a recognized `Option` pattern. // i.e. `_`, `None`, `Some(..)`, or a reference to any of those. -fn try_parse_pattern( - cx: &LateContext<'tcx>, - pat_kind: &'tcx PatKind<'_>, - ctxt: SyntaxContext, -) -> Option> { - fn f( - cx: &LateContext<'tcx>, - pat_kind: &'tcx PatKind<'_>, - ref_count: usize, - ctxt: SyntaxContext, - ) -> Option> { - match pat_kind { +fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option> { + fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize, ctxt: SyntaxContext) -> Option> { + match pat.kind { PatKind::Wild => Some(OptionPat::Wild), - PatKind::Ref(ref_pat, _) => f(cx, &ref_pat.kind, ref_count + 1, ctxt), + 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) => { + 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_kind, 0, ctxt) + f(cx, pat, 0, ctxt) } // Checks for an expression wrapped by the `Some` constructor. Returns the contained expression. diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index d20739a9f74cc..61e672781f840 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -6,7 +6,7 @@ use crate::{is_expn_of, match_def_path, paths}; use if_chain::if_chain; use rustc_ast::ast::{self, LitKind}; use rustc_hir as hir; -use rustc_hir::{Block, BorrowKind, Expr, ExprKind, LoopSource, Node, Pat, StmtKind, UnOp}; +use rustc_hir::{Arm, Block, BorrowKind, Expr, ExprKind, LoopSource, MatchSource, Node, Pat, StmtKind, UnOp}; use rustc_lint::LateContext; use rustc_span::{sym, ExpnKind, Span, Symbol}; @@ -115,6 +115,33 @@ impl<'hir> IfLet<'hir> { } } +pub enum IfLetOrMatch<'hir> { + Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource), + /// scrutinee, pattern, then block, else block + IfLet( + &'hir Expr<'hir>, + &'hir Pat<'hir>, + &'hir Expr<'hir>, + Option<&'hir Expr<'hir>>, + ), +} + +impl<'hir> IfLetOrMatch<'hir> { + pub fn parse(cx: &LateContext<'_>, expr: &Expr<'hir>) -> Option { + match expr.kind { + ExprKind::Match(expr, arms, source) => Some(Self::Match(expr, arms, source)), + _ => IfLet::hir(cx, expr).map( + |IfLet { + let_expr, + let_pat, + if_then, + if_else, + }| { Self::IfLet(let_expr, let_pat, if_then, if_else) }, + ), + } + } +} + pub struct IfOrIfLet<'hir> { pub cond: &'hir Expr<'hir>, pub r#else: Option<&'hir Expr<'hir>>, diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 7e03cf56e8a86..6cecdd6b19963 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -808,6 +808,13 @@ pub fn capture_local_usage(cx: &LateContext<'tcx>, e: &Expr<'_>) -> CaptureKind capture_expr_ty = e; } }, + ExprKind::Let(pat, ..) => { + let mutability = match pat_capture_kind(cx, pat) { + CaptureKind::Value => Mutability::Not, + CaptureKind::Ref(m) => m, + }; + return CaptureKind::Ref(mutability); + }, ExprKind::Match(_, arms, _) => { let mut mutability = Mutability::Not; for capture in arms.iter().map(|arm| pat_capture_kind(cx, arm.pat)) {