Skip to content

Commit d04dab1

Browse files
committed
Flag bufreader.lines().filter_map(Result::ok) as suspicious
1 parent 00e9372 commit d04dab1

File tree

8 files changed

+208
-0
lines changed

8 files changed

+208
-0
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -4645,6 +4645,7 @@ Released 2018-09-13
46454645
[`let_underscore_untyped`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_untyped
46464646
[`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value
46474647
[`let_with_type_underscore`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_with_type_underscore
4648+
[`lines_filter_map_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok
46484649
[`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist
46494650
[`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
46504651
[`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal

clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
231231
crate::let_with_type_underscore::LET_WITH_TYPE_UNDERSCORE_INFO,
232232
crate::lifetimes::EXTRA_UNUSED_LIFETIMES_INFO,
233233
crate::lifetimes::NEEDLESS_LIFETIMES_INFO,
234+
crate::lines_filter_map_ok::LINES_FILTER_MAP_OK_INFO,
234235
crate::literal_representation::DECIMAL_LITERAL_REPRESENTATION_INFO,
235236
crate::literal_representation::INCONSISTENT_DIGIT_GROUPING_INFO,
236237
crate::literal_representation::LARGE_DIGIT_GROUPS_INFO,

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ mod let_if_seq;
170170
mod let_underscore;
171171
mod let_with_type_underscore;
172172
mod lifetimes;
173+
mod lines_filter_map_ok;
173174
mod literal_representation;
174175
mod loops;
175176
mod macro_use;
@@ -940,6 +941,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
940941
store.register_late_pass(|_| Box::new(allow_attributes::AllowAttribute));
941942
store.register_late_pass(move |_| Box::new(manual_main_separator_str::ManualMainSeparatorStr::new(msrv())));
942943
store.register_late_pass(|_| Box::new(unnecessary_struct_initialization::UnnecessaryStruct));
944+
store.register_late_pass(|_| Box::new(lines_filter_map_ok::LinesFilterMapOk));
943945
// add lints here, do not remove this comment, it's used in `new_lint`
944946
}
945947

+93
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
use clippy_utils::{
2+
diagnostics::span_lint_and_then, is_diag_item_method, is_trait_method, match_def_path, path_to_local_id, paths,
3+
ty::match_type,
4+
};
5+
use rustc_errors::Applicability;
6+
use rustc_hir::{Body, Closure, Expr, ExprKind};
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_session::{declare_lint_pass, declare_tool_lint};
9+
use rustc_span::sym;
10+
11+
declare_clippy_lint! {
12+
/// ### What it does
13+
/// Detect uses of `b.lines().filter_map(Result::ok)` or `b.lines().flat_map(Result::ok)`
14+
/// when `b` implements `BufRead`.
15+
///
16+
/// ### Why is this bad?
17+
/// `b.lines()` might produce an infinite stream of `Err` and `filter_map(Result::ok)`
18+
/// will also run forever. Using `map_while(Result::ok)` would stop after the first
19+
/// `Err` is emitted.
20+
///
21+
/// For example, on some platforms, `std::fs::File::open(path)` might return `Ok(fs)`
22+
/// when `path` is a directory, and reading from `fs` will then return an error.
23+
/// If `fs` was inadvertently put in a `BufReader`, calling `lines()` on it would
24+
/// return an infinite stream of `Err` variants.
25+
///
26+
/// ### Example
27+
/// ```rust
28+
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
29+
/// # let _ = || -> io::Result<()> {
30+
/// let reader = BufReader::new(File::open("some path")?);
31+
/// for line in reader.lines().flat_map(Result::ok) {
32+
/// println!("{line}");
33+
/// }
34+
/// # Ok(()) };
35+
/// ```
36+
/// Use instead:
37+
/// ```rust
38+
/// # use std::{fs::File, io::{self, BufRead, BufReader}};
39+
/// # let _ = || -> io::Result<()> {
40+
/// let reader = BufReader::new(File::open("some path")?);
41+
/// for line in reader.lines().map_while(Result::ok) {
42+
/// println!("{line}");
43+
/// }
44+
/// # Ok(()) };
45+
/// ```
46+
#[clippy::version = "1.70.0"]
47+
pub LINES_FILTER_MAP_OK,
48+
nursery,
49+
"`BufRead::lines()` might produce an infinite stream of errors"
50+
}
51+
declare_lint_pass!(LinesFilterMapOk => [LINES_FILTER_MAP_OK]);
52+
53+
impl LateLintPass<'_> for LinesFilterMapOk {
54+
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
55+
if let ExprKind::MethodCall(fm_method, fm_receiver, [fm_arg], fm_span) = expr.kind &&
56+
is_trait_method(cx, expr, sym::Iterator) &&
57+
(fm_method.ident.as_str() == "filter_map" || fm_method.ident.as_str() == "flat_map") &&
58+
match_type(cx, cx.typeck_results().expr_ty_adjusted(fm_receiver), &paths::STD_IO_LINES)
59+
{
60+
let lint = match &fm_arg.kind {
61+
// Detect `Result::ok`
62+
ExprKind::Path(qpath) =>
63+
cx.qpath_res(qpath, fm_arg.hir_id).opt_def_id().map(|did|
64+
match_def_path(cx, did, &paths::CORE_RESULT_OK_METHOD)).unwrap_or_default(),
65+
// Detect `|x| x.ok()`
66+
ExprKind::Closure(Closure { body, .. }) =>
67+
if let Body { params: [param], value, .. } = cx.tcx.hir().body(*body) &&
68+
let ExprKind::MethodCall(method, receiver, [], _) = value.kind &&
69+
path_to_local_id(receiver, param.pat.hir_id) &&
70+
let Some(method_did) = cx.typeck_results().type_dependent_def_id(value.hir_id)
71+
{
72+
is_diag_item_method(cx, method_did, sym::Result) && method.ident.as_str() == "ok"
73+
} else {
74+
false
75+
}
76+
_ => false,
77+
};
78+
if !lint {
79+
return;
80+
}
81+
span_lint_and_then(cx,
82+
LINES_FILTER_MAP_OK,
83+
fm_span,
84+
&format!("`{}()` will run forever on an infinite stream of `Err` returned by `lines()`", fm_method.ident),
85+
|diag| {
86+
diag.span_note(
87+
fm_receiver.span,
88+
"This expression returning `std::io::Lines` may produce an infinite stream of `Err` in case of a read error");
89+
diag.span_suggestion(fm_span, "replace with", "map_while(Result::ok)", Applicability::MaybeIncorrect);
90+
});
91+
}
92+
}
93+
}

clippy_utils/src/paths.rs

+2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub const CLONE_TRAIT_METHOD: [&str; 4] = ["core", "clone", "Clone", "clone"];
2323
pub const CORE_ITER_CLONED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "cloned"];
2424
pub const CORE_ITER_COPIED: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "copied"];
2525
pub const CORE_ITER_FILTER: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "filter"];
26+
pub const CORE_RESULT_OK_METHOD: [&str; 4] = ["core", "result", "Result", "ok"];
2627
pub const CSTRING_AS_C_STR: [&str; 5] = ["alloc", "ffi", "c_str", "CString", "as_c_str"];
2728
pub const DEFAULT_TRAIT_METHOD: [&str; 4] = ["core", "default", "Default", "default"];
2829
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"];
113114
pub const STDOUT: [&str; 4] = ["std", "io", "stdio", "stdout"];
114115
pub const CONVERT_IDENTITY: [&str; 3] = ["core", "convert", "identity"];
115116
pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"];
117+
pub const STD_IO_LINES: [&str; 3] = ["std", "io", "Lines"];
116118
pub const STD_IO_SEEK: [&str; 3] = ["std", "io", "Seek"];
117119
pub const STD_IO_SEEK_FROM_CURRENT: [&str; 4] = ["std", "io", "SeekFrom", "Current"];
118120
pub const STD_IO_SEEKFROM_START: [&str; 4] = ["std", "io", "SeekFrom", "Start"];

