suggest alternatives to iterate an array of ranges #11862
Conversation
Co-authored-by: ThinkerDreamer <74881094+ThinkerDreamer@users.noreply.github.com>
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
|
Generally I think suggestions should not be machine applicable if they change the semantics of the program, so you might want to change those so they aren't applied automatically. |
There was a problem hiding this comment.
This looks good for a start! 👍 I do concur with @pitaj's suggestion about applicability, and I have an idea on improving the error message, and we may want to add other tests where e.g. the range or even the whole array is in a variable, but otherwise this is mostly merge-worthy already.
| "for loop over a single range inside an array, rather than iterating over the elements in the range directly", | ||
| "did you mean to iterate over the range instead?", | ||
| sugg.to_string(), | ||
| applicability, |
There was a problem hiding this comment.
| applicability, | |
| Applicability::Unspecified, |
| cx, | ||
| SINGLE_ELEMENT_LOOP, | ||
| arg.span, | ||
| "for loop over a single range inside an array, rather than iterating over the elements in the range directly", |
There was a problem hiding this comment.
Perhaps we can find a shorter way to phrase this? I like the clarity though. Maybe something like "This loops only once with <loop pattern> being <range expr>", which is admittedly harder to construct.
There was a problem hiding this comment.
We discussed a lot about the message of this lint, but opted for a longer and more verbose one. I adjusted it to contain your version, which is shorter, but now the text "0..5" is written twice in the lint message, which feels redundant to me.
There was a problem hiding this comment.
I see it as "item" (the looping variable name) and "0..5", rather than 0..5 twice. Am I missing something?
There was a problem hiding this comment.
This is the current output of the lint:
error: this loops only once with item being 0..5
--> $DIR/single_element_loop.rs:24:17
|
LL | for item in [0..5] {
| ^^^^^^ help: did you mean to iterate over the range instead?: `0..5`
"0-5" is mentioned in the lint error message and in the suggestion. I thing mentioning it once should be enough, but mentioning it twice will probably not hurt.
There was a problem hiding this comment.
... and with a lowercase "t" the error message actually passes the tests, which were failing with my commit from earlier today. :)
| cx, | ||
| SINGLE_ELEMENT_LOOP, | ||
| arg.span, | ||
| format!("this loops only once with {pat_snip} being {range_expr}").as_str(), |
There was a problem hiding this comment.
| format!("this loops only once with {pat_snip} being {range_expr}").as_str(), | |
| format!("this loops only once with `{pat_snip}` being `{range_expr}`").as_str(), |
|
Thank you! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
|
Congratulations, @christophbeberweil and @ThinkerDreamer to your first successful clippy PR! 🎉 If any of you like to contribute further to clippy and need help to either pick or solve another issue, either ping me anywhere or go to the clippy channel on the rust-zulip. |
|
Thanks @llogiq! And thanks @christophbeberweil for the great pairing! 😊 |
|
Thanks @llogiq and @ThinkerDreamer, I enjoyed hacking on clippy with both of you 🥳 |
works towards #7125
changelog: [
single_element_loop]: suggest better syntax when iterating over an array of a single range@ThinkerDreamer and myself worked on this issue during a workshop by @llogiq at the RustLab 2023 conference. It is our first contribution to clippy.
When iterating over an array of only one element, which is a range, our change suggests to replace the array with the contained range itself. Additionally, a hint is printed stating that the user probably intended to iterate over the range and not the array. If the single element in the array is not a range, the previous suggestion in the form of
let {pat_snip} = {prefix}{arg_snip};{block_str}is used.This change lints the array with the single range directly, so any prefixes or suffixes are covered as well.