refactor(missing_asserts_for_indexing): overhaul diagnostics (#16120)

changelog: [`missing_asserts_for_indexing`]: overhaul diagnostics

Author: Jarcho

clippy_lints/src/missing_asserts_for_indexing.rs

@@ -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};
@@ -67,16 +67,13 @@ 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, 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.note_once("asserting the length before indexing will elide bounds checks");
     });
 }
 
@@ -213,15 +210,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.
@@ -354,86 +342,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,
@@ -442,9 +413,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});`",
@@ -469,6 +439,6 @@ impl LateLintPass<'_> for MissingAssertsForIndexing {
             ControlFlow::<!, ()>::Continue(())
         });
 
-        report_indexes(cx, &map);
+        report_indexes(cx, map);
     }
 }