diff --git a/clippy_lints/src/missing_asserts_for_indexing.rs b/clippy_lints/src/missing_asserts_for_indexing.rs index d78299fe08be..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>, }, @@ -244,28 +246,41 @@ 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, + is_first_highest: true, + asserted_len: *asserted_len, + assert_span: *assert_span, + slice, + indexes: vec![expr.span], + comparison: *comparison, + }; + } }, 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, }); @@ -284,13 +299,16 @@ 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 + && expr.span.lo() <= slice.span.lo() { *entry = IndexEntry::AssertWithIndex { highest_index: *highest_index, indexes: mem::take(indexes), + is_first_highest: *is_first_highest, slice, assert_span: expr.span, comparison, @@ -325,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 { @@ -378,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() {} 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