refactor(lines_filter_map_ok): move to under methods/ (rust-lang#15925)

changelog: none

Author: Jarcho

clippy_lints/src/declared_lints.rs

@@ -258,7 +258,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::lifetimes::ELIDABLE_LIFETIME_NAMES_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,
@@ -403,6 +402,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::ITER_SKIP_ZERO_INFO,
     crate::methods::ITER_WITH_DRAIN_INFO,
     crate::methods::JOIN_ABSOLUTE_PATHS_INFO,
+    crate::methods::LINES_FILTER_MAP_OK_INFO,
     crate::methods::MANUAL_CONTAINS_INFO,
     crate::methods::MANUAL_C_STR_LITERALS_INFO,
     crate::methods::MANUAL_FILTER_MAP_INFO,

clippy_lints/src/lib.rs

@@ -191,7 +191,6 @@ mod let_if_seq;
 mod let_underscore;
 mod let_with_type_underscore;
 mod lifetimes;
-mod lines_filter_map_ok;
 mod literal_representation;
 mod literal_string_with_formatting_args;
 mod loops;
@@ -742,7 +741,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(conf)));
     store.register_late_pass(|_| Box::new(unnecessary_struct_initialization::UnnecessaryStruct));
     store.register_late_pass(move |_| Box::new(unnecessary_box_returns::UnnecessaryBoxReturns::new(conf)));
-    store.register_late_pass(move |_| Box::new(lines_filter_map_ok::LinesFilterMapOk::new(conf)));
     store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule));
     store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation::new(conf)));
     store.register_early_pass(move || Box::new(excessive_nesting::ExcessiveNesting::new(conf)));

clippy_lints/src/lines_filter_map_ok.rs

@@ -0,0 +1,85 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::msrvs::{self, Msrv};
+use clippy_utils::res::{MaybeDef, MaybeResPath, MaybeTypeckRes};
+use clippy_utils::sym;
+use rustc_errors::Applicability;
+use rustc_hir::{Body, Closure, Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_span::Span;
+
+use super::LINES_FILTER_MAP_OK;
+
+pub(super) fn check_flatten(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, call_span: Span, msrv: Msrv) {
+    if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator)
+        && cx
+            .typeck_results()
+            .expr_ty_adjusted(recv)
+            .is_diag_item(cx, sym::IoLines)
+        && msrv.meets(cx, msrvs::MAP_WHILE)
+    {
+        emit(cx, recv, "flatten", call_span);
+    }
+}
+
+pub(super) fn check_filter_or_flat_map(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    recv: &Expr<'_>,
+    method_name: &'static str,
+    method_arg: &Expr<'_>,
+    call_span: Span,
+    msrv: Msrv,
+) {
+    if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator)
+        && cx
+            .typeck_results()
+            .expr_ty_adjusted(recv)
+            .is_diag_item(cx, sym::IoLines)
+        && match method_arg.kind {
+            // Detect `Result::ok`
+            ExprKind::Path(ref qpath) => cx
+                .qpath_res(qpath, method_arg.hir_id)
+                .is_diag_item(cx, sym::result_ok_method),
+            // Detect `|x| x.ok()`
+            ExprKind::Closure(&Closure { body, .. }) => {
+                if let Body {
+                    params: [param], value, ..
+                } = cx.tcx.hir_body(body)
+                    && let ExprKind::MethodCall(method, receiver, [], _) = value.kind
+                {
+                    method.ident.name == sym::ok
+                        && receiver.res_local_id() == Some(param.pat.hir_id)
+                        && cx.ty_based_def(*value).is_diag_item(cx, sym::result_ok_method)
+                } else {
+                    false
+                }
+            },
+            _ => false,
+        }
+        && msrv.meets(cx, msrvs::MAP_WHILE)
+    {
+        emit(cx, recv, method_name, call_span);
+    }
+}
+
+fn emit(cx: &LateContext<'_>, recv: &Expr<'_>, method_name: &'static str, call_span: Span) {
+    span_lint_and_then(
+        cx,
+        LINES_FILTER_MAP_OK,
+        call_span,
+        format!("`{method_name}()` will run forever if the iterator repeatedly produces an `Err`"),
+        |diag| {
+            diag.span_note(
+                recv.span,
+                "this expression returning a `std::io::Lines` may produce \
+                        an infinite number of `Err` in case of a read error",
+            );
+            diag.span_suggestion(
+                call_span,
+                "replace with",
+                "map_while(Result::ok)",
+                Applicability::MaybeIncorrect,
+            );
+        },
+    );
+}

clippy_lints/src/methods/lines_filter_map_ok.rs

@@ -56,6 +56,7 @@ mod iter_with_drain;
 mod iterator_step_by_zero;
 mod join_absolute_paths;
 mod lib;
+mod lines_filter_map_ok;
 mod manual_c_str_literals;
 mod manual_contains;
 mod manual_inspect;
