Auto merge of rust-lang#6801 - Jarcho:manual_match_fix, r=phansch

Fix `manual_map` false positives

fixes: rust-lang#6795
fixes: rust-lang#6797
fixes: rust-lang#6811
fixes: rust-lang#6819

changelog: Fix false positives for `manual_map` when `return`, `break`, `continue`, `yield`, `await`, and partially moved values are used.
changelog: Don't expand macros in suggestions  for `manual_map`

Author: bors

clippy_lints/src/lib.rs

@@ -1543,6 +1543,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::WHILE_LET_ON_ITERATOR),
         LintId::of(&main_recursion::MAIN_RECURSION),
         LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
+        LintId::of(&manual_map::MANUAL_MAP),
         LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(&manual_strip::MANUAL_STRIP),
         LintId::of(&manual_unwrap_or::MANUAL_UNWRAP_OR),
@@ -1774,6 +1775,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&loops::WHILE_LET_ON_ITERATOR),
         LintId::of(&main_recursion::MAIN_RECURSION),
         LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
+        LintId::of(&manual_map::MANUAL_MAP),
         LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
         LintId::of(&map_clone::MAP_CLONE),
         LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
@@ -2050,7 +2052,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&floating_point_arithmetic::SUBOPTIMAL_FLOPS),
         LintId::of(&future_not_send::FUTURE_NOT_SEND),
         LintId::of(&let_if_seq::USELESS_LET_IF_SEQ),
-        LintId::of(&manual_map::MANUAL_MAP),
         LintId::of(&missing_const_for_fn::MISSING_CONST_FOR_FN),
         LintId::of(&mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL),
         LintId::of(&mutex_atomic::MUTEX_INTEGER),

clippy_lints/src/manual_map.rs

@@ -2,24 +2,32 @@ use crate::{
     map_unit_fn::OPTION_MAP_UNIT_FN,
     matches::MATCH_AS_REF,
     utils::{
-        is_allowed, is_type_diagnostic_item, match_def_path, match_var, paths, peel_hir_expr_refs,
-        peel_mid_ty_refs_is_mutable, snippet_with_applicability, span_lint_and_sugg,
+        can_partially_move_ty, is_allowed, is_type_diagnostic_item, match_def_path, match_var, paths,
+        peel_hir_expr_refs, peel_mid_ty_refs_is_mutable, snippet_with_applicability, snippet_with_context,
+        span_lint_and_sugg,
     },
 };
 use rustc_ast::util::parser::PREC_POSTFIX;
 use rustc_errors::Applicability;
-use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, QPath};
+use rustc_hir::{
+    def::Res,
+    intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor},
+    Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, Path, QPath,
+};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
-use rustc_span::symbol::{sym, Ident};
+use rustc_span::{
+    symbol::{sym, Ident},
+    SyntaxContext,
+};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for usages of `match` which could be implemented using `map`
     ///
     /// **Why is this bad?** Using the `map` method is clearer and more concise.
     ///
-    /// **Known problems:** `map` is not capable of representing some control flow which works fine in `match`.
+    /// **Known problems:** None.
     ///
     /// **Example:**
     ///
@@ -34,7 +42,7 @@ declare_clippy_lint! {
     /// Some(0).map(|x| x + 1);
     /// ```
     pub MANUAL_MAP,
-    nursery,
+    style,
     "reimplementation of `map`"
 }
 
@@ -52,43 +60,46 @@ impl LateLintPass<'_> for ManualMap {
         {
             let (scrutinee_ty, ty_ref_count, ty_mutability) =
                 peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee));
-            if !is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type)
-                || !is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type)
+            if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type)
+                && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type))
             {
                 return;
             }
 
-            let (some_expr, some_pat, pat_ref_count, is_wild_none) =
-                match (try_parse_pattern(cx, arm1.pat), try_parse_pattern(cx, arm2.pat)) {
-                    (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count }))
-                        if is_none_expr(cx, arm1.body) =>
-                    {
-                        (arm2.body, pattern, ref_count, true)
-                    },
-                    (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count }))
-                        if is_none_expr(cx, arm1.body) =>
-                    {
-                        (arm2.body, pattern, ref_count, false)
-                    },
-                    (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild))
-                        if is_none_expr(cx, arm2.body) =>
-                    {
-                        (arm1.body, pattern, ref_count, true)
-                    },
-                    (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None))
-                        if is_none_expr(cx, arm2.body) =>
-                    {
-                        (arm1.body, pattern, ref_count, false)
-                    },
-                    _ => return,
-                };
+            let expr_ctxt = expr.span.ctxt();
+            let (some_expr, some_pat, pat_ref_count, is_wild_none) = match (
+                try_parse_pattern(cx, arm1.pat, expr_ctxt),
+                try_parse_pattern(cx, arm2.pat, expr_ctxt),
+            ) {
+                (Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count }))
+                    if is_none_expr(cx, arm1.body) =>
+                {
+                    (arm2.body, pattern, ref_count, true)
+                },
+                (Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count }))
+                    if is_none_expr(cx, arm1.body) =>
+                {
+                    (arm2.body, pattern, ref_count, false)
+                },
+                (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild))
+                    if is_none_expr(cx, arm2.body) =>
+                {
+                    (arm1.body, pattern, ref_count, true)
+                },
+                (Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None))
+                    if is_none_expr(cx, arm2.body) =>
+                {
+                    (arm1.body, pattern, ref_count, false)
+                },
+                _ => return,
+            };
 
             // Top level or patterns aren't allowed in closures.
             if matches!(some_pat.kind, PatKind::Or(_)) {
                 return;
             }
 
-            let some_expr = match get_some_expr(cx, some_expr) {
+            let some_expr = match get_some_expr(cx, some_expr, expr_ctxt) {
                 Some(expr) => expr,
                 None => return,
             };
@@ -99,6 +110,10 @@ impl LateLintPass<'_> for ManualMap {
                 return;
             }
 
+            if !can_move_expr_to_closure(cx, some_expr) {
+                return;
+            }
+
             // Determine which binding mode to use.
             let explicit_ref = some_pat.contains_explicit_ref_binding();
             let binding_ref = explicit_ref.or_else(|| (ty_ref_count != pat_ref_count).then(|| ty_mutability));
@@ -111,47 +126,50 @@ impl LateLintPass<'_> for ManualMap {
 
             let mut app = Applicability::MachineApplicable;
 
-            // Remove address-of expressions from the scrutinee. `as_ref` will be called,
-            // the type is copyable, or the option is being passed by value.
+            // Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
+            // it's being passed by value.
             let scrutinee = peel_hir_expr_refs(scrutinee).0;
-            let scrutinee_str = snippet_with_applicability(cx, scrutinee.span, "_", &mut app);
-            let scrutinee_str = if expr.precedence().order() < PREC_POSTFIX {
-                // Parens are needed to chain method calls.
-                format!("({})", scrutinee_str)
-            } else {
-                scrutinee_str.into()
-            };
+            let scrutinee_str = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
+            let scrutinee_str =
+                if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
+                    format!("({})", scrutinee_str)
+                } else {
+                    scrutinee_str.into()
+                };
 
             let body_str = if let PatKind::Binding(annotation, _, some_binding, None) = some_pat.kind {
-                if let Some(func) = can_pass_as_func(cx, some_binding, some_expr) {
-                    snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
-                } else {
-                    if match_var(some_expr, some_binding.name)
-                        && !is_allowed(cx, MATCH_AS_REF, expr.hir_id)
-                        && binding_ref.is_some()
-                    {
-                        return;
-                    }
+                match can_pass_as_func(cx, some_binding, some_expr) {
+                    Some(func) if func.span.ctxt() == some_expr.span.ctxt() => {
+                        snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
+                    },
+                    _ => {
+                        if match_var(some_expr, some_binding.name)
+                            && !is_allowed(cx, MATCH_AS_REF, expr.hir_id)
+                            && binding_ref.is_some()
+                        {
+                            return;
+                        }
 
-                    // `ref` and `ref mut` annotations were handled earlier.
-                    let annotation = if matches!(annotation, BindingAnnotation::Mutable) {
-                        "mut "
-                    } else {
-                        ""
-                    };
-                    format!(
-                        "|{}{}| {}",
-                        annotation,
-                        some_binding,
-                        snippet_with_applicability(cx, some_expr.span, "..", &mut app)
-                    )
+                        // `ref` and `ref mut` annotations were handled earlier.
+                        let annotation = if matches!(annotation, BindingAnnotation::Mutable) {
+                            "mut "
+                        } else {
+                            ""
+                        };
+                        format!(
+                            "|{}{}| {}",
+                            annotation,
+                            some_binding,
+                            snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
+                        )
+                    },
                 }
             } else if !is_wild_none && explicit_ref.is_none() {
                 // TODO: handle explicit reference annotations.
                 format!(
                     "|{}| {}",
-                    snippet_with_applicability(cx, some_pat.span, "..", &mut app),
-                    snippet_with_applicability(cx, some_expr.span, "..", &mut app)
+                    snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app),
+                    snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
                 )
             } else {
                 // Refutable bindings and mixed reference annotations can't be handled by `map`.
@@ -171,6 +189,51 @@ impl LateLintPass<'_> for ManualMap {
     }
 }
 
