Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-27 15:11:30 +00:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #10534 - samueltardieu:lines-filter-map-ok, r=llogiq

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

This lint detects a problem that happened recently in https://github.com/uutils/coreutils and is described in https://github.com/rust-lang/rust/issues/64144.

changelog: [`lines_filter_map_ok`]: new lint
This commit is contained in:
bors 2023-04-01 12:41:08 +00:00
commit ac4838c554
8 changed files with 215 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -4647,6 +4647,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

1
clippy_lints/src/declared_lints.rs
View file

@ -232,6 +232,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,

2
clippy_lints/src/lib.rs
View file

@ -171,6 +171,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;
@ -949,6 +950,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
avoid_breaking_exported_api,
))
});
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`
}

100
clippy_lints/src/lines_filter_map_ok.rs Normal file
View file

@ -0,0 +1,100 @@
use clippy_utils::{
diagnostics::span_lint_and_then, is_diag_item_method, is_trait_method, match_def_path, path_to_local_id, paths,
ty::match_type,
};
use rustc_errors::Applicability;
use rustc_hir::{Body, Closure, 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 `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 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
/// ```rust
/// # 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:
/// ```rust
/// # 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()` or `flat_map()` might cause an infinite loop"
}
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") &&
match_type(cx, cx.typeck_results().expr_ty_adjusted(fm_receiver), &paths::STD_IO_LINES)
{
let lint = match &fm_arg.kind {
// Detect `Result::ok`
ExprKind::Path(qpath) =>
cx.qpath_res(qpath, fm_arg.hir_id).opt_def_id().map(|did|
match_def_path(cx, did, &paths::CORE_RESULT_OK_METHOD)).unwrap_or_default(),
// 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 &&
path_to_local_id(receiver, param.pat.hir_id) &&
let Some(method_did) = cx.typeck_results().type_dependent_def_id(value.hir_id)
{
is_diag_item_method(cx, method_did, sym::Result) && method.ident.as_str() == "ok"
} else {
false
}
_ => false,
};
if lint {
span_lint_and_then(cx,
LINES_FILTER_MAP_OK,
fm_span,
&format!("`{}()` will run forever if the iterator repeatedly produces an `Err`", fm_method.ident),
|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);
});
}
}
}
}

2
clippy_utils/src/paths.rs
View file

@ -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_LINES: [&str; 3] = ["std", "io", "Lines"];
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"];

29
tests/ui/lines_filter_map_ok.fixed Normal file
View file

@ -0,0 +1,29 @@
// run-rustfix
#![allow(unused, clippy::map_identity)]
#![warn(clippy::lines_filter_map_ok)]
use std::io::{self, BufRead, BufReader};
fn main() -> io::Result<()> {
let f = std::fs::File::open("/")?;
// Lint
BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ());
// Lint
let f = std::fs::File::open("/")?;
BufReader::new(f).lines().map_while(Result::ok).for_each(|_| ());
let s = "foo\nbar\nbaz\n";
// Lint
io::stdin().lines().map_while(Result::ok).for_each(|_| ());
// Lint
io::stdin().lines().map_while(Result::ok).for_each(|_| ());
// 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)
io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ());
Ok(())
}

29
tests/ui/lines_filter_map_ok.rs Normal file
View file

@ -0,0 +1,29 @@
// run-rustfix
#![allow(unused, clippy::map_identity)]
#![warn(clippy::lines_filter_map_ok)]
use std::io::{self, BufRead, BufReader};
fn main() -> io::Result<()> {
let f = std::fs::File::open("/")?;
// Lint
BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ());
// Lint
let f = std::fs::File::open("/")?;
BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ());
let s = "foo\nbar\nbaz\n";
// Lint
io::stdin().lines().filter_map(Result::ok).for_each(|_| ());
// Lint
io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ());
// 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)
io::stdin().lines().filter_map(|x| x.err()).for_each(|_| ());
Ok(())
}

51
tests/ui/lines_filter_map_ok.stderr Normal file
View file

@ -0,0 +1,51 @@
error: `filter_map()` will run forever if the iterator repeatedly produces an `Err`
--> $DIR/lines_filter_map_ok.rs:11: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
--> $DIR/lines_filter_map_ok.rs:11:5
|
LL | BufReader::new(f).lines().filter_map(Result::ok).for_each(|_| ());
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= note: `-D clippy::lines-filter-map-ok` implied by `-D warnings`
error: `flat_map()` will run forever if the iterator repeatedly produces an `Err`
--> $DIR/lines_filter_map_ok.rs:14:31
|
LL | BufReader::new(f).lines().flat_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
--> $DIR/lines_filter_map_ok.rs:14:5
|
LL | BufReader::new(f).lines().flat_map(Result::ok).for_each(|_| ());
| ^^^^^^^^^^^^^^^^^^^^^^^^^
error: `filter_map()` will run forever if the iterator repeatedly produces an `Err`
--> $DIR/lines_filter_map_ok.rs:17: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
--> $DIR/lines_filter_map_ok.rs:17:5
|
LL | io::stdin().lines().filter_map(Result::ok).for_each(|_| ());
| ^^^^^^^^^^^^^^^^^^^
error: `filter_map()` will run forever if the iterator repeatedly produces an `Err`
--> $DIR/lines_filter_map_ok.rs:19: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
--> $DIR/lines_filter_map_ok.rs:19:5
|
LL | io::stdin().lines().filter_map(|x| x.ok()).for_each(|_| ());
| ^^^^^^^^^^^^^^^^^^^
error: aborting due to 4 previous errors