diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 9d2532f8e47f..af829d0862a1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 96077d0bd00f..2334e195957c 100644 --- a/clippy_lints/src/lib.rs +++ b/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))); diff --git a/clippy_lints/src/lines_filter_map_ok.rs b/clippy_lints/src/lines_filter_map_ok.rs deleted file mode 100644 index 65e922ac07d3..000000000000 --- a/clippy_lints/src/lines_filter_map_ok.rs +++ /dev/null @@ -1,141 +0,0 @@ -use clippy_config::Conf; -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, LateLintPass}; -use rustc_session::impl_lint_pass; -use rustc_span::Symbol; - -pub struct LinesFilterMapOk { - msrv: Msrv, -} - -impl LinesFilterMapOk { - pub fn new(conf: &Conf) -> Self { - Self { msrv: conf.msrv } - } -} - -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 = 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 = 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" -} - -impl_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_args, fm_span) = expr.kind - && cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator) - && let fm_method_name = fm_method.ident.name - && matches!(fm_method_name, sym::filter_map | sym::flat_map | sym::flatten) - && cx - .typeck_results() - .expr_ty_adjusted(fm_receiver) - .is_diag_item(cx, sym::IoLines) - && should_lint(cx, fm_args, fm_method_name) - && self.msrv.meets(cx, msrvs::MAP_WHILE) - { - span_lint_and_then( - cx, - LINES_FILTER_MAP_OK, - fm_span, - format!("`{fm_method_name}()` will run forever if the iterator repeatedly produces an `Err`",), - |diag| { - diag.span_note( - fm_receiver.span, - "this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error"); - diag.span_suggestion( - fm_span, - "replace with", - "map_while(Result::ok)", - Applicability::MaybeIncorrect, - ); - }, - ); - } - } -} - -fn should_lint(cx: &LateContext<'_>, args: &[Expr<'_>], method_name: Symbol) -> bool { - match args { - [] => method_name == sym::flatten, - [fm_arg] => { - match &fm_arg.kind { - // Detect `Result::ok` - ExprKind::Path(qpath) => cx - .qpath_res(qpath, fm_arg.hir_id) - .opt_def_id() - .is_some_and(|did| cx.tcx.is_diagnostic_item(sym::result_ok_method, did)), - // 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 - .typeck_results() - .type_dependent_def_id(value.hir_id) - .opt_parent(cx) - .opt_impl_ty(cx) - .is_diag_item(cx, sym::Result) - } else { - false - } - }, - _ => false, - } - }, - _ => false, - } -} diff --git a/clippy_lints/src/methods/lines_filter_map_ok.rs b/clippy_lints/src/methods/lines_filter_map_ok.rs new file mode 100644 index 000000000000..baf5b6f93f63 --- /dev/null +++ b/clippy_lints/src/methods/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, + ); + }, + ); +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c9066be51c44..54b04a8fff05 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.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 = 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 = 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. @@ -5165,6 +5216,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); @@ -5174,20 +5234,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); diff --git a/tests/ui/lines_filter_map_ok.fixed b/tests/ui/lines_filter_map_ok.fixed index 977e31c744a2..0617f06fafb7 100644 --- a/tests/ui/lines_filter_map_ok.fixed +++ b/tests/ui/lines_filter_map_ok.fixed @@ -1,39 +1,37 @@ -#![allow(unused, clippy::map_identity)] +#![allow(clippy::map_identity)] #![warn(clippy::lines_filter_map_ok)] use std::io::{self, BufRead, BufReader}; fn main() -> io::Result<()> { + // Lint: + let f = std::fs::File::open("/")?; - // Lint BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint let f = std::fs::File::open("/")?; BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint let f = std::fs::File::open("/")?; BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - let s = "foo\nbar\nbaz\n"; - // Lint io::stdin().lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint io::stdin().lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint io::stdin().lines().map_while(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Do not lint (not a `Lines` iterator) + + // Do not lint: + + // not a `Lines` iterator io::stdin() .lines() .map(std::convert::identity) .filter_map(|x| x.ok()) .for_each(|_| ()); - // Do not lint (not a `Result::ok()` extractor) + // not a `Result::ok()` extractor io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ()); Ok(()) } diff --git a/tests/ui/lines_filter_map_ok.rs b/tests/ui/lines_filter_map_ok.rs index 2196075bc445..dfd1ec431a52 100644 --- a/tests/ui/lines_filter_map_ok.rs +++ b/tests/ui/lines_filter_map_ok.rs @@ -1,39 +1,37 @@ -#![allow(unused, clippy::map_identity)] +#![allow(clippy::map_identity)] #![warn(clippy::lines_filter_map_ok)] use std::io::{self, BufRead, BufReader}; fn main() -> io::Result<()> { + // Lint: + let f = std::fs::File::open("/")?; - // Lint BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint let f = std::fs::File::open("/")?; BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint let f = std::fs::File::open("/")?; BufReader::new(f).lines().flatten().for_each(|_| ()); //~^ lines_filter_map_ok - let s = "foo\nbar\nbaz\n"; - // Lint io::stdin().lines().filter_map(Result::ok).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ()); //~^ lines_filter_map_ok - // Lint io::stdin().lines().flatten().for_each(|_| ()); //~^ lines_filter_map_ok - // Do not lint (not a `Lines` iterator) + + // Do not lint: + + // not a `Lines` iterator io::stdin() .lines() .map(std::convert::identity) .filter_map(|x| x.ok()) .for_each(|_| ()); - // Do not lint (not a `Result::ok()` extractor) + // not a `Result::ok()` extractor io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ()); Ok(()) } diff --git a/tests/ui/lines_filter_map_ok.stderr b/tests/ui/lines_filter_map_ok.stderr index f9038eec9fb2..c05dadd1b5f2 100644 --- a/tests/ui/lines_filter_map_ok.stderr +++ b/tests/ui/lines_filter_map_ok.stderr @@ -1,11 +1,11 @@ error: `filter_map()` will run forever if the iterator repeatedly produces an `Err` - --> tests/ui/lines_filter_map_ok.rs:9:31 + --> tests/ui/lines_filter_map_ok.rs:10:31 | LL | BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)` | note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error - --> tests/ui/lines_filter_map_ok.rs:9:5 + --> tests/ui/lines_filter_map_ok.rs:10:5 | LL | BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -25,49 +25,49 @@ LL | BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: `flatten()` will run forever if the iterator repeatedly produces an `Err` - --> tests/ui/lines_filter_map_ok.rs:17:31 + --> tests/ui/lines_filter_map_ok.rs:16:31 | LL | BufReader::new(f).lines().flatten().for_each(|_| ()); | ^^^^^^^^^ help: replace with: `map_while(Result::ok)` | note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error - --> tests/ui/lines_filter_map_ok.rs:17:5 + --> tests/ui/lines_filter_map_ok.rs:16:5 | LL | BufReader::new(f).lines().flatten().for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^^^^ error: `filter_map()` will run forever if the iterator repeatedly produces an `Err` - --> tests/ui/lines_filter_map_ok.rs:22:25 + --> tests/ui/lines_filter_map_ok.rs:19:25 | LL | io::stdin().lines().filter_map(Result::ok).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)` | note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error - --> tests/ui/lines_filter_map_ok.rs:22:5 + --> tests/ui/lines_filter_map_ok.rs:19:5 | LL | io::stdin().lines().filter_map(Result::ok).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^ error: `filter_map()` will run forever if the iterator repeatedly produces an `Err` - --> tests/ui/lines_filter_map_ok.rs:25:25 + --> tests/ui/lines_filter_map_ok.rs:21:25 | LL | io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)` | note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error - --> tests/ui/lines_filter_map_ok.rs:25:5 + --> tests/ui/lines_filter_map_ok.rs:21:5 | LL | io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^ error: `flatten()` will run forever if the iterator repeatedly produces an `Err` - --> tests/ui/lines_filter_map_ok.rs:28:25 + --> tests/ui/lines_filter_map_ok.rs:23:25 | LL | io::stdin().lines().flatten().for_each(|_| ()); | ^^^^^^^^^ help: replace with: `map_while(Result::ok)` | note: this expression returning a `std::io::Lines` may produce an infinite number of `Err` in case of a read error - --> tests/ui/lines_filter_map_ok.rs:28:5 + --> tests/ui/lines_filter_map_ok.rs:23:5 | LL | io::stdin().lines().flatten().for_each(|_| ()); | ^^^^^^^^^^^^^^^^^^^