Skip to content

Commit 8892420

Browse files
PartiallyUntypedAlexendoo
PartiallyUntyped
and
Alexendoo
committed
New Lint: Result_filter_map
Added the `Result` mirror of `option_filter_map` to catch ``` .into_iter().filter(Result::is_ok).map(Result::unwrap) ``` changelog: New Lint: [`result_filter_map`] Co-authored-by: Alex Macleod <[email protected]>
1 parent 29bdc8b commit 8892420

File tree

10 files changed

+217
-31
lines changed

10 files changed

+217
-31
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5470,6 +5470,7 @@ Released 2018-09-13
54705470
[`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization
54715471
[`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
54725472
[`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
5473+
[`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map
54735474
[`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
54745475
[`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
54755476
[`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn

‎clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
419419
crate::methods::READ_LINE_WITHOUT_TRIM_INFO,
420420
crate::methods::REDUNDANT_AS_STR_INFO,
421421
crate::methods::REPEAT_ONCE_INFO,
422+
crate::methods::RESULT_FILTER_MAP_INFO,
422423
crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO,
423424
crate::methods::SEARCH_IS_SOME_INFO,
424425
crate::methods::SEEK_FROM_CURRENT_INFO,

‎clippy_lints/src/methods/filter_map.rs

Lines changed: 63 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustc_span::symbol::{sym, Ident, Symbol};
1414
use rustc_span::Span;
1515
use std::borrow::Cow;
1616

17-
use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP};
17+
use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP, RESULT_FILTER_MAP};
1818

1919
fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool {
2020
match &expr.kind {
@@ -46,6 +46,9 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) ->
4646
fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
4747
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
4848
}
49+
fn is_ok_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
50+
is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_ok))
51+
}
4952

5053
#[derive(Debug, Copy, Clone)]
5154
enum OffendingFilterExpr<'tcx> {
@@ -273,6 +276,18 @@ fn is_filter_some_map_unwrap(
273276
(iterator || option) && is_option_filter_map(cx, filter_arg, map_arg)
274277
}
275278

279+
/// is `filter(|x| x.is_ok()).map(|x| x.unwrap())`
280+
fn is_filter_ok_map_unwrap(
281+
cx: &LateContext<'_>,
282+
expr: &hir::Expr<'_>,
283+
filter_arg: &hir::Expr<'_>,
284+
map_arg: &hir::Expr<'_>,
285+
) -> bool {
286+
// result has no filter, so we only check for iterators
287+
let iterator = is_trait_method(cx, expr, sym::Iterator);
288+
iterator && is_ok_filter_map(cx, filter_arg, map_arg)
289+
}
290+
276291
/// lint use of `filter().map()` or `find().map()` for `Iterators`
277292
#[allow(clippy::too_many_arguments)]
278293
pub(super) fn check(
@@ -300,30 +315,21 @@ pub(super) fn check(
300315
return;
301316
}
302317

303-
if is_trait_method(cx, map_recv, sym::Iterator)
304-
305-
// filter(|x| ...is_some())...
306-
&& let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
307-
&& let filter_body = cx.tcx.hir().body(filter_body_id)
308-
&& let [filter_param] = filter_body.params
309-
// optional ref pattern: `filter(|&x| ..)`
310-
&& let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
311-
(ref_pat, true)
312-
} else {
313-
(filter_param.pat, false)
314-
}
315-
316-
&& let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
317-
&& let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
318+
if is_filter_ok_map_unwrap(cx, expr, filter_arg, map_arg) {
319+
span_lint_and_sugg(
320+
cx,
321+
RESULT_FILTER_MAP,
322+
filter_span.with_hi(expr.span.hi()),
323+
"`filter` for `Ok` followed by `unwrap`",
324+
"consider using `flatten` instead",
325+
reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(),
326+
Applicability::MachineApplicable,
327+
);
318328

319-
&& let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
320-
&& let map_body = cx.tcx.hir().body(map_body_id)
321-
&& let [map_param] = map_body.params
322-
&& let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
329+
return;
330+
}
323331

324-
&& let Some(check_result) =
325-
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
326-
{
332+
if let Some((map_param_ident, check_result)) = is_find_or_filter(cx, map_recv, filter_arg, map_arg) {
327333
let span = filter_span.with_hi(expr.span.hi());
328334
let (filter_name, lint) = if is_find {
329335
("find", MANUAL_FIND_MAP)
@@ -395,6 +401,40 @@ pub(super) fn check(
395401
}
396402
}
397403

404+
fn is_find_or_filter<'a>(
405+
cx: &LateContext<'a>,
406+
map_recv: &hir::Expr<'_>,
407+
filter_arg: &hir::Expr<'_>,
408+
map_arg: &hir::Expr<'_>,
409+
) -> Option<(Ident, CheckResult<'a>)> {
410+
if is_trait_method(cx, map_recv, sym::Iterator)
411+
// filter(|x| ...is_some())...
412+
&& let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
413+
&& let filter_body = cx.tcx.hir().body(filter_body_id)
414+
&& let [filter_param] = filter_body.params
415+
// optional ref pattern: `filter(|&x| ..)`
416+
&& let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
417+
(ref_pat, true)
418+
} else {
419+
(filter_param.pat, false)
420+
}
421+
422+
&& let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
423+
&& let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
424+
425+
&& let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
426+
&& let map_body = cx.tcx.hir().body(map_body_id)
427+
&& let [map_param] = map_body.params
428+
&& let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
429+
430+
&& let Some(check_result) =
431+
offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
432+
{
433+
return Some((map_param_ident, check_result));
434+
}
435+
None
436+
}
437+
398438
fn acceptable_methods(method: &PathSegment<'_>) -> bool {
399439
let methods: [Symbol; 8] = [
400440
sym::clone,

‎clippy_lints/src/methods/mod.rs

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1175,7 +1175,8 @@ declare_clippy_lint! {
11751175

11761176
declare_clippy_lint! {
11771177
/// ### What it does
1178-
/// Checks for indirect collection of populated `Option`
1178+
/// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may
1179+
/// be replaced with a `.flatten()` call.
11791180
///
11801181
/// ### Why is this bad?
11811182
/// `Option` is like a collection of 0-1 things, so `flatten`
@@ -3752,6 +3753,29 @@ declare_clippy_lint! {
37523753
"using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`"
37533754
}
37543755

3756+
declare_clippy_lint! {
3757+
/// ### What it does
3758+
/// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may
3759+
/// be replaced with a `.flatten()` call.
3760+
///
3761+
/// ### Why is this bad?
3762+
/// `Result` implements `IntoIterator<Item = T>`. This means that `Result` can be flattened
3763+
/// automatically without suspicious-looking `unwrap` calls.
3764+
///
3765+
/// ### Example
3766+
/// ```no_run
3767+
/// let _ = std::iter::empty::<Result<i32, ()>>().filter(Result::is_ok).map(Result::unwrap);
3768+
/// ```
3769+
/// Use instead:
3770+
/// ```no_run
3771+
/// let _ = std::iter::empty::<Result<i32, ()>>().flatten();
3772+
/// ```
3773+
#[clippy::version = "1.76.0"]
3774+
pub RESULT_FILTER_MAP,
3775+
complexity,
3776+
"filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation"
3777+
}
3778+
37553779
pub struct Methods {
37563780
avoid_breaking_exported_api: bool,
37573781
msrv: Msrv,
@@ -3903,6 +3927,7 @@ impl_lint_pass!(Methods => [
39033927
UNNECESSARY_FALLIBLE_CONVERSIONS,
39043928
JOIN_ABSOLUTE_PATHS,
39053929
OPTION_MAP_OR_ERR_OK,
3930+
RESULT_FILTER_MAP,
39063931
]);
39073932

39083933
/// Extracts a method call name, args, and `Span` of the method name.

‎tests/ui/option_filter_map.fixed

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,18 @@
33

44
fn main() {
55
let _ = Some(Some(1)).flatten();
6+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
67
let _ = Some(Some(1)).flatten();
8+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
79
let _ = Some(1).map(odds_out).flatten();
10+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
811
let _ = Some(1).map(odds_out).flatten();
12+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
913

1014
let _ = vec![Some(1)].into_iter().flatten();
15+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
1116
let _ = vec![Some(1)].into_iter().flatten();
17+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
1218
let _ = vec![1]
1319
.into_iter()
1420
.map(odds_out)

‎tests/ui/option_filter_map.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,29 @@
33

44
fn main() {
55
let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
6+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
67
let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
8+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
79
let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
10+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
811
let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
12+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
913

1014
let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
15+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
1116
let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
17+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
1218
let _ = vec![1]
1319
.into_iter()
1420
.map(odds_out)
1521
.filter(Option::is_some)
22+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
1623
.map(Option::unwrap);
1724
let _ = vec![1]
1825
.into_iter()
1926
.map(odds_out)
2027
.filter(|o| o.is_some())
28+
//~^ ERROR: `filter` for `Some` followed by `unwrap`
2129
.map(|o| o.unwrap());
2230
}
2331

‎tests/ui/option_filter_map.stderr

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,48 +8,50 @@ LL | let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
88
= help: to override `-D warnings` add `#[allow(clippy::option_filter_map)]`
99

1010
error: `filter` for `Some` followed by `unwrap`
11-
--> $DIR/option_filter_map.rs:6:27
11+
--> $DIR/option_filter_map.rs:7:27
1212
|
1313
LL | let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
1414
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
1515

1616
error: `filter` for `Some` followed by `unwrap`
17-
--> $DIR/option_filter_map.rs:7:35
17+
--> $DIR/option_filter_map.rs:9:35
1818
|
1919
LL | let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
2020
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
2121

2222
error: `filter` for `Some` followed by `unwrap`
23-
--> $DIR/option_filter_map.rs:8:35
23+
--> $DIR/option_filter_map.rs:11:35
2424
|
2525
LL | let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
2626
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
2727

2828
error: `filter` for `Some` followed by `unwrap`
29-
--> $DIR/option_filter_map.rs:10:39
29+
--> $DIR/option_filter_map.rs:14:39
3030
|
3131
LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
3232
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
3333

3434
error: `filter` for `Some` followed by `unwrap`
35-
--> $DIR/option_filter_map.rs:11:39
35+
--> $DIR/option_filter_map.rs:16:39
3636
|
3737
LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
3838
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
3939

4040
error: `filter` for `Some` followed by `unwrap`
41-
--> $DIR/option_filter_map.rs:15:10
41+
--> $DIR/option_filter_map.rs:21:10
4242
|
4343
LL | .filter(Option::is_some)
4444
| __________^
45+
LL | |
4546
LL | | .map(Option::unwrap);
4647
| |____________________________^ help: consider using `flatten` instead: `flatten()`
4748

4849
error: `filter` for `Some` followed by `unwrap`
49-
--> $DIR/option_filter_map.rs:20:10
50+
--> $DIR/option_filter_map.rs:27:10
5051
|
5152
LL | .filter(|o| o.is_some())
5253
| __________^
54+
LL | |
5355
LL | | .map(|o| o.unwrap());
5456
| |____________________________^ help: consider using `flatten` instead: `flatten()`
5557

‎tests/ui/result_filter_map.fixed

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
#![warn(clippy::result_filter_map)]
2+
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
3+
4+
fn odds_out(x: i32) -> Result<i32, ()> {
5+
if x % 2 == 0 { Ok(x) } else { Err(()) }
6+
}
7+
8+
fn main() {
9+
// Unlike the Option version, Result does not have a filter operation => we will check for iterators
10+
// only
11+
let _ = vec![Ok(1) as Result<i32, ()>]
12+
.into_iter()
13+
.flatten();
14+
15+
let _ = vec![Ok(1) as Result<i32, ()>]
16+
.into_iter()
17+
.flatten();
18+
19+
let _ = vec![1]
20+
.into_iter()
21+
.map(odds_out)
22+
.flatten();
23+
let _ = vec![1]
24+
.into_iter()
25+
.map(odds_out)
26+
.flatten();
27+
}

‎tests/ui/result_filter_map.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#![warn(clippy::result_filter_map)]
2+
#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
3+
4+
fn odds_out(x: i32) -> Result<i32, ()> {
5+
if x % 2 == 0 { Ok(x) } else { Err(()) }
6+
}
7+
8+
fn main() {
9+
// Unlike the Option version, Result does not have a filter operation => we will check for iterators
10+
// only
11+
let _ = vec![Ok(1) as Result<i32, ()>]
12+
.into_iter()
13+
.filter(Result::is_ok)
14+
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
15+
.map(Result::unwrap);
16+
17+
let _ = vec![Ok(1) as Result<i32, ()>]
18+
.into_iter()
19+
.filter(|o| o.is_ok())
20+
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
21+
.map(|o| o.unwrap());
22+
23+
let _ = vec![1]
24+
.into_iter()
25+
.map(odds_out)
26+
.filter(Result::is_ok)
27+
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
28+
.map(Result::unwrap);
29+
let _ = vec![1]
30+
.into_iter()
31+
.map(odds_out)
32+
.filter(|o| o.is_ok())
33+
//~^ ERROR: `filter` for `Ok` followed by `unwrap`
34+
.map(|o| o.unwrap());
35+
}

‎tests/ui/result_filter_map.stderr

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
error: `filter` for `Ok` followed by `unwrap`
2+
--> $DIR/result_filter_map.rs:13:10
3+
|
4+
LL | .filter(Result::is_ok)
5+
| __________^
6+
LL | |
7+
LL | | .map(Result::unwrap);
8+
| |____________________________^ help: consider using `flatten` instead: `flatten()`
9+
|
10+
= note: `-D clippy::result-filter-map` implied by `-D warnings`
11+
= help: to override `-D warnings` add `#[allow(clippy::result_filter_map)]`
12+
13+
error: `filter` for `Ok` followed by `unwrap`
14+
--> $DIR/result_filter_map.rs:19:10
15+
|
16+
LL | .filter(|o| o.is_ok())
17+
| __________^
18+
LL | |
19+
LL | | .map(|o| o.unwrap());
20+
| |____________________________^ help: consider using `flatten` instead: `flatten()`
21+
22+
error: `filter` for `Ok` followed by `unwrap`
23+
--> $DIR/result_filter_map.rs:26:10
24+
|
25+
LL | .filter(Result::is_ok)
26+
| __________^
27+
LL | |
28+
LL | | .map(Result::unwrap);
29+
| |____________________________^ help: consider using `flatten` instead: `flatten()`
30+
31+
error: `filter` for `Ok` followed by `unwrap`
32+
--> $DIR/result_filter_map.rs:32:10
33+
|
34+
LL | .filter(|o| o.is_ok())
35+
| __________^
36+
LL | |
37+
LL | | .map(|o| o.unwrap());
38+
| |____________________________^ help: consider using `flatten` instead: `flatten()`
39+
40+
error: aborting due to 4 previous errors
41+

0 commit comments

Comments
 (0)