Skip to content

Commit 81d2300

Browse files
ada4aada4a
committed
move to methods/, for real this time
1 parent 37ebd4c commit 81d2300

File tree

4 files changed

+148
-136
lines changed

4 files changed

+148
-136
lines changed

‎clippy_lints/src/declared_lints.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
258258
crate::lifetimes::ELIDABLE_LIFETIME_NAMES_INFO,
259259
crate::lifetimes::EXTRA_UNUSED_LIFETIMES_INFO,
260260
crate::lifetimes::NEEDLESS_LIFETIMES_INFO,
261-
crate::lines_filter_map_ok::LINES_FILTER_MAP_OK_INFO,
262261
crate::literal_representation::DECIMAL_LITERAL_REPRESENTATION_INFO,
263262
crate::literal_representation::INCONSISTENT_DIGIT_GROUPING_INFO,
264263
crate::literal_representation::LARGE_DIGIT_GROUPS_INFO,
@@ -403,6 +402,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
403402
crate::methods::ITER_SKIP_ZERO_INFO,
404403
crate::methods::ITER_WITH_DRAIN_INFO,
405404
crate::methods::JOIN_ABSOLUTE_PATHS_INFO,
405+
crate::methods::LINES_FILTER_MAP_OK_INFO,
406406
crate::methods::MANUAL_CONTAINS_INFO,
407407
crate::methods::MANUAL_C_STR_LITERALS_INFO,
408408
crate::methods::MANUAL_FILTER_MAP_INFO,

‎clippy_lints/src/lib.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,6 @@ mod let_if_seq;
191191
mod let_underscore;
192192
mod let_with_type_underscore;
193193
mod lifetimes;
194-
mod lines_filter_map_ok;
195194
mod literal_representation;
196195
mod literal_string_with_formatting_args;
197196
mod loops;
@@ -742,7 +741,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
742741
store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(conf)));
743742
store.register_late_pass(|_| Box::new(unnecessary_struct_initialization::UnnecessaryStruct));
744743
store.register_late_pass(move |_| Box::new(unnecessary_box_returns::UnnecessaryBoxReturns::new(conf)));
745-
store.register_late_pass(move |_| Box::new(lines_filter_map_ok::LinesFilterMapOk::new(conf)));
746744
store.register_late_pass(|_| Box::new(tests_outside_test_module::TestsOutsideTestModule));
747745
store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation::new(conf)));
748746
store.register_early_pass(move || Box::new(excessive_nesting::ExcessiveNesting::new(conf)));

‎clippy_lints/src/methods/lines_filter_map_ok.rs‎

Lines changed: 68 additions & 120 deletions
Original file line numberDiff line numberDiff line change
@@ -1,137 +1,85 @@
1-
use clippy_config::Conf;
21
use clippy_utils::diagnostics::span_lint_and_then;
32
use clippy_utils::msrvs::{self, Msrv};
43
use clippy_utils::res::{MaybeDef, MaybeResPath, MaybeTypeckRes};
54
use clippy_utils::sym;
65
use rustc_errors::Applicability;
76
use rustc_hir::{Body, Closure, Expr, ExprKind};
8-
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_session::impl_lint_pass;
10-
use rustc_span::Symbol;
7+
use rustc_lint::LateContext;
8+
use rustc_span::Span;
119

12-
pub struct LinesFilterMapOk {
13-
msrv: Msrv,
14-
}
10+
use super::LINES_FILTER_MAP_OK;
1511

