diff --git a/clippy_lints/src/missing_asserts_for_indexing.rs b/clippy_lints/src/missing_asserts_for_indexing.rs index 005f1c929a9a..43ee94c86622 100644 --- a/clippy_lints/src/missing_asserts_for_indexing.rs +++ b/clippy_lints/src/missing_asserts_for_indexing.rs @@ -7,12 +7,12 @@ use clippy_utils::higher::{If, Range}; use clippy_utils::macros::{find_assert_eq_args, first_node_macro_backtrace, root_macro_call}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::visitors::for_each_expr_without_closures; -use clippy_utils::{eq_expr_value, hash_expr}; +use clippy_utils::{eq_expr_value, hash_expr, is_wild}; use rustc_ast::{BinOpKind, LitKind, RangeLimits}; use rustc_data_structures::packed::Pu128; use rustc_data_structures::unhash::UnindexMap; use rustc_errors::{Applicability, Diag}; -use rustc_hir::{Block, Body, Expr, ExprKind, UnOp}; +use rustc_hir::{Arm, Block, Body, Expr, ExprKind, MatchSource, Node, PatExprKind, PatKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::{Span, Spanned, Symbol, sym}; @@ -123,6 +123,101 @@ fn len_comparison<'hir>( } } +/// Checks if the index expression is inside a `match` on the slice's length +/// whose arms make the indexing infallible. +/// This only applies when the non-wildcard arms are integer literals that +/// contiguously cover `0..=max` otherwise the wildcard +/// arm gives no lower bound on the length and the lint fires as usual. +fn is_index_covered_by_match(cx: &LateContext<'_>, index_expr: &Expr<'_>, slice: &Expr<'_>) -> bool { + let mut containing_arm = None; + for (_, node) in cx.tcx.hir_parent_iter(index_expr.hir_id) { + match node { + Node::Expr(expr) => { + if let ExprKind::Match(scrutinee, arms, MatchSource::Normal) = expr.kind + && is_slice_len_expr(cx, scrutinee, slice) + && match_arms_bound_len(cx, index_expr, arms, containing_arm) + { + return arms.iter().any(|arm| is_wild(arm.pat)); + } + }, + Node::Block(_) | Node::Stmt(_) | Node::LetStmt(_) => {}, + Node::Arm(arm) => containing_arm = Some(arm), + _ => return false, + } + } + false +} + +/// Checks if the non-wildcard arms are integer literals contiguously covering +/// `0..=max`, so that the wildcard arm implies `len > max`. Returns `true` +/// only if the index used is at most `max`, i.e. provably in bounds. +fn match_arms_bound_len( + cx: &LateContext<'_>, + index_expr: &Expr<'_>, + arms: &[Arm<'_>], + containing_arm: Option<&Arm<'_>>, +) -> bool { + let ExprKind::Index(_, index_lit, _) = index_expr.kind else { + return false; + }; + let Some(index) = upper_index_expr(cx, index_lit) else { + return false; + }; + + // index inside a literal arm `n => ...`: len is exactly `n` there, + // so anything at or past `n` is out of bounds + if let Some(arm) = containing_arm + && let Some(n) = arm_len_literal(arm) + && index >= n + { + return false; + } + + let mut matched_lens = Vec::new(); + for arm in arms { + if arm.guard.is_some() { + return false; + } + if is_wild(arm.pat) { + continue; + } + let Some(n) = arm_len_literal(arm) else { + return false; + }; + matched_lens.push(n); + } + + matched_lens.sort_unstable(); + matched_lens.dedup(); + + // literals must be exactly 0..=max, so `_` implies len > max + let Some(&max) = matched_lens.last() else { + return false; + }; + matched_lens.len() == max + 1 && index <= max +} + +fn arm_len_literal(arm: &Arm<'_>) -> Option { + if let PatKind::Expr(pat_expr) = arm.pat.kind + && let PatExprKind::Lit { lit, .. } = pat_expr.kind + && let LitKind::Int(Pu128(n), _) = lit.node + { + Some(n as usize) + } else { + None + } +} + +fn is_slice_len_expr(cx: &LateContext<'_>, expr: &Expr<'_>, slice: &Expr<'_>) -> bool { + if let ExprKind::MethodCall(method, recv, [], _) = expr.kind + && method.ident.name == sym::len + { + eq_expr_value(cx, expr.span.ctxt(), recv, slice) + } else { + false + } +} + /// Attempts to extract parts out of an `assert!`-like expression /// in the form `assert!(some_slice.len() > 5)`. /// @@ -239,6 +334,7 @@ fn check_index<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Uni if let ExprKind::Index(slice, index_lit, _) = expr.kind && cx.typeck_results().expr_ty_adjusted(slice).peel_refs().is_slice() && let Some(index) = upper_index_expr(cx, index_lit) + && !is_index_covered_by_match(cx, expr, slice) { let hash = hash_expr(cx, slice); diff --git a/tests/ui/missing_asserts_for_indexing.fixed b/tests/ui/missing_asserts_for_indexing.fixed index f877cbc1f114..5253d201676f 100644 --- a/tests/ui/missing_asserts_for_indexing.fixed +++ b/tests/ui/missing_asserts_for_indexing.fixed @@ -180,4 +180,35 @@ mod issue15988 { } } +mod issue17398 { + // ok + fn fix_match_case(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => println!("{}", supported[0]), + _ => println!("{} or {}", supported[0], supported[1]), + } + } + + // ok + fn one_index_too_high(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + _ => println!("{} {} {}", supported[0], supported[1], supported[2]), + } + } + + // The `2 =>` arm's `supported[2]` is NOT suppressed (len == 2 there), + // but single remaining access never lints, same as `single_access`. + fn literal_arm_single_unsuppressed(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + 2 => println!("{} {}", supported[0], supported[2]), + _ => println!("{} {}", supported[0], supported[2]), + } + } +} + fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing.rs b/tests/ui/missing_asserts_for_indexing.rs index 8084e0b71be9..9a238fcfd52f 100644 --- a/tests/ui/missing_asserts_for_indexing.rs +++ b/tests/ui/missing_asserts_for_indexing.rs @@ -180,4 +180,35 @@ mod issue15988 { } } +mod issue17398 { + // ok + fn fix_match_case(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => println!("{}", supported[0]), + _ => println!("{} or {}", supported[0], supported[1]), + } + } + + // ok + fn one_index_too_high(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + _ => println!("{} {} {}", supported[0], supported[1], supported[2]), + } + } + + // The `2 =>` arm's `supported[2]` is NOT suppressed (len == 2 there), + // but single remaining access never lints, same as `single_access`. + fn literal_arm_single_unsuppressed(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + 2 => println!("{} {}", supported[0], supported[2]), + _ => println!("{} {}", supported[0], supported[2]), + } + } +} + fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.rs b/tests/ui/missing_asserts_for_indexing_unfixable.rs index b8cdb71bd30a..f61c29097c84 100644 --- a/tests/ui/missing_asserts_for_indexing_unfixable.rs +++ b/tests/ui/missing_asserts_for_indexing_unfixable.rs @@ -85,4 +85,53 @@ fn issue14255(v1: &[u8]) { //~^ missing_asserts_for_indexing } +// not contiguous from 0: `_` can still be 0 +fn non_contiguous(supported: &[u8]) { + match supported.len() { + 5 => {}, + _ => println!("{} or {}", supported[0], supported[1]), + //~^ missing_asserts_for_indexing + } +} + +// wildcard only: no length information +#[allow(clippy::match_single_binding)] +fn wildcard_only(supported: &[u8]) { + match supported.len() { + _ => println!("{} or {}", supported[0], supported[1]), + //~^ missing_asserts_for_indexing + } +} + +// guard defeats the exclusion +fn with_guard(supported: &[u8], flag: bool) { + match supported.len() { + 0 if flag => {}, + 1 => {}, + _ => println!("{} or {}", supported[0], supported[1]), + //~^ missing_asserts_for_indexing + } +} + +// arms only guarantee len > 1, but 2 and 3 are indexed +fn index_too_high(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + _ => println!("{} {}", supported[2], supported[3]), + //~^ missing_asserts_for_indexing + } +} + +// literal arm pins len to exactly 2, so indexing at 2 and 3 must lint +fn literal_arm_out_of_bounds(supported: &[u8]) { + match supported.len() { + 0 => {}, + 1 => {}, + 2 => println!("{} {}", supported[2], supported[3]), + //~^ missing_asserts_for_indexing + _ => {}, + } +} + fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.stderr b/tests/ui/missing_asserts_for_indexing_unfixable.stderr index fe929aa36dba..4fa83c452df5 100644 --- a/tests/ui/missing_asserts_for_indexing_unfixable.stderr +++ b/tests/ui/missing_asserts_for_indexing_unfixable.stderr @@ -89,5 +89,45 @@ LL | let _ = v1[0] + v1[1] + v1[2]; | = help: consider asserting the length before indexing: `assert!(v1.len() > 2);` -error: aborting due to 10 previous errors +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:92:35 + | +LL | _ => println!("{} or {}", supported[0], supported[1]), + | ^^^^^^^^^^^^ ^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(supported.len() > 1);` + +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:101:35 + | +LL | _ => println!("{} or {}", supported[0], supported[1]), + | ^^^^^^^^^^^^ ^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(supported.len() > 1);` + +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:111:35 + | +LL | _ => println!("{} or {}", supported[0], supported[1]), + | ^^^^^^^^^^^^ ^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(supported.len() > 1);` + +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:121:32 + | +LL | _ => println!("{} {}", supported[2], supported[3]), + | ^^^^^^^^^^^^ ^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(supported.len() > 3);` + +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:131:32 + | +LL | 2 => println!("{} {}", supported[2], supported[3]), + | ^^^^^^^^^^^^ ^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(supported.len() > 3);` + +error: aborting due to 15 previous errors