Skip to content

Commit bc84d30

Browse files
JarchoJarcho
committed
Limit needless_borrow to only work if the pattern is not from a macro
1 parent 8f84628 commit bc84d30

File tree

5 files changed

+90
-79
lines changed

5 files changed

+90
-79
lines changed

‎clippy_lints/src/needless_borrow.rs

+42-63
Original file line numberDiff line numberDiff line change
@@ -4,21 +4,18 @@
44
55
use clippy_utils::diagnostics::span_lint_and_then;
66
use clippy_utils::source::{snippet_opt, snippet_with_applicability, snippet_with_context};
7-
use clippy_utils::{get_parent_expr, path_to_local};
7+
use clippy_utils::{get_parent_expr, in_macro, path_to_local};
88
use if_chain::if_chain;
99
use rustc_ast::util::parser::PREC_POSTFIX;
1010
use rustc_data_structures::fx::FxIndexMap;
1111
use rustc_errors::Applicability;
12-
use rustc_hir::{
13-
BindingAnnotation, Block, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, ImplItem, Item, Mutability, Node, Pat,
14-
PatKind, TraitItem, UnOp,
15-
};
16-
use rustc_lint::{LateContext, LateLintPass, Lint, LintContext};
12+
use rustc_hir::{BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, UnOp};
13+
use rustc_lint::{LateContext, LateLintPass, LintContext};
1714
use rustc_middle::lint::in_external_macro;
1815
use rustc_middle::ty;
1916
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
2017
use rustc_session::{declare_tool_lint, impl_lint_pass};
21-
use rustc_span::{Span, SyntaxContext};
18+
use rustc_span::Span;
2219

