new lint: missing_asserts_for_indexing

[y21]

Fixes #8296

This lint looks for repeated slice indexing and suggests adding an assert! beforehand that helps LLVM elide bounds checks. The lint documentation has an example.

I'm not really sure what category this should be in. It seems like a nice lint for the perf category but I suspect this has a pretty high FP rate, so it might have to be a pedantic lint or something.
I'm also not sure about the name. If someone knows a better name for this lint, I'd be fine with changing it.

changelog: new lint [missing_asserts_for_indexing]

[rustbot]

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

[y21]

Oh huh, I just tried adding a test for multiple different slices (ie slices from different bindings) in the same block:

fn foo(a: &[u8], b: &[u8]) {
  let _ = a[0] + a[1];
  let _ = b[0] + b[1];
}

and it seems like it thinks all of those array accesses reference the same slice. SpanlessHash doesn't hash the local path expressions "correctly" and both expressions (a and b) result in the same hash:

rust-clippy/clippy_utils/src/hir_utils.rs

Lines 878 to 883 in 9283497

	pub fn hash_path(&mut self, path: &Path<'_>) {
	match path.res {
	// constant hash since equality is dependant on inter-expression context
	// e.g. The expressions `if let Some(x) = foo() {}` and `if let Some(y) = foo() {}` are considered equal
	// even though the binding names are different and they have different `HirId`s.
	Res::Local(_) => 1_usize.hash(&mut self.s),

Not really sure why it is doing what it is doing (always hashing 1) or how I'd differentiate between different slices now :/

[llogiq]

I think the problem is that if two idents are the same constant value, SpanlessEq returns true. So if SpanlessHash hashed the ident, it would have two different idents hashed to different values and you'd end up with equal values distributed to different buckets. Boom.

What we could do is hash locals to the constant value if any and else hash the ident, I think.

[bors]

☔ The latest upstream changes (presumably #10578) made this pull request unmergeable. Please resolve the merge conflicts.

[Alexendoo]

There'd be the case where locals are deeper in other expressions like a.field vs b.field, there's also the possibility of natural hash collisions if we solve that. We probably want to do something similar to https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/fn.search_same.html

[bors]

☔ The latest upstream changes (presumably #10716) made this pull request unmergeable. Please resolve the merge conflicts.

[y21]

Right now this only works if integer literals are used for indexing and it'd be interesting if this could be extended to also work for some simple arithmetic, like

fn foo(x: &[i32], n: usize) -> i32 {
  // suggest `assert!(x.len() > n + 3);` here
  x[n] + x[n + 1] + x[n + 2] + x[n + 3]
}

But that probably complicates the logic quite a bit, so maybe it's worth doing this in a separate PR?

[Alexendoo]

I'm not sure about the arithmetic case, does that version still get optimised to a single bounds check? In any case yeah let's keep it (relatively) simple to start with

[Alexendoo]

Nominating this for discussion in the next clippy meeting. Tuesday the 13th, 15:00 UTC @ https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy

I'd like to discuss what category this should fit into

[y21]

I'm not sure about the arithmetic case, does that version still get optimised to a single bounds check?

Huh, good point. It doesn't look like it gets optimized to a single bounds check as I've written it with the assert. I am guessing that the fact that n + 3 can overflow messes with it.
Writing the assert like this reduces it to one bounds check (+ the check that it doesn't overflow), but it's kinda weird, so probably better to not do this for now (it also seems way more fragile, very slight changes to the assert miss the optimization, e.g. changing it from && to &).

I'd like to discuss what category this should fit into

👍
I was originally thinking about the perf category, but the more I think about it, I'm guessing that a lot of code does this sort of pattern, and having to assert this everywhere is probably quite annoying, especially when performance isnt' as much of a concern (to the point where one has to worry about branching). So maybe it should be a pedantic or restriction lint

[y21]

Changed the category to restriction as decided in the clippy meeting

[Alexendoo]

Thanks! LGTM with a rebase

@bors delegate+

[bors]

✌️ @y21, you can now approve this pull request!

If @Alexendoo told you to "r=me" after making some further change, please make that change, then do @bors r=@Alexendoo

[y21]

I rebased the PR and also had to change a few things in the tests since uitest has changed quite a bit since I've created this PR (intentionally put this into its own commit so reviewing that is easier).

[Alexendoo]

changed quite a bit since I've created this PR

I wonder how that happened 😅

I really need to start going through my assigned PRs bottom to top so things don't get left like this

Thanks again! @bors r+

[bors]

📌 Commit 790922c has been approved by Alexendoo

It is now in the queue for this repository.

[bors]

⌛ Testing commit 790922c with merge 79c684d...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 79c684d to master...