16-
impl LinesFilterMapOk {
17-
pub fn new(conf: &Conf) -> Self {
18-
Self { msrv: conf.msrv }
12+
pub(super) fn check_flatten(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, call_span: Span, msrv: Msrv) {
13+
if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator)
14+
&& cx
15+
.typeck_results()
16+
.expr_ty_adjusted(recv)
17+
.is_diag_item(cx, sym::IoLines)
18+
&& msrv.meets(cx, msrvs::MAP_WHILE)
19+
{
20+
emit(cx, recv, "flatten", call_span);
1921
}
2022
}
2123

22-
declare_clippy_lint! {
23-
/// ### What it does
24-
/// Checks for usage of `lines.filter_map(Result::ok)` or `lines.flat_map(Result::ok)`
25-
/// when `lines` has type `std::io::Lines`.
26-
///
27-
/// ### Why is this bad?
28-
/// `Lines` instances might produce a never-ending stream of `Err`, in which case
29-
/// `filter_map(Result::ok)` will enter an infinite loop while waiting for an
30-
/// `Ok` variant. Calling `next()` once is sufficient to enter the infinite loop,
31-
/// even in the absence of explicit loops in the user code.
32-
///
33-
/// This situation can arise when working with user-provided paths. On some platforms,
34-
/// `std::fs::File::open(path)` might return `Ok(fs)` even when `path` is a directory,
35-
/// but any later attempt to read from `fs` will return an error.
36-
///
37-
/// ### Known problems
38-
/// This lint suggests replacing `filter_map()` or `flat_map()` applied to a `Lines`
39-
/// instance in all cases. There are two cases where the suggestion might not be
40-
/// appropriate or necessary:
41-
///
42-
/// - If the `Lines` instance can never produce any error, or if an error is produced
43-
/// only once just before terminating the iterator, using `map_while()` is not
44-
/// necessary but will not do any harm.
45-
/// - If the `Lines` instance can produce intermittent errors then recover and produce
46-
/// successful results, using `map_while()` would stop at the first error.
47-
///
48-
/// ### Example
49-
/// ```no_run
50-
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
51-
/// # let _ = || -> io::Result<()> {
52-
/// let mut lines = BufReader::new(File::open("some-path")?).lines().filter_map(Result::ok);
53-
/// // If "some-path" points to a directory, the next statement never terminates:
54-
/// let first_line: Option<String> = lines.next();
55-
/// # Ok(()) };
56-
/// ```
57-
/// Use instead:
58-
/// ```no_run
59-
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
60-
/// # let _ = || -> io::Result<()> {
61-
/// let mut lines = BufReader::new(File::open("some-path")?).lines().map_while(Result::ok);
62-
/// let first_line: Option<String> = lines.next();
63-
/// # Ok(()) };
64-
/// ```
65-
#[clippy::version = "1.70.0"]
66-
pub LINES_FILTER_MAP_OK,
67-
suspicious,
68-
"filtering `std::io::Lines` with `filter_map()`, `flat_map()`, or `flatten()` might cause an infinite loop"
69-
}
70-
71-
impl_lint_pass!(LinesFilterMapOk => [LINES_FILTER_MAP_OK]);
72-
73-
impl LateLintPass<'_> for LinesFilterMapOk {
74-
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
75-
if let ExprKind::MethodCall(fm_method, fm_receiver, fm_args, fm_span) = expr.kind
76-
&& cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator)
77-
&& let fm_method_name = fm_method.ident.name
78-
&& matches!(fm_method_name, sym::filter_map | sym::flat_map | sym::flatten)
79-
&& cx
80-
.typeck_results()
81-
.expr_ty_adjusted(fm_receiver)
82-
.is_diag_item(cx, sym::IoLines)
83-
&& should_lint(cx, fm_args, fm_method_name)
84-
&& self.msrv.meets(cx, msrvs::MAP_WHILE)
85-
{
86-
span_lint_and_then(
87-
cx,
88-
LINES_FILTER_MAP_OK,
89-
fm_span,
90-
format!("`{fm_method_name}()` will run forever if the iterator repeatedly produces an `Err`"),
91-
|diag| {
92-
diag.span_note(
93-
fm_receiver.span,
94-
"this expression returning a `std::io::Lines` may produce \
95-
an infinite number of `Err` in case of a read error",
96-
);
97-
diag.span_suggestion(
98-
fm_span,
99-
"replace with",
100-
"map_while(Result::ok)",
101-
Applicability::MaybeIncorrect,
102-
);
103-
},
104-
);
24+
pub(super) fn check_filter_or_flat_map(
25+
cx: &LateContext<'_>,
26+
expr: &Expr<'_>,
27+
recv: &Expr<'_>,
28+
method_name: &'static str,
29+
method_arg: &Expr<'_>,
30+
call_span: Span,
31+
msrv: Msrv,
32+
) {
33+
if cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator)
34+
&& cx
35+
.typeck_results()
36+
.expr_ty_adjusted(recv)
37+
.is_diag_item(cx, sym::IoLines)
38+
&& match method_arg.kind {
39+
// Detect `Result::ok`
40+
ExprKind::Path(ref qpath) => cx
41+
.qpath_res(qpath, method_arg.hir_id)
42+
.is_diag_item(cx, sym::result_ok_method),
43+
// Detect `|x| x.ok()`
44+
ExprKind::Closure(&Closure { body, .. }) => {
45+
if let Body {
46+
params: [param], value, ..
47+
} = cx.tcx.hir_body(body)
48+
&& let ExprKind::MethodCall(method, receiver, [], _) = value.kind
49+
{
50+
method.ident.name == sym::ok
51+
&& receiver.res_local_id() == Some(param.pat.hir_id)
52+
&& cx.ty_based_def(*value).is_diag_item(cx, sym::result_ok_method)
53+
} else {
54+
false
55+
}
56+
},
57+
_ => false,
10558
}
59+
&& msrv.meets(cx, msrvs::MAP_WHILE)
60+
{
61+
emit(cx, recv, method_name, call_span);
10662
}
10763
}
10864