+// Checks if the expression can be moved into a closure as is.
+fn can_move_expr_to_closure(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+    struct V<'cx, 'tcx> {
+        cx: &'cx LateContext<'tcx>,
+        make_closure: bool,
+    }
+    impl Visitor<'tcx> for V<'_, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            match e.kind {
+                ExprKind::Break(..)
+                | ExprKind::Continue(_)
+                | ExprKind::Ret(_)
+                | ExprKind::Yield(..)
+                | ExprKind::InlineAsm(_)
+                | ExprKind::LlvmInlineAsm(_) => {
+                    self.make_closure = false;
+                },
+                // Accessing a field of a local value can only be done if the type isn't
+                // partially moved.
+                ExprKind::Field(base_expr, _)
+                    if matches!(
+                        base_expr.kind,
+                        ExprKind::Path(QPath::Resolved(_, Path { res: Res::Local(_), .. }))
+                    ) && can_partially_move_ty(self.cx, self.cx.typeck_results().expr_ty(base_expr)) =>
+                {
+                    // TODO: check if the local has been partially moved. Assume it has for now.
+                    self.make_closure = false;
+                    return;
+                }
+                _ => (),
+            };
+            walk_expr(self, e);
+        }
+    }
+
+    let mut v = V { cx, make_closure: true };
+    v.visit_expr(expr);
+    v.make_closure
+}
+
 // Checks whether the expression could be passed as a function, or whether a closure is needed.
 // Returns the function to be passed to `map` if it exists.
 fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
@@ -198,11 +261,11 @@ enum OptionPat<'a> {
 
 // Try to parse into a recognized `Option` pattern.
 // i.e. `_`, `None`, `Some(..)`, or a reference to any of those.
-fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) -> Option<OptionPat<'tcx>> {
-    fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize) -> Option<OptionPat<'tcx>> {
+fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option<OptionPat<'tcx>> {
+    fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize, ctxt: SyntaxContext) -> Option<OptionPat<'tcx>> {
         match pat.kind {
             PatKind::Wild => Some(OptionPat::Wild),
-            PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1),
+            PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt),
             PatKind::Path(QPath::Resolved(None, path))
                 if path
                     .res
@@ -215,18 +278,19 @@ fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) -> Option<Optio
                 if path
                     .res
                     .opt_def_id()
-                    .map_or(false, |id| match_def_path(cx, id, &paths::OPTION_SOME)) =>
+                    .map_or(false, |id| match_def_path(cx, id, &paths::OPTION_SOME))
+                    && pat.span.ctxt() == ctxt =>
             {
                 Some(OptionPat::Some { pattern, ref_count })
             },
             _ => None,
         }
     }
-    f(cx, pat, 0)
+    f(cx, pat, 0, ctxt)
 }
 
 // Checks for an expression wrapped by the `Some` constructor. Returns the contained expression.
-fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
+fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, ctxt: SyntaxContext) -> Option<&'tcx Expr<'tcx>> {
     // TODO: Allow more complex expressions.
     match expr.kind {
         ExprKind::Call(
@@ -235,7 +299,7 @@ fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx E
                 ..
             },
             [arg],
-        ) => {
+        ) if ctxt == expr.span.ctxt() => {
             if match_def_path(cx, path.res.opt_def_id()?, &paths::OPTION_SOME) {
                 Some(arg)
             } else {
@@ -249,7 +313,7 @@ fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx E
                 ..
             },
             _,
-        ) => get_some_expr(cx, expr),
+        ) => get_some_expr(cx, expr, ctxt),
         _ => None,
     }
 }