Skip to content

Commit 4f2559c

Browse files
JarchoJarcho
committed
Fix manual_map: do not expand macros in suggestions
1 parent 09a827a commit 4f2559c

File tree

5 files changed

+146
-72
lines changed

5 files changed

+146
-72
lines changed

‎clippy_lints/src/manual_map.rs

Lines changed: 80 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@ use crate::{
33
matches::MATCH_AS_REF,
44
utils::{
55
can_partially_move_ty, is_allowed, is_type_diagnostic_item, match_def_path, match_var, paths,
6-
peel_hir_expr_refs, peel_mid_ty_refs_is_mutable, snippet_with_applicability, span_lint_and_sugg,
6+
peel_hir_expr_refs, peel_mid_ty_refs_is_mutable, snippet_with_applicability, snippet_with_context,
7+
span_lint_and_sugg,
78
},
89
};
910
use rustc_ast::util::parser::PREC_POSTFIX;
@@ -16,7 +17,10 @@ use rustc_hir::{
1617
use rustc_lint::{LateContext, LateLintPass, LintContext};
1718
use rustc_middle::lint::in_external_macro;
1819
use rustc_session::{declare_lint_pass, declare_tool_lint};
19-
use rustc_span::symbol::{sym, Ident};
20+
use rustc_span::{
21+
symbol::{sym, Ident},
22+
SyntaxContext,
23+
};
2024

2125
declare_clippy_lint! {
2226
/// **What it does:** Checks for usages of `match` which could be implemented using `map`
@@ -56,43 +60,46 @@ impl LateLintPass<'_> for ManualMap {
5660
{
5761
let (scrutinee_ty, ty_ref_count, ty_mutability) =
5862
peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee));
59-
if !is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type)
60-
|| !is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type)
63+
if !(is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type)
64+
&& is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type))
6165
{
6266
return;
6367
}
6468

65-
let (some_expr, some_pat, pat_ref_count, is_wild_none) =
66-
match (try_parse_pattern(cx, arm1.pat), try_parse_pattern(cx, arm2.pat)) {
67-
(Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count }))
68-
if is_none_expr(cx, arm1.body) =>
69-
{
70-
(arm2.body, pattern, ref_count, true)
71-
},
72-
(Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count }))
73-
if is_none_expr(cx, arm1.body) =>
74-
{
75-
(arm2.body, pattern, ref_count, false)
76-
},
77-
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild))
78-
if is_none_expr(cx, arm2.body) =>
79-
{
80-
(arm1.body, pattern, ref_count, true)
81-
},
82-
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None))
83-
if is_none_expr(cx, arm2.body) =>
84-
{
85-
(arm1.body, pattern, ref_count, false)
86-
},
87-
_ => return,
88-
};
69+
let expr_ctxt = expr.span.ctxt();
70+
let (some_expr, some_pat, pat_ref_count, is_wild_none) = match (
71+
try_parse_pattern(cx, arm1.pat, expr_ctxt),
72+
try_parse_pattern(cx, arm2.pat, expr_ctxt),
73+
) {
74+
(Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count }))
75+
if is_none_expr(cx, arm1.body) =>
76+
{
77+
(arm2.body, pattern, ref_count, true)
78+
},
79+
(Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count }))
80+
if is_none_expr(cx, arm1.body) =>
81+
{
82+
(arm2.body, pattern, ref_count, false)
83+
},
84+
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild))
85+
if is_none_expr(cx, arm2.body) =>
86+
{
87+
(arm1.body, pattern, ref_count, true)
88+
},
89+
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None))
90+
if is_none_expr(cx, arm2.body) =>
91+
{
92+
(arm1.body, pattern, ref_count, false)
93+
},
94+
_ => return,
95+
};
8996

9097
// Top level or patterns aren't allowed in closures.
9198
if matches!(some_pat.kind, PatKind::Or(_)) {
9299
return;
93100
}
94101

