-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
needless_range_loop produces code with different behaviour #6930
Comments
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
You have to create a perma link with the |
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 |
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 |
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. |
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 |
(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) |
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 My benchmarking gave me those numbers:
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
Maybe other people have a different opinions. Can you link the IRLO thread please? |
Thanks. I tried throwing some black_box around, but that has just made the 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 |
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 (You can also |
I'm not sure about clippy's lint machinery, but the underlying |
Clippy just uses the same diagnostic machinery as rustc (with a few convenience wrapper functions). So this should be possible 👍 |
I think if there's a suggestion that changes semantics, the suggestion should provide a warning that it does. Probably it should also have an increased severity or something designating that it fixes a bug. I think silently changing semantics is hostile, even under the best of intentions. |
@rustbot claim |
needless_range_loop suggests changing:
to
However, these are only equivalent is
out
has at leastbase
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 )
The text was updated successfully, but these errors were encountered: