Skip to content

refactor(missing_asserts_for_indexing): overhaul diagnostics #16120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

refactor(missing_asserts_for_indexing): overhaul diagnostics #16120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bc6c78
clean-up: pass things by value to avoid copying
ada4a Nov 20, 2025
c08e456
use `note_once`
ada4a Nov 20, 2025
b10b5f2
point the suggestion at each individual span
ada4a Nov 20, 2025
2379120
use `span_suggestion_verbose`
ada4a Nov 20, 2025
bc76aef
fix: adjust the applicability for the suggestion
ada4a Nov 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 33 additions & 62 deletions clippy_lints/src/missing_asserts_for_indexing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use clippy_utils::comparisons::{Rel, normalize_comparison};
use clippy_utils::diagnostics::span_lint_and_then;
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;
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 rustc_ast::{BinOpKind, LitKind, RangeLimits};
Expand Down Expand Up @@ -67,16 +67,14 @@ declare_clippy_lint! {
}
declare_lint_pass!(MissingAssertsForIndexing => [MISSING_ASSERTS_FOR_INDEXING]);

fn report_lint<F>(cx: &LateContext<'_>, full_span: Span, msg: &'static str, indexes: &[Span], f: F)
fn report_lint<F>(cx: &LateContext<'_>, index_spans: Vec<Span>, msg: &'static str, f: F)
where
F: FnOnce(&mut Diag<'_, ()>),
{
span_lint_and_then(cx, MISSING_ASSERTS_FOR_INDEXING, full_span, msg, |diag| {
span_lint_and_then(cx, MISSING_ASSERTS_FOR_INDEXING, index_spans.clone(), msg, |diag| {
f(diag);
for span in indexes {
diag.span_note(*span, "slice indexed here");
}
diag.note("asserting the length before indexing will elide bounds checks");
diag.span_labels(index_spans, "slice indexed here");
diag.note_once("asserting the length before indexing will elide bounds checks");
});
}

Expand Down Expand Up @@ -213,15 +211,6 @@ impl<'hir> IndexEntry<'hir> {
| IndexEntry::IndexWithoutAssert { slice, .. } => slice,
}
}

pub fn index_spans(&self) -> Option<&[Span]> {
match self {
IndexEntry::StrayAssert { .. } => None,
IndexEntry::AssertWithIndex { indexes, .. } | IndexEntry::IndexWithoutAssert { indexes, .. } => {
Some(indexes)
},
}
}
}

/// Extracts the upper index of a slice indexing expression.
Expand Down Expand Up @@ -354,86 +343,69 @@ fn check_assert<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Un
/// Inspects indexes and reports lints.
///
/// Called at the end of this lint after all indexing and `assert!` expressions have been collected.
fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap<u64, Vec<IndexEntry<'_>>>) {
for bucket in map.values() {
fn report_indexes(cx: &LateContext<'_>, map: UnindexMap<u64, Vec<IndexEntry<'_>>>) {
for bucket in map.into_values() {
for entry in bucket {
let Some(full_span) = entry
.index_spans()
.and_then(|spans| spans.first().zip(spans.last()))
.map(|(low, &high)| low.to(high))
else {
continue;
};

match *entry {
match entry {
IndexEntry::AssertWithIndex {
highest_index,
is_first_highest,
asserted_len,
ref indexes,
indexes,
comparison,
assert_span,
slice,
macro_call,
} if indexes.len() > 1 && !is_first_highest => {
let mut app = Applicability::MachineApplicable;
let slice_str = snippet_with_applicability(cx, slice.span, "_", &mut app);
// if we have found an `assert!`, let's also check that it's actually right
// and if it covers the highest index and if not, suggest the correct length
let sugg = match comparison {
// `v.len() < 5` and `v.len() <= 5` does nothing in terms of bounds checks.
// The user probably meant `v.len() > 5`
LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => Some(
format!("assert!({}.len() > {highest_index})", snippet(cx, slice.span, "..")),
),
LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => {
Some(format!("assert!({slice_str}.len() > {highest_index})",))
},
// `5 < v.len()` == `v.len() > 5`
LengthComparison::IntLessThanLength if asserted_len < highest_index => Some(format!(
"assert!({}.len() > {highest_index})",
snippet(cx, slice.span, "..")
)),
LengthComparison::IntLessThanLength if asserted_len < highest_index => {
Some(format!("assert!({slice_str}.len() > {highest_index})",))
},
// `5 <= v.len() == `v.len() >= 5`
LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => Some(format!(
"assert!({}.len() > {highest_index})",
snippet(cx, slice.span, "..")
)),
LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => {
Some(format!("assert!({slice_str}.len() > {highest_index})",))
},
// `highest_index` here is rather a length, so we need to add 1 to it
LengthComparison::LengthEqualInt if asserted_len < highest_index + 1 => match macro_call {
sym::assert_eq_macro => Some(format!(
"assert_eq!({}.len(), {})",
snippet(cx, slice.span, ".."),
highest_index + 1
)),
sym::debug_assert_eq_macro => Some(format!(
"debug_assert_eq!({}.len(), {})",
snippet(cx, slice.span, ".."),
highest_index + 1
)),
_ => Some(format!(
"assert!({}.len() == {})",
snippet(cx, slice.span, ".."),
highest_index + 1
)),
sym::assert_eq_macro => {
Some(format!("assert_eq!({slice_str}.len(), {})", highest_index + 1))
},
sym::debug_assert_eq_macro => {
Some(format!("debug_assert_eq!({slice_str}.len(), {})", highest_index + 1))
},
_ => Some(format!("assert!({slice_str}.len() == {})", highest_index + 1)),
},
_ => None,
};

if let Some(sugg) = sugg {
report_lint(
cx,
full_span,
"indexing into a slice multiple times with an `assert` that does not cover the highest index",
indexes,
"indexing into a slice multiple times with an `assert` that does not cover the highest index",
|diag| {
diag.span_suggestion(
diag.span_suggestion_verbose(
assert_span,
"provide the highest index that is indexed with",
sugg,
Applicability::MachineApplicable,
app,
);
},
);
}
},
IndexEntry::IndexWithoutAssert {
ref indexes,
indexes,
highest_index,
is_first_highest,
slice,
Expand All @@ -442,9 +414,8 @@ fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap<u64, Vec<IndexEntry<'_>
// adding an `assert!` that covers the highest index
report_lint(
cx,
full_span,
"indexing into a slice multiple times without an `assert`",
indexes,
"indexing into a slice multiple times without an `assert`",
|diag| {
diag.help(format!(
"consider asserting the length before indexing: `assert!({}.len() > {highest_index});`",
Expand All @@ -469,6 +440,6 @@ impl LateLintPass<'_> for MissingAssertsForIndexing {
ControlFlow::<!, ()>::Continue(())
});

report_indexes(cx, &map);
report_indexes(cx, map);
}
}
Loading