95-
let some_expr = match get_some_expr(cx, some_expr) {
102+
let some_expr = match get_some_expr(cx, some_expr, expr_ctxt) {
96103
Some(expr) => expr,
97104
None => return,
98105
};
@@ -119,47 +126,50 @@ impl LateLintPass<'_> for ManualMap {
119126

120127
let mut app = Applicability::MachineApplicable;
121128

122-
// Remove address-of expressions from the scrutinee. `as_ref` will be called,
123-
// the type is copyable, or the option is being passed by value.
129+
// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
130+
// it's being passed by value.
124131
let scrutinee = peel_hir_expr_refs(scrutinee).0;
125-
let scrutinee_str = snippet_with_applicability(cx, scrutinee.span, "_", &mut app);
126-
let scrutinee_str = if expr.precedence().order() < PREC_POSTFIX {
127-
// Parens are needed to chain method calls.
128-
format!("({})", scrutinee_str)
129-
} else {
130-
scrutinee_str.into()
131-
};
132+
let scrutinee_str = snippet_with_context(cx, scrutinee.span, expr_ctxt, "..", &mut app);
133+
let scrutinee_str =
134+
if scrutinee.span.ctxt() == expr.span.ctxt() && scrutinee.precedence().order() < PREC_POSTFIX {
135+
format!("({})", scrutinee_str)
136+
} else {
137+
scrutinee_str.into()
138+
};
132139

133140
let body_str = if let PatKind::Binding(annotation, _, some_binding, None) = some_pat.kind {
134-
if let Some(func) = can_pass_as_func(cx, some_binding, some_expr) {
135-
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
136-
} else {
137-
if match_var(some_expr, some_binding.name)
138-
&& !is_allowed(cx, MATCH_AS_REF, expr.hir_id)
139-
&& binding_ref.is_some()
140-
{
141-
return;
142-
}
141+
match can_pass_as_func(cx, some_binding, some_expr) {
142+
Some(func) if func.span.ctxt() == some_expr.span.ctxt() => {
143+
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
144+
},
145+
_ => {
146+
if match_var(some_expr, some_binding.name)
147+
&& !is_allowed(cx, MATCH_AS_REF, expr.hir_id)
148+
&& binding_ref.is_some()
149+
{
150+
return;
151+
}
143152

144-
// `ref` and `ref mut` annotations were handled earlier.
145-
let annotation = if matches!(annotation, BindingAnnotation::Mutable) {
146-
"mut "
147-
} else {
148-
""
149-
};
150-
format!(
151-
"|{}{}| {}",
152-
annotation,
153-
some_binding,
154-
snippet_with_applicability(cx, some_expr.span, "..", &mut app)
155-
)
153+
// `ref` and `ref mut` annotations were handled earlier.
154+
let annotation = if matches!(annotation, BindingAnnotation::Mutable) {
155+
"mut "
156+
} else {
157+
""
158+
};
159+
format!(
160+
"|{}{}| {}",
161+
annotation,
162+
some_binding,
163+
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
164+
)
165+
},
156166
}
157167
} else if !is_wild_none && explicit_ref.is_none() {
158168
// TODO: handle explicit reference annotations.
159169
format!(
160170
"|{}| {}",
161-
snippet_with_applicability(cx, some_pat.span, "..", &mut app),
162-
snippet_with_applicability(cx, some_expr.span, "..", &mut app)
171+
snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app),
172+
snippet_with_context(cx, some_expr.span, expr_ctxt, "..", &mut app)
163173
)
164174
} else {
165175
// Refutable bindings and mixed reference annotations can't be handled by `map`.
@@ -246,11 +256,11 @@ enum OptionPat<'a> {
246256

247257
// Try to parse into a recognized `Option` pattern.
248258
// i.e. `_`, `None`, `Some(..)`, or a reference to any of those.
249-
fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) -> Option<OptionPat<'tcx>> {
250-
fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize) -> Option<OptionPat<'tcx>> {
259+
fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ctxt: SyntaxContext) -> Option<OptionPat<'tcx>> {
260+
fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize, ctxt: SyntaxContext) -> Option<OptionPat<'tcx>> {
251261
match pat.kind {
252262
PatKind::Wild => Some(OptionPat::Wild),
253-
PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1),
263+
PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1, ctxt),
254264
PatKind::Path(QPath::Resolved(None, path))
255265
if path
256266
.res
@@ -263,18 +273,19 @@ fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) -> Option<Optio
263273
if path
264274
.res
265275
.opt_def_id()
266-
.map_or(false, |id| match_def_path(cx, id, &paths::OPTION_SOME)) =>
276+
.map_or(false, |id| match_def_path(cx, id, &paths::OPTION_SOME))
277+
&& pat.span.ctxt() == ctxt =>
267278
{
268279
Some(OptionPat::Some { pattern, ref_count })
269280
},
270281
_ => None,
271282
}
272283
}
273-
f(cx, pat, 0)
284+
f(cx, pat, 0, ctxt)
274285
}
275286

