while_let_on_iterator Improvements

[Jarcho]

fixes: #6491
fixes: #6231
fixes: #5844
fixes: #1924
fixes: #1033

The check for whether a field can be borrowed should probably be moved to utils at some point, but it would require some cleanup work and knowing what parts can actually be shared.

changelog: Suggest &mut iter when the iterator is used after the loop.
changelog: Suggest &mut iter when the iterator is a field in a struct.
changelog: Don't lint when the iterator is a field in a struct, and the struct is used in the loop.
changelog: Lint when the loop is nested in another loop, but suggest &mut iter unless the iterator is from a local declared inside the loop.

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[bors]

☔ The latest upstream changes (presumably #6971) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #6931) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey, this is my first bigger review in Clippy. I therefor also annotated some NITs which are probably fine. The work is really impressive, and you put a lot of effort into this! The implementation looks good beside the suggestions. I'm not completely done with the tests yet, but they are on the to-do list for the weekend.

A thing that is not directly connected to your changes: Could you also extend the documentation of the lint itself a bit? The current documentation is very basic. Even extending it by the way the example should be written as would be a great improvement IMO.

@Manishearth Could you look over these suggestions? 🙃

---

I've also tested the lint using lintcheck this added 3 new lint triggers which seem valid and the suggestions (I've tested) also seem to work fine 👍

The lint triggers with the linted code

1. proc-macro2-1.0.24/src/parse.rs:444:5

   while let Some((i, ch)) = chars.next() {
       match ch {
           '"' => {
               n = i;
               break;
           }
           '#' => {}
           _ => return Err(LexError),
       }
   }

2. serde-1.0.118/src/private/de.rs:2830

   while let Some(item) = self.iter.next() {
       // Items in the vector are nulled out when used by a struct.
       if let Some((ref key, ref content)) = *item {
           self.pending_content = Some(content);
           return seed.deserialize(ContentRefDeserializer::new(key)).map(Some);
       }
   }

3. serde-1.0.118/src/private/de.rs:2932

   while let Some(item) = self.iter.next() {
        if let Some((ref key, ref content)) = *item {
            // Do not take(), instead borrow this entry. The internally tagged
            // enum does its own buffering so we can't tell whether this entry
            // is going to be consumed. Borrowing here leaves the entry
            // available for later flattened fields.
            self.pending = Some(content);
            return seed.deserialize(ContentRefDeserializer::new(key)).map(Some);
        }
    }

[Manishearth]

@xFrednet your review looks great so far! Nice catches!

[bors]

☔ The latest upstream changes (presumably #6951) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Sorry for taking so long for the second review, I've been busy due to an assignment that I have to hand in on the 4th. This PR is high on the TODO list after that handin 🙃

[xFrednet]

Thank you for adding your changes in a second commit that helped with the review a lot and sorry, for taking so long for the second review 😅.

I only have one more small NIT where it would be nice if you could fix that even if it's not part of your changes. The rest looks really solid and can be merged after a rebase IMO. In don't know if @Manishearth wants to do a final review.

---

The small NIT is in test/ui/while_let_on_iterator.rs#L151-L159:

    // This should not cause an ICE and suggest:
    //
    // for _ in values.iter() {}
    //
    use std::collections::HashSet;
    let mut values = HashSet::new();
    values.insert(1);

    while let Some(..) = values.iter().next() {}

This code doesn't trigger the lint as the iterator is always newly generated. Could you remove the suggestion part of the comment above?

[Manishearth]

Your review looks great, I'll let you r+ when ready!

@bors delegate=xFrednet

[bors]

✌️ @xFrednet can now approve this pull request

[xFrednet]

The final update looks good. Thank you!
@bors r+

[bors]

📌 Commit cd0db8a has been approved by xFrednet

[bors]

⌛ Testing commit cd0db8a with merge 90fb7c4...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 90fb7c4 to master...