From 31236a491571f6fcf3d3be458ada764848f85734 Mon Sep 17 00:00:00 2001 From: Vishruth-Thimmaiah Date: Wed, 29 Jan 2025 15:06:03 +0530 Subject: [PATCH 1/2] fix[`missing_asserts_for_indexing`]: ignore asserts after indexing --- .../src/missing_asserts_for_indexing.rs | 19 ++++++++++------- .../missing_asserts_for_indexing_unfixable.rs | 6 ++++++ ...sing_asserts_for_indexing_unfixable.stderr | 21 ++++++++++++++++++- 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/missing_asserts_for_indexing.rs b/clippy_lints/src/missing_asserts_for_indexing.rs index d78299fe08be..5ce3e73ae16d 100644 --- a/clippy_lints/src/missing_asserts_for_indexing.rs +++ b/clippy_lints/src/missing_asserts_for_indexing.rs @@ -244,14 +244,16 @@ fn check_index<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Uni assert_span, slice, } => { - *entry = IndexEntry::AssertWithIndex { - highest_index: index, - asserted_len: *asserted_len, - assert_span: *assert_span, - slice, - indexes: vec![expr.span], - comparison: *comparison, - }; + if slice.span.lo() > assert_span.lo() { + *entry = IndexEntry::AssertWithIndex { + highest_index: index, + asserted_len: *asserted_len, + assert_span: *assert_span, + slice, + indexes: vec![expr.span], + comparison: *comparison, + }; + } }, IndexEntry::IndexWithoutAssert { highest_index, indexes, .. @@ -287,6 +289,7 @@ fn check_assert<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Un indexes, slice, } = entry + && expr.span.lo() <= slice.span.lo() { *entry = IndexEntry::AssertWithIndex { highest_index: *highest_index, diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.rs b/tests/ui/missing_asserts_for_indexing_unfixable.rs index a520151a2dd9..2fac9d7a59c2 100644 --- a/tests/ui/missing_asserts_for_indexing_unfixable.rs +++ b/tests/ui/missing_asserts_for_indexing_unfixable.rs @@ -73,4 +73,10 @@ pub fn issue11856(values: &[i32]) -> usize { ascending.len() } +fn assert_after_indexing(v1: &[u8]) { + let _ = v1[1] + v1[2]; + //~^ ERROR: indexing into a slice multiple times without an `assert` + assert!(v1.len() > 2); +} + fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.stderr b/tests/ui/missing_asserts_for_indexing_unfixable.stderr index 24109b052a8a..1674861c9ed7 100644 --- a/tests/ui/missing_asserts_for_indexing_unfixable.stderr +++ b/tests/ui/missing_asserts_for_indexing_unfixable.stderr @@ -180,5 +180,24 @@ LL | let _ = x[0] + x[1]; | ^^^^ = note: asserting the length before indexing will elide bounds checks -error: aborting due to 8 previous errors +error: indexing into a slice multiple times without an `assert` + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:77:13 + | +LL | let _ = v1[1] + v1[2]; + | ^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(v1.len() > 2);` +note: slice indexed here + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:77:13 + | +LL | let _ = v1[1] + v1[2]; + | ^^^^^ +note: slice indexed here + --> tests/ui/missing_asserts_for_indexing_unfixable.rs:77:21 + | +LL | let _ = v1[1] + v1[2]; + | ^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: aborting due to 9 previous errors From 2cd3ea1f9d756f9029e969ed1758492e7620ab99 Mon Sep 17 00:00:00 2001 From: Vishruth-Thimmaiah Date: Wed, 29 Jan 2025 19:27:32 +0530 Subject: [PATCH 2/2] fix[`missing_asserts_for_indexing`]: ignore lint if first index is highest --- .../src/missing_asserts_for_indexing.rs | 25 ++++++++++++++++--- tests/ui/missing_asserts_for_indexing.fixed | 10 ++++++++ tests/ui/missing_asserts_for_indexing.rs | 10 ++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/missing_asserts_for_indexing.rs b/clippy_lints/src/missing_asserts_for_indexing.rs index 5ce3e73ae16d..d9acf655b8af 100644 --- a/clippy_lints/src/missing_asserts_for_indexing.rs +++ b/clippy_lints/src/missing_asserts_for_indexing.rs @@ -168,6 +168,7 @@ enum IndexEntry<'hir> { /// if the `assert!` asserts the right length. AssertWithIndex { highest_index: usize, + is_first_highest: bool, asserted_len: usize, assert_span: Span, slice: &'hir Expr<'hir>, @@ -177,6 +178,7 @@ enum IndexEntry<'hir> { /// Indexing without an `assert!` IndexWithoutAssert { highest_index: usize, + is_first_highest: bool, indexes: Vec, slice: &'hir Expr<'hir>, }, @@ -247,6 +249,7 @@ fn check_index<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Uni if slice.span.lo() > assert_span.lo() { *entry = IndexEntry::AssertWithIndex { highest_index: index, + is_first_highest: true, asserted_len: *asserted_len, assert_span: *assert_span, slice, @@ -256,18 +259,28 @@ fn check_index<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Uni } }, IndexEntry::IndexWithoutAssert { - highest_index, indexes, .. + highest_index, + indexes, + is_first_highest, + .. } | IndexEntry::AssertWithIndex { - highest_index, indexes, .. + highest_index, + indexes, + is_first_highest, + .. } => { indexes.push(expr.span); + if *is_first_highest { + (*is_first_highest) = *highest_index >= index; + } *highest_index = (*highest_index).max(index); }, } } else { indexes.push(IndexEntry::IndexWithoutAssert { highest_index: index, + is_first_highest: true, indexes: vec![expr.span], slice, }); @@ -286,6 +299,7 @@ fn check_assert<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Un if let Some(entry) = entry { if let IndexEntry::IndexWithoutAssert { highest_index, + is_first_highest, indexes, slice, } = entry @@ -294,6 +308,7 @@ fn check_assert<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut Un *entry = IndexEntry::AssertWithIndex { highest_index: *highest_index, indexes: mem::take(indexes), + is_first_highest: *is_first_highest, slice, assert_span: expr.span, comparison, @@ -328,12 +343,13 @@ fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap match *entry { IndexEntry::AssertWithIndex { highest_index, + is_first_highest, asserted_len, ref indexes, comparison, assert_span, slice, - } if indexes.len() > 1 => { + } if indexes.len() > 1 && !is_first_highest => { // 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 { @@ -381,8 +397,9 @@ fn report_indexes(cx: &LateContext<'_>, map: &UnindexMap IndexEntry::IndexWithoutAssert { ref indexes, highest_index, + is_first_highest, slice, - } if indexes.len() > 1 => { + } if indexes.len() > 1 && !is_first_highest => { // if there was no `assert!` but more than one index, suggest // adding an `assert!` that covers the highest index report_lint( diff --git a/tests/ui/missing_asserts_for_indexing.fixed b/tests/ui/missing_asserts_for_indexing.fixed index 3bbafe0bba3f..6e803322f65a 100644 --- a/tests/ui/missing_asserts_for_indexing.fixed +++ b/tests/ui/missing_asserts_for_indexing.fixed @@ -139,4 +139,14 @@ fn issue11835(v1: &[u8], v2: &[u8], v3: &[u8], v4: &[u8]) { let _ = v4[0] + v4[1] + v4[2]; } +// ok +fn same_index_multiple_times(v1: &[u8]) { + let _ = v1[0] + v1[0]; +} + +// ok +fn highest_index_first(v1: &[u8]) { + let _ = v1[2] + v1[1] + v1[0]; +} + fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing.rs b/tests/ui/missing_asserts_for_indexing.rs index f8ea0173c13f..4614a8ef5d0d 100644 --- a/tests/ui/missing_asserts_for_indexing.rs +++ b/tests/ui/missing_asserts_for_indexing.rs @@ -139,4 +139,14 @@ fn issue11835(v1: &[u8], v2: &[u8], v3: &[u8], v4: &[u8]) { let _ = v4[0] + v4[1] + v4[2]; } +// ok +fn same_index_multiple_times(v1: &[u8]) { + let _ = v1[0] + v1[0]; +} + +// ok +fn highest_index_first(v1: &[u8]) { + let _ = v1[2] + v1[1] + v1[0]; +} + fn main() {}