276287
// Checks for an expression wrapped by the `Some` constructor. Returns the contained expression.
277-
fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
288+
fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, ctxt: SyntaxContext) -> Option<&'tcx Expr<'tcx>> {
278289
// TODO: Allow more complex expressions.
279290
match expr.kind {
280291
ExprKind::Call(
@@ -283,7 +294,7 @@ fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx E
283294
..
284295
},
285296
[arg],
286-
) => {
297+
) if ctxt == expr.span.ctxt() => {
287298
if match_def_path(cx, path.res.opt_def_id()?, &paths::OPTION_SOME) {
288299
Some(arg)
289300
} else {
@@ -297,7 +308,7 @@ fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx E
297308
..
298309
},
299310
_,
300-
) => get_some_expr(cx, expr),
311+
) => get_some_expr(cx, expr, ctxt),
301312
_ => None,
302313
}
303314
}

‎clippy_utils/src/lib.rs

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,11 @@ use rustc_middle::ty::subst::{GenericArg, GenericArgKind};
7373
use rustc_middle::ty::{self, layout::IntegerExt, DefIdTree, Ty, TyCtxt, TypeFoldable};
7474
use rustc_semver::RustcVersion;
7575
use rustc_session::Session;
76-
use rustc_span::hygiene::{ExpnKind, MacroKind};
76+
use rustc_span::hygiene::{self, ExpnKind, MacroKind};
7777
use rustc_span::source_map::original_sp;
7878
use rustc_span::sym;
7979
use rustc_span::symbol::{kw, Symbol};
80-
use rustc_span::{BytePos, Pos, Span, DUMMY_SP};
80+
use rustc_span::{BytePos, Pos, Span, SyntaxContext, DUMMY_SP};
8181
use rustc_target::abi::Integer;
8282
use rustc_trait_selection::traits::query::normalize::AtExt;
8383
use smallvec::SmallVec;
@@ -758,6 +758,35 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>(
758758
reindent_multiline(snip, true, indent)
759759
}
760760

761+
/// Same as `snippet_with_applicability`, but first walks the span up to the given context. This
762+
/// will result in the macro call, rather then the expansion, if the span is from a child context.
763+
/// If the span is not from a child context, it will be used directly instead.
764+
///
765+
/// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR node
766+
/// would result in `box []`. If given the context of the address of expression, this function will
767+
/// correctly get a snippet of `vec![]`.
768+
pub fn snippet_with_context(
769+
cx: &LateContext<'_>,
770+
span: Span,
771+
outer: SyntaxContext,
772+
default: &'a str,
773+
applicability: &mut Applicability,
774+
) -> Cow<'a, str> {
775+
let outer_span = hygiene::walk_chain(span, outer);
776+
let span = if outer_span.ctxt() != outer {
777+
// The span is from a macro argument, and the outer context is the macro using the argument
778+
if *applicability != Applicability::Unspecified {
779+
*applicability = Applicability::MaybeIncorrect;
780+
}
781+
// TODO: get the argument span.
782+
span
783+
} else {
784+
outer_span
785+
};
786+
787+
snippet_with_applicability(cx, span, default, applicability)
788+
}
789+
761790
/// Returns a new Span that extends the original Span to the first non-whitespace char of the first
762791
/// line.
763792
///

‎tests/ui/manual_map_option.fixed

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,4 +110,9 @@ fn main() {
110110
}
111111
}
112112
}
113+
114+
// #6811
115+
Some(0).map(|x| vec![x]);
116+
117+
option_env!("").map(String::from);
113118
}

‎tests/ui/manual_map_option.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,4 +162,15 @@ fn main() {
162162
}
163163
}
164164
}
165+
166+
// #6811
167+
match Some(0) {
168+
Some(x) => Some(vec![x]),
169+
None => None,
170+
};
171+
172+
match option_env!("") {
173+
Some(x) => Some(String::from(x)),
174+
None => None,
175+
};
165176
}

‎tests/ui/manual_map_option.stderr

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,5 +154,23 @@ LL | | None => None,
154154
LL | | };
155155
| |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
156156

157-
error: aborting due to 17 previous errors
157+
error: manual implementation of `Option::map`
158+
--> $DIR/manual_map_option.rs:167:5
159+
|
160+
LL | / match Some(0) {
161+
LL | | Some(x) => Some(vec![x]),
162+
LL | | None => None,
163+
LL | | };
164+
| |_____^ help: try this: `Some(0).map(|x| vec![x])`
165+
166+
error: manual implementation of `Option::map`
167+
--> $DIR/manual_map_option.rs:172:5
168+
|
169+
LL | / match option_env!("") {
170+
LL | | Some(x) => Some(String::from(x)),
171+
LL | | None => None,
172+
LL | | };
173+
| |_____^ help: try this: `option_env!("").map(String::from)`
174+
175+
error: aborting due to 19 previous errors
158176

0 commit comments

Comments
 (0)