Skip to content
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.

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

False positives and false negatives for clippy::missing_asserts_for_indexing. #14079

Open
sendittothenewts opened this issue Jan 26, 2025 · 1 comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@sendittothenewts
Copy link

Summary

The code let _square = slice[0] * slice[0]; falsely fires the clippy::missing_asserts_for_indexing lint. Following the suggestion to add assert!(slice.len() > 0); fires the clippy::len_zero lint instead (as does assert!(slice.len() >= 1);). Following clippy::len_zero's suggestion to change it to assert!(!slice.is_empty()); causes clippy::missing_asserts_for_indexing to reappear. Adding assert_ne!(slice.len(), 0); has no effect (arguably a false negative for clippy::len_zero), and assert!(slice.len() != 0); just gets both lints firing.

The false positive for clippy::missing_asserts_for_indexing occurs for multiple accesses to any index, but the unfortunate interaction with clippy::len_zero is specific to the case where that index is 0.

On a somewhat related note, clippy::missing_asserts_for_indexing also seems to ignore the ordering of things. While it correctly fires for let _product = slice[0] * slice[1];, it produces a false positive for let _product = slice[1] * slice[0]; and a false negative for let _product = slice[0] * slice[1]; assert!(slice.len() > 1);.

Possible fixes:

  1. Ignore any assert! that does not come before the first indexing operation.
  2. In the absence of such an assert!, treat the first indexing operation as equivalent to an assert!
  3. If the preceding fixes turn out to be tricky, ignoring multiple accesses to the same index would provide a partial fix
  4. Teaching clippy::missing_asserts_for_indexing about .is_empty() is possible, but this would not be needed if either of the previous two points were implemented.

Lint Name

clippy::missing_asserts_for_indexing

Reproducer

I tried this code:

#![warn(clippy::missing_asserts_for_indexing)]
pub fn foo(slice: &[i32]) -> i32 {
	assert!(!slice.is_empty());
	slice[0] * slice[0]
}
pub fn bar(slice: &[i32]) -> i32{
	slice[1] * slice[0]
}
pub fn baz(slice: &[i32]) -> i32 {
	let product = slice[0] * slice[1]; assert!(slice.len() > 1);
	product
}

I expected to see this happen:

A warning for baz, but nothing for foo and bar.

Instead, this happened:

warning: indexing into a slice multiple times without an `assert`
 --> src/bin/slice.rs:4:2
  |
4 |     slice[0] * slice[0]
  |     ^^^^^^^^^^^^^^^^^^^
  |
  = help: consider asserting the length before indexing: `assert!(slice.len() > 0);`
note: slice indexed here
 --> src/bin/slice.rs:4:2
  |
4 |     slice[0] * slice[0]
  |     ^^^^^^^^
note: slice indexed here
 --> src/bin/slice.rs:4:13
  |
4 |     slice[0] * slice[0]
  |                ^^^^^^^^
  = note: asserting the length before indexing will elide bounds checks
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_asserts_for_indexing
note: the lint level is defined here
 --> src/bin/slice.rs:1:9
  |
1 | #![warn(clippy::missing_asserts_for_indexing)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: indexing into a slice multiple times without an `assert`
 --> src/bin/slice.rs:7:2
  |
7 |     slice[1] * slice[0]
  |     ^^^^^^^^^^^^^^^^^^^
  |
  = help: consider asserting the length before indexing: `assert!(slice.len() > 1);`
note: slice indexed here
 --> src/bin/slice.rs:7:2
  |
7 |     slice[1] * slice[0]
  |     ^^^^^^^^
note: slice indexed here
 --> src/bin/slice.rs:7:13
  |
7 |     slice[1] * slice[0]
  |                ^^^^^^^^
  = note: asserting the length before indexing will elide bounds checks
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#missing_asserts_for_indexing

warning: `foo` (bin "slice") generated 2 warnings

Version

rustc 1.86.0-nightly (f7cc13af8 2025-01-25)
binary: rustc
commit-hash: f7cc13af822fe68c64fec0b05aa9dd1412451f7c
commit-date: 2025-01-25
host: x86_64-unknown-linux-gnu
release: 1.86.0-nightly
LLVM version: 19.1.7

and also on

rustc 1.84.0 (9fc6b4312 2025-01-07)
binary: rustc
commit-hash: 9fc6b43126469e3858e2fe86cafb4f0fd5068869
commit-date: 2025-01-07
host: x86_64-unknown-linux-gnu
release: 1.84.0
LLVM version: 19.1.5

and probably on every version since Rust 1.74.0.
@sendittothenewts sendittothenewts added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Jan 26, 2025
@sendittothenewts
Copy link
Author

Of course, manually memoizing with something like let slice0 = slice[0]; let _square = slice0 * slice0; does work for the same index case, but IMHO worsens readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

No branches or pull requests

1 participant