109-
fn should_lint(cx: &LateContext<'_>, args: &[Expr<'_>], method_name: Symbol) -> bool {
110-
match args {
111-
[] => method_name == sym::flatten,
112-
[fm_arg] => {
113-
match fm_arg.kind {
114-
// Detect `Result::ok`
115-
ExprKind::Path(ref qpath) => cx
116-
.qpath_res(qpath, fm_arg.hir_id)
117-
.is_diag_item(cx, sym::result_ok_method),
118-
// Detect `|x| x.ok()`
119-
ExprKind::Closure(&Closure { body, .. }) => {
120-
if let Body {
121-
params: [param], value, ..
122-
} = cx.tcx.hir_body(body)
123-
&& let ExprKind::MethodCall(method, receiver, [], _) = value.kind
124-
{
125-
method.ident.name == sym::ok
126-
&& receiver.res_local_id() == Some(param.pat.hir_id)
127-
&& cx.ty_based_def(*value).is_diag_item(cx, sym::result_ok_method)
128-
} else {
129-
false
130-
}
131-
},
132-
_ => false,
133-
}
65+
fn emit(cx: &LateContext<'_>, recv: &Expr<'_>, method_name: &'static str, call_span: Span) {
66+
span_lint_and_then(
67+
cx,
68+
LINES_FILTER_MAP_OK,
69+
call_span,
70+
format!("`{method_name}()` will run forever if the iterator repeatedly produces an `Err`"),
71+
|diag| {
72+
diag.span_note(
73+
recv.span,
74+
"this expression returning a `std::io::Lines` may produce \
75+
an infinite number of `Err` in case of a read error",
76+
);
77+
diag.span_suggestion(
78+
call_span,
79+
"replace with",
80+
"map_while(Result::ok)",
81+
Applicability::MaybeIncorrect,
82+
);
13483
},
135-
_ => false,
136-
}
84+
);
13785
}

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 79 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ mod iter_with_drain;
5656
mod iterator_step_by_zero;
5757
mod join_absolute_paths;
5858
mod lib;
59+
mod lines_filter_map_ok;
5960
mod manual_c_str_literals;
6061
mod manual_contains;
6162
mod manual_inspect;
@@ -4665,6 +4666,55 @@ declare_clippy_lint! {
46654666
"making no use of the \"map closure\" when calling `.map_or_else(|| 2 * k, |n| n)`"
46664667
}
46674668

4669+
declare_clippy_lint! {
4670+
/// ### What it does
4671+
/// Checks for usage of `lines.filter_map(Result::ok)` or `lines.flat_map(Result::ok)`
4672+
/// when `lines` has type `std::io::Lines`.
4673+
///
4674+
/// ### Why is this bad?
4675+
/// `Lines` instances might produce a never-ending stream of `Err`, in which case
4676+
/// `filter_map(Result::ok)` will enter an infinite loop while waiting for an
4677+
/// `Ok` variant. Calling `next()` once is sufficient to enter the infinite loop,
4678+
/// even in the absence of explicit loops in the user code.
4679+
///
4680+
/// This situation can arise when working with user-provided paths. On some platforms,
4681+
/// `std::fs::File::open(path)` might return `Ok(fs)` even when `path` is a directory,
4682+
/// but any later attempt to read from `fs` will return an error.
4683+
///
4684+
/// ### Known problems
4685+
/// This lint suggests replacing `filter_map()` or `flat_map()` applied to a `Lines`
4686+
/// instance in all cases. There are two cases where the suggestion might not be
4687+
/// appropriate or necessary:
4688+
///
4689+
/// - If the `Lines` instance can never produce any error, or if an error is produced
4690+
/// only once just before terminating the iterator, using `map_while()` is not
4691+
/// necessary but will not do any harm.
4692+
/// - If the `Lines` instance can produce intermittent errors then recover and produce
4693+
/// successful results, using `map_while()` would stop at the first error.
4694+
///
4695+
/// ### Example
4696+
/// ```no_run
4697+
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
4698+
/// # let _ = || -> io::Result<()> {
4699+
/// let mut lines = BufReader::new(File::open("some-path")?).lines().filter_map(Result::ok);
4700+
/// // If "some-path" points to a directory, the next statement never terminates:
4701+
/// let first_line: Option<String> = lines.next();
4702+
/// # Ok(()) };
4703+
/// ```
4704+
/// Use instead:
4705+
/// ```no_run
4706+
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
4707+
/// # let _ = || -> io::Result<()> {
4708+
/// let mut lines = BufReader::new(File::open("some-path")?).lines().map_while(Result::ok);
4709+
/// let first_line: Option<String> = lines.next();
4710+
/// # Ok(()) };
4711+
/// ```
4712+
#[clippy::version = "1.70.0"]
4713+
pub LINES_FILTER_MAP_OK,
4714+
suspicious,
4715+
"filtering `std::io::Lines` with `filter_map()`, `flat_map()`, or `flatten()` might cause an infinite loop"
4716+
}
4717+
46684718
#[expect(clippy::struct_excessive_bools)]
46694719
pub struct Methods {
46704720
avoid_breaking_exported_api: bool,
@@ -4847,6 +4897,7 @@ impl_lint_pass!(Methods => [
48474897
IP_CONSTANT,
48484898
REDUNDANT_ITER_CLONED,
48494899
UNNECESSARY_OPTION_MAP_OR_ELSE,
4900+
LINES_FILTER_MAP_OK,
48504901
]);
48514902

48524903
/// Extracts a method call name, args, and `Span` of the method name.
@@ -5165,6 +5216,15 @@ impl Methods {
51655216
unnecessary_filter_map::check(cx, expr, arg, name);
51665217
filter_map_bool_then::check(cx, expr, arg, call_span);
51675218
filter_map_identity::check(cx, expr, arg, span);
5219+
lines_filter_map_ok::check_filter_or_flat_map(
5220+
cx,
5221+
expr,
5222+
recv,
5223+
"filter_map",
5224+
arg,
5225+
call_span,
5226+
self.msrv,
5227+
);
51685228
},
51695229
(sym::find_map, [arg]) => {
51705230
unused_enumerate_index::check(cx, expr, recv, arg);
@@ -5174,20 +5234,26 @@ impl Methods {
51745234
unused_enumerate_index::check(cx, expr, recv, arg);
51755235
flat_map_identity::check(cx, expr, arg, span);
51765236
flat_map_option::check(cx, expr, arg, span);
5237+
lines_filter_map_ok::check_filter_or_flat_map(
5238+
cx, expr, recv, "flat_map", arg, call_span, self.msrv,
5239+
);
51775240
},
5178-
(sym::flatten, []) => match method_call(recv) {
5179-
Some((sym::map, recv, [map_arg], map_span, _)) => {
5180-
map_flatten::check(cx, expr, recv, map_arg, map_span);
5181-
},
5182-
Some((sym::cloned, recv2, [], _, _)) => iter_overeager_cloned::check(
5183-
cx,
5184-
expr,
5185-
recv,
5186-
recv2,
5187-
iter_overeager_cloned::Op::LaterCloned,
5188-
true,
5189-
),
5190-
_ => {},
5241+
(sym::flatten, []) => {
5242+
match method_call(recv) {
5243+
Some((sym::map, recv, [map_arg], map_span, _)) => {
5244+
map_flatten::check(cx, expr, recv, map_arg, map_span);
5245+
},
5246+
Some((sym::cloned, recv2, [], _, _)) => iter_overeager_cloned::check(
5247+
cx,
5248+
expr,
5249+
recv,
5250+
recv2,
5251+
iter_overeager_cloned::Op::LaterCloned,
5252+
true,
5253+
),
5254+
_ => {},
5255+
}
5256+
lines_filter_map_ok::check_flatten(cx, expr, recv, call_span, self.msrv);
51915257
},
51925258
(sym::fold, [init, acc]) => {
51935259
manual_try_fold::check(cx, expr, init, acc, call_span, self.msrv);

0 commit comments

Comments
 (0)