tests/ui/lines_filter_map_ok.fixed

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::map_identity)]
4+
#![warn(clippy::lines_filter_map_ok)]
5+
6+
use std::io::{self, BufRead, BufReader};
7+
8+
fn main() -> io::Result<()> {
9+
let f = std::fs::File::open("/")?;
10+
// Lint
11+
BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ());
12+
// Lint
13+
let f = std::fs::File::open("/")?;
14+
BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ());
15+
let s = "foo\nbar\nbaz\n";
16+
// Lint
17+
io::stdin().lines().map_while(Result::ok).for_each(|_| ());
18+
// Lint
19+
io::stdin().lines().map_while(Result::ok).for_each(|_| ());
20+
// Do not lint (not a `Lines` iterator)
21+
io::stdin()
22+
.lines()
23+
.map(std::convert::identity)
24+
.filter_map(|x| x.ok())
25+
.for_each(|_| ());
26+
// Do not lint (not a `Result::ok()` extractor)
27+
io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ());
28+
Ok(())
29+
}

tests/ui/lines_filter_map_ok.rs

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// run-rustfix
2+
3+
#![allow(unused, clippy::map_identity)]
4+
#![warn(clippy::lines_filter_map_ok)]
5+
6+
use std::io::{self, BufRead, BufReader};
7+
8+
fn main() -> io::Result<()> {
9+
let f = std::fs::File::open("/")?;
10+
// Lint
11+
BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ());
12+
// Lint
13+
let f = std::fs::File::open("/")?;
14+
BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ());
15+
let s = "foo\nbar\nbaz\n";
16+
// Lint
17+
io::stdin().lines().filter_map(Result::ok).for_each(|_| ());
18+
// Lint
19+
io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ());
20+
// Do not lint (not a `Lines` iterator)
21+
io::stdin()
22+
.lines()
23+
.map(std::convert::identity)
24+
.filter_map(|x| x.ok())
25+
.for_each(|_| ());
26+
// Do not lint (not a `Result::ok()` extractor)
27+
io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ());
28+
Ok(())
29+
}

