needless_range_loop produces code with different behaviour

[ChrisJefferson]

needless_range_loop suggests changing:

pub fn foo_a(out: &mut Vec<usize>, inx: &[usize], base: usize) {
    for i in 1 .. base {
        out[i] = *inx.get(i).unwrap_or(&0);
    }
}

to

pub fn foo_b(out: &mut Vec<usize>, inx: &[usize], base: usize) {
    for (i, item) in out.iter_mut().enumerate().take(base).skip(1) {
        *item = *inx.get(i).unwrap_or(&0);
    }
}

However, these are only equivalent is out has at least base elements. If not, then this changes behaviour (the first version panics with out of bounds, the second silently truncates the loop).

See this example at https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6f00aec168a98758df574f0734a2233e (edit: this link was previously broken, thanks)

(I noticed this will discussing needless_range_loop in #6075 )

[flip1995]

So the lint silently improves your code. I don't think this needs a fix, but we should mention this in the documentation/note message. Related: #6929

You should use the iterator variant and add a size check, and handle the case of out.len() <= base explicitly.

See this example at https://play.rust-lang.org/

You have to create a perma link with the Share button at the top.

[ChrisJefferson]

I realise we previously discussed my dislike of this, but I do not think this "improves my code". It silently hides a serious bug.

It might be worth adding a take_exactly function to the standard library (name to be discussed), which would panic if it couldn't get enough values, then that would keep the same semantics (and probably the same performance as take)

[flip1995]

If this was "improving my code", so would Rust just ignoring all out-of-bounds writes to arrays.

Panicking because of an out-of-bounds error is a bug in your code, not a feature. If you intended to panic when the index is out-of-bounds, you should explicitly write code addressing this and not hide it behind an indexing operation. If you want to use the Rust safeguard of bounds-checking as a "feature", you should have a test for this, which would catch this semantic change.

Either way, we should mention this in the documentation and possibly add a note to the lint message.

---

If you don't want bounds-checking there's get_unchecked, which is obviously unsafe.

[ChrisJefferson]

I never intend to make use of out-of-bounds panic, or any of the other types of panicking, but my code still panics several times a week, because I write buggy code :)

I do think this should be added to the lint message -- in general it might be nice to have a general statement (does it exist somewhere? If so I couldn't find it), of exactly the guarantees (if any) clippy makes about changes it suggests -- should they never change behaviour of code (except where explicitly stated), or should users always carefully consider changes?

I realise this might be an unreasonable request, as there are too many things that can happen with almost any lint, if user's code does strange enough things, but then it might still be good for that to be explicitly documented somewhere.

[flip1995]

We should have something like this, but currently such a definition does not exist. As a rule of thumbs, Clippy should always suggest code that is semantically equivalent or if it can't like in this case mention this at least in the documentation. Changing the semantics like in this case is acceptable, because it removes a (unintended) panic. The other way around is never acceptable.

I also tend to add a short note to the lint, where take is involved.

[ChrisJefferson]

(I previously put this in the wrong issue, sorry if anyone got pinged by this).

I'd just like to drop one final suggestion, which was given to be over on internals.rust_lang.org, to change the suggested code to:

for (i,item) in out[..base].iter_mut().enumerate.skip(1) {
... }

This has (I think) a few major advantages. It's easy to read (to me), it preserves the original semantics (panic if base is too large), and (according to a simple benchmark), it's the fastest version (although I know benchmarking is hard, and I might have messed it up, so comments welcome!)

I run https://gist.github.com/ChrisJefferson/f5e17e65fb1ee49c5d0a2f7fca12c3f5 with cargo +nightly bench.

(a) for loop, (b) clippy's suggestion, (c) v[..base].iter_mut().enumerate().skip(1)

test tests::bench_a ... bench: 12,253 ns/iter (+/- 1,640)
test tests::bench_b ... bench: 10,035 ns/iter (+/- 2,066)
test tests::bench_c ... bench: 9,345 ns/iter (+/- 671)

[flip1995]

The problem with your benchmark code is, that you pass the arguments directly to the functions, so they just get constant folded away and the compiler can just constant fold everything, so everything has the same execution time. You have to pass the arguments with foo(black_box(arg1), black_box(arg2) ...).

My benchmarking gave me those numbers:

lint_test2_index        time:   [152.19 us 153.19 us 154.42 us]

lint_test2_iter         time:   [17.423 us 17.484 us 17.547 us]

lint_test2_range        time:   [17.143 us 17.213 us 17.293 us]

So yeah, this is pretty much the same. (The range version produces a bit more assembler: https://godbolt.org/z/cdEY9s). Personally I still prefer to suggest take here, because

1. You avoid out-of-bounds errors; and
2. You don't mix slicing with iterators.

which was given to be over on internals.rust_lang.org

Maybe other people have a different opinions. Can you link the IRLO thread please?

[ChrisJefferson]

Thanks. I tried throwing some black_box around, but that has just made the for loop go fastest, so I must have put it in the wrong place. Also, I must be using a different profiling library than you I guess (as we get different output). If you could share what your code, that would be handy.

The thread (and the relevant post) is: https://internals.rust-lang.org/t/new-function-suggestion-iter-take-exactly/14290/8

This was me suggesting there should be a "take_exactly" function (which is disjoint from this discussion, but there it was suggested that v[..L] might be useful for implementing take_exactly.

[flip1995]

Also, I must be using a different profiling library than you I guess (as we get different output). If you could share what your code, that would be handy.

Sure! https://github.com/flip1995/dirty-bench I use this hacky thing for things I want quick benchmarks for, so don't use it if you want to do serious benchmarking!

Just run cargo bench --bench lint_test2 on the repo and you should get the same results (modulo machine specs).

(You can also cargo install cargo-criterion and then run cargo criterion --bench lint_bench2 if you want nice graphs as HTML output)

[estebank]

I'm not sure about clippy's lint machinery, but the underlying DiagnosticBuilder lets you specify more than one suggestion. If that is available, you could suggest both with an appropriate message for both cases explaining the change of behavior, or alternatively extend the current suggestion to include a length assertion before the expression which would make it clearer to the user that they could remove the assertion. This later option would likely be less nice to implement and would likely look worse.

[flip1995]

Clippy just uses the same diagnostic machinery as rustc (with a few convenience wrapper functions). So this should be possible 👍