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?

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?

@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.