Flag bufreader.lines().filter_map(Result::ok) as suspicious

[samueltardieu]

This lint detects a problem that happened recently in https://github.com/uutils/coreutils and is described in rust-lang/rust#64144.

changelog: [lines_filter_map_ok]: new lint

[rustbot]

r? @flip1995

(rustbot has picked a reviewer for you, use r? to override)

[tertsdiepraam]

Wow this is awesome! Thanks!

I'm new to clippy lints but if I understand correctly, this checks that there is a BufRead on which the lines() and filter_map(Result::ok) methods are called right?

There is a variation of this that I've seen with .filter_map(|l| l.ok()). That is not covered by this implementation, is it? It might not be super important as other lints might already suggest rewriting the closure into Result::ok, which will in turn trigger this lint.

The same pattern also happens with Stdin, which has a separate lines method and does not implement BufRead. It does however return the same Lines struct. I'm not sure it's really an issue with Stdin though as I haven't gotten it to get into an infinite loop yet, but I think it's possible.

[samueltardieu]

I'm new to clippy lints but if I understand correctly, this checks that there is a BufRead on which the lines() and filter_map(Result::ok) methods are called right?

Yes.

There is a variation of this that I've seen with .filter_map(|l| l.ok()). That is not covered by this implementation, is it? It might not be super important as other lints might already suggest rewriting the closure into Result::ok, which will in turn trigger this lint.

I'll have a look, as there is a lint for possible η-reduction but it is activated only in pedantic mode.

The same pattern also happens with Stdin, which has a separate lines method and does not implement BufRead. It does however return the same Lines struct. I'm not sure it's really an issue with Stdin though as I haven't gotten it to get into an infinite loop yet, but I think it's possible.

Indeed. stdin().lock().lines() is covered, as the lines() method comes from BufRead, but stdin.lines() is a special case which could be easily integrated by checking for the Lines type. I'll put this PR in draft mode until I implement it.

[bors]

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

[samueltardieu]

The proposed lint now triggers with anything returning std::io::Lines (including std::io::stdin().lines()), and detects |x| x.ok() in addition to Result::ok.

A lintcheck with the 500 top most downloaded recent crates shows no hit.

[bors]

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

[samueltardieu]

r? @llogiq

[llogiq]

I wonder why we should lint stdin.lines(). The infinite loop problem only appears on reading directories, and I haven't yet been able to pipe one to stdin.

[samueltardieu]

I wonder why we should lint stdin.lines(). The infinite loop problem only appears on reading directories, and I haven't yet been able to pipe one to stdin.

On Linux :

$ cat t.rs
fn main() {
    for l in std::io::stdin().lines() {
        println!("l = {l:?}");
    }
}
$ rustc t.rs
$ ./t < /
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
l = Err(Os { code: 21, kind: IsADirectory, message: "Is a directory" })
^C

[llogiq]

Ok then. I think the docs could use some editing for wording, otherwise it looks ok.

[samueltardieu]

Ok then. I think the docs could use some editing for wording, otherwise it looks ok.

I have reworked the wording, tell me if you'd change other things.

[llogiq]

The only possible problem is that the user could check for in.is_file() manually, in which case we'd have a false positive. That said, I'd be ok with just adding that to the "known problems" section, as the code to check for this case could be quite hairy

[samueltardieu]

Note that even if in.is_file(), an I/O error while reading the file (e.g., a USB key removed while reading) will likely persist, in which case the implementation of Lines.next() will return an Err forever (I have also verified this experimentally by creating a small fuse FS returning a read error).

I've added a "Known problems" section to the lint.

I've used "suspicious" as a category. Is that ok, or do you prefer it to be put in the "nursery"?

[llogiq]

I think suspicious is likely fine, as the lint highlights a real problem and I haven't encountered a case where ignoring an error on one line would have produced a good outcome.

@bors r+

[bors]

📌 Commit 6601d85 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6601d85 with merge ac4838c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing ac4838c to master...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing ac4838c to master...