2320
declare_clippy_lint! {
2421
/// **What it does:** Checks for address of operations (`&`) that are going to
@@ -76,16 +73,19 @@ pub struct NeedlessBorrow {
7673
/// other.
7774
current_body: Option<BodyId>,
7875
/// The list of locals currently being checked by the lint.
76+
/// If the value is `None`, then the binding has been seen as a ref pattern, but is not linted.
77+
/// This is needed for or patterns where one of the branches can be linted, but another can not
78+
/// be.
79+
///
80+
/// e.g. `m!(x) | Foo::Bar(ref x)`
7981
ref_locals: FxIndexMap<HirId, Option<RefPat>>,
8082
}
8183

8284
struct RefPat {
83-
/// The lint to use for this suggestion. Either `needless_borrow` or `ref_binding_to_reference`.
84-
lint: &'static Lint,
85+
/// Whether every usage of the binding is dereferenced.
86+
always_deref: bool,
8587
/// The spans of all the ref bindings for this local.
8688
spans: Vec<Span>,
87-
/// The context replacements are to be made in.
88-
ctxt: SyntaxContext,
8989
/// The applicability of this suggestion.
9090
app: Applicability,
9191
/// All the replacements which need to be made.
@@ -144,17 +144,17 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
144144
if let Some(opt_prev_pat) = self.ref_locals.get_mut(&id) {
145145
// This binding id has been seen before. Add this pattern to the list of changes.
146146
if let Some(prev_pat) = opt_prev_pat {
147-
if prev_pat.ctxt == pat.span.ctxt() {
147+
if in_macro(pat.span) {
148+
// Doesn't match the context of the previous pattern. Can't lint here.
149+
*opt_prev_pat = None;
150+
} else {
148151
prev_pat.spans.push(pat.span);
149152
prev_pat.replacements.push((
150153
pat.span,
151-
snippet_with_context(cx, name.span, prev_pat.ctxt, "..", &mut prev_pat.app)
154+
snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut prev_pat.app)
152155
.0
153156
.into(),
154157
));
155-
} else {
156-
// Doesn't match the context of the previous pattern. Can't lint here.
157-
*opt_prev_pat = None;
158158
}
159159
}
160160
return;
@@ -165,46 +165,20 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
165165
if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind();
166166
// only lint immutable refs, because borrowed `&mut T` cannot be moved out
167167
if let ty::Ref(_, _, Mutability::Not) = *tam.kind();
168+
if !in_macro(pat.span);
168169
then {
169-
let map = cx.tcx.hir();
170-
let mut iter = map.parent_iter(pat.hir_id);
171-
let expr_ctxt = loop {
172-
// Find the context shared by the pattern, and it's scope use site.
173-
match iter.next() {
174-
None => {
175-
// This shouldn't happen, but mark this as unfixable if it does.
176-
self.ref_locals.insert(id, None);
177-
return;
178-
}
179-
Some((_, Node::ImplItem(ImplItem { span, .. })
180-
| Node::TraitItem(TraitItem { span, .. })
181-
| Node::Item(Item { span, .. })
182-
| Node::Block( Block { span, ..})
183-
| Node::Expr( Expr { span, .. }))) => break span.ctxt(),
184-
_ => (),
185-
}
186-
};
187-
188-
if pat.span.ctxt() == expr_ctxt {
189-
let mut app = Applicability::MachineApplicable;
190-
let snip = snippet_with_context(cx, name.span, expr_ctxt, "..", &mut app).0;
191-
self.current_body = self.current_body.or(cx.enclosing_body);
192-
self.ref_locals.insert(
193-
id,
194-
Some(RefPat {
195-
lint: NEEDLESS_BORROW,
196-
spans: vec![pat.span],
197-
ctxt: expr_ctxt,
198-
app,
199-
replacements: vec![(pat.span, snip.into())],
200-
}),
201-
);
202-
} else {
203-
// The context of the pattern is different than the context using the binding.
204-
// Changing the pattern might affect other code which needs the ref binding.
205-
// Mark the local as having been seen, but not fixable.
206-
self.ref_locals.insert(id, None);
207-
}
170+
let mut app = Applicability::MachineApplicable;
171+
let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0;
172+
self.current_body = self.current_body.or(cx.enclosing_body);
173+
self.ref_locals.insert(
174+
id,
175+
Some(RefPat {
176+
always_deref: true,
177+
spans: vec![pat.span],
178+
app,
179+
replacements: vec![(pat.span, snip.into())],
180+
}),
181+
);
208182
}
209183
}
210184
}
@@ -217,7 +191,11 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
217191
let app = pat.app;
218192
span_lint_and_then(
219193
cx,
220-
pat.lint,
194+
if pat.always_deref {
195+
NEEDLESS_BORROW
196+
} else {
197+
REF_BINDING_TO_REFERENCE
198+
},
221199
pat.spans,
222200
"this pattern creates a reference to a reference",
223201
|diag| {
@@ -258,24 +236,25 @@ impl NeedlessBorrow {
258236
span,
259237
kind: ExprKind::Unary(UnOp::Deref, _),
260238
..
261-
}) if span.ctxt() == pat.ctxt => {
239+
}) if !in_macro(span) => {
262240
// Remove explicit deref.
263-
let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
241+
let snip = snippet_with_context(cx, e.span, span.ctxt(), "..", &mut pat.app).0;
264242
pat.replacements.push((span, snip.into()));
265243
},
266-
Some(parent) if parent.span.ctxt() == pat.ctxt => {
244+
Some(parent) if !in_macro(parent.span) => {
267245
// Double reference might be needed at this point.
268246
if parent.precedence().order() == PREC_POSTFIX {
269-
// Parenthesis would be needed here, don't lint.
247+
// Parentheses would be needed here, don't lint.
270248
*outer_pat = None;
271249
} else {
272-
let snip = snippet_with_context(cx, e.span, pat.ctxt, "..", &mut pat.app).0;
250+
pat.always_deref = false;
251+
let snip = snippet_with_context(cx, e.span, parent.span.ctxt(), "..", &mut pat.app).0;
273252
pat.replacements.push((e.span, format!("&{}", snip)));
274253
}
275254
},
276-
_ if e.span.ctxt() == pat.ctxt => {
255+
_ if !in_macro(e.span) => {
277256
// Double reference might be needed at this point.
278-
pat.lint = REF_BINDING_TO_REFERENCE;
257+
pat.always_deref = false;
279258
let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app);
280259
pat.replacements.push((e.span, format!("&{}", snip)));
281260
},

‎clippy_utils/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1213,12 +1213,12 @@ pub fn match_function_call<'tcx>(
12131213
path: &[&str],
12141214
) -> Option<&'tcx [Expr<'tcx>]> {
12151215
if_chain! {
1216-
if let ExprKind::Call(ref fun, ref args) = expr.kind;
1216+
if let ExprKind::Call(ref fun, args) = expr.kind;
12171217
if let ExprKind::Path(ref qpath) = fun.kind;
12181218
if let Some(fun_def_id) = cx.qpath_res(qpath, fun.hir_id).opt_def_id();
12191219
if match_def_path(cx, fun_def_id, path);
12201220
then {
1221-
return Some(&args)
1221+
return Some(args)
12221222
}
12231223
};
12241224
None

‎tests/ui/needless_borrow_pat.rs

+27-1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,23 @@ macro_rules! m3 {
1515
Some(ref $i)
1616
};
1717
}
18+
macro_rules! if_chain {
19+
(if $e:expr; $($rest:tt)*) => {
20+
if $e {
21+
if_chain!($($rest)*)
22+
}
23+
};
24+
25+
(if let $p:pat = $e:expr; $($rest:tt)*) => {
26+
if let $p = $e {
27+
if_chain!($($rest)*)
28+
}
29+
};
30+
31+
(then $b:block) => {
32+
$b
33+
};
34+
}
1835

