Weird warning for needless_range_loop

[antoyo]

Hello.

I found a weird warning for a range loop.

In the following code:

#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    let string = "test";
    let vec = vec![0, 1, 2, 3];
    for i in 0 .. string.len() {
        println!("{}", i);
        println!("{}", vec[i]);
    }
}

I get the following warning:

src/main.rs:7:5: 10:6 warning: the loop variable `i` is used to index `vec`. Consider using `for (i, item) in vec.iter().enumerate()` or similar iterators, #[warn(needless_range_loop)] on by default
src/main.rs: 7     for i in 0 .. string.len() {
src/main.rs: 8         println!("{}", i);
src/main.rs: 9         println!("{}", vec[i]);
src/main.rs:10     }
src/main.rs:7:5: 10:6 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop

This is wrong since I am iterating over the string, not the vec.

If I remove the line using the vec, the warning is gone.

Thanks to fix this issue.

(By the way, is there an option to span errors instead of warnings in clippy?)

[Manishearth]

cc @birkenfeld

[birkenfeld]

This only looks strange, but this lint doesn't check the range end expression at all. It only sees that you're indexing vec (and only vec) with i and therefore could iterate over it.

In this case, the iteratee could be for (i, item) in vec.iter().enumerate().take(string.len()). Of course we could try to integrate that suggestion into the message.

[Manishearth]

SGTM

[antoyo]

@birkenfeld Well, I made the example simpler, but I my original code, both the clippy suggestion and yours won't work.

Here is my example:

fn levenshtein_distance(string1: &str, string2: &str) -> usize {
    fn distance(i: usize, j: usize, d: &[Vec<usize>], string1: &str, string2: &str) -> usize {
        match (i, j) {
            (i, 0) => i,
            (0, j) => j,
            (i, j) => {
                let delta =
                    if string1.chars().nth(i - 1) == string2.chars().nth(j - 1) {
                        0
                    }
                    else {
                        1
                    };
                *[
                    d[i - 1][j] + 1,
                    d[i][j - 1] + 1,
                    d[i - 1][j - 1] + delta
                ].iter().min().unwrap()
            },
        }
    }

    let mut d = vec![];
    for i in 0 .. string1.len() + 1 {
        d.push(vec![]);
        for j in 0 .. string2.len() + 1 {
            let dist = distance(i, j, &d, string1, string2);
            d[i].push(dist);
        }
    }
    d[string1.len()][string2.len()]
}

clippy gives me this warning on the line:

for i in 0 .. string1.len() + 1 {

but I could not iterate over the vector since it is empty.

[antoyo]

Is this a bug?

[ghost]

Just adding onto this: I got this error for something along the lines of:

for i in 0..N {
    let var1 = v[i];
    let var2 = v[i + 1];
    // ...
}

And this can't be replaced by a simple enumerate. (Sure, it can be replaced by zip, but the code looks simpler otherwise.)

[mcarton]

@Undeterminant do you know about windows?

[ghost]

@Mccarton: I did not at the time, although the point was that the lint's suggestion of just using enumerate wasn't good enough.

[birkenfeld]

@Undeterminant this is possible to write with enumerate, but it isn't pretty at all:

for (i, el) in v.iter().enumerate() {
    let var1 = el;
    let var2 = v[i + 1];
    // ...
}

I'm beginning to think that needless_range_loop should be pruned back quite a bit.

[wfraser]

Agree. This lint doesn't seem to check what else you're doing with the range variable in the loop. While it can be replaced by using enumerate, I agree the result looks uglier many times.

Simple example:

fn u32_from_bytes(bytes: &[u8], big_endian: bool) -> u32 {
    let mut out = 0u32;
    for i in 0..4 {
        let shift = if big_endian {
            3 - i
        } else {
            i
        };
        out |= (bytes[i] as u32) << (8 * shift);
    }
    out
}

vs

pub fn u32_from_bytes(bytes: &[u8], big_endian: bool) -> u32 {
    let mut out = 0u32;
    for (i, byte) in bytes[0..4].iter().enumerate() {
        let shift = if big_endian {
            3 - i
        } else {
            i
        };
        out |= (*byte as u32) << (8 * shift);
    }
    out
}

2nd is needlessly more verbose.

[felix91gr]

@antoyo I've put the example you mentioned at #398 (comment) to the test, and Clippy no longer issues a lint: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=d0fd172ad188002310d57c72846a32f7

Does this mean that this issue has been resolved? Lemme know if I tested it alright :)

[antoyo]

Does this mean that this issue has been resolved? Lemme know if I tested it alright :)

While that example seems fixed, the example in the original post still trigger a warning.
I don't know if this warning is wrong, though, because while it can change the behavior when the vector contains less elements than the string, the clippy suggestion prevents a "out of bounds" panic.
However, maybe there are other cases where this would change an otherwise correct behavior.
I don't know how the heuristic works.

[felix91gr]

While that example seems fixed, the example in the original post still trigger a warning. I don't know if this warning is wrong, though, because while it can change the behavior when the vector contains less elements than the string, the clippy suggestion prevents a "out of bounds" panic. However, maybe there are other cases where this would change an otherwise correct behavior. I don't know how the heuristic works.

Just so that we share the same clippy and code, here's a playground with the original example:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=8cacdb71710ad541bb70c18b98237b62

So... indeed, the program crashes when the vector contains less elements than the string. That's not good. This lint's suggestion should, I believe, prevent that kind of crash from these kinds of loop all the time.

---

Anyways. I think the best way to analyze this lint's suggestion is to see what happens case by case, and see if this matches the intent of the person who wrote the code being linted:

1. vec.len() < string.len():

• Original code: crashes due to out of bounds.
• Suggested code: accesses only up to what vec can provide, and exits afterwards. This makes sense to me. A more sophisticated usage of vec inside of the loop might trigger a different lint, one that perhaps suggests using a zip or something similar. But for the purposes of the original post's example, I think the suggested code is pretty much as best as a reader (such as Clippy) could deduce of the programmer's intent.

2. vec.len() == string.len(): Original code and Suggested code behave in exactly the same way.
3. vec.len() > string.len(): Original code and Suggested code behave in exactly the same way.

What do you think, @antoyo? In this particular example, the suggestion seems very reasonable to me.

Could there be a more elaborate counterexample that triggers an incorrect suggestion?