Skip to content

Commit

Permalink
Fix manual_map: do not expand macros in suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarcho
Jarcho committed Feb 28, 2021
1 parent 09a827a commit a3278a1
Show file tree
Hide file tree
Showing 5 changed files with 146 additions and 72 deletions.
149 changes: 80 additions & 69 deletions clippy_lints/src/manual_map.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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`
Expand Down Expand Up @@ -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,
};
Expand All @@ -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`.
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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 {
Expand All @@ -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,
}
}
Expand Down
33 changes: 31 additions & 2 deletions clippy_utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
///
Expand Down
5 changes: 5 additions & 0 deletions tests/ui/manual_map_option.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,9 @@ fn main() {
}
}
}

// #6811
Some(0).map(|x| vec![x]);

option_env!("").map(String::from);
}
11 changes: 11 additions & 0 deletions tests/ui/manual_map_option.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
20 changes: 19 additions & 1 deletion tests/ui/manual_map_option.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit a3278a1

Please sign in to comment.