needless_range_loop intrusiveness

[leonardo-m]

This is a case where needless_range_loop probably gives a good suggestion:

fn main() {
    let v = [10, 20, 30];
    for i in 0 .. v.len() {
        println!("{} {}", i, v[i]);
    }
}

Clippy says:

warning: the loop variable `i` is used to index `v`
 --> src/main.rs:3:14
  |
3 |     for i in 0 .. v.len() {
  |              ^^^^^^^^^^^^
  |
  = 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
  |
3 |     for (i, <item>) in v.iter().enumerate() {
  |         ^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^

warning: 1 warning emitted

But in many many cases similar suggestions are not so good. I suspect that replacing a hundred regular range for loops with iter/iter_mut + enumerates increases the work (the time) the compiler (the back-end) has to do to optimize the code. And in many cases (most cases in my codebase) this suggestion messes up the signficant amount of imperative code inside the loop. So while in general I think such lint is useful to have in Clippy, for me it's one of the most annoying Clippy lints. So I suggest to reduce the number of situations where it fires. and move the rest of situations into a pedentic bin that's disabled by default.

[ebroto]

This is a case where needless_range_loop probably gives a good suggestion

And in many cases (most cases in my codebase) this suggestion messes up the signficant amount of imperative code inside the loop.

So I suggest to reduce the number of situations where it fires.

Could you please elaborate on which cases you find problematic and why? Ideally giving some examples :)

[leonardo-m]

In this loop:

for i in 6 .. L {
    if primes[i] && reverse_digits(to!{i, u32}, &mut digs).iter().any(|&d| BAD[to!{d, usize}]) {
        primes[i] = false;
    }
}

Clippy has suggested me to write:

for (i, pi) in primes.iter_mut().enumerate().take(L).skip(6) {
    if *pi && reverse_digits(to!{i, u32}, &mut digs).iter().any(|&d| BAD[to!{d, usize}]) {
        *pi = false;
    }
}

While this simplifies the code inside the loop slightly (replacing two primes[i] with *pi), the resulting code seems quite more complex than the original one (and I think this change also increases the pressure on the LLVM, because it has to optimize tons of stuff away. Try to look at the asm of rust generated without optimizations. It's hundreds of asm lines for every basic iter_mut+enumerate+take+skip loop).

[ebroto]

Thanks for taking the time to come up with an example.

About the increase in compile time, I would say that the problem you point out would be general for iterators, right? Indexing would probably increase run time due to bounds checking, so IMO this seems like a trade-off that depends on each case (which Clippy can't detect/reason about) and should be left for the user to decide.

About splitting the lint, I think we could benefit from hearing more opinions on this. Personally, I think the suggestion is fine (modulo false positives of course, and #878 / #398, which seems could be solved by suggesting windows) and there has been a good amount of work to reduce the number of cases where it triggers already.

I will leave the issue opened so that other folks can help to move the discussion forward.

[flip1995]

I don't think the argument against this lint, that it increases compile time is valid. One principle of the Rust Language is exactly this trade-off: sacrifice compile time for safe and efficient code. The efficient code comes from the zero-cost-abstractions Rust provides. Using iterators is one of those zero-cost-abstractions. In addition to that iterators over indices is idiomatic Rust style. If you don't agree with this, I see that this lint isn't for you. But as long as there is no FP, I wouldn't reduce the cases this lint triggers for.

[camsteffen]

Related #3032

I agree that the lint is too aggressive. Here is an example I came up with. It may be a little contrived, but I think it illustrates the problem. The loop is not primarily a loop over the Vec. The fact that the counter variable is used to index a Vec is just an implementation detail. So I think the lint should not fire if the counter is used in any other way.

let feelings = vec!["alive", "tired", "very tired"];
for miles in 0..3 {
    let calories = miles * 100;
    let feeling = feelings[miles];
    println!("You need {} calories to run {} miles. Then you will feel {}.", calories, miles, feeling);
}

[ChrisJefferson]

I would like a version of this clippy where enumerate can be used, but no take or skip are required -- in those cases I find the clippy produces longer, and more confusing, code.

[flip1995]

I just put this code into godbolt, to make my point more clear: https://godbolt.org/z/8njPf6

The iterator version optimizes to 177 lines of assembler code, while the index code optimizes to 308 lines of assembler code.

As for the performance of both variants on big vectors of the size 42000:

index         time:   [27.852 us 27.913 us 27.980 us]

iter          time:   [4.1047 us 4.1124 us 4.1207 us]

So yeah, I don't think there is an argument left for not triggering this lint in the examples given here. #3032 is a good example how this lint could be enhanced with suggesting zip as appropriate, as is the example given by @camsteffen.

Clippy should not suggest less optimal code, just because someone likes the (objectively worse) index style better than the idiomatic iterator style. If performance is not of concern for you, feel free to disable this lint or the whole clippy::perf group. If you can come up with an example where the index version performs better (code size and/or speed) and this lint triggers, let me know and I'll reopen this issue.

[ChrisJefferson]

I'm not using this clippy for performance, I am using it for (what I view as) clarity.

I did't even know how to tell this was a "perf" clippy -- out of interest, how would I find that out? it doesn't seem listed on the terminal, and at https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop it seems to be "style"?

[ChrisJefferson]

While I haven't benchmarked, I took the snippet of code which was being triggered for me, and in godbolt the "traditional loop" version produces significantly less assembler:

https://godbolt.org/z/v6KoPq

[flip1995]

While I haven't benchmarked, I took the snippet of code which was being triggered for me, and in godbolt the "traditional loop" version produces significantly less assembler:

It produces 9 lines less assembler, yes. But the performance speaks for itself:

index        time:   [135.99 us 136.60 us 137.24 us]

iter         time:   [15.552 us 15.642 us 15.737 us]

I'm not using this clippy for performance, I am using it for (what I view as) clarity.

To avoid future confusion: We call this a "lint" not a "clippy". Clippy is the tool name providing those "lints".

I guess this lint is classified as style, because it is more idiomatic in Rust to use iterators, whenever possible. The classification is guided by what the lint author/reviewer thinks is the most fitting group. I guess we could reclassify this as perf. But generally to find out the lint group, you look at the Clippy documentation page.

The documentation states:

Just iterating the collection itself makes the intent more clear and is probably faster.

So it mentions that this lint also addresses performance, not only style.

[ChrisJefferson]

I still believe this lint has grown too large, if we look at the doc page, it says "Checks for looping over the range of 0..len of some collection just to get the values by index", so at least the documentation should be updated to discuss any a..b range.

Would a PR which split the check into two pieces, one just handing the case in the documentation (no take or skip required) (which I would count as 'style'), and the current case (counted as 'perf'), be acceptable, in principle?

[flip1995]

so at least the documentation should be updated to discuss any a..b range.

I agree the documentation should be updated and improved.

Would a PR which split the check into two pieces, one just handing the case in the documentation (no take or skip required) (which I would count as 'style'), and the current case (counted as 'perf'), be acceptable, in principle?

I still don't see the point in that. Then we would have one lint, that suggests optimal code and a second lint, that lints almost the same, but worse. Using iterators everywhere instead of "traditional loops" is idiomatic Rust, and therefore suggesting skip and/or take in a style lint is correct.

[DavidAntliff]

FWIW, there are some cases where replacing for i in 0..vec.len() with for (i, _) in vec.iter().enumerate() isn't equivalent, because the latter results in an immutable borrow, and the former does not.

Using the clippy recommendation doesn't compile in cases where a mutable borrow is taken within the loop.

fn main() {
    let mut m = vec![1, 2, 3, 4];
    
    for i in 0..m.len() {
    //for (i, _) in m.iter().enumerate() {
        
        let item = &mut m[i];
        *item = 42;
    }
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3e864737e89846549256a38b9b8664c5

[keks]

I have this code

fn foo(chunk: u8) {
    let mut buf = [' '; 8];
    for i in 0..8 {
        if chunk & (1 << i) > 0 {
            buf[i] = '#';
        }
    }
    print!("{}", String::from_iter(buf))
}

and clippy complains about this lint.

In this case, I don't think there will be bounds checking because it is clear at compile time that all values in 0..8 can index [_, 8], and I'd expect rustc to figure that out.

I've tried asking rust-analyzer to rewrite this, and all it does is transform this into (0..8).for_each(|i| { /* loop body */}), which I don't think is any better (it definitely doesn't help with bounds checking).