while_let_on_iterator should not trigger on borrowed iterators if the iterator is subsequently used.

[peterjoel]

Lint name: while_let_on_iterator

I tried this code:

fn parse_value<V>(tokens: TokenStream) -> parse::Result<(ValueExpr<V>, TokenStream)>
where
    ValueExpr<V>: Parse,
{
    let mut it = tokens.into_iter();
    let mut value = TokenStream::new();
    while let Some(tt) = it.next() {
        if let TokenTree::Punct(p) = &tt {
            if p.as_char() == ',' {
                break;
            }
        }
        value.append(tt);
    }
    Ok((ValueExpr::parse.parse2(value)?, it.collect()))
}

I expected to see this happen:

This should not trigger the lint because the iterator is used again after the loop. The purpose of explicitly calling next() is to make it clear that the iterator may not be fully consumed in the loop and the remaining items can be processed elsewhere.

Instead, this happened: The lint triggers and tells me to use a for loop.

Meta

Rust version (rustc -Vv):

rustc 1.55.0 (c8dfcfe04 2021-09-06)
binary: rustc
commit-hash: c8dfcfe046a7680554bf4eb612bad840e7631c4b
commit-date: 2021-09-06
host: x86_64-apple-darwin
release: 1.55.0
LLVM version: 12.0.1

[rustbot]

Error: Label ` can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

[camsteffen]

The rustbot comment seems to be caused by #7627 @xFrednet (edit: er the fix didn't work 😢)

[xFrednet]

It sadly seems that way. The bot is a bit inconsistent when it comes to mentions inside html comments. I'll create another fix PR (See #7670) I would mark these three comments as off-topic once #7670 is merged

[Jarcho]

It's hard to call this one a false positive as the suggestion is correct in that you can use a for loop there. You have a subjective point about why not to use one which I personally disagree with. I would assume you were using the iterator within the loop, not necessarily afterwards. It might be a good idea to split the into two so you could disable just part of it.

As an aside I would be writing the code more like this (if you like combinators):

let mut it = tokens.into_iter();
let value: TokenStream = (&mut it).take_while(|x| match x {
    TokenTree::Punct(p) => p.as_char() != ',',
    _ => true,
}).collect();
Ok((ValueExpr::parse.parse2(value)?, it.collect()))

[Jarcho]

Looks like clippy is correct there. You can use for mut ptr in &mut iter there.

[jjl]

yeah, i deleted my comment when i realised (some half an hour later...).

i'm not sure if the lint suggestion is more obvious from the cli, but it really wasn't through lsp-mode

[peterjoel]

@jackh726 My point about it being better style not to use a for loop is definitely debatable, but I agree that it is much more expressive to use take_while here. It can be further improved with matches!:

let value: TokenStream = (&mut it)
    .take_while(|x| !matches!(x, TokenTree::Punct(p) if  p.as_char() == ','))
    .collect();

I think by_ref can be used as an alternative to &mut it.

let value: TokenStream = it.by_ref()
    .take_while(|x| !matches!(x, TokenTree::Punct(p) if  p.as_char() == ','))
    .collect();

[peterjoel]

@mikerite There should be a clippy lint for this!

[jjl]

If clippy were to have suggested by_ref, i think i would have understood much more quickly.

[Jarcho]

It could definitely change to use by_ref if people prefer that. I have no opinion either way here; I think they both look fine.