Auto merge of rust-lang#10949 - y21:issue8010, r=Alexendoo

[`manual_filter_map`]: lint on `matches` and pattern matching

Fixes rust-lang#8010

Previously this lint only worked specifically for a very limited set of methods on the filter call (`.filter(|opt| opt.is_some())` and `.filter(|res| res.is_ok())`). This PR extends it to also recognize `matches!` in the `filter` and pattern matching with `if let` or `match` in the `map`.

Example:
```rs
enum Enum {
  A(i32),
  B,
}

let _ = [Enum::A(123), Enum::B].into_iter()
  .filter(|x| matches!(x, Enum::A(_)))
  .map(|x| if let Enum::A(s) = x { s } else { unreachable!() });
```
Now suggests:
```diff
-  .filter(|x| matches!(x, Enum::A(_))).map(if let Enum::A(s) = x { s } else { unreachable!() })
+  .filter_map(|x| match x { Enum::A(s) => Some(s), _ => None })
```

Adding this required a somewhat large change in code because it originally seemed to be specifically written with only method calls in the filter in mind, and `matches!` has different behavior in the map, so this new setup should make it possible to support more "generic" cases that need different handling for the filter and map calls.

changelog: [`manual_filter_map`]: lint on `matches` and pattern matching (and some internal refactoring)

Author: bors

clippy_lints/src/methods/filter_map.rs

@@ -1,7 +1,9 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::macros::{is_panic, root_macro_call};
 use clippy_utils::source::{indent_of, reindent_multiline, snippet};
 use clippy_utils::ty::is_type_diagnostic_item;
-use clippy_utils::{is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
+use clippy_utils::{higher, is_trait_method, path_to_local_id, peel_blocks, SpanlessEq};
+use hir::{Body, HirId, MatchSource, Pat};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
@@ -10,7 +12,7 @@ use rustc_hir::{Closure, Expr, ExprKind, PatKind, PathSegment, QPath, UnOp};
 use rustc_lint::LateContext;
 use rustc_middle::ty::adjustment::Adjust;
 use rustc_span::source_map::Span;
-use rustc_span::symbol::{sym, Symbol};
+use rustc_span::symbol::{sym, Ident, Symbol};
 use std::borrow::Cow;
 
 use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP};
@@ -48,6 +50,214 @@ fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_ar
     is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
 }
 
