diff --git a/CHANGELOG.md b/CHANGELOG.md index abd7167502b9..f49980f712b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1348,6 +1348,7 @@ Released 2018-09-13 [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten [`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref [`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool +[`match_on_vec_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_on_vec_items [`match_overlapping_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm [`match_ref_pats`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats [`match_same_arms`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index 7916996e990d..81ddc8c0067c 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -358,9 +358,9 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> { }, (Some(Constant::Vec(vec)), _) => { if !vec.is_empty() && vec.iter().all(|x| *x == vec[0]) { - match vec[0] { - Constant::F32(x) => Some(Constant::F32(x)), - Constant::F64(x) => Some(Constant::F64(x)), + match vec.get(0) { + Some(Constant::F32(x)) => Some(Constant::F32(*x)), + Some(Constant::F64(x)) => Some(Constant::F64(*x)), _ => None, } } else { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index dee4188b75f3..f7d8314cceea 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -249,6 +249,7 @@ mod macro_use; mod main_recursion; mod map_clone; mod map_unit_fn; +mod match_on_vec_items; mod matches; mod mem_discriminant; mod mem_forget; @@ -629,6 +630,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &map_clone::MAP_CLONE, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, + &match_on_vec_items::MATCH_ON_VEC_ITEMS, &matches::INFALLIBLE_DESTRUCTURING_MATCH, &matches::MATCH_AS_REF, &matches::MATCH_BOOL, @@ -1060,6 +1062,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box future_not_send::FutureNotSend); store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls); store.register_late_pass(|| box if_let_mutex::IfLetMutex); + store.register_late_pass(|| box match_on_vec_items::MatchOnVecItems); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1277,6 +1280,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&map_clone::MAP_CLONE), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), + LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), LintId::of(&matches::MATCH_AS_REF), LintId::of(&matches::MATCH_BOOL), @@ -1641,6 +1645,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::NEVER_LOOP), LintId::of(&loops::REVERSE_RANGE_LOOP), LintId::of(&loops::WHILE_IMMUTABLE_CONDITION), + LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS), LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM), LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT), LintId::of(&methods::CLONE_DOUBLE_REF), diff --git a/clippy_lints/src/match_on_vec_items.rs b/clippy_lints/src/match_on_vec_items.rs new file mode 100644 index 000000000000..4071406cc84c --- /dev/null +++ b/clippy_lints/src/match_on_vec_items.rs @@ -0,0 +1,89 @@ +use crate::utils::{is_type_diagnostic_item, snippet, span_lint_and_sugg, walk_ptrs_ty}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, MatchSource}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +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. + /// + /// **Known problems:** None. + /// + /// **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), + /// _ => {}, + /// } + /// ``` + pub MATCH_ON_VEC_ITEMS, + correctness, + "matching on vector elements can panic" +} + +declare_lint_pass!(MatchOnVecItems => [MATCH_ON_VEC_ITEMS]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MatchOnVecItems { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) { + if_chain! { + if !in_external_macro(cx.sess(), expr.span); + if let ExprKind::Match(ref 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<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> { + if_chain! { + if let ExprKind::Index(ref array, _) = expr.kind; + let ty = cx.tables.expr_ty(array); + let ty = walk_ptrs_ty(ty); + if is_type_diagnostic_item(cx, ty, sym!(vec_type)); + + then { + return Some(expr); + } + } + + None +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 2c466aa20c67..fbcf4fde5e57 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1144,6 +1144,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "matches", }, + Lint { + name: "match_on_vec_items", + group: "correctness", + desc: "matching on vector elements can panic", + deprecation: None, + module: "match_on_vec_items", + }, Lint { name: "match_overlapping_arm", group: "style", diff --git a/tests/ui/match_on_vec_items.rs b/tests/ui/match_on_vec_items.rs new file mode 100644 index 000000000000..0bb39d77e461 --- /dev/null +++ b/tests/ui/match_on_vec_items.rs @@ -0,0 +1,130 @@ +#![warn(clippy::match_on_vec_items)] + +fn match_with_wildcard() { + let arr = vec![0, 1, 2, 3]; + let range = 1..3; + let idx = 1; + + // Lint, may panic + match arr[idx] { + 0 => println!("0"), + 1 => println!("1"), + _ => {}, + } + + // Lint, may panic + match arr[range] { + [0, 1] => println!("0 1"), + [1, 2] => println!("1 2"), + _ => {}, + } +} + +fn match_without_wildcard() { + let arr = vec![0, 1, 2, 3]; + let range = 1..3; + let idx = 2; + + // Lint, may panic + match arr[idx] { + 0 => println!("0"), + 1 => println!("1"), + num => {}, + } + + // Lint, may panic + match arr[range] { + [0, 1] => println!("0 1"), + [1, 2] => println!("1 2"), + [ref sub @ ..] => {}, + } +} + +fn match_wildcard_and_action() { + let arr = vec![0, 1, 2, 3]; + let range = 1..3; + let idx = 3; + + // Lint, may panic + match arr[idx] { + 0 => println!("0"), + 1 => println!("1"), + _ => println!("Hello, World!"), + } + + // Lint, may panic + match arr[range] { + [0, 1] => println!("0 1"), + [1, 2] => println!("1 2"), + _ => println!("Hello, World!"), + } +} + +fn match_vec_ref() { + let arr = &vec![0, 1, 2, 3]; + let range = 1..3; + let idx = 3; + + // Lint, may panic + match arr[idx] { + 0 => println!("0"), + 1 => println!("1"), + _ => {}, + } + + // Lint, may panic + match arr[range] { + [0, 1] => println!("0 1"), + [1, 2] => println!("1 2"), + _ => {}, + } +} + +fn match_with_get() { + let arr = vec![0, 1, 2, 3]; + let range = 1..3; + let idx = 3; + + // Ok + match arr.get(idx) { + Some(0) => println!("0"), + Some(1) => println!("1"), + _ => {}, + } + + // Ok + match arr.get(range) { + Some(&[0, 1]) => println!("0 1"), + Some(&[1, 2]) => println!("1 2"), + _ => {}, + } +} + +fn match_with_array() { + let arr = [0, 1, 2, 3]; + let range = 1..3; + let idx = 3; + + // Ok + match arr[idx] { + 0 => println!("0"), + 1 => println!("1"), + _ => {}, + } + + // Ok + match arr[range] { + [0, 1] => println!("0 1"), + [1, 2] => println!("1 2"), + _ => {}, + } +} + +fn main() { + match_with_wildcard(); + match_without_wildcard(); + match_wildcard_and_action(); + match_vec_ref(); + match_with_get(); + match_with_array(); +} diff --git a/tests/ui/match_on_vec_items.stderr b/tests/ui/match_on_vec_items.stderr new file mode 100644 index 000000000000..49446d715abe --- /dev/null +++ b/tests/ui/match_on_vec_items.stderr @@ -0,0 +1,52 @@ +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:9:11 + | +LL | match arr[idx] { + | ^^^^^^^^ help: try this: `arr.get(idx)` + | + = note: `-D clippy::match-on-vec-items` implied by `-D warnings` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:16:11 + | +LL | match arr[range] { + | ^^^^^^^^^^ help: try this: `arr.get(range)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:29:11 + | +LL | match arr[idx] { + | ^^^^^^^^ help: try this: `arr.get(idx)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:36:11 + | +LL | match arr[range] { + | ^^^^^^^^^^ help: try this: `arr.get(range)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:49:11 + | +LL | match arr[idx] { + | ^^^^^^^^ help: try this: `arr.get(idx)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:56:11 + | +LL | match arr[range] { + | ^^^^^^^^^^ help: try this: `arr.get(range)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:69:11 + | +LL | match arr[idx] { + | ^^^^^^^^ help: try this: `arr.get(idx)` + +error: indexing into a vector may panic + --> $DIR/match_on_vec_items.rs:76:11 + | +LL | match arr[range] { + | ^^^^^^^^^^ help: try this: `arr.get(range)` + +error: aborting due to 8 previous errors +