Auto merge of rust-lang#11869 - PartiallyTyped:result-filter-map, r=Alexendoo

New Lint: `result_filter_map` / Mirror of `option_filter_map` Added the `Result` mirror of `option_filter_map`. changelog: New Lint: [`result_filter_map`] I had to move around some code because the function def was too long 🙃. I have also added some pattern checks on `option_filter_map`

Author: bors

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

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,

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,

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<Item = T>`. This means that `Result` can be flattened
+ /// automatically without suspicious-looking `unwrap` calls.
+ ///
+ /// ### Example
+ /// ```no_run
+ /// let _ = std::iter::empty::<Result<i32, ()>>().filter(Result::is_ok).map(Result::unwrap);
+ /// ```
+ /// Use instead:
+ /// ```no_run
+ /// let _ = std::iter::empty::<Result<i32, ()>>().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.

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)

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());
 }
 

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()`
 

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<i32, ()> {
+ 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<i32, ()>]
+ .into_iter()
+ .flatten();
+
+ let _ = vec![Ok(1) as Result<i32, ()>]
+ .into_iter()
+ .flatten();
+
+ let _ = vec![1]
+ .into_iter()
+ .map(odds_out)
+ .flatten();
+ let _ = vec![1]
+ .into_iter()
+ .map(odds_out)
+ .flatten();
+}

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<i32, ()> {
+ 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<i32, ()>]
+ .into_iter()
+ .filter(Result::is_ok)
+ //~^ ERROR: `filter` for `Ok` followed by `unwrap`
+ .map(Result::unwrap);
+
+ let _ = vec![Ok(1) as Result<i32, ()>]
+ .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());
+}

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
+