From a3278a16d31198d45c710c3c4dde433fc77d8d90 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 28 Feb 2021 09:03:21 -0500 Subject: [PATCH] Fix `manual_map`: do not expand macros in suggestions --- clippy_lints/src/manual_map.rs | 149 ++++++++++++++++-------------- clippy_utils/src/lib.rs | 33 ++++++- tests/ui/manual_map_option.fixed | 5 + tests/ui/manual_map_option.rs | 11 +++ tests/ui/manual_map_option.stderr | 20 +++- 5 files changed, 146 insertions(+), 72 deletions(-) diff --git a/clippy_lints/src/manual_map.rs b/clippy_lints/src/manual_map.rs index e6e700045..983a10e8e 100644 --- a/clippy_lints/src/manual_map.rs +++ b/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> { - fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize) -> Option> { +fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option> { + fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize, ctxt: SyntaxContext) -> Option> { 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 + .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, } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 055239435..5d1093ea0 100644 --- a/clippy_utils/src/lib.rs +++ b/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. /// diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed index 428aac439..e6fa10d22 100644 --- a/tests/ui/manual_map_option.fixed +++ b/tests/ui/manual_map_option.fixed @@ -110,4 +110,9 @@ fn main() { } } } + + // #6811 + Some(0).map(|x| vec![x]); + + option_env!("").map(String::from); } diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs index 0f4a5bb2e..7c2100299 100644 --- a/tests/ui/manual_map_option.rs +++ b/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, + }; } diff --git a/tests/ui/manual_map_option.stderr b/tests/ui/manual_map_option.stderr index 49a517377..2d13213cf 100644 --- a/tests/ui/manual_map_option.stderr +++ b/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