From 3d8d7341509e4868c91fa7fe24703e69c21a9869 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 3 Jun 2022 13:10:57 -0400 Subject: [PATCH] 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 cb52000fa55..fb44df57de6 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -258,7 +258,6 @@ 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 @@ 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 2e47a287d5c..a8b6c5a5d63 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -51,8 +51,8 @@ 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 3fdd0db8187..50b67918fc0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -286,7 +286,6 @@ macro_rules! declare_clippy_lint { 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 583b577ffe2..00000000000 --- 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 00000000000..2917f85c45f --- /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 ade47b1ed88..7fb449028e7 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -14,6 +14,7 @@ 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 @@ "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 @@ pub fn new(msrv: Option) -> Self { NEEDLESS_MATCH, COLLAPSIBLE_MATCH, MANUAL_UNWRAP_OR, + MATCH_ON_VEC_ITEMS, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -750,6 +789,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { 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);