From 81e44502ac2e86258dbc880bdf702eab0030c4fb Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 12:14:24 -0400 Subject: [PATCH 1/7] Merge `CollapsibleMatch` 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 - .../src/{ => matches}/collapsible_match.rs | 70 ++++----------- .../src/matches/match_like_matches.rs | 34 +++---- clippy_lints/src/matches/mod.rs | 90 ++++++++++++++++--- clippy_lints/src/matches/needless_match.rs | 30 +++---- .../src/matches/redundant_pattern_match.rs | 20 +++-- 9 files changed, 139 insertions(+), 113 deletions(-) rename clippy_lints/src/{ => matches}/collapsible_match.rs (71%) diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 51b3babef..b1da36a0d 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -38,7 +38,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(casts::UNNECESSARY_CAST), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(collapsible_if::COLLAPSIBLE_IF), - LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), LintId::of(copies::IFS_SAME_COND), LintId::of(copies::IF_SAME_THEN_ELSE), @@ -143,6 +142,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(match_result_ok::MATCH_RESULT_OK), LintId::of(match_str_case_mismatch::MATCH_STR_CASE_MISMATCH), + LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), 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 29bfc660d..58e18b710 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -93,7 +93,6 @@ store.register_lints(&[ cognitive_complexity::COGNITIVE_COMPLEXITY, collapsible_if::COLLAPSIBLE_ELSE_IF, collapsible_if::COLLAPSIBLE_IF, - collapsible_match::COLLAPSIBLE_MATCH, comparison_chain::COMPARISON_CHAIN, copies::BRANCHES_SHARING_CODE, copies::IFS_SAME_COND, @@ -263,6 +262,7 @@ store.register_lints(&[ match_on_vec_items::MATCH_ON_VEC_ITEMS, match_result_ok::MATCH_RESULT_OK, match_str_case_mismatch::MATCH_STR_CASE_MISMATCH, + matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, 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 ea2e10824..07e90b8b6 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -12,7 +12,6 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(casts::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(collapsible_if::COLLAPSIBLE_ELSE_IF), LintId::of(collapsible_if::COLLAPSIBLE_IF), - LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(dereference::NEEDLESS_BORROW), @@ -50,6 +49,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ 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::MATCH_LIKE_MATCHES_MACRO), LintId::of(matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6c3d7594f..6e16d470a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -192,7 +192,6 @@ mod casts; mod checked_conversions; mod cognitive_complexity; mod collapsible_if; -mod collapsible_match; mod comparison_chain; mod copies; mod copy_iterator; @@ -568,7 +567,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(len_zero::LenZero)); store.register_late_pass(|| Box::new(attrs::Attributes)); store.register_late_pass(|| Box::new(blocks_in_if_conditions::BlocksInIfConditions)); - store.register_late_pass(|| Box::new(collapsible_match::CollapsibleMatch)); store.register_late_pass(|| Box::new(unicode::Unicode)); store.register_late_pass(|| Box::new(uninit_vec::UninitVec)); store.register_late_pass(|| Box::new(unit_hash::UnitHash)); diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/matches/collapsible_match.rs similarity index 71% rename from clippy_lints/src/collapsible_match.rs rename to clippy_lints/src/matches/collapsible_match.rs index ec55009f3..07021f1bc 100644 --- a/clippy_lints/src/collapsible_match.rs +++ b/clippy_lints/src/matches/collapsible_match.rs @@ -6,68 +6,28 @@ use if_chain::if_chain; use rustc_errors::MultiSpan; use rustc_hir::LangItem::OptionNone; use rustc_hir::{Arm, Expr, Guard, HirId, Let, Pat, PatKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_lint::LateContext; use rustc_span::Span; -declare_clippy_lint! { - /// ### What it does - /// Finds nested `match` or `if let` expressions where the patterns may be "collapsed" together - /// without adding any branches. - /// - /// Note that this lint is not intended to find _all_ cases where nested match patterns can be merged, but only - /// cases where merging would most likely make the code more readable. - /// - /// ### Why is this bad? - /// It is unnecessarily verbose and complex. - /// - /// ### Example - /// ```rust - /// fn func(opt: Option>) { - /// let n = match opt { - /// Some(n) => match n { - /// Ok(n) => n, - /// _ => return, - /// } - /// None => return, - /// }; - /// } - /// ``` - /// Use instead: - /// ```rust - /// fn func(opt: Option>) { - /// let n = match opt { - /// Some(Ok(n)) => n, - /// _ => return, - /// }; - /// } - /// ``` - #[clippy::version = "1.50.0"] - pub COLLAPSIBLE_MATCH, - style, - "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." -} +use super::COLLAPSIBLE_MATCH; -declare_lint_pass!(CollapsibleMatch => [COLLAPSIBLE_MATCH]); - -impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { - match IfLetOrMatch::parse(cx, expr) { - Some(IfLetOrMatch::Match(_, arms, _)) => { - if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) { - for arm in arms { - check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); - } - } - }, - Some(IfLetOrMatch::IfLet(_, pat, body, els)) => { - check_arm(cx, false, pat, body, None, els); - }, - None => {}, +pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { + if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) { + for arm in arms { + check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); } } } +pub(super) fn check_if_let<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + body: &'tcx Expr<'_>, + else_expr: Option<&'tcx Expr<'_>>, +) { + check_arm(cx, false, pat, body, None, else_expr); +} + fn check_arm<'tcx>( cx: &LateContext<'tcx>, outer_is_match: bool, diff --git a/clippy_lints/src/matches/match_like_matches.rs b/clippy_lints/src/matches/match_like_matches.rs index 2e1f7646e..a68eec842 100644 --- a/clippy_lints/src/matches/match_like_matches.rs +++ b/clippy_lints/src/matches/match_like_matches.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::is_wild; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::{higher, is_wild}; use rustc_ast::{Attribute, LitKind}; use rustc_errors::Applicability; use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat}; @@ -11,22 +11,24 @@ use rustc_span::source_map::Spanned; use super::MATCH_LIKE_MATCHES_MACRO; /// Lint a `match` or `if let .. { .. } else { .. }` expr that could be replaced by `matches!` -pub(crate) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(higher::IfLet { - let_pat, +pub(crate) 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<'_>, +) { + find_matches_sugg( + cx, let_expr, - if_then, - if_else: Some(if_else), - }) = higher::IfLet::hir(cx, expr) - { - find_matches_sugg( - cx, - let_expr, - IntoIterator::into_iter([(&[][..], Some(let_pat), if_then, None), (&[][..], None, if_else, None)]), - expr, - true, - ); - } + IntoIterator::into_iter([ + (&[][..], Some(let_pat), then_expr, None), + (&[][..], None, else_expr, None), + ]), + expr, + true, + ); } pub(super) fn check_match<'tcx>( diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 3d8391bce..4a77fb8c3 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -1,12 +1,14 @@ use clippy_utils::source::{snippet_opt, span_starts_with, walk_span_to_context}; -use clippy_utils::{meets_msrv, msrvs}; +use clippy_utils::{higher, meets_msrv, msrvs}; use rustc_hir::{Arm, Expr, ExprKind, Local, MatchSource, Pat}; use rustc_lexer::{tokenize, TokenKind}; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{Span, SpanData, SyntaxContext}; +mod collapsible_match; mod infallible_destructuring_match; mod match_as_ref; mod match_bool; @@ -610,6 +612,44 @@ declare_clippy_lint! { "`match` or match-like `if let` that are unnecessary" } +declare_clippy_lint! { + /// ### What it does + /// Finds nested `match` or `if let` expressions where the patterns may be "collapsed" together + /// without adding any branches. + /// + /// Note that this lint is not intended to find _all_ cases where nested match patterns can be merged, but only + /// cases where merging would most likely make the code more readable. + /// + /// ### Why is this bad? + /// It is unnecessarily verbose and complex. + /// + /// ### Example + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(n) => match n { + /// Ok(n) => n, + /// _ => return, + /// } + /// None => return, + /// }; + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn func(opt: Option>) { + /// let n = match opt { + /// Some(Ok(n)) => n, + /// _ => return, + /// }; + /// } + /// ``` + #[clippy::version = "1.50.0"] + pub COLLAPSIBLE_MATCH, + style, + "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -644,19 +684,29 @@ impl_lint_pass!(Matches => [ MATCH_LIKE_MATCHES_MACRO, MATCH_SAME_ARMS, NEEDLESS_MATCH, + COLLAPSIBLE_MATCH, ]); impl<'tcx> LateLintPass<'tcx> for Matches { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if expr.span.from_expansion() { + if in_external_macro(cx.sess(), expr.span) { return; } + let from_expansion = expr.span.from_expansion(); if let ExprKind::Match(ex, arms, source) = expr.kind { if !span_starts_with(cx, expr.span, "match") { return; } - if !contains_cfg_arm(cx, expr, ex, arms) { + + collapsible_match::check_match(cx, arms); + if !from_expansion { + // These don't depend on a relationship between multiple arms + match_wild_err_arm::check(cx, ex, arms); + wild_in_or_pats::check(cx, arms); + } + + if !from_expansion && !contains_cfg_arm(cx, expr, ex, arms) { if source == MatchSource::Normal { if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO) && match_like_matches::check_match(cx, expr, ex, arms)) @@ -680,16 +730,32 @@ impl<'tcx> LateLintPass<'tcx> for Matches { } match_ref_pats::check(cx, ex, arms.iter().map(|el| el.pat), expr); } - - // These don't depend on a relationship between multiple arms - match_wild_err_arm::check(cx, ex, arms); - wild_in_or_pats::check(cx, arms); - } else { - if meets_msrv(self.msrv, msrvs::MATCHES_MACRO) { - match_like_matches::check(cx, expr); + } else if let Some(if_let) = higher::IfLet::hir(cx, expr) { + collapsible_match::check_if_let(cx, if_let.let_pat, if_let.if_then, if_let.if_else); + if !from_expansion { + if let Some(else_expr) = if_let.if_else { + if meets_msrv(self.msrv, msrvs::MATCHES_MACRO) { + match_like_matches::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, + expr, + if_let.let_pat, + if_let.let_expr, + if_let.if_else.is_some(), + ); + needless_match::check_if_let(cx, expr, &if_let); } + } else if !from_expansion { redundant_pattern_match::check(cx, expr); - needless_match::check(cx, expr); } } diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index f920ad465..fa19cddd3 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -47,20 +47,18 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], /// some_enum /// } /// ``` -pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { - if let Some(ref if_let) = higher::IfLet::hir(cx, ex) { - if !is_else_clause(cx.tcx, ex) && expr_ty_matches_p_ty(cx, if_let.let_expr, ex) && check_if_let(cx, if_let) { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - NEEDLESS_MATCH, - ex.span, - "this if-let expression is unnecessary", - "replace it with", - snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(), - applicability, - ); - } +pub(crate) fn check_if_let<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'_>, if_let: &higher::IfLet<'tcx>) { + if !is_else_clause(cx.tcx, ex) && expr_ty_matches_p_ty(cx, if_let.let_expr, ex) && check_if_let_inner(cx, if_let) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + NEEDLESS_MATCH, + ex.span, + "this if-let expression is unnecessary", + "replace it with", + snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(), + applicability, + ); } } @@ -77,7 +75,7 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>]) true } -fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { +fn check_if_let_inner(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { if let Some(if_else) = if_let.if_else { if !pat_same_as_expr(if_let.let_pat, peel_blocks_with_stmt(if_let.if_then)) { return false; @@ -85,7 +83,7 @@ fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { // Recursively check for each `else if let` phrase, if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) { - return check_if_let(cx, nested_if_let); + return check_if_let_inner(cx, nested_if_let); } if matches!(if_else.kind, ExprKind::Block(..)) { diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs index 1a8b9d15f..095cd43ea 100644 --- a/clippy_lints/src/matches/redundant_pattern_match.rs +++ b/clippy_lints/src/matches/redundant_pattern_match.rs @@ -18,19 +18,21 @@ use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty}; use rustc_span::sym; pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if let Some(higher::IfLet { - if_else, - let_pat, - let_expr, - .. - }) = higher::IfLet::hir(cx, expr) - { - find_sugg_for_if_let(cx, expr, let_pat, let_expr, "if", if_else.is_some()); - } else if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) { + if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) { find_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false); } } +pub(super) fn check_if_let<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'_>, + pat: &'tcx Pat<'_>, + scrutinee: &'tcx Expr<'_>, + has_else: bool, +) { + find_sugg_for_if_let(cx, expr, pat, scrutinee, "if", has_else); +} + // Extract the generic arguments out of a type fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option> { if_chain! { From b337f9e62e8e8adee47684757c1ad101067beae2 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 12:29:54 -0400 Subject: [PATCH 2/7] Merge `ManualUnwrapOr` into `Matches` lint pass --- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_complexity.rs | 2 +- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/manual_unwrap_or.rs | 123 ------------------- clippy_lints/src/matches/manual_unwrap_or.rs | 83 +++++++++++++ clippy_lints/src/matches/mod.rs | 35 +++++- 7 files changed, 120 insertions(+), 129 deletions(-) delete mode 100644 clippy_lints/src/manual_unwrap_or.rs create mode 100644 clippy_lints/src/matches/manual_unwrap_or.rs diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index b1da36a0d..46af1d8fb 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -136,7 +136,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(manual_map::MANUAL_MAP), LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(manual_strip::MANUAL_STRIP), - LintId::of(manual_unwrap_or::MANUAL_UNWRAP_OR), LintId::of(map_clone::MAP_CLONE), LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), @@ -144,6 +143,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(match_str_case_mismatch::MATCH_STR_CASE_MISMATCH), LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), + LintId::of(matches::MANUAL_UNWRAP_OR), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_LIKE_MATCHES_MACRO), LintId::of(matches::MATCH_OVERLAPPING_ARM), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 29b38a01b..4f1c3673f 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -25,9 +25,9 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(loops::SINGLE_ELEMENT_LOOP), LintId::of(loops::WHILE_LET_LOOP), LintId::of(manual_strip::MANUAL_STRIP), - LintId::of(manual_unwrap_or::MANUAL_UNWRAP_OR), LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), + LintId::of(matches::MANUAL_UNWRAP_OR), LintId::of(matches::MATCH_AS_REF), LintId::of(matches::MATCH_SINGLE_BINDING), LintId::of(matches::NEEDLESS_MATCH), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 58e18b710..cb52000fa 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -254,7 +254,6 @@ store.register_lints(&[ manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, manual_ok_or::MANUAL_OK_OR, manual_strip::MANUAL_STRIP, - manual_unwrap_or::MANUAL_UNWRAP_OR, map_clone::MAP_CLONE, map_err_ignore::MAP_ERR_IGNORE, map_unit_fn::OPTION_MAP_UNIT_FN, @@ -264,6 +263,7 @@ store.register_lints(&[ match_str_case_mismatch::MATCH_STR_CASE_MISMATCH, matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, + matches::MANUAL_UNWRAP_OR, matches::MATCH_AS_REF, matches::MATCH_BOOL, matches::MATCH_LIKE_MATCHES_MACRO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6e16d470a..3fdd0db81 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -283,7 +283,6 @@ mod manual_map; mod manual_non_exhaustive; mod manual_ok_or; mod manual_strip; -mod manual_unwrap_or; mod map_clone; mod map_err_ignore; mod map_unit_fn; @@ -834,7 +833,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(repeat_once::RepeatOnce)); store.register_late_pass(|| Box::new(unwrap_in_result::UnwrapInResult)); store.register_late_pass(|| Box::new(self_assignment::SelfAssignment)); - store.register_late_pass(|| Box::new(manual_unwrap_or::ManualUnwrapOr)); store.register_late_pass(|| Box::new(manual_ok_or::ManualOkOr)); store.register_late_pass(|| Box::new(float_equality_without_abs::FloatEqualityWithoutAbs)); store.register_late_pass(|| Box::new(semicolon_if_nothing_returned::SemicolonIfNothingReturned)); diff --git a/clippy_lints/src/manual_unwrap_or.rs b/clippy_lints/src/manual_unwrap_or.rs deleted file mode 100644 index b3a91d9f1..000000000 --- a/clippy_lints/src/manual_unwrap_or.rs +++ /dev/null @@ -1,123 +0,0 @@ -use clippy_utils::consts::constant_simple; -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::{indent_of, reindent_multiline, snippet_opt}; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::usage::contains_return_break_continue_macro; -use clippy_utils::{in_constant, is_lang_ctor, path_to_local_id, sugg}; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk}; -use rustc_hir::{Arm, Expr, ExprKind, PatKind}; -use rustc_lint::LintContext; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::lint::in_external_macro; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::sym; - -declare_clippy_lint! { - /// ### What it does - /// Finds patterns that reimplement `Option::unwrap_or` or `Result::unwrap_or`. - /// - /// ### Why is this bad? - /// Concise code helps focusing on behavior instead of boilerplate. - /// - /// ### Example - /// ```rust - /// let foo: Option = None; - /// match foo { - /// Some(v) => v, - /// None => 1, - /// }; - /// ``` - /// - /// Use instead: - /// ```rust - /// let foo: Option = None; - /// foo.unwrap_or(1); - /// ``` - #[clippy::version = "1.49.0"] - pub MANUAL_UNWRAP_OR, - complexity, - "finds patterns that can be encoded more concisely with `Option::unwrap_or` or `Result::unwrap_or`" -} - -declare_lint_pass!(ManualUnwrapOr => [MANUAL_UNWRAP_OR]); - -impl<'tcx> LateLintPass<'tcx> for ManualUnwrapOr { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if in_external_macro(cx.sess(), expr.span) || in_constant(cx, expr.hir_id) { - return; - } - lint_manual_unwrap_or(cx, expr); - } -} - -fn lint_manual_unwrap_or<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> { - if_chain! { - if arms.len() == 2; - if arms.iter().all(|arm| arm.guard.is_none()); - if let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| { - match arm.pat.kind { - PatKind::Path(ref qpath) => is_lang_ctor(cx, qpath, OptionNone), - PatKind::TupleStruct(ref qpath, [pat], _) => - matches!(pat.kind, PatKind::Wild) && is_lang_ctor(cx, qpath, ResultErr), - _ => false, - } - }); - let unwrap_arm = &arms[1 - idx]; - if let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = unwrap_arm.pat.kind; - if is_lang_ctor(cx, qpath, OptionSome) || is_lang_ctor(cx, qpath, ResultOk); - if let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind; - if path_to_local_id(unwrap_arm.body, binding_hir_id); - if cx.typeck_results().expr_adjustments(unwrap_arm.body).is_empty(); - if !contains_return_break_continue_macro(or_arm.body); - then { - Some(or_arm) - } else { - None - } - } - } - - if_chain! { - if let ExprKind::Match(scrutinee, match_arms, _) = expr.kind; - let ty = cx.typeck_results().expr_ty(scrutinee); - if let Some(ty_name) = 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 - }; - if let Some(or_arm) = applicable_or_arm(cx, match_arms); - if let Some(or_body_snippet) = snippet_opt(cx, or_arm.body.span); - if let Some(indent) = indent_of(cx, expr.span); - if constant_simple(cx, cx.typeck_results(), or_arm.body).is_some(); - then { - let reindented_or_body = - reindent_multiline(or_body_snippet.into(), true, Some(indent)); - - let suggestion = if scrutinee.span.from_expansion() { - // we don't want parentheses around macro, e.g. `(some_macro!()).unwrap_or(0)` - sugg::Sugg::hir_with_macro_callsite(cx, scrutinee, "..") - } - else { - sugg::Sugg::hir(cx, scrutinee, "..").maybe_par() - }; - - span_lint_and_sugg( - cx, - MANUAL_UNWRAP_OR, expr.span, - &format!("this pattern reimplements `{}::unwrap_or`", ty_name), - "replace with", - format!( - "{}.unwrap_or({})", - suggestion, - reindented_or_body, - ), - Applicability::MachineApplicable, - ); - } - } -} diff --git a/clippy_lints/src/matches/manual_unwrap_or.rs b/clippy_lints/src/matches/manual_unwrap_or.rs new file mode 100644 index 000000000..e1111c80f --- /dev/null +++ b/clippy_lints/src/matches/manual_unwrap_or.rs @@ -0,0 +1,83 @@ +use clippy_utils::consts::constant_simple; +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::{indent_of, reindent_multiline, snippet_opt}; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::usage::contains_return_break_continue_macro; +use clippy_utils::{is_lang_ctor, path_to_local_id, sugg}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk}; +use rustc_hir::{Arm, Expr, PatKind}; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::MANUAL_UNWRAP_OR; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, scrutinee: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) { + let ty = cx.typeck_results().expr_ty(scrutinee); + if_chain! { + if let Some(ty_name) = 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 + }; + if let Some(or_arm) = applicable_or_arm(cx, arms); + if let Some(or_body_snippet) = snippet_opt(cx, or_arm.body.span); + if let Some(indent) = indent_of(cx, expr.span); + if constant_simple(cx, cx.typeck_results(), or_arm.body).is_some(); + then { + let reindented_or_body = + reindent_multiline(or_body_snippet.into(), true, Some(indent)); + + let suggestion = if scrutinee.span.from_expansion() { + // we don't want parentheses around macro, e.g. `(some_macro!()).unwrap_or(0)` + sugg::Sugg::hir_with_macro_callsite(cx, scrutinee, "..") + } + else { + sugg::Sugg::hir(cx, scrutinee, "..").maybe_par() + }; + + span_lint_and_sugg( + cx, + MANUAL_UNWRAP_OR, expr.span, + &format!("this pattern reimplements `{}::unwrap_or`", ty_name), + "replace with", + format!( + "{}.unwrap_or({})", + suggestion, + reindented_or_body, + ), + Applicability::MachineApplicable, + ); + } + } +} + +fn applicable_or_arm<'a>(cx: &LateContext<'_>, arms: &'a [Arm<'a>]) -> Option<&'a Arm<'a>> { + if_chain! { + if arms.len() == 2; + if arms.iter().all(|arm| arm.guard.is_none()); + if let Some((idx, or_arm)) = arms.iter().enumerate().find(|(_, arm)| { + match arm.pat.kind { + PatKind::Path(ref qpath) => is_lang_ctor(cx, qpath, OptionNone), + PatKind::TupleStruct(ref qpath, [pat], _) => + matches!(pat.kind, PatKind::Wild) && is_lang_ctor(cx, qpath, ResultErr), + _ => false, + } + }); + let unwrap_arm = &arms[1 - idx]; + if let PatKind::TupleStruct(ref qpath, [unwrap_pat], _) = unwrap_arm.pat.kind; + if is_lang_ctor(cx, qpath, OptionSome) || is_lang_ctor(cx, qpath, ResultOk); + if let PatKind::Binding(_, binding_hir_id, ..) = unwrap_pat.kind; + if path_to_local_id(unwrap_arm.body, binding_hir_id); + if cx.typeck_results().expr_adjustments(unwrap_arm.body).is_empty(); + if !contains_return_break_continue_macro(or_arm.body); + then { + Some(or_arm) + } else { + None + } + } +} diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 4a77fb8c3..ade47b1ed 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -1,5 +1,5 @@ use clippy_utils::source::{snippet_opt, span_starts_with, walk_span_to_context}; -use clippy_utils::{higher, meets_msrv, msrvs}; +use clippy_utils::{higher, in_constant, meets_msrv, msrvs}; use rustc_hir::{Arm, Expr, ExprKind, Local, MatchSource, Pat}; use rustc_lexer::{tokenize, TokenKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; @@ -10,6 +10,7 @@ use rustc_span::{Span, SpanData, SyntaxContext}; mod collapsible_match; mod infallible_destructuring_match; +mod manual_unwrap_or; mod match_as_ref; mod match_bool; mod match_like_matches; @@ -650,6 +651,33 @@ declare_clippy_lint! { "Nested `match` or `if let` expressions where the patterns may be \"collapsed\" together." } +declare_clippy_lint! { + /// ### What it does + /// Finds patterns that reimplement `Option::unwrap_or` or `Result::unwrap_or`. + /// + /// ### Why is this bad? + /// Concise code helps focusing on behavior instead of boilerplate. + /// + /// ### Example + /// ```rust + /// let foo: Option = None; + /// match foo { + /// Some(v) => v, + /// None => 1, + /// }; + /// ``` + /// + /// Use instead: + /// ```rust + /// let foo: Option = None; + /// foo.unwrap_or(1); + /// ``` + #[clippy::version = "1.49.0"] + pub MANUAL_UNWRAP_OR, + complexity, + "finds patterns that can be encoded more concisely with `Option::unwrap_or` or `Result::unwrap_or`" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -685,6 +713,7 @@ impl_lint_pass!(Matches => [ MATCH_SAME_ARMS, NEEDLESS_MATCH, COLLAPSIBLE_MATCH, + MANUAL_UNWRAP_OR, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -722,6 +751,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches { match_as_ref::check(cx, ex, arms, expr); needless_match::check_match(cx, ex, arms, expr); + if !in_constant(cx, expr.hir_id) { + manual_unwrap_or::check(cx, expr, ex, arms); + } + if self.infallible_destructuring_match_linted { self.infallible_destructuring_match_linted = false; } else { From 3d8d7341509e4868c91fa7fe24703e69c21a9869 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 13:10:57 -0400 Subject: [PATCH 3/7] Move `MatchOnVecItems` into `Matches` lint pass --- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.register_pedantic.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/match_on_vec_items.rs | 104 ------------------ .../src/matches/match_on_vec_items.rs | 61 ++++++++++ clippy_lints/src/matches/mod.rs | 40 +++++++ 6 files changed, 103 insertions(+), 108 deletions(-) delete mode 100644 clippy_lints/src/match_on_vec_items.rs create mode 100644 clippy_lints/src/matches/match_on_vec_items.rs diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index cb52000fa..fb44df57d 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -258,7 +258,6 @@ store.register_lints(&[ map_err_ignore::MAP_ERR_IGNORE, map_unit_fn::OPTION_MAP_UNIT_FN, map_unit_fn::RESULT_MAP_UNIT_FN, - match_on_vec_items::MATCH_ON_VEC_ITEMS, match_result_ok::MATCH_RESULT_OK, match_str_case_mismatch::MATCH_STR_CASE_MISMATCH, matches::COLLAPSIBLE_MATCH, @@ -267,6 +266,7 @@ store.register_lints(&[ matches::MATCH_AS_REF, matches::MATCH_BOOL, matches::MATCH_LIKE_MATCHES_MACRO, + matches::MATCH_ON_VEC_ITEMS, matches::MATCH_OVERLAPPING_ARM, matches::MATCH_REF_PATS, matches::MATCH_SAME_ARMS, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 2e47a287d..a8b6c5a5d 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -51,8 +51,8 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(macro_use::MACRO_USE_IMPORTS), LintId::of(manual_assert::MANUAL_ASSERT), LintId::of(manual_ok_or::MANUAL_OK_OR), - LintId::of(match_on_vec_items::MATCH_ON_VEC_ITEMS), LintId::of(matches::MATCH_BOOL), + LintId::of(matches::MATCH_ON_VEC_ITEMS), LintId::of(matches::MATCH_SAME_ARMS), LintId::of(matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS), LintId::of(matches::MATCH_WILD_ERR_ARM), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3fdd0db81..50b67918f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -286,7 +286,6 @@ mod manual_strip; mod map_clone; mod map_err_ignore; mod map_unit_fn; -mod match_on_vec_items; mod match_result_ok; mod match_str_case_mismatch; mod matches; @@ -815,7 +814,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(if_not_else::IfNotElse)); store.register_late_pass(|| Box::new(equatable_if_let::PatternEquality)); store.register_late_pass(|| Box::new(mut_mutex_lock::MutMutexLock)); - store.register_late_pass(|| Box::new(match_on_vec_items::MatchOnVecItems)); store.register_late_pass(|| Box::new(manual_async_fn::ManualAsyncFn)); store.register_late_pass(|| Box::new(vec_resize_to_zero::VecResizeToZero)); store.register_late_pass(|| Box::new(panic_in_result_fn::PanicInResultFn)); diff --git a/clippy_lints/src/match_on_vec_items.rs b/clippy_lints/src/match_on_vec_items.rs deleted file mode 100644 index 583b577ff..000000000 --- a/clippy_lints/src/match_on_vec_items.rs +++ /dev/null @@ -1,104 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet; -use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item}; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, LangItem, MatchSource}; -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; - -declare_clippy_lint! { - /// ### What it does - /// Checks for `match vec[idx]` or `match vec[n..m]`. - /// - /// ### Why is this bad? - /// This can panic at runtime. - /// - /// ### Example - /// ```rust, no_run - /// let arr = vec![0, 1, 2, 3]; - /// let idx = 1; - /// - /// // Bad - /// match arr[idx] { - /// 0 => println!("{}", 0), - /// 1 => println!("{}", 3), - /// _ => {}, - /// } - /// ``` - /// Use instead: - /// ```rust, no_run - /// let arr = vec![0, 1, 2, 3]; - /// let idx = 1; - /// - /// // Good - /// match arr.get(idx) { - /// Some(0) => println!("{}", 0), - /// Some(1) => println!("{}", 3), - /// _ => {}, - /// } - /// ``` - #[clippy::version = "1.45.0"] - pub MATCH_ON_VEC_ITEMS, - pedantic, - "matching on vector elements can panic" -} - -declare_lint_pass!(MatchOnVecItems => [MATCH_ON_VEC_ITEMS]); - -impl<'tcx> LateLintPass<'tcx> for MatchOnVecItems { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if_chain! { - if !in_external_macro(cx.sess(), expr.span); - if let ExprKind::Match(match_expr, _, MatchSource::Normal) = expr.kind; - if let Some(idx_expr) = is_vec_indexing(cx, match_expr); - if let ExprKind::Index(vec, idx) = idx_expr.kind; - - then { - // FIXME: could be improved to suggest surrounding every pattern with Some(_), - // but only when `or_patterns` are stabilized. - span_lint_and_sugg( - cx, - MATCH_ON_VEC_ITEMS, - match_expr.span, - "indexing into a vector may panic", - "try this", - format!( - "{}.get({})", - snippet(cx, vec.span, ".."), - snippet(cx, idx.span, "..") - ), - Applicability::MaybeIncorrect - ); - } - } - } -} - -fn is_vec_indexing<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { - if_chain! { - if let ExprKind::Index(array, index) = expr.kind; - if is_vector(cx, array); - if !is_full_range(cx, index); - - then { - return Some(expr); - } - } - - None -} - -fn is_vector(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - let ty = cx.typeck_results().expr_ty(expr); - let ty = ty.peel_refs(); - is_type_diagnostic_item(cx, ty, sym::Vec) -} - -fn is_full_range(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - let ty = cx.typeck_results().expr_ty(expr); - let ty = ty.peel_refs(); - is_type_lang_item(cx, ty, LangItem::RangeFull) -} diff --git a/clippy_lints/src/matches/match_on_vec_items.rs b/clippy_lints/src/matches/match_on_vec_items.rs new file mode 100644 index 000000000..2917f85c4 --- /dev/null +++ b/clippy_lints/src/matches/match_on_vec_items.rs @@ -0,0 +1,61 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet; +use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, LangItem}; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::MATCH_ON_VEC_ITEMS; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, scrutinee: &'tcx Expr<'_>) { + if_chain! { + if let Some(idx_expr) = is_vec_indexing(cx, scrutinee); + if let ExprKind::Index(vec, idx) = idx_expr.kind; + + then { + // FIXME: could be improved to suggest surrounding every pattern with Some(_), + // but only when `or_patterns` are stabilized. + span_lint_and_sugg( + cx, + MATCH_ON_VEC_ITEMS, + scrutinee.span, + "indexing into a vector may panic", + "try this", + format!( + "{}.get({})", + snippet(cx, vec.span, ".."), + snippet(cx, idx.span, "..") + ), + Applicability::MaybeIncorrect + ); + } + } +} + +fn is_vec_indexing<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + if_chain! { + if let ExprKind::Index(array, index) = expr.kind; + if is_vector(cx, array); + if !is_full_range(cx, index); + + then { + return Some(expr); + } + } + + None +} + +fn is_vector(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let ty = cx.typeck_results().expr_ty(expr); + let ty = ty.peel_refs(); + is_type_diagnostic_item(cx, ty, sym::Vec) +} + +fn is_full_range(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let ty = cx.typeck_results().expr_ty(expr); + let ty = ty.peel_refs(); + is_type_lang_item(cx, ty, LangItem::RangeFull) +} diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index ade47b1ed..7fb449028 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -14,6 +14,7 @@ mod manual_unwrap_or; mod match_as_ref; mod match_bool; mod match_like_matches; +mod match_on_vec_items; mod match_ref_pats; mod match_same_arms; mod match_single_binding; @@ -678,6 +679,43 @@ declare_clippy_lint! { "finds patterns that can be encoded more concisely with `Option::unwrap_or` or `Result::unwrap_or`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `match vec[idx]` or `match vec[n..m]`. + /// + /// ### Why is this bad? + /// This can panic at runtime. + /// + /// ### Example + /// ```rust, no_run + /// let arr = vec![0, 1, 2, 3]; + /// let idx = 1; + /// + /// // Bad + /// match arr[idx] { + /// 0 => println!("{}", 0), + /// 1 => println!("{}", 3), + /// _ => {}, + /// } + /// ``` + /// Use instead: + /// ```rust, no_run + /// let arr = vec![0, 1, 2, 3]; + /// let idx = 1; + /// + /// // Good + /// match arr.get(idx) { + /// Some(0) => println!("{}", 0), + /// Some(1) => println!("{}", 3), + /// _ => {}, + /// } + /// ``` + #[clippy::version = "1.45.0"] + pub MATCH_ON_VEC_ITEMS, + pedantic, + "matching on vector elements can panic" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -714,6 +752,7 @@ impl_lint_pass!(Matches => [ NEEDLESS_MATCH, COLLAPSIBLE_MATCH, MANUAL_UNWRAP_OR, + MATCH_ON_VEC_ITEMS, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -750,6 +789,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { match_wild_enum::check(cx, ex, arms); match_as_ref::check(cx, ex, arms, expr); needless_match::check_match(cx, ex, arms, expr); + match_on_vec_items::check(cx, ex); if !in_constant(cx, expr.hir_id) { manual_unwrap_or::check(cx, expr, ex, arms); From 8c8a52eeeb9ca47575200eacd1ab86bf46e8dc15 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 13:16:02 -0400 Subject: [PATCH 4/7] Move `MatchStrCaseMismatch` into `Matches` lint pass --- clippy_lints/src/lib.register_all.rs | 2 +- clippy_lints/src/lib.register_correctness.rs | 2 +- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 - .../{ => matches}/match_str_case_mismatch.rs | 69 +++++-------------- clippy_lints/src/matches/mod.rs | 34 +++++++++ 6 files changed, 53 insertions(+), 58 deletions(-) rename clippy_lints/src/{ => matches}/match_str_case_mismatch.rs (64%) diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 46af1d8fb..15e744229 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -140,7 +140,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(match_result_ok::MATCH_RESULT_OK), - LintId::of(match_str_case_mismatch::MATCH_STR_CASE_MISMATCH), LintId::of(matches::COLLAPSIBLE_MATCH), LintId::of(matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(matches::MANUAL_UNWRAP_OR), @@ -149,6 +148,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(matches::MATCH_OVERLAPPING_ARM), LintId::of(matches::MATCH_REF_PATS), LintId::of(matches::MATCH_SINGLE_BINDING), + LintId::of(matches::MATCH_STR_CASE_MISMATCH), LintId::of(matches::NEEDLESS_MATCH), LintId::of(matches::REDUNDANT_PATTERN_MATCHING), LintId::of(matches::SINGLE_MATCH), diff --git a/clippy_lints/src/lib.register_correctness.rs b/clippy_lints/src/lib.register_correctness.rs index 6bf2c4bba..50cdd0af9 100644 --- a/clippy_lints/src/lib.register_correctness.rs +++ b/clippy_lints/src/lib.register_correctness.rs @@ -39,7 +39,7 @@ store.register_group(true, "clippy::correctness", Some("clippy_correctness"), ve LintId::of(loops::ITER_NEXT_LOOP), LintId::of(loops::NEVER_LOOP), LintId::of(loops::WHILE_IMMUTABLE_CONDITION), - LintId::of(match_str_case_mismatch::MATCH_STR_CASE_MISMATCH), + LintId::of(matches::MATCH_STR_CASE_MISMATCH), LintId::of(mem_replace::MEM_REPLACE_WITH_UNINIT), LintId::of(methods::CLONE_DOUBLE_REF), LintId::of(methods::ITERATOR_STEP_BY_ZERO), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index fb44df57d..59ba295a8 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -259,7 +259,6 @@ store.register_lints(&[ map_unit_fn::OPTION_MAP_UNIT_FN, map_unit_fn::RESULT_MAP_UNIT_FN, match_result_ok::MATCH_RESULT_OK, - match_str_case_mismatch::MATCH_STR_CASE_MISMATCH, matches::COLLAPSIBLE_MATCH, matches::INFALLIBLE_DESTRUCTURING_MATCH, matches::MANUAL_UNWRAP_OR, @@ -271,6 +270,7 @@ store.register_lints(&[ matches::MATCH_REF_PATS, matches::MATCH_SAME_ARMS, matches::MATCH_SINGLE_BINDING, + matches::MATCH_STR_CASE_MISMATCH, matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS, matches::MATCH_WILD_ERR_ARM, matches::NEEDLESS_MATCH, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 50b67918f..7335b4a35 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -287,7 +287,6 @@ mod map_clone; mod map_err_ignore; mod map_unit_fn; mod match_result_ok; -mod match_str_case_mismatch; mod matches; mod mem_forget; mod mem_replace; @@ -875,7 +874,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: )) }); store.register_late_pass(move || Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks)); - store.register_late_pass(|| Box::new(match_str_case_mismatch::MatchStrCaseMismatch)); store.register_late_pass(move || Box::new(format_args::FormatArgs)); store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray)); store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes)); diff --git a/clippy_lints/src/match_str_case_mismatch.rs b/clippy_lints/src/matches/match_str_case_mismatch.rs similarity index 64% rename from clippy_lints/src/match_str_case_mismatch.rs rename to clippy_lints/src/matches/match_str_case_mismatch.rs index d97a87882..8302ce426 100644 --- a/clippy_lints/src/match_str_case_mismatch.rs +++ b/clippy_lints/src/matches/match_str_case_mismatch.rs @@ -3,46 +3,13 @@ use clippy_utils::ty::is_type_diagnostic_item; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::lint::in_external_macro; +use rustc_hir::{Arm, Expr, ExprKind, PatKind}; +use rustc_lint::LateContext; use rustc_middle::ty; -use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::symbol::Symbol; use rustc_span::{sym, Span}; -declare_clippy_lint! { - /// ### What it does - /// Checks for `match` expressions modifying the case of a string with non-compliant arms - /// - /// ### Why is this bad? - /// The arm is unreachable, which is likely a mistake - /// - /// ### Example - /// ```rust - /// # let text = "Foo"; - /// match &*text.to_ascii_lowercase() { - /// "foo" => {}, - /// "Bar" => {}, - /// _ => {}, - /// } - /// ``` - /// Use instead: - /// ```rust - /// # let text = "Foo"; - /// match &*text.to_ascii_lowercase() { - /// "foo" => {}, - /// "bar" => {}, - /// _ => {}, - /// } - /// ``` - #[clippy::version = "1.58.0"] - pub MATCH_STR_CASE_MISMATCH, - correctness, - "creation of a case altering match expression with non-compliant arms" -} - -declare_lint_pass!(MatchStrCaseMismatch => [MATCH_STR_CASE_MISMATCH]); +use super::MATCH_STR_CASE_MISMATCH; #[derive(Debug)] enum CaseMethod { @@ -52,25 +19,21 @@ enum CaseMethod { AsciiUppercase, } -impl<'tcx> LateLintPass<'tcx> for MatchStrCaseMismatch { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { - if !in_external_macro(cx.tcx.sess, expr.span); - if let ExprKind::Match(match_expr, arms, MatchSource::Normal) = expr.kind; - if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty(match_expr).kind(); - if let ty::Str = ty.kind(); - then { - let mut visitor = MatchExprVisitor { - cx, - case_method: None, - }; +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, scrutinee: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) { + if_chain! { + if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty(scrutinee).kind(); + if let ty::Str = ty.kind(); + then { + let mut visitor = MatchExprVisitor { + cx, + case_method: None, + }; - visitor.visit_expr(match_expr); + visitor.visit_expr(scrutinee); - if let Some(case_method) = visitor.case_method { - if let Some((bad_case_span, bad_case_sym)) = verify_case(&case_method, arms) { - lint(cx, &case_method, bad_case_span, bad_case_sym.as_str()); - } + if let Some(case_method) = visitor.case_method { + if let Some((bad_case_span, bad_case_sym)) = verify_case(&case_method, arms) { + lint(cx, &case_method, bad_case_span, bad_case_sym.as_str()); } } } diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 7fb449028..a58c71ca5 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -18,6 +18,7 @@ mod match_on_vec_items; mod match_ref_pats; mod match_same_arms; mod match_single_binding; +mod match_str_case_mismatch; mod match_wild_enum; mod match_wild_err_arm; mod needless_match; @@ -716,6 +717,37 @@ declare_clippy_lint! { "matching on vector elements can panic" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `match` expressions modifying the case of a string with non-compliant arms + /// + /// ### Why is this bad? + /// The arm is unreachable, which is likely a mistake + /// + /// ### Example + /// ```rust + /// # let text = "Foo"; + /// match &*text.to_ascii_lowercase() { + /// "foo" => {}, + /// "Bar" => {}, + /// _ => {}, + /// } + /// ``` + /// Use instead: + /// ```rust + /// # let text = "Foo"; + /// match &*text.to_ascii_lowercase() { + /// "foo" => {}, + /// "bar" => {}, + /// _ => {}, + /// } + /// ``` + #[clippy::version = "1.58.0"] + pub MATCH_STR_CASE_MISMATCH, + correctness, + "creation of a case altering match expression with non-compliant arms" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -753,6 +785,7 @@ impl_lint_pass!(Matches => [ COLLAPSIBLE_MATCH, MANUAL_UNWRAP_OR, MATCH_ON_VEC_ITEMS, + MATCH_STR_CASE_MISMATCH, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -790,6 +823,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches { match_as_ref::check(cx, ex, arms, expr); needless_match::check_match(cx, ex, arms, expr); match_on_vec_items::check(cx, ex); + match_str_case_mismatch::check(cx, ex, arms); if !in_constant(cx, expr.hir_id) { manual_unwrap_or::check(cx, expr, ex, arms); From dbc7753fb2b5e1af5c00f3c1e246cfd594611c75 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 13:35:10 -0400 Subject: [PATCH 5/7] Merge `SignificantDropInScrutinee` 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_suspicious.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/matches/mod.rs | 83 ++++++++++- .../significant_drop_in_scrutinee.rs | 132 ++++-------------- 6 files changed, 109 insertions(+), 114 deletions(-) rename clippy_lints/src/{ => matches}/significant_drop_in_scrutinee.rs (75%) diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 15e744229..435c0829a 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -151,6 +151,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(matches::MATCH_STR_CASE_MISMATCH), LintId::of(matches::NEEDLESS_MATCH), LintId::of(matches::REDUNDANT_PATTERN_MATCHING), + LintId::of(matches::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(matches::SINGLE_MATCH), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(mem_replace::MEM_REPLACE_OPTION_WITH_NONE), @@ -282,7 +283,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(self_assignment::SELF_ASSIGNMENT), LintId::of(self_named_constructors::SELF_NAMED_CONSTRUCTORS), LintId::of(serde_api::SERDE_API_MISUSE), - LintId::of(significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT), LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 59ba295a8..7fc6ff8a2 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -276,6 +276,7 @@ store.register_lints(&[ matches::NEEDLESS_MATCH, matches::REDUNDANT_PATTERN_MATCHING, matches::REST_PAT_IN_FULLY_BOUND_STRUCTS, + matches::SIGNIFICANT_DROP_IN_SCRUTINEE, matches::SINGLE_MATCH, matches::SINGLE_MATCH_ELSE, matches::WILDCARD_ENUM_MATCH_ARM, @@ -479,7 +480,6 @@ store.register_lints(&[ shadow::SHADOW_REUSE, shadow::SHADOW_SAME, shadow::SHADOW_UNRELATED, - significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE, single_char_lifetime_names::SINGLE_CHAR_LIFETIME_NAMES, single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS, size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 43ced5070..7b13713c3 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -24,12 +24,12 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(loops::EMPTY_LOOP), LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(loops::MUT_RANGE_BOUND), + LintId::of(matches::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(methods::NO_EFFECT_REPLACE), LintId::of(methods::SUSPICIOUS_MAP), LintId::of(mut_key::MUTABLE_KEY_TYPE), LintId::of(octal_escapes::OCTAL_ESCAPES), LintId::of(rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT), - LintId::of(significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7335b4a35..742409d75 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -367,7 +367,6 @@ mod self_named_constructors; mod semicolon_if_nothing_returned; mod serde_api; mod shadow; -mod significant_drop_in_scrutinee; mod single_char_lifetime_names; mod single_component_path_imports; mod size_of_in_element_count; @@ -886,7 +885,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation)); store.register_early_pass(|| Box::new(doc_link_with_quotes::DocLinkWithQuotes)); store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion)); - store.register_late_pass(|| Box::new(significant_drop_in_scrutinee::SignificantDropInScrutinee)); let allow_dbg_in_tests = conf.allow_dbg_in_tests; store.register_late_pass(move || Box::new(dbg_macro::DbgMacro::new(allow_dbg_in_tests))); let cargo_ignore_publish = conf.cargo_ignore_publish; diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index a58c71ca5..3c0dc8041 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -25,6 +25,7 @@ mod needless_match; mod overlapping_arms; mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; +mod significant_drop_in_scrutinee; mod single_match; mod wild_in_or_pats; @@ -748,6 +749,82 @@ declare_clippy_lint! { "creation of a case altering match expression with non-compliant arms" } +declare_clippy_lint! { + /// ### What it does + /// Check for temporaries returned from function calls in a match scrutinee that have the + /// `clippy::has_significant_drop` attribute. + /// + /// ### Why is this bad? + /// The `clippy::has_significant_drop` attribute can be added to types whose Drop impls have + /// an important side-effect, such as unlocking a mutex, making it important for users to be + /// able to accurately understand their lifetimes. When a temporary is returned in a function + /// call in a match scrutinee, its lifetime lasts until the end of the match block, which may + /// be surprising. + /// + /// For `Mutex`es this can lead to a deadlock. This happens when the match scrutinee uses a + /// function call that returns a `MutexGuard` and then tries to lock again in one of the match + /// arms. In that case the `MutexGuard` in the scrutinee will not be dropped until the end of + /// the match block and thus will not unlock. + /// + /// ### Example + /// ```rust.ignore + /// # use std::sync::Mutex; + /// + /// # struct State {} + /// + /// # impl State { + /// # fn foo(&self) -> bool { + /// # true + /// # } + /// + /// # fn bar(&self) {} + /// # } + /// + /// + /// let mutex = Mutex::new(State {}); + /// + /// match mutex.lock().unwrap().foo() { + /// true => { + /// mutex.lock().unwrap().bar(); // Deadlock! + /// } + /// false => {} + /// }; + /// + /// println!("All done!"); + /// + /// ``` + /// Use instead: + /// ```rust + /// # use std::sync::Mutex; + /// + /// # struct State {} + /// + /// # impl State { + /// # fn foo(&self) -> bool { + /// # true + /// # } + /// + /// # fn bar(&self) {} + /// # } + /// + /// let mutex = Mutex::new(State {}); + /// + /// let is_foo = mutex.lock().unwrap().foo(); + /// match is_foo { + /// true => { + /// mutex.lock().unwrap().bar(); + /// } + /// false => {} + /// }; + /// + /// println!("All done!"); + /// ``` + #[clippy::version = "1.60.0"] + pub SIGNIFICANT_DROP_IN_SCRUTINEE, + suspicious, + "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -786,6 +863,7 @@ impl_lint_pass!(Matches => [ MANUAL_UNWRAP_OR, MATCH_ON_VEC_ITEMS, MATCH_STR_CASE_MISMATCH, + SIGNIFICANT_DROP_IN_SCRUTINEE, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -796,9 +874,12 @@ impl<'tcx> LateLintPass<'tcx> for Matches { let from_expansion = expr.span.from_expansion(); if let ExprKind::Match(ex, arms, source) = expr.kind { - if !span_starts_with(cx, expr.span, "match") { + if source == MatchSource::Normal && !span_starts_with(cx, expr.span, "match") { return; } + if matches!(source, MatchSource::Normal | MatchSource::ForLoopDesugar) { + significant_drop_in_scrutinee::check(cx, expr, ex, source); + } collapsible_match::check_match(cx, arms); if !from_expansion { diff --git a/clippy_lints/src/significant_drop_in_scrutinee.rs b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs similarity index 75% rename from clippy_lints/src/significant_drop_in_scrutinee.rs rename to clippy_lints/src/matches/significant_drop_in_scrutinee.rs index 2f7819cb4..a211dc18f 100644 --- a/clippy_lints/src/significant_drop_in_scrutinee.rs +++ b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs @@ -5,98 +5,24 @@ use clippy_utils::source::{indent_of, snippet}; use rustc_errors::{Applicability, Diagnostic}; use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_hir::{Expr, ExprKind, MatchSource}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_lint::{LateContext, LintContext}; use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::{Ty, TypeAndMut}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; -declare_clippy_lint! { - /// ### What it does - /// Check for temporaries returned from function calls in a match scrutinee that have the - /// `clippy::has_significant_drop` attribute. - /// - /// ### Why is this bad? - /// The `clippy::has_significant_drop` attribute can be added to types whose Drop impls have - /// an important side-effect, such as unlocking a mutex, making it important for users to be - /// able to accurately understand their lifetimes. When a temporary is returned in a function - /// call in a match scrutinee, its lifetime lasts until the end of the match block, which may - /// be surprising. - /// - /// For `Mutex`es this can lead to a deadlock. This happens when the match scrutinee uses a - /// function call that returns a `MutexGuard` and then tries to lock again in one of the match - /// arms. In that case the `MutexGuard` in the scrutinee will not be dropped until the end of - /// the match block and thus will not unlock. - /// - /// ### Example - /// ```rust.ignore - /// # use std::sync::Mutex; - /// - /// # struct State {} - /// - /// # impl State { - /// # fn foo(&self) -> bool { - /// # true - /// # } - /// - /// # fn bar(&self) {} - /// # } - /// - /// - /// let mutex = Mutex::new(State {}); - /// - /// match mutex.lock().unwrap().foo() { - /// true => { - /// mutex.lock().unwrap().bar(); // Deadlock! - /// } - /// false => {} - /// }; - /// - /// println!("All done!"); - /// - /// ``` - /// Use instead: - /// ```rust - /// # use std::sync::Mutex; - /// - /// # struct State {} - /// - /// # impl State { - /// # fn foo(&self) -> bool { - /// # true - /// # } - /// - /// # fn bar(&self) {} - /// # } - /// - /// let mutex = Mutex::new(State {}); - /// - /// let is_foo = mutex.lock().unwrap().foo(); - /// match is_foo { - /// true => { - /// mutex.lock().unwrap().bar(); - /// } - /// false => {} - /// }; - /// - /// println!("All done!"); - /// ``` - #[clippy::version = "1.60.0"] - pub SIGNIFICANT_DROP_IN_SCRUTINEE, - suspicious, - "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" -} +use super::SIGNIFICANT_DROP_IN_SCRUTINEE; -declare_lint_pass!(SignificantDropInScrutinee => [SIGNIFICANT_DROP_IN_SCRUTINEE]); - -impl<'tcx> LateLintPass<'tcx> for SignificantDropInScrutinee { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, expr) { - for found in suggestions { - span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| { - set_diagnostic(diag, cx, expr, found); - }); - } +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + scrutinee: &'tcx Expr<'_>, + source: MatchSource, +) { + if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, scrutinee, source) { + for found in suggestions { + span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| { + set_diagnostic(diag, cx, expr, found); + }); } } } @@ -148,28 +74,18 @@ fn set_diagnostic<'tcx>(diag: &mut Diagnostic, cx: &LateContext<'tcx>, expr: &'t /// may have a surprising lifetime. fn has_significant_drop_in_scrutinee<'tcx, 'a>( cx: &'a LateContext<'tcx>, - expr: &'tcx Expr<'tcx>, + scrutinee: &'tcx Expr<'tcx>, + source: MatchSource, ) -> Option<(Vec, &'static str)> { - match expr.kind { - ExprKind::Match(match_expr, _, source) => { - match source { - MatchSource::Normal | MatchSource::ForLoopDesugar => { - let mut helper = SigDropHelper::new(cx); - helper.find_sig_drop(match_expr).map(|drops| { - let message = if source == MatchSource::Normal { - "temporary with significant drop in match scrutinee" - } else { - "temporary with significant drop in for loop" - }; - (drops, message) - }) - }, - // MatchSource of TryDesugar or AwaitDesugar is out of scope for this lint - MatchSource::TryDesugar | MatchSource::AwaitDesugar => None, - } - }, - _ => None, - } + let mut helper = SigDropHelper::new(cx); + helper.find_sig_drop(scrutinee).map(|drops| { + let message = if source == MatchSource::Normal { + "temporary with significant drop in match scrutinee" + } else { + "temporary with significant drop in for loop" + }; + (drops, message) + }) } struct SigDropHelper<'a, 'tcx> { From 67cb5ec29f0e1adb55f945a8acea20c47f586c56 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 14:31:36 -0400 Subject: [PATCH 6/7] Move `TryErr` into `Matches` lint pass --- clippy_lints/src/lib.register_lints.rs | 2 +- clippy_lints/src/lib.register_restriction.rs | 2 +- clippy_lints/src/lib.rs | 2 - clippy_lints/src/matches/mod.rs | 41 ++++ clippy_lints/src/matches/try_err.rs | 145 +++++++++++++++ clippy_lints/src/try_err.rs | 186 ------------------- 6 files changed, 188 insertions(+), 190 deletions(-) create mode 100644 clippy_lints/src/matches/try_err.rs delete mode 100644 clippy_lints/src/try_err.rs diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 7fc6ff8a2..c8cebca89 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -279,6 +279,7 @@ store.register_lints(&[ matches::SIGNIFICANT_DROP_IN_SCRUTINEE, matches::SINGLE_MATCH, matches::SINGLE_MATCH_ELSE, + matches::TRY_ERR, matches::WILDCARD_ENUM_MATCH_ARM, matches::WILDCARD_IN_OR_PATTERNS, mem_forget::MEM_FORGET, @@ -521,7 +522,6 @@ store.register_lints(&[ transmute::USELESS_TRANSMUTE, transmute::WRONG_TRANSMUTE, transmuting_null::TRANSMUTING_NULL, - try_err::TRY_ERR, types::BORROWED_BOX, types::BOX_COLLECTION, types::LINKEDLIST, diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs index 304b59554..942a53459 100644 --- a/clippy_lints/src/lib.register_restriction.rs +++ b/clippy_lints/src/lib.register_restriction.rs @@ -30,6 +30,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION), LintId::of(map_err_ignore::MAP_ERR_IGNORE), LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS), + LintId::of(matches::TRY_ERR), LintId::of(matches::WILDCARD_ENUM_MATCH_ARM), LintId::of(mem_forget::MEM_FORGET), LintId::of(methods::CLONE_ON_REF_PTR), @@ -67,7 +68,6 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve LintId::of(strings::STRING_SLICE), LintId::of(strings::STRING_TO_STRING), LintId::of(strings::STR_TO_STRING), - LintId::of(try_err::TRY_ERR), LintId::of(types::RC_BUFFER), LintId::of(types::RC_MUTEX), LintId::of(undocumented_unsafe_blocks::UNDOCUMENTED_UNSAFE_BLOCKS), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 742409d75..d9941d75f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -385,7 +385,6 @@ mod trailing_empty_array; mod trait_bounds; mod transmute; mod transmuting_null; -mod try_err; mod types; mod undocumented_unsafe_blocks; mod unicode; @@ -700,7 +699,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ); store.register_late_pass(move || Box::new(pass_by_ref_or_value)); store.register_late_pass(|| Box::new(ref_option_ref::RefOptionRef)); - store.register_late_pass(|| Box::new(try_err::TryErr)); store.register_late_pass(|| Box::new(bytecount::ByteCount)); store.register_late_pass(|| Box::new(infinite_iter::InfiniteIter)); store.register_late_pass(|| Box::new(inline_fn_without_body::InlineFnWithoutBody)); diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 3c0dc8041..fee7471f1 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -27,6 +27,7 @@ mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; mod significant_drop_in_scrutinee; mod single_match; +mod try_err; mod wild_in_or_pats; declare_clippy_lint! { @@ -825,6 +826,41 @@ declare_clippy_lint! { "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usages of `Err(x)?`. + /// + /// ### Why is this bad? + /// The `?` operator is designed to allow calls that + /// can fail to be easily chained. For example, `foo()?.bar()` or + /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will + /// always return), it is more clear to write `return Err(x)`. + /// + /// ### Example + /// ```rust + /// fn foo(fail: bool) -> Result { + /// if fail { + /// Err("failed")?; + /// } + /// Ok(0) + /// } + /// ``` + /// Could be written: + /// + /// ```rust + /// fn foo(fail: bool) -> Result { + /// if fail { + /// return Err("failed".into()); + /// } + /// Ok(0) + /// } + /// ``` + #[clippy::version = "1.38.0"] + pub TRY_ERR, + restriction, + "return errors explicitly rather than hiding them behind a `?`" +} + #[derive(Default)] pub struct Matches { msrv: Option, @@ -864,6 +900,7 @@ impl_lint_pass!(Matches => [ MATCH_ON_VEC_ITEMS, MATCH_STR_CASE_MISMATCH, SIGNIFICANT_DROP_IN_SCRUTINEE, + TRY_ERR, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -888,6 +925,10 @@ impl<'tcx> LateLintPass<'tcx> for Matches { wild_in_or_pats::check(cx, arms); } + if source == MatchSource::TryDesugar { + try_err::check(cx, expr, ex); + } + if !from_expansion && !contains_cfg_arm(cx, expr, ex, arms) { if source == MatchSource::Normal { if !(meets_msrv(self.msrv, msrvs::MATCHES_MACRO) diff --git a/clippy_lints/src/matches/try_err.rs b/clippy_lints/src/matches/try_err.rs new file mode 100644 index 000000000..0491a0679 --- /dev/null +++ b/clippy_lints/src/matches/try_err.rs @@ -0,0 +1,145 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{get_parent_expr, is_lang_ctor, match_def_path, paths}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::LangItem::ResultErr; +use rustc_hir::{Expr, ExprKind, LangItem, MatchSource, QPath}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, Ty}; +use rustc_span::{hygiene, sym}; + +use super::TRY_ERR; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, scrutinee: &'tcx Expr<'_>) { + // Looks for a structure like this: + // match ::std::ops::Try::into_result(Err(5)) { + // ::std::result::Result::Err(err) => + // #[allow(unreachable_code)] + // return ::std::ops::Try::from_error(::std::convert::From::from(err)), + // ::std::result::Result::Ok(val) => + // #[allow(unreachable_code)] + // val, + // }; + if_chain! { + if let ExprKind::Call(match_fun, try_args) = scrutinee.kind; + if let ExprKind::Path(ref match_fun_path) = match_fun.kind; + if matches!(match_fun_path, QPath::LangItem(LangItem::TryTraitBranch, ..)); + if let Some(try_arg) = try_args.get(0); + if let ExprKind::Call(err_fun, err_args) = try_arg.kind; + if let Some(err_arg) = err_args.get(0); + if let ExprKind::Path(ref err_fun_path) = err_fun.kind; + if is_lang_ctor(cx, err_fun_path, ResultErr); + if let Some(return_ty) = find_return_type(cx, &expr.kind); + then { + let prefix; + let suffix; + let err_ty; + + if let Some(ty) = result_error_type(cx, return_ty) { + prefix = "Err("; + suffix = ")"; + err_ty = ty; + } else if let Some(ty) = poll_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Err("; + suffix = "))"; + err_ty = ty; + } else if let Some(ty) = poll_option_result_error_type(cx, return_ty) { + prefix = "Poll::Ready(Some(Err("; + suffix = ")))"; + err_ty = ty; + } else { + return; + }; + + let expr_err_ty = cx.typeck_results().expr_ty(err_arg); + let span = hygiene::walk_chain(err_arg.span, try_arg.span.ctxt()); + let mut applicability = Applicability::MachineApplicable; + let origin_snippet = snippet_with_applicability(cx, span, "_", &mut applicability); + let ret_prefix = if get_parent_expr(cx, expr).map_or(false, |e| matches!(e.kind, ExprKind::Ret(_))) { + "" // already returns + } else { + "return " + }; + let suggestion = if err_ty == expr_err_ty { + format!("{}{}{}{}", ret_prefix, prefix, origin_snippet, suffix) + } else { + format!("{}{}{}.into(){}", ret_prefix, prefix, origin_snippet, suffix) + }; + + span_lint_and_sugg( + cx, + TRY_ERR, + expr.span, + "returning an `Err(_)` with the `?` operator", + "try this", + suggestion, + applicability, + ); + } + } +} + +/// Finds function return type by examining return expressions in match arms. +fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { + if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr { + for arm in arms.iter() { + if let ExprKind::Ret(Some(ret)) = arm.body.kind { + return Some(cx.typeck_results().expr_ty(ret)); + } + } + } + None +} + +/// Extracts the error type from Result. +fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(_, subst) = ty.kind(); + if is_type_diagnostic_item(cx, ty, sym::Result); + then { + Some(subst.type_at(1)) + } else { + None + } + } +} + +/// Extracts the error type from Poll>. +fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind(); + if match_def_path(cx, def.did(), &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Result, ready_def.did()); + then { + Some(ready_subst.type_at(1)) + } else { + None + } + } +} + +/// Extracts the error type from Poll>>. +fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + if_chain! { + if let ty::Adt(def, subst) = ty.kind(); + if match_def_path(cx, def.did(), &paths::POLL); + let ready_ty = subst.type_at(0); + + if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Option, ready_def.did()); + let some_ty = ready_subst.type_at(0); + + if let ty::Adt(some_def, some_subst) = some_ty.kind(); + if cx.tcx.is_diagnostic_item(sym::Result, some_def.did()); + then { + Some(some_subst.type_at(1)) + } else { + None + } + } +} diff --git a/clippy_lints/src/try_err.rs b/clippy_lints/src/try_err.rs deleted file mode 100644 index e108f7be1..000000000 --- a/clippy_lints/src/try_err.rs +++ /dev/null @@ -1,186 +0,0 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{get_parent_expr, is_lang_ctor, match_def_path, paths}; -use if_chain::if_chain; -use rustc_errors::Applicability; -use rustc_hir::LangItem::ResultErr; -use rustc_hir::{Expr, ExprKind, LangItem, MatchSource, QPath}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Ty}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{hygiene, sym}; - -declare_clippy_lint! { - /// ### What it does - /// Checks for usages of `Err(x)?`. - /// - /// ### Why is this bad? - /// The `?` operator is designed to allow calls that - /// can fail to be easily chained. For example, `foo()?.bar()` or - /// `foo(bar()?)`. Because `Err(x)?` can't be used that way (it will - /// always return), it is more clear to write `return Err(x)`. - /// - /// ### Example - /// ```rust - /// fn foo(fail: bool) -> Result { - /// if fail { - /// Err("failed")?; - /// } - /// Ok(0) - /// } - /// ``` - /// Could be written: - /// - /// ```rust - /// fn foo(fail: bool) -> Result { - /// if fail { - /// return Err("failed".into()); - /// } - /// Ok(0) - /// } - /// ``` - #[clippy::version = "1.38.0"] - pub TRY_ERR, - restriction, - "return errors explicitly rather than hiding them behind a `?`" -} - -declare_lint_pass!(TryErr => [TRY_ERR]); - -impl<'tcx> LateLintPass<'tcx> for TryErr { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - // Looks for a structure like this: - // match ::std::ops::Try::into_result(Err(5)) { - // ::std::result::Result::Err(err) => - // #[allow(unreachable_code)] - // return ::std::ops::Try::from_error(::std::convert::From::from(err)), - // ::std::result::Result::Ok(val) => - // #[allow(unreachable_code)] - // val, - // }; - if_chain! { - if !in_external_macro(cx.tcx.sess, expr.span); - if let ExprKind::Match(match_arg, _, MatchSource::TryDesugar) = expr.kind; - if let ExprKind::Call(match_fun, try_args) = match_arg.kind; - if let ExprKind::Path(ref match_fun_path) = match_fun.kind; - if matches!(match_fun_path, QPath::LangItem(LangItem::TryTraitBranch, ..)); - if let Some(try_arg) = try_args.get(0); - if let ExprKind::Call(err_fun, err_args) = try_arg.kind; - if let Some(err_arg) = err_args.get(0); - if let ExprKind::Path(ref err_fun_path) = err_fun.kind; - if is_lang_ctor(cx, err_fun_path, ResultErr); - if let Some(return_ty) = find_return_type(cx, &expr.kind); - then { - let prefix; - let suffix; - let err_ty; - - if let Some(ty) = result_error_type(cx, return_ty) { - prefix = "Err("; - suffix = ")"; - err_ty = ty; - } else if let Some(ty) = poll_result_error_type(cx, return_ty) { - prefix = "Poll::Ready(Err("; - suffix = "))"; - err_ty = ty; - } else if let Some(ty) = poll_option_result_error_type(cx, return_ty) { - prefix = "Poll::Ready(Some(Err("; - suffix = ")))"; - err_ty = ty; - } else { - return; - }; - - let expr_err_ty = cx.typeck_results().expr_ty(err_arg); - let span = hygiene::walk_chain(err_arg.span, try_arg.span.ctxt()); - let mut applicability = Applicability::MachineApplicable; - let origin_snippet = snippet_with_applicability(cx, span, "_", &mut applicability); - let ret_prefix = if get_parent_expr(cx, expr).map_or(false, |e| matches!(e.kind, ExprKind::Ret(_))) { - "" // already returns - } else { - "return " - }; - let suggestion = if err_ty == expr_err_ty { - format!("{}{}{}{}", ret_prefix, prefix, origin_snippet, suffix) - } else { - format!("{}{}{}.into(){}", ret_prefix, prefix, origin_snippet, suffix) - }; - - span_lint_and_sugg( - cx, - TRY_ERR, - expr.span, - "returning an `Err(_)` with the `?` operator", - "try this", - suggestion, - applicability, - ); - } - } - } -} - -/// Finds function return type by examining return expressions in match arms. -fn find_return_type<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx ExprKind<'_>) -> Option> { - if let ExprKind::Match(_, arms, MatchSource::TryDesugar) = expr { - for arm in arms.iter() { - if let ExprKind::Ret(Some(ret)) = arm.body.kind { - return Some(cx.typeck_results().expr_ty(ret)); - } - } - } - None -} - -/// Extracts the error type from Result. -fn result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(_, subst) = ty.kind(); - if is_type_diagnostic_item(cx, ty, sym::Result); - then { - Some(subst.type_at(1)) - } else { - None - } - } -} - -/// Extracts the error type from Poll>. -fn poll_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(def, subst) = ty.kind(); - if match_def_path(cx, def.did(), &paths::POLL); - let ready_ty = subst.type_at(0); - - if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Result, ready_def.did()); - then { - Some(ready_subst.type_at(1)) - } else { - None - } - } -} - -/// Extracts the error type from Poll>>. -fn poll_option_result_error_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { - if_chain! { - if let ty::Adt(def, subst) = ty.kind(); - if match_def_path(cx, def.did(), &paths::POLL); - let ready_ty = subst.type_at(0); - - if let ty::Adt(ready_def, ready_subst) = ready_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Option, ready_def.did()); - let some_ty = ready_subst.type_at(0); - - if let ty::Adt(some_def, some_subst) = some_ty.kind(); - if cx.tcx.is_diagnostic_item(sym::Result, some_def.did()); - then { - Some(some_subst.type_at(1)) - } else { - None - } - } -} From 68c411ff943c71cd86d18a0b50f23b8da9d78181 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 19:01:44 -0400 Subject: [PATCH 7/7] 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,