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

Author: samueltardieu

CHANGELOG.md

@@ -4645,6 +4645,7 @@ Released 2018-09-13
 [`let_underscore_untyped`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_untyped
 [`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
 [`let_with_type_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_with_type_underscore
+[`lines_filter_map_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok
 [`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist
 [`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
 [`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal

clippy_lints/src/declared_lints.rs

@@ -231,6 +231,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::let_with_type_underscore::LET_WITH_TYPE_UNDERSCORE_INFO,
     crate::lifetimes::EXTRA_UNUSED_LIFETIMES_INFO,
     crate::lifetimes::NEEDLESS_LIFETIMES_INFO,
+    crate::lines_filter_map_ok::LINES_FILTER_MAP_OK_INFO,
     crate::literal_representation::DECIMAL_LITERAL_REPRESENTATION_INFO,
     crate::literal_representation::INCONSISTENT_DIGIT_GROUPING_INFO,
     crate::literal_representation::LARGE_DIGIT_GROUPS_INFO,

clippy_lints/src/lib.rs

@@ -170,6 +170,7 @@ mod let_if_seq;
 mod let_underscore;
 mod let_with_type_underscore;
 mod lifetimes;
+mod lines_filter_map_ok;
 mod literal_representation;
 mod loops;
 mod macro_use;
@@ -938,6 +939,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(let_with_type_underscore::UnderscoreTyped));
     store.register_late_pass(|_| Box::new(allow_attributes::AllowAttribute));
     store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(msrv())));
+    store.register_late_pass(|_| Box::new(lines_filter_map_ok::LinesFilterMapOk));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/lines_filter_map_ok.rs

@@ -0,0 +1,70 @@
+use clippy_utils::{diagnostics::span_lint_and_then, is_trait_method, match_def_path, match_trait_method, paths};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detect uses of `b.lines().filter_map(Result::ok)` or `b.lines().flat_map(Result::ok)`
+    /// when `b` implements `BufRead`.
+    ///
+    /// ### Why is this bad?
+    /// `b.lines()` might produce an infinite stream of `Err` and `filter_map(Result::ok)`
+    /// will also run forever. Using `map_while(Result::ok)` would stop after the first
+    /// `Err` is emitted.
+    ///
+    /// For example, on some platforms, `std::fs::File::open(path)` might return `Ok(fs)`
+    /// when `path` is a directory, and reading from `fs` will then return an error.
+    /// If `fs` was inadvertently put in a `BufReader`, calling `lines()` on it would
+    /// return an infinite stream of `Err` variants.
+    ///
+    /// ### Example
+    /// ```rust
+    /// #use std::{fs::File, io::{BufRead, BufReader}};
+    /// let reader = BufReader::new(File::open("some path")?);
+    /// for line in reader.lines().flat_map(Result::ok) {
+    ///     println!("{line}");
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// #use std::{fs::File, io::{BufRead, BufReader}};
+    /// let reader = BufReader::new(File::open("some path")?);
+    /// for line in reader.lines().map_while(Result::ok) {
+    ///     println!("{line}");
+    /// }
+    /// ```
+    #[clippy::version = "1.70.0"]
+    pub LINES_FILTER_MAP_OK,
+    nursery,
+    "`BufRead::lines()` might produce an infinite stream of errors"
+}
+declare_lint_pass!(LinesFilterMapOk => [LINES_FILTER_MAP_OK]);
+
+impl LateLintPass<'_> for LinesFilterMapOk {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if let ExprKind::MethodCall(fm_method, fm_receiver, [fm_arg], fm_span) = expr.kind &&
+            is_trait_method(cx, expr, sym::Iterator) &&
+            (fm_method.ident.as_str() == "filter_map" || fm_method.ident.as_str() == "flat_map") &&
+            let ExprKind::MethodCall(lines_method, _, &[], _) = fm_receiver.kind &&
+            match_trait_method(cx, fm_receiver, &paths::STD_IO_BUFREAD) &&
+            lines_method.ident.as_str() == "lines" &&
+            let ExprKind::Path(qpath) = &fm_arg.kind &&
+            let Some(did) = cx.qpath_res(qpath, fm_arg.hir_id).opt_def_id() &&
+            match_def_path(cx, did, &paths::CORE_RESULT_OK_METHOD)
+        {
+            span_lint_and_then(cx,
+                LINES_FILTER_MAP_OK,
+                fm_span,
+                &format!("`{}()` will run forever on an infinite stream of `Err` returned by `lines()`", fm_method.ident),
+                |diag| {
+                    diag.span_note(
+                        fm_receiver.span,
+                        "`BufRead::lines()` may produce an infinite stream of `Err` in case of a read error");
+                    diag.span_suggestion(fm_span, "replace with", "map_while(Result::ok)", Applicability::MaybeIncorrect);
+                });
+        }
+    }
+}