1936
#[allow(dead_code)]
2037
fn main() {
@@ -32,7 +49,7 @@ fn main() {
3249
None => return,
3350
};
3451

35-
// Ok, the pattern is in a different context than the match arm
52+
// Ok, the pattern is from a macro
3653
let _: &String = match Some(&x) {
3754
m3!(x) => x,
3855
None => return,
@@ -94,6 +111,15 @@ fn main() {
94111
let _: &u32 = match E::A(&0) {
95112
E::A(ref x) | E::B(ref x) => *x,
96113
};
114+
115+
// Err, reference to &String.
116+
if_chain! {
117+
if true;
118+
if let Some(ref x) = Some(&String::new());
119+
then {
120+
f1(x);
121+
}
122+
}
97123
}
98124

99125
// Err, reference to a &String

‎tests/ui/needless_borrow_pat.stderr

+18-12
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
error: this pattern creates a reference to a reference
2-
--> $DIR/needless_borrow_pat.rs:43:14
2+
--> $DIR/needless_borrow_pat.rs:60:14
33
|
44
LL | Some(ref x) => x,
55
| ^^^^^ help: try this: `x`
66
|
77
= note: `-D clippy::needless-borrow` implied by `-D warnings`
88

99
error: this pattern creates a reference to a reference
10-
--> $DIR/needless_borrow_pat.rs:49:14
10+
--> $DIR/needless_borrow_pat.rs:66:14
1111
|
1212
LL | Some(ref x) => *x,
1313
| ^^^^^
@@ -18,7 +18,7 @@ LL | Some(x) => x,
1818
| ^ ^
1919

2020
error: this pattern creates a reference to a reference
21-
--> $DIR/needless_borrow_pat.rs:55:14
21+
--> $DIR/needless_borrow_pat.rs:72:14
2222
|
2323
LL | Some(ref x) => {
2424
| ^^^^^
@@ -31,19 +31,19 @@ LL | f1(x);
3131
|
3232

3333
error: this pattern creates a reference to a reference
34-
--> $DIR/needless_borrow_pat.rs:65:14
34+
--> $DIR/needless_borrow_pat.rs:82:14
3535
|
3636
LL | Some(ref x) => m1!(x),
3737
| ^^^^^ help: try this: `x`
3838

3939
error: this pattern creates a reference to a reference
40-
--> $DIR/needless_borrow_pat.rs:70:15
40+
--> $DIR/needless_borrow_pat.rs:87:15
4141
|
4242
LL | let _ = |&ref x: &&String| {
4343
| ^^^^^ help: try this: `x`
4444

4545
error: this pattern creates a reference to a reference
46-
--> $DIR/needless_borrow_pat.rs:75:10
46+
--> $DIR/needless_borrow_pat.rs:92:10
4747
|
4848
LL | let (ref y,) = (&x,);
4949
| ^^^^^
@@ -55,13 +55,13 @@ LL | let _: &String = y;
5555
|
5656

5757
error: this pattern creates a reference to a reference
58-
--> $DIR/needless_borrow_pat.rs:85:14
58+
--> $DIR/needless_borrow_pat.rs:102:14
5959
|
6060
LL | Some(ref x) => x.0,
6161
| ^^^^^ help: try this: `x`
6262

6363
error: this pattern creates a reference to a reference
64-
--> $DIR/needless_borrow_pat.rs:95:14
64+
--> $DIR/needless_borrow_pat.rs:112:14
6565
|
6666
LL | E::A(ref x) | E::B(ref x) => *x,
6767
| ^^^^^ ^^^^^
@@ -72,7 +72,13 @@ LL | E::A(x) | E::B(x) => x,
7272
| ^ ^ ^
7373

7474
error: this pattern creates a reference to a reference
75-
--> $DIR/needless_borrow_pat.rs:100:12
75+
--> $DIR/needless_borrow_pat.rs:118:21
76+
|
77+
LL | if let Some(ref x) = Some(&String::new());
78+
| ^^^^^ help: try this: `x`
79+
80+
error: this pattern creates a reference to a reference
81+
--> $DIR/needless_borrow_pat.rs:126:12
7682
|
7783
LL | fn f2<'a>(&ref x: &&'a String) -> &'a String {
7884
| ^^^^^
@@ -85,13 +91,13 @@ LL | x
8591
|
8692

8793
error: this pattern creates a reference to a reference
88-
--> $DIR/needless_borrow_pat.rs:107:11
94+
--> $DIR/needless_borrow_pat.rs:133:11
8995
|
9096
LL | fn f(&ref x: &&String) {
9197
| ^^^^^ help: try this: `x`
9298

9399
error: this pattern creates a reference to a reference
94-
--> $DIR/needless_borrow_pat.rs:115:11
100+
--> $DIR/needless_borrow_pat.rs:141:11
95101
|
96102
LL | fn f(&ref x: &&String) {
97103
| ^^^^^
@@ -102,5 +108,5 @@ LL | fn f(&x: &&String) {
102108
LL | let _: &String = x;
103109
|
104110

105-
error: aborting due to 11 previous errors
111+
error: aborting due to 12 previous errors
106112

‎tests/ui/ref_binding_to_reference.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ macro_rules! m3 {
2020
fn main() {
2121
let x = String::new();
2222

23-
// Ok, the pattern is in a different context than the match arm
23+
// Ok, the pattern is from a macro
2424
let _: &&String = match Some(&x) {
2525
m3!(x) => x,
2626
None => return,

0 commit comments

Comments
 (0)