Wrong suggestion on while_let_on_iterator

[llogiq]

Clippy will suggest to iterate over the supplied value without regards to borrowing. For example,

while let Some(foo) = self.iter.next() { .. }

will lead clippy to produce the following suggestion:

for foo in self.iter { .. }

wich would move iter out of self, which is clearly not what we want. The correct suggestion would be:

for foo in &self.iter { .. }

But only if &self.iter : Iterator<_>. Otherwise the whole lint is wrong, and the while let loop should be left alone.

[llogiq]

This needs another trait impl lookup (where one first needs to construct the type of a ref of the expression, which would be &self.iter in my example).

[ssokolow]

It's also possible that it has to be &mut self.iter.

That's what would have been required in my case, if the lint weren't rendered completely spurious by my decision to call &mut self methods in the loop body.

[oli-obk]

@ssokolow I read through your post, and I'm very confused by the error.

struct Foo<T> {
    in_iter: T,
    next_word: Vec<u32>,
}

impl<T: Iterator<Item=i32>> Foo<T> {
    fn bar(&mut self) {
        for x in &mut self.in_iter {
            self.next_word.clear();
        }
    }
}

works fine. Can you share some more code?

[ssokolow]

Is this enough?

impl<'a> WordOffsets<'a> {
    /// Helper to deduplicate the code involved in advancing to the next word in the iterator
    fn _next_word(&mut self, end_offset: usize, skip: bool) -> Option<(usize, usize)> {
        // We have to update our state variables no matter what the outcome, so do this first.
        let skipping = replace(&mut self.skipping, skip);
        let start_offset = replace(&mut self.start_offset, end_offset);

        // If our previous "word" is non-empty and we're not skipping it, return it
        if start_offset < end_offset && !skipping {
            Some((start_offset, end_offset))
        } else {
            None
        }
    }
}
impl<'a> Iterator for WordOffsets<'a> {
    type Item = (usize, usize);

    fn next(&mut self) -> Option<(usize, usize)> {
        // Get the next grapheme cluster and its byte index
        // Note: Using `while let` instead of `for` is necessary to avoid a borrow conflict
        #[allow(while_let_on_iterator)]
        while let Some((byte_offset, grapheme)) = self.in_iter.next() {
            // Extract the base `char` so `classify_char` can call things like `is_uppercase`
            let base = grapheme.chars().nth(0).expect("non-empty grapheme cluster");

            // Identify character types and map transitions between them to actions
            let curr_type = classify_char(base);
            let curr_action = transition_to_action(replace(&mut self.prev_type, curr_type),
                                                   curr_type, self.strict);

            // Actually apply the action to the iterator's state and, if the action returns an
            // accumulated word, return it.
            // TODO: Consider using an enum for the skip=true/false
            let prev_offset = replace(&mut self.prev_offset, byte_offset);
            if let Some(pair) = match curr_action {
                CCaseAction::Skip => { self._next_word(byte_offset, true) },
                CCaseAction::StartWord if self.suppress != byte_offset => {
                    self._next_word(byte_offset, false) },
                CCaseAction::AlreadyStartedWord if self.suppress != prev_offset => {
                    self._next_word(prev_offset, false)
                },
                CCaseAction::Suppress => { self.suppress = byte_offset; None },
                _ => { None }, // Use Literal as the fallback behaviour

            } {
                return Some(pair);
            }
        }

        // Drain the remaining graphemes into a final word, if present
        let in_len = self.in_len;
        self._next_word(in_len, true)
    }
}

EDIT: ...and, huh. That code style looks noticeably less readable in GitHub's color scheme compared to Vim's default.

[oli-obk]

Oh... I somehow misread that _next_word is a method.

Yea, the only way to do that here would be to place skipping and start_offset into their own struct which implements the _next_word method. Then you could do self.whatnot._next_word().

So yea, we need to figure out whether a borrow is reused inside the loop before suggesting this change.