From 8892420aa742e1f8e2ae80edd694aba59ac60e87 Mon Sep 17 00:00:00 2001 From: Quinn Sinclair Date: Mon, 4 Dec 2023 02:20:47 +0100 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/methods/filter_map.rs | 86 +++++++++++++++++++------- clippy_lints/src/methods/mod.rs | 27 +++++++- tests/ui/option_filter_map.fixed | 6 ++ tests/ui/option_filter_map.rs | 8 +++ tests/ui/option_filter_map.stderr | 16 ++--- tests/ui/result_filter_map.fixed | 27 ++++++++ tests/ui/result_filter_map.rs | 35 +++++++++++ tests/ui/result_filter_map.stderr | 41 ++++++++++++ 10 files changed, 217 insertions(+), 31 deletions(-) create mode 100644 tests/ui/result_filter_map.fixed create mode 100644 tests/ui/result_filter_map.rs create mode 100644 tests/ui/result_filter_map.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 70aff7f60e059..da6bb32cd45b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5470,6 +5470,7 @@ Released 2018-09-13 [`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used +[`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map [`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2ce938fbe92ac..8ae68da3e9f19 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -419,6 +419,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::methods::READ_LINE_WITHOUT_TRIM_INFO, crate::methods::REDUNDANT_AS_STR_INFO, crate::methods::REPEAT_ONCE_INFO, + crate::methods::RESULT_FILTER_MAP_INFO, crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO, crate::methods::SEARCH_IS_SOME_INFO, crate::methods::SEEK_FROM_CURRENT_INFO, diff --git a/clippy_lints/src/methods/filter_map.rs b/clippy_lints/src/methods/filter_map.rs index 844ab40cab1a0..fad45041aca12 100644 --- a/clippy_lints/src/methods/filter_map.rs +++ b/clippy_lints/src/methods/filter_map.rs @@ -14,7 +14,7 @@ use rustc_span::symbol::{sym, Ident, Symbol}; use rustc_span::Span; use std::borrow::Cow; -use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP}; +use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP, RESULT_FILTER_MAP}; fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool { match &expr.kind { @@ -46,6 +46,9 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool { is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some)) } +fn is_ok_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool { + is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_ok)) +} #[derive(Debug, Copy, Clone)] enum OffendingFilterExpr<'tcx> { @@ -273,6 +276,18 @@ fn is_filter_some_map_unwrap( (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg) } +/// is `filter(|x| x.is_ok()).map(|x| x.unwrap())` +fn is_filter_ok_map_unwrap( + cx: &LateContext<'_>, + expr: &hir::Expr<'_>, + filter_arg: &hir::Expr<'_>, + map_arg: &hir::Expr<'_>, +) -> bool { + // result has no filter, so we only check for iterators + let iterator = is_trait_method(cx, expr, sym::Iterator); + iterator && is_ok_filter_map(cx, filter_arg, map_arg) +} + /// lint use of `filter().map()` or `find().map()` for `Iterators` #[allow(clippy::too_many_arguments)] pub(super) fn check( @@ -300,30 +315,21 @@ pub(super) fn check( return; } - if is_trait_method(cx, map_recv, sym::Iterator) - - // filter(|x| ...is_some())... - && let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind - && let filter_body = cx.tcx.hir().body(filter_body_id) - && let [filter_param] = filter_body.params - // optional ref pattern: `filter(|&x| ..)` - && let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind { - (ref_pat, true) - } else { - (filter_param.pat, false) - } - - && let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind - && let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id) + if is_filter_ok_map_unwrap(cx, expr, filter_arg, map_arg) { + span_lint_and_sugg( + cx, + RESULT_FILTER_MAP, + filter_span.with_hi(expr.span.hi()), + "`filter` for `Ok` followed by `unwrap`", + "consider using `flatten` instead", + reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(), + Applicability::MachineApplicable, + ); - && let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind - && let map_body = cx.tcx.hir().body(map_body_id) - && let [map_param] = map_body.params - && let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind + return; + } - && let Some(check_result) = - offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref) - { + if let Some((map_param_ident, check_result)) = is_find_or_filter(cx, map_recv, filter_arg, map_arg) { let span = filter_span.with_hi(expr.span.hi()); let (filter_name, lint) = if is_find { ("find", MANUAL_FIND_MAP) @@ -395,6 +401,40 @@ pub(super) fn check( } } +fn is_find_or_filter<'a>( + cx: &LateContext<'a>, + map_recv: &hir::Expr<'_>, + filter_arg: &hir::Expr<'_>, + map_arg: &hir::Expr<'_>, +) -> Option<(Ident, CheckResult<'a>)> { + if is_trait_method(cx, map_recv, sym::Iterator) + // filter(|x| ...is_some())... + && let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind + && let filter_body = cx.tcx.hir().body(filter_body_id) + && let [filter_param] = filter_body.params + // optional ref pattern: `filter(|&x| ..)` + && let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind { + (ref_pat, true) + } else { + (filter_param.pat, false) + } + + && let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind + && let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id) + + && let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind + && let map_body = cx.tcx.hir().body(map_body_id) + && let [map_param] = map_body.params + && let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind + + && let Some(check_result) = + offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref) + { + return Some((map_param_ident, check_result)); + } + None +} + fn acceptable_methods(method: &PathSegment<'_>) -> bool { let methods: [Symbol; 8] = [ sym::clone, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index b4f60ffadd773..a94524b3ba6bd 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1175,7 +1175,8 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for indirect collection of populated `Option` + /// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may + /// be replaced with a `.flatten()` call. /// /// ### Why is this bad? /// `Option` is like a collection of 0-1 things, so `flatten` @@ -3752,6 +3753,29 @@ declare_clippy_lint! { "using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may + /// be replaced with a `.flatten()` call. + /// + /// ### Why is this bad? + /// `Result` implements `IntoIterator`. This means that `Result` can be flattened + /// automatically without suspicious-looking `unwrap` calls. + /// + /// ### Example + /// ```no_run + /// let _ = std::iter::empty::>().filter(Result::is_ok).map(Result::unwrap); + /// ``` + /// Use instead: + /// ```no_run + /// let _ = std::iter::empty::>().flatten(); + /// ``` + #[clippy::version = "1.76.0"] + pub RESULT_FILTER_MAP, + complexity, + "filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Msrv, @@ -3903,6 +3927,7 @@ impl_lint_pass!(Methods => [ UNNECESSARY_FALLIBLE_CONVERSIONS, JOIN_ABSOLUTE_PATHS, OPTION_MAP_OR_ERR_OK, + RESULT_FILTER_MAP, ]); /// Extracts a method call name, args, and `Span` of the method name. diff --git a/tests/ui/option_filter_map.fixed b/tests/ui/option_filter_map.fixed index ee004c0e194bb..b1c3cfa48a446 100644 --- a/tests/ui/option_filter_map.fixed +++ b/tests/ui/option_filter_map.fixed @@ -3,12 +3,18 @@ fn main() { let _ = Some(Some(1)).flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(Some(1)).flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(1).map(odds_out).flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(1).map(odds_out).flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![Some(1)].into_iter().flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![Some(1)].into_iter().flatten(); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![1] .into_iter() .map(odds_out) diff --git a/tests/ui/option_filter_map.rs b/tests/ui/option_filter_map.rs index eae2fa176a890..2550b9cd2b369 100644 --- a/tests/ui/option_filter_map.rs +++ b/tests/ui/option_filter_map.rs @@ -3,21 +3,29 @@ fn main() { let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap()); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap()); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap()); + //~^ ERROR: `filter` for `Some` followed by `unwrap` let _ = vec![1] .into_iter() .map(odds_out) .filter(Option::is_some) + //~^ ERROR: `filter` for `Some` followed by `unwrap` .map(Option::unwrap); let _ = vec![1] .into_iter() .map(odds_out) .filter(|o| o.is_some()) + //~^ ERROR: `filter` for `Some` followed by `unwrap` .map(|o| o.unwrap()); } diff --git a/tests/ui/option_filter_map.stderr b/tests/ui/option_filter_map.stderr index 148f9d02f5e16..6a0fc10822bdf 100644 --- a/tests/ui/option_filter_map.stderr +++ b/tests/ui/option_filter_map.stderr @@ -8,48 +8,50 @@ LL | let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap); = help: to override `-D warnings` add `#[allow(clippy::option_filter_map)]` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:6:27 + --> $DIR/option_filter_map.rs:7:27 | LL | let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:7:35 + --> $DIR/option_filter_map.rs:9:35 | LL | let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:8:35 + --> $DIR/option_filter_map.rs:11:35 | LL | let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:10:39 + --> $DIR/option_filter_map.rs:14:39 | LL | let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:11:39 + --> $DIR/option_filter_map.rs:16:39 | LL | let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:15:10 + --> $DIR/option_filter_map.rs:21:10 | LL | .filter(Option::is_some) | __________^ +LL | | LL | | .map(Option::unwrap); | |____________________________^ help: consider using `flatten` instead: `flatten()` error: `filter` for `Some` followed by `unwrap` - --> $DIR/option_filter_map.rs:20:10 + --> $DIR/option_filter_map.rs:27:10 | LL | .filter(|o| o.is_some()) | __________^ +LL | | LL | | .map(|o| o.unwrap()); | |____________________________^ help: consider using `flatten` instead: `flatten()` diff --git a/tests/ui/result_filter_map.fixed b/tests/ui/result_filter_map.fixed new file mode 100644 index 0000000000000..e8943dbc72da5 --- /dev/null +++ b/tests/ui/result_filter_map.fixed @@ -0,0 +1,27 @@ +#![warn(clippy::result_filter_map)] +#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)] + +fn odds_out(x: i32) -> Result { + if x % 2 == 0 { Ok(x) } else { Err(()) } +} + +fn main() { + // Unlike the Option version, Result does not have a filter operation => we will check for iterators + // only + let _ = vec![Ok(1) as Result] + .into_iter() + .flatten(); + + let _ = vec![Ok(1) as Result] + .into_iter() + .flatten(); + + let _ = vec![1] + .into_iter() + .map(odds_out) + .flatten(); + let _ = vec![1] + .into_iter() + .map(odds_out) + .flatten(); +} diff --git a/tests/ui/result_filter_map.rs b/tests/ui/result_filter_map.rs new file mode 100644 index 0000000000000..bfe47ffcf380c --- /dev/null +++ b/tests/ui/result_filter_map.rs @@ -0,0 +1,35 @@ +#![warn(clippy::result_filter_map)] +#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)] + +fn odds_out(x: i32) -> Result { + if x % 2 == 0 { Ok(x) } else { Err(()) } +} + +fn main() { + // Unlike the Option version, Result does not have a filter operation => we will check for iterators + // only + let _ = vec![Ok(1) as Result] + .into_iter() + .filter(Result::is_ok) + //~^ ERROR: `filter` for `Ok` followed by `unwrap` + .map(Result::unwrap); + + let _ = vec![Ok(1) as Result] + .into_iter() + .filter(|o| o.is_ok()) + //~^ ERROR: `filter` for `Ok` followed by `unwrap` + .map(|o| o.unwrap()); + + let _ = vec![1] + .into_iter() + .map(odds_out) + .filter(Result::is_ok) + //~^ ERROR: `filter` for `Ok` followed by `unwrap` + .map(Result::unwrap); + let _ = vec![1] + .into_iter() + .map(odds_out) + .filter(|o| o.is_ok()) + //~^ ERROR: `filter` for `Ok` followed by `unwrap` + .map(|o| o.unwrap()); +} diff --git a/tests/ui/result_filter_map.stderr b/tests/ui/result_filter_map.stderr new file mode 100644 index 0000000000000..4687794949d34 --- /dev/null +++ b/tests/ui/result_filter_map.stderr @@ -0,0 +1,41 @@ +error: `filter` for `Ok` followed by `unwrap` + --> $DIR/result_filter_map.rs:13:10 + | +LL | .filter(Result::is_ok) + | __________^ +LL | | +LL | | .map(Result::unwrap); + | |____________________________^ help: consider using `flatten` instead: `flatten()` + | + = note: `-D clippy::result-filter-map` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::result_filter_map)]` + +error: `filter` for `Ok` followed by `unwrap` + --> $DIR/result_filter_map.rs:19:10 + | +LL | .filter(|o| o.is_ok()) + | __________^ +LL | | +LL | | .map(|o| o.unwrap()); + | |____________________________^ help: consider using `flatten` instead: `flatten()` + +error: `filter` for `Ok` followed by `unwrap` + --> $DIR/result_filter_map.rs:26:10 + | +LL | .filter(Result::is_ok) + | __________^ +LL | | +LL | | .map(Result::unwrap); + | |____________________________^ help: consider using `flatten` instead: `flatten()` + +error: `filter` for `Ok` followed by `unwrap` + --> $DIR/result_filter_map.rs:32:10 + | +LL | .filter(|o| o.is_ok()) + | __________^ +LL | | +LL | | .map(|o| o.unwrap()); + | |____________________________^ help: consider using `flatten` instead: `flatten()` + +error: aborting due to 4 previous errors +