+#[derive(Debug, Copy, Clone)]
+enum OffendingFilterExpr<'tcx> {
+    /// `.filter(|opt| opt.is_some())`
+    IsSome {
+        /// The receiver expression
+        receiver: &'tcx Expr<'tcx>,
+        /// If `Some`, then this contains the span of an expression that possibly contains side
+        /// effects: `.filter(|opt| side_effect(opt).is_some())`
+        ///                         ^^^^^^^^^^^^^^^^
+        ///
+        /// We will use this later for warning the user that the suggested fix may change
+        /// the behavior.
+        side_effect_expr_span: Option<Span>,
+    },
+    /// `.filter(|res| res.is_ok())`
+    IsOk {
+        /// The receiver expression
+        receiver: &'tcx Expr<'tcx>,
+        /// See `IsSome`
+        side_effect_expr_span: Option<Span>,
+    },
+    /// `.filter(|enum| matches!(enum, Enum::A(_)))`
+    Matches {
+        /// The DefId of the variant being matched
+        variant_def_id: hir::def_id::DefId,
+    },
+}
+
+#[derive(Debug)]
+enum CalledMethod {
+    OptionIsSome,
+    ResultIsOk,
+}
+
+/// The result of checking a `map` call, returned by `OffendingFilterExpr::check_map_call`
+#[derive(Debug)]
+enum CheckResult<'tcx> {
+    Method {
+        map_arg: &'tcx Expr<'tcx>,
+        /// The method that was called inside of `filter`
+        method: CalledMethod,
+        /// See `OffendingFilterExpr::IsSome`
+        side_effect_expr_span: Option<Span>,
+    },
+    PatternMatching {
+        /// The span of the variant being matched
+        /// if let Some(s) = enum
+        ///        ^^^^^^^
+        variant_span: Span,
+        /// if let Some(s) = enum
+        ///             ^
+        variant_ident: Ident,
+    },
+}
+
+impl<'tcx> OffendingFilterExpr<'tcx> {
+    pub fn check_map_call(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        map_body: &'tcx Body<'tcx>,
+        map_param_id: HirId,
+        filter_param_id: HirId,
+        is_filter_param_ref: bool,
+    ) -> Option<CheckResult<'tcx>> {
+        match *self {
+            OffendingFilterExpr::IsSome {
+                receiver,
+                side_effect_expr_span,
+            }
+            | OffendingFilterExpr::IsOk {
+                receiver,
+                side_effect_expr_span,
+            } => {
+                // check if closure ends with expect() or unwrap()
+                if let ExprKind::MethodCall(seg, map_arg, ..) = map_body.value.kind
+                    && matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or)
+                    // .map(|y| f(y).copied().unwrap())
+                    //          ~~~~
+                    && let map_arg_peeled = match map_arg.kind {
+                        ExprKind::MethodCall(method, original_arg, [], _) if acceptable_methods(method) => {
+                            original_arg
+                        },
+                        _ => map_arg,
+                    }
+                    // .map(|y| y[.acceptable_method()].unwrap())
+                    && let simple_equal = (path_to_local_id(receiver, filter_param_id)
+                        && path_to_local_id(map_arg_peeled, map_param_id))
+                    && let eq_fallback = (|a: &Expr<'_>, b: &Expr<'_>| {
+                        // in `filter(|x| ..)`, replace `*x` with `x`
+                        let a_path = if_chain! {
+                            if !is_filter_param_ref;
+                            if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind;
+                            then { expr_path } else { a }
+                        };
+                        // let the filter closure arg and the map closure arg be equal
+                        path_to_local_id(a_path, filter_param_id)
+                            && path_to_local_id(b, map_param_id)
+                            && cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
+                    })
+                    && (simple_equal
+                        || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(receiver, map_arg_peeled))
+                {
+                    Some(CheckResult::Method {
+                        map_arg,
+                        side_effect_expr_span,
+                        method: match self {
+                            OffendingFilterExpr::IsSome { .. } => CalledMethod::OptionIsSome,
+                            OffendingFilterExpr::IsOk { .. } => CalledMethod::ResultIsOk,
+                            OffendingFilterExpr::Matches { .. } => unreachable!("only IsSome and IsOk can get here"),
+                        }
+                    })
+                } else {
+                    None
+                }
+            },
+            OffendingFilterExpr::Matches { variant_def_id } => {
+                let expr_uses_local = |pat: &Pat<'_>, expr: &Expr<'_>| {
+                    if let PatKind::TupleStruct(QPath::Resolved(_, path), [subpat], _) = pat.kind
+                        && let PatKind::Binding(_, local_id, ident, _) = subpat.kind
+                        && path_to_local_id(expr.peel_blocks(), local_id)
+                        && let Some(local_variant_def_id) = path.res.opt_def_id()
+                        && local_variant_def_id == variant_def_id
+                    {
+                        Some((ident, pat.span))
+                    } else {
+                        None
+                    }
+                };
+
+                // look for:
+                // `if let Variant   (v) =         enum { v } else { unreachable!() }`
+                //         ^^^^^^^    ^            ^^^^            ^^^^^^^^^^^^^^^^^^
+                //    variant_span  variant_ident  scrutinee       else_ (blocks peeled later)
+                // OR
+                // `match enum {   Variant       (v) => v,      _ => unreachable!() }`
+                //        ^^^^     ^^^^^^^        ^                  ^^^^^^^^^^^^^^
+                //     scrutinee  variant_span  variant_ident        else_
+                let (scrutinee, else_, variant_ident, variant_span) =
+                    match higher::IfLetOrMatch::parse(cx, map_body.value) {
+                        // For `if let` we want to check that the variant matching arm references the local created by its pattern
+                        Some(higher::IfLetOrMatch::IfLet(sc, pat, then, Some(else_)))
+                            if let Some((ident, span)) = expr_uses_local(pat, then) =>
+                        {
+                            (sc, else_, ident, span)
+                        },
+                        // For `match` we want to check that the "else" arm is the wildcard (`_`) pattern
+                        // and that the variant matching arm references the local created by its pattern
+                        Some(higher::IfLetOrMatch::Match(sc, [arm, wild_arm], MatchSource::Normal))
+                            if let PatKind::Wild = wild_arm.pat.kind
+                                && let Some((ident, span)) = expr_uses_local(arm.pat, arm.body.peel_blocks()) =>
+                        {
+                            (sc, wild_arm.body, ident, span)
+                        },
+                        _ => return None,
+                    };
+
+                if path_to_local_id(scrutinee, map_param_id)
+                    // else branch should be a `panic!` or `unreachable!` macro call
+                    && let Some(mac) = root_macro_call(else_.peel_blocks().span)
+                    && (is_panic(cx, mac.def_id) || cx.tcx.opt_item_name(mac.def_id) == Some(sym::unreachable))
+                {
+                    Some(CheckResult::PatternMatching { variant_span, variant_ident })
+                } else {
+                    None
+                }
+            },
+        }
+    }
+
+    fn hir(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, filter_param_id: HirId) -> Option<Self> {
+        if let ExprKind::MethodCall(path, receiver, [], _) = expr.kind
+            && let Some(recv_ty) = cx.typeck_results().expr_ty(receiver).peel_refs().ty_adt_def()
+        {
+            // we still want to lint if the expression possibly contains side effects,
+            // *but* it can't be machine-applicable then, because that can change the behavior of the program:
+            // .filter(|x| effect(x).is_some()).map(|x| effect(x).unwrap())
+            // vs.
+            // .filter_map(|x| effect(x))
+            // 
+            // the latter only calls `effect` once
+            let side_effect_expr_span = receiver.can_have_side_effects().then_some(receiver.span);
+
+            if cx.tcx.is_diagnostic_item(sym::Option, recv_ty.did())
+                && path.ident.name == sym!(is_some)
+            {
+                Some(Self::IsSome { receiver, side_effect_expr_span })
+            } else if cx.tcx.is_diagnostic_item(sym::Result, recv_ty.did())
+                && path.ident.name == sym!(is_ok)
+            {
+                Some(Self::IsOk { receiver, side_effect_expr_span })
+            } else {
+                None
+            }
+        } else if let Some(macro_call) = root_macro_call(expr.span)
+            && cx.tcx.get_diagnostic_name(macro_call.def_id) == Some(sym::matches_macro)
+            // we know for a fact that the wildcard pattern is the second arm
+            && let ExprKind::Match(scrutinee, [arm, _], _) = expr.kind
+            && path_to_local_id(scrutinee, filter_param_id)
+            && let PatKind::TupleStruct(QPath::Resolved(_, path), ..) = arm.pat.kind
+            && let Some(variant_def_id) = path.res.opt_def_id()
+        {
+            Some(OffendingFilterExpr::Matches { variant_def_id })
+        } else {
+            None
+        }
+    }
+}
+
 /// is `filter(|x| x.is_some()).map(|x| x.unwrap())`
 fn is_filter_some_map_unwrap(
     cx: &LateContext<'_>,
@@ -102,55 +312,18 @@ pub(super) fn check(
         } else {
             (filter_param.pat, false)
         };
-        // closure ends with is_some() or is_ok()
+
         if let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind;
-        if let ExprKind::MethodCall(path, filter_arg, [], _) = filter_body.value.kind;
-        if let Some(opt_ty) = cx.typeck_results().expr_ty(filter_arg).peel_refs().ty_adt_def();
-        if let Some(is_result) = if cx.tcx.is_diagnostic_item(sym::Option, opt_ty.did()) {
-            Some(false)
-        } else if cx.tcx.is_diagnostic_item(sym::Result, opt_ty.did()) {
-            Some(true)
-        } else {
-            None
-        };
-        if path.ident.name.as_str() == if is_result { "is_ok" } else { "is_some" };
+        if let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id);
 