clippy_utils/src/paths.rs

@@ -23,6 +23,7 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
 pub const CORE_ITER_CLONED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "cloned"];
 pub const CORE_ITER_COPIED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "copied"];
 pub const CORE_ITER_FILTER: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "filter"];
+pub const CORE_RESULT_OK_METHOD: [&str; 4] = ["core", "result", "Result", "ok"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["alloc", "ffi", "c_str", "CString", "as_c_str"];
 pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
 pub const DEREF_MUT_TRAIT_METHOD: [&str; 5] = ["core", "ops", "deref", "DerefMut", "deref_mut"];
@@ -113,6 +114,7 @@ pub const STDERR: [&str; 4] = ["std", "io", "stdio", "stderr"];
 pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"];
 pub const CONVERT_IDENTITY: [&str; 3] = ["core", "convert", "identity"];
 pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"];
+pub const STD_IO_BUFREAD: [&str; 3] = ["std", "io", "BufRead"];
 pub const STD_IO_SEEK: [&str; 3] = ["std", "io", "Seek"];
 pub const STD_IO_SEEK_FROM_CURRENT: [&str; 4] = ["std", "io", "SeekFrom", "Current"];
 pub const STD_IO_SEEKFROM_START: [&str; 4] = ["std", "io", "SeekFrom", "Start"];

tests/ui/lines_filter_map_ok.fixed

@@ -0,0 +1,23 @@
+// run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::lines_filter_map_ok)]
+
+use std::io::{self, BufRead, BufReader};
+
+fn main() -> io::Result<()> {
+    let f = std::fs::File::open("/tmp/t.rs")?;
+    // Lint
+    BufReader::new(f)
+        .lines()
+        .map_while(Result::ok)
+        .for_each(|l| println!("{l}"));
+    // Lint
+    let f = std::fs::File::open("/tmp/t.rs")?;
+    BufReader::new(f)
+        .lines()
+        .map_while(Result::ok)
+        .for_each(|l| println!("{l}"));
+    let s = "foo\nbar\nbaz\n";
+    Ok(())
+}

tests/ui/lines_filter_map_ok.rs

@@ -0,0 +1,23 @@
+// run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::lines_filter_map_ok)]
+
+use std::io::{self, BufRead, BufReader};
+
+fn main() -> io::Result<()> {
+    let f = std::fs::File::open("/tmp/t.rs")?;
+    // Lint
+    BufReader::new(f)
+        .lines()
+        .filter_map(Result::ok)
+        .for_each(|l| println!("{l}"));
+    // Lint
+    let f = std::fs::File::open("/tmp/t.rs")?;
+    BufReader::new(f)
+        .lines()
+        .flat_map(Result::ok)
+        .for_each(|l| println!("{l}"));
+    let s = "foo\nbar\nbaz\n";
+    Ok(())
+}

tests/ui/lines_filter_map_ok.stderr

@@ -0,0 +1,29 @@
+error: `filter_map()` will run forever on an infinite stream of `Err` returned by `lines()`
+  --> $DIR/lines_filter_map_ok.rs:13:10
+   |
+LL |         .filter_map(Result::ok)
+   |          ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
+   |
+note: `BufRead::lines()` may produce an infinite stream of `Err` in case of a read error
+  --> $DIR/lines_filter_map_ok.rs:11:5
+   |
+LL | /     BufReader::new(f)
+LL | |         .lines()
+   | |________________^
+   = note: `-D clippy::lines-filter-map-ok` implied by `-D warnings`
+
+error: `flat_map()` will run forever on an infinite stream of `Err` returned by `lines()`
+  --> $DIR/lines_filter_map_ok.rs:19:10
+   |
+LL |         .flat_map(Result::ok)
+   |          ^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
+   |
+note: `BufRead::lines()` may produce an infinite stream of `Err` in case of a read error
+  --> $DIR/lines_filter_map_ok.rs:17:5
+   |
+LL | /     BufReader::new(f)
+LL | |         .lines()
+   | |________________^
+
+error: aborting due to 2 previous errors
+