tests/ui/lines_filter_map_ok.stderr

+51
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
error: `filter_map()` will run forever on an infinite stream of `Err` returned by `lines()`
2+
--> $DIR/lines_filter_map_ok.rs:11:31
3+
|
4+
LL | BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ());
5+
| ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
6+
|
7+
note: This expression returning `std::io::Lines` may produce an infinite stream of `Err` in case of a read error
8+
--> $DIR/lines_filter_map_ok.rs:11:5
9+
|
10+
LL | BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ());
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^
12+
= note: `-D clippy::lines-filter-map-ok` implied by `-D warnings`
13+
14+
error: `flat_map()` will run forever on an infinite stream of `Err` returned by `lines()`
15+
--> $DIR/lines_filter_map_ok.rs:14:31
16+
|
17+
LL | BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ());
18+
| ^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
19+
|
20+
note: This expression returning `std::io::Lines` may produce an infinite stream of `Err` in case of a read error
21+
--> $DIR/lines_filter_map_ok.rs:14:5
22+
|
23+
LL | BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ());
24+
| ^^^^^^^^^^^^^^^^^^^^^^^^^
25+
26+
error: `filter_map()` will run forever on an infinite stream of `Err` returned by `lines()`
27+
--> $DIR/lines_filter_map_ok.rs:17:25
28+
|
29+
LL | io::stdin().lines().filter_map(Result::ok).for_each(|_| ());
30+
| ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
31+
|
32+
note: This expression returning `std::io::Lines` may produce an infinite stream of `Err` in case of a read error
33+
--> $DIR/lines_filter_map_ok.rs:17:5
34+
|
35+
LL | io::stdin().lines().filter_map(Result::ok).for_each(|_| ());
36+
| ^^^^^^^^^^^^^^^^^^^
37+
38+
error: `filter_map()` will run forever on an infinite stream of `Err` returned by `lines()`
39+
--> $DIR/lines_filter_map_ok.rs:19:25
40+
|
41+
LL | io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ());
42+
| ^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `map_while(Result::ok)`
43+
|
44+
note: This expression returning `std::io::Lines` may produce an infinite stream of `Err` in case of a read error
45+
--> $DIR/lines_filter_map_ok.rs:19:5
46+
|
47+
LL | io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ());
48+
| ^^^^^^^^^^^^^^^^^^^
49+
50+
error: aborting due to 4 previous errors
51+

0 commit comments

Comments
 (0)