-        // ...map(|x| ...unwrap())
         if let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind;
         let map_body = cx.tcx.hir().body(map_body_id);
         if let [map_param] = map_body.params;
         if let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind;
-        // closure ends with expect() or unwrap()
-        if let ExprKind::MethodCall(seg, map_arg, ..) = map_body.value.kind;
-        if matches!(seg.ident.name, sym::expect | sym::unwrap | sym::unwrap_or);
-
-        // .filter(..).map(|y| f(y).copied().unwrap())
-        //                     ~~~~
-        let map_arg_peeled = match map_arg.kind {
-            ExprKind::MethodCall(method, original_arg, [], _) if acceptable_methods(method) => {
-                original_arg
-            },
-            _ => map_arg,
-        };
 
-        // .filter(|x| x.is_some()).map(|y| y[.acceptable_method()].unwrap())
-        let simple_equal = path_to_local_id(filter_arg, filter_param_id)
-            && path_to_local_id(map_arg_peeled, map_param_id);
+        if let Some(check_result) =
+            offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref);
 
-        let eq_fallback = |a: &Expr<'_>, b: &Expr<'_>| {
-            // in `filter(|x| ..)`, replace `*x` with `x`
-            let a_path = if_chain! {
-                if !is_filter_param_ref;
-                if let ExprKind::Unary(UnOp::Deref, expr_path) = a.kind;
-                then { expr_path } else { a }
-            };
-            // let the filter closure arg and the map closure arg be equal
-            path_to_local_id(a_path, filter_param_id)
-                && path_to_local_id(b, map_param_id)
-                && cx.typeck_results().expr_ty_adjusted(a) == cx.typeck_results().expr_ty_adjusted(b)
-        };
-
-        if simple_equal || SpanlessEq::new(cx).expr_fallback(eq_fallback).eq_expr(filter_arg, map_arg_peeled);
         then {
             let span = filter_span.with_hi(expr.span.hi());
             let (filter_name, lint) = if is_find {
@@ -159,22 +332,53 @@ pub(super) fn check(
                 ("filter", MANUAL_FILTER_MAP)
             };
             let msg = format!("`{filter_name}(..).map(..)` can be simplified as `{filter_name}_map(..)`");
-            let (to_opt, deref) = if is_result {
-                (".ok()", String::new())
-            } else {
-                let derefs = cx.typeck_results()
-                    .expr_adjustments(map_arg)
-                    .iter()
-                    .filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
-                    .count();
 
-                ("", "*".repeat(derefs))
+            let (sugg, note_and_span, applicability) = match check_result {
+                CheckResult::Method { map_arg, method, side_effect_expr_span } => {
+                    let (to_opt, deref) = match method {
+                        CalledMethod::ResultIsOk => (".ok()", String::new()),
+                        CalledMethod::OptionIsSome => {
+                            let derefs = cx.typeck_results()
+                                .expr_adjustments(map_arg)
+                                .iter()
+                                .filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
+                                .count();
+
+                            ("", "*".repeat(derefs))
+                        }
+                    };
+
+                    let sugg = format!(
+                        "{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
+                        snippet(cx, map_arg.span, ".."),
+                    );
+                    let (note_and_span, applicability) = if let Some(span) = side_effect_expr_span {
+                        let note = "the suggestion might change the behavior of the program when merging `filter` and `map`, \
+                            because this expression potentially contains side effects and will only execute once";
+
+                        (Some((note, span)), Applicability::MaybeIncorrect)
+                    } else {
+                        (None, Applicability::MachineApplicable)
+                    };
+
+                    (sugg, note_and_span, applicability)
+                }
+                CheckResult::PatternMatching { variant_span, variant_ident } => {
+                    let pat = snippet(cx, variant_span, "<pattern>");
+
+                    (format!("{filter_name}_map(|{map_param_ident}| match {map_param_ident} {{ \
+                        {pat} => Some({variant_ident}), \
+                        _ => None \
+                    }})"), None, Applicability::MachineApplicable)
+                }
             };
-            let sugg = format!(
-                "{filter_name}_map(|{map_param_ident}| {deref}{}{to_opt})",
-                snippet(cx, map_arg.span, ".."),
-            );
-            span_lint_and_sugg(cx, lint, span, &msg, "try", sugg, Applicability::MachineApplicable);
+            span_lint_and_then(cx, lint, span, &msg, |diag| {
+                diag.span_suggestion(span, "try", sugg, applicability);
+
+                if let Some((note, span)) = note_and_span {
+                    diag.span_note(span, note);
+                }
+            });
         }
     }
 }

