Fix manual_map: do not expand macros in suggestions

clippy_lints/src/manual_map.rs

@@ -3,7 +3,8 @@ use crate::{
     matches::MATCH_AS_REF,
     utils::{
         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, span_lint_and_sugg,
+        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;
@@ -16,7 +17,10 @@ use rustc_hir::{
 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`
@@ -56,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,
             };
@@ -119,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`.
@@ -246,11 +256,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
@@ -263,18 +273,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(
@@ -283,7 +294,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 {
@@ -297,7 +308,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,
     }
 }

clippy_utils/src/lib.rs

@@ -73,11 +73,11 @@ use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
 use rustc_middle::ty::{self, layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable};
 use rustc_semver::RustcVersion;
 use rustc_session::Session;
-use rustc_span::hygiene::{ExpnKind, MacroKind};
+use rustc_span::hygiene::{self, ExpnKind, MacroKind};
 use rustc_span::source_map::original_sp;
 use rustc_span::sym;
 use rustc_span::symbol::{kw, Symbol};
-use rustc_span::{BytePos, Pos, Span, DUMMY_SP};
+use rustc_span::{BytePos, Pos, Span, SyntaxContext, DUMMY_SP};
 use rustc_target::abi::Integer;
 use rustc_trait_selection::traits::query::normalize::AtExt;
 use smallvec::SmallVec;
@@ -758,6 +758,35 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>(
     reindent_multiline(snip, true, indent)
 }
 
+/// Same as `snippet_with_applicability`, but first walks the span up to the given context. This
+/// will result in the macro call, rather then the expansion, if the span is from a child context.
+/// If the span is not from a child context, it will be used directly instead.
+///
+/// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR node
+/// would result in `box []`. If given the context of the address of expression, this function will
+/// correctly get a snippet of `vec![]`.
+pub fn snippet_with_context(
+    cx: &LateContext<'_>,
+    span: Span,
+    outer: SyntaxContext,
+    default: &'a str,
+    applicability: &mut Applicability,
+) -> Cow<'a, str> {
+    let outer_span = hygiene::walk_chain(span, outer);
+    let span = if outer_span.ctxt() == outer {
+        outer_span
+    } else {
+        // The span is from a macro argument, and the outer context is the macro using the argument
+        if *applicability != Applicability::Unspecified {
+            *applicability = Applicability::MaybeIncorrect;
+        }
+        // TODO: get the argument span.
+        span
+    };
+
+    snippet_with_applicability(cx, span, default, applicability)
+}
+
 /// Returns a new Span that extends the original Span to the first non-whitespace char of the first
 /// line.
 ///

tests/ui/manual_map_option.fixed

@@ -110,4 +110,9 @@ fn main() {
             }
         }
     }
+
+    // #6811
+    Some(0).map(|x| vec![x]);
+
+    option_env!("").map(String::from);
 }

tests/ui/manual_map_option.rs

@@ -162,4 +162,15 @@ fn main() {
             }
         }
     }
+
+    // #6811
+    match Some(0) {
+        Some(x) => Some(vec![x]),
+        None => None,
+    };
+
+    match option_env!("") {
+        Some(x) => Some(String::from(x)),
+        None => None,
+    };
 }

tests/ui/manual_map_option.stderr

@@ -154,5 +154,23 @@ LL | |         None => None,
 LL | |     };
    | |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
 
-error: aborting due to 17 previous errors
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option.rs:167:5
+   |
+LL | /     match Some(0) {
+LL | |         Some(x) => Some(vec![x]),
+LL | |         None => None,
+LL | |     };
+   | |_____^ help: try this: `Some(0).map(|x| vec![x])`
+
+error: manual implementation of `Option::map`
+  --> $DIR/manual_map_option.rs:172:5
+   |
+LL | /     match option_env!("") {
+LL | |         Some(x) => Some(String::from(x)),
+LL | |         None => None,
+LL | |     };
+   | |_____^ help: try this: `option_env!("").map(String::from)`
+
+error: aborting due to 19 previous errors