@@ -4665,6 +4666,55 @@ declare_clippy_lint! {
     "making no use of the \"map closure\" when calling `.map_or_else(|| 2 * k, |n| n)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `lines.filter_map(Result::ok)` or `lines.flat_map(Result::ok)`
+    /// when `lines` has type `std::io::Lines`.
+    ///
+    /// ### Why is this bad?
+    /// `Lines` instances might produce a never-ending stream of `Err`, in which case
+    /// `filter_map(Result::ok)` will enter an infinite loop while waiting for an
+    /// `Ok` variant. Calling `next()` once is sufficient to enter the infinite loop,
+    /// even in the absence of explicit loops in the user code.
+    ///
+    /// This situation can arise when working with user-provided paths. On some platforms,
+    /// `std::fs::File::open(path)` might return `Ok(fs)` even when `path` is a directory,
+    /// but any later attempt to read from `fs` will return an error.
+    ///
+    /// ### Known problems
+    /// This lint suggests replacing `filter_map()` or `flat_map()` applied to a `Lines`
+    /// instance in all cases. There are two cases where the suggestion might not be
+    /// appropriate or necessary:
+    ///
+    /// - If the `Lines` instance can never produce any error, or if an error is produced
+    ///   only once just before terminating the iterator, using `map_while()` is not
+    ///   necessary but will not do any harm.
+    /// - If the `Lines` instance can produce intermittent errors then recover and produce
+    ///   successful results, using `map_while()` would stop at the first error.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # use std::{fs::File, io::{self, BufRead, BufReader}};
+    /// # let _ = || -> io::Result<()> {
+    /// let mut lines = BufReader::new(File::open("some-path")?).lines().filter_map(Result::ok);
+    /// // If "some-path" points to a directory, the next statement never terminates:
+    /// let first_line: Option<String> = lines.next();
+    /// # Ok(()) };
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # use std::{fs::File, io::{self, BufRead, BufReader}};
+    /// # let _ = || -> io::Result<()> {
+    /// let mut lines = BufReader::new(File::open("some-path")?).lines().map_while(Result::ok);
+    /// let first_line: Option<String> = lines.next();
+    /// # Ok(()) };
+    /// ```
+    #[clippy::version = "1.70.0"]
+    pub LINES_FILTER_MAP_OK,
+    suspicious,
+    "filtering `std::io::Lines` with `filter_map()`, `flat_map()`, or `flatten()` might cause an infinite loop"
+}
+
 #[expect(clippy::struct_excessive_bools)]
 pub struct Methods {
     avoid_breaking_exported_api: bool,
@@ -4847,6 +4897,7 @@ impl_lint_pass!(Methods => [
     IP_CONSTANT,
     REDUNDANT_ITER_CLONED,
     UNNECESSARY_OPTION_MAP_OR_ELSE,
+    LINES_FILTER_MAP_OK,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -5169,6 +5220,15 @@ impl Methods {
                     unnecessary_filter_map::check(cx, expr, arg, name);
                     filter_map_bool_then::check(cx, expr, arg, call_span);
                     filter_map_identity::check(cx, expr, arg, span);
+                    lines_filter_map_ok::check_filter_or_flat_map(
+                        cx,
+                        expr,
+                        recv,
+                        "filter_map",
+                        arg,
+                        call_span,
+                        self.msrv,
+                    );
                 },
                 (sym::find_map, [arg]) => {
                     unused_enumerate_index::check(cx, expr, recv, arg);
@@ -5178,20 +5238,26 @@ impl Methods {
                     unused_enumerate_index::check(cx, expr, recv, arg);
                     flat_map_identity::check(cx, expr, arg, span);
                     flat_map_option::check(cx, expr, arg, span);
+                    lines_filter_map_ok::check_filter_or_flat_map(
+                        cx, expr, recv, "flat_map", arg, call_span, self.msrv,
+                    );
                 },
-                (sym::flatten, []) => match method_call(recv) {
-                    Some((sym::map, recv, [map_arg], map_span, _)) => {
-                        map_flatten::check(cx, expr, recv, map_arg, map_span);
-                    },
-                    Some((sym::cloned, recv2, [], _, _)) => iter_overeager_cloned::check(
-                        cx,
-                        expr,
-                        recv,
-                        recv2,
-                        iter_overeager_cloned::Op::LaterCloned,
-                        true,
-                    ),
-                    _ => {},
+                (sym::flatten, []) => {
+                    match method_call(recv) {
+                        Some((sym::map, recv, [map_arg], map_span, _)) => {
+                            map_flatten::check(cx, expr, recv, map_arg, map_span);
+                        },
+                        Some((sym::cloned, recv2, [], _, _)) => iter_overeager_cloned::check(
+                            cx,
+                            expr,
+                            recv,
+                            recv2,
+                            iter_overeager_cloned::Op::LaterCloned,
+                            true,
+                        ),
+                        _ => {},
+                    }
+                    lines_filter_map_ok::check_flatten(cx, expr, recv, call_span, self.msrv);
                 },
                 (sym::fold, [init, acc]) => {
                     manual_try_fold::check(cx, expr, init, acc, call_span, self.msrv);