clippy_utils/src/higher.rs

@@ -138,6 +138,7 @@ impl<'hir> IfLet<'hir> {
 }
 
 /// An `if let` or `match` expression. Useful for lints that trigger on one or the other.
+#[derive(Debug)]
 pub enum IfLetOrMatch<'hir> {
     /// Any `match` expression
     Match(&'hir Expr<'hir>, &'hir [Arm<'hir>], MatchSource),

tests/ui/manual_filter_map.fixed

@@ -120,3 +120,27 @@ fn issue_8920() {
         .iter()
         .filter_map(|f| f.result_field.to_owned().ok());
 }
+
+fn issue8010() {
+    #[derive(Clone)]
+    enum Enum {
+        A(i32),
+        B,
+    }
+
+    let iter = [Enum::A(123), Enum::B].into_iter();
+
+    let _x = iter.clone().filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
+    let _x = iter.clone().filter(|x| matches!(x, Enum::B)).map(|x| match x {
+        Enum::A(s) => s,
+        _ => unreachable!(),
+    });
+    let _x = iter
+        .clone()
+        .filter_map(|x| match x { Enum::A(s) => Some(s), _ => None });
+    #[allow(clippy::unused_unit)]
+    let _x = iter
+        .clone()
+        .filter(|x| matches!(x, Enum::B))
+        .map(|x| if let Enum::B = x { () } else { unreachable!() });
+}