False positive on loop variable being used to index a slice

[dave-andersen]

Clippy's needless range loop is false positiving on a case where the iteration may stop before the end of the loop, i.e.

   let stop_point = min( thing.len(), some_other_thing );
   for i in 0..stop_point {
     do something with thing[i]
     do something with i
   }

The code could be transformed to take a new slice of thing and then use enumerate, but it's not really clear it would be an improvement -- and it's a somewhat nontrivial change.

clippy 0.0.212 (99cb9ccb9 2020-05-11)

Clippy output:

warning: the loop variable `i` is used to index `searchkey`
   --> src/main.rs:184:18
    |
184 |         for i in 0..skl {
    |                  ^^^^^^

Code:

   fn key_compare(&self, start_pos: usize, searchkey: &[u8]) -> core::cmp::Ordering {
        let skl = std::cmp::min(searchkey.len(), self.num_digits() - start_pos);
        let default_return = if skl < searchkey.len() {
            core::cmp::Ordering::Less
        } else {
            core::cmp::Ordering::Equal
        };
        for i in 0..skl {
            let da = self.digit_at(i + start_pos);
            if da < searchkey[i] {
                return core::cmp::Ordering::Less;
            } else if da > searchkey[i] {
                return core::cmp::Ordering::Greater;
            }
        }
        default_return
    }

[flip1995]

Complete message:

warning: the loop variable `i` is used to index `searchkey`
  --> src/main.rs:15:14
   |
15 |     for i in 0..skl {
   |              ^^^^^^
   |
   = note: `#[warn(clippy::needless_range_loop)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
help: consider using an iterator
   |
15 |     for (i, <item>) in searchkey.iter().enumerate().take(skl) {
   |         ^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The suggested code is semantically equivalent and makes use of iterators. So exactly what the lint should detect. The improvement is to have more idiomatic code, since using iterators over ranges+indexing is the rusty way.

What am I missing?

[dave-andersen]

I guess it's style - which means we should probably close this bug. Because 'i' is already being used to index a different thing (but in basically the same way), it feels less clear to me, but that could just be my C brain being dumb. What I don't like about it is that by separating the values of i and the limit set by 'skl', it makes it less clear that the dereference using i :

self.digit_at(i + start_pos);

follows the bound that i < skl. But, I agree that it's not a false positive if there's stylistic consensus about the enumerate+take version being more Rusty!