Skip to content

Commit 484a2af

Browse files
committed
Improve needless_borrow suggestion for pattern refs
Suggest changing the usages of the binding to match the new type. Lint in macros in some cases.
1 parent 61b4ca1 commit 484a2af

File tree

8 files changed

+457
-73
lines changed

8 files changed

+457
-73
lines changed

clippy_lints/src/needless_borrow.rs

+56-21
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,14 @@
33
//! This lint is **warn** by default
44
55
use clippy_utils::diagnostics::span_lint_and_then;
6-
use clippy_utils::source::snippet_opt;
6+
use clippy_utils::source::{snippet_opt, snippet_with_applicability, snippet_with_context};
7+
use clippy_utils::visitors::foreach_local_usage;
8+
use clippy_utils::{get_parent_expr, get_pat_usable_location};
79
use if_chain::if_chain;
810
use rustc_errors::Applicability;
9-
use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind};
10-
use rustc_lint::{LateContext, LateLintPass};
11+
use rustc_hir::{BindingAnnotation, BorrowKind, Expr, ExprKind, Mutability, Pat, PatKind, UnOp};
12+
use rustc_lint::{LateContext, LateLintPass, LintContext};
13+
use rustc_middle::lint::in_external_macro;
1114
use rustc_middle::ty;
1215
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
1316
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -34,9 +37,7 @@ declare_clippy_lint! {
3437
"taking a reference that is going to be automatically dereferenced"
3538
}
3639

37-
#[derive(Default)]
3840
pub struct NeedlessBorrow;
39-
4041
impl_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW]);
4142

4243
impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
@@ -82,31 +83,65 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow {
8283
}
8384
}
8485
fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) {
85-
if pat.span.from_expansion() {
86-
return;
87-
}
8886
if_chain! {
89-
if let PatKind::Binding(BindingAnnotation::Ref, .., name, _) = pat.kind;
90-
if let ty::Ref(_, tam, mutbl) = *cx.typeck_results().pat_ty(pat).kind();
91-
if mutbl == Mutability::Not;
92-
if let ty::Ref(_, _, mutbl) = *tam.kind();
87+
if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind;
88+
if !in_external_macro(cx.sess(), pat.span);
89+
if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind();
9390
// only lint immutable refs, because borrowed `&mut T` cannot be moved out
94-
if mutbl == Mutability::Not;
91+
if let ty::Ref(_, _, Mutability::Not) = *tam.kind();
9592
then {
93+
let (span, search_loc) = match get_pat_usable_location(cx.tcx, pat) {
94+
Some(x) => x,
95+
None => return,
96+
};
97+
let ctxt = span.ctxt();
98+
if pat.span.ctxt() != ctxt {
99+
// The context of the pattern is different than the context using the binding.
100+
// Changing the pattern might affect other code which needs the ref binding.
101+
return;
102+
}
103+
104+
let mut app = Applicability::MachineApplicable;
105+
let mut can_fix = true;
106+
let mut changes = vec![
107+
(pat.span, snippet_with_context(cx, name.span, ctxt, "..", &mut app).0.into_owned()),
108+
];
109+
110+
foreach_local_usage(cx, id, search_loc, |e| {
111+
if matches!(
112+
cx.typeck_results().expr_adjustments(e),
113+
[Adjustment { kind: Adjust::Deref(_), .. }, Adjustment { kind: Adjust::Deref(_), .. }, ..]
114+
) {
115+
// Compiler inserted deref, nothing to change here
116+
} else {
117+
match get_parent_expr(cx, e) {
118+
Some(&Expr { span, kind: ExprKind::Unary(UnOp::Deref, _), .. })
119+
if span.ctxt() == ctxt => {
120+
// Remove explicit deref.
121+
let snip = snippet_with_context(cx, e.span, ctxt, "..", &mut app).0;
122+
changes.push((span, snip.into()));
123+
}
124+
_ if e.span.ctxt() == ctxt => {
125+
// Double reference might be needed at this point.
126+
let snip = snippet_with_applicability(cx, e.span, "..", &mut app);
127+
changes.push((e.span, format!("&{}", snip)));
128+
}
129+
_ => can_fix = false,
130+
}
131+
}
132+
});
133+
134+
if !can_fix {
135+
return;
136+
}
137+
96138
span_lint_and_then(
97139
cx,
98140
NEEDLESS_BORROW,
99141
pat.span,
100142
"this pattern creates a reference to a reference",
101143
|diag| {
102-
if let Some(snippet) = snippet_opt(cx, name.span) {
103-
diag.span_suggestion(
104-
pat.span,
105-
"change this to",
106-
snippet,
107-
Applicability::MachineApplicable,
108-
);
109-
}
144+
diag.multipart_suggestion("try this", changes, app);
110145
}
111146
)
112147
}

clippy_utils/src/lib.rs

+74-3
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,9 @@ use rustc_hir::def_id::{DefId, LOCAL_CRATE};
6363
use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
6464
use rustc_hir::LangItem::{ResultErr, ResultOk};
6565
use rustc_hir::{
66-
def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, HirId, Impl,
67-
ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path, PathSegment,
68-
QPath, Stmt, StmtKind, TraitItem, TraitItemKind, TraitRef, TyKind,
66+
def, Arm, BindingAnnotation, Block, Body, Constness, Destination, Expr, ExprKind, FnDecl, GenericArgs, Guard,
67+
HirId, Impl, ImplItem, ImplItemKind, Item, ItemKind, LangItem, Local, MatchSource, Node, Param, Pat, PatKind, Path,
68+
PathSegment, QPath, Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitRef, TyKind,
6969
};
7070
use rustc_lint::{LateContext, Level, Lint, LintContext};
7171
use rustc_middle::hir::exports::Export;
@@ -1469,6 +1469,77 @@ pub fn run_lints(cx: &LateContext<'_>, lints: &[&'static Lint], id: HirId) -> bo
14691469
})
14701470
}
14711471

1472+
pub enum PatUsableLocation<'tcx> {
1473+
Decl,
1474+
Body(&'tcx Expr<'tcx>),
1475+
Local(&'tcx [Stmt<'tcx>], Option<&'tcx Expr<'tcx>>),
1476+
Arm(Option<&'tcx Expr<'tcx>>, &'tcx Expr<'tcx>),
1477+
}
1478+
1479+
/// Gets all the expressions a pattern binding is visible to, and the span encompassing all the
1480+
/// expression as well as the pattern. Note name shadowing is not considered here.
1481+
pub fn get_pat_usable_location(tcx: TyCtxt<'tcx>, pat: &Pat<'_>) -> Option<(Span, PatUsableLocation<'tcx>)> {
1482+
let map = tcx.hir();
1483+
let mut parents = map.parent_iter(pat.hir_id);
1484+
loop {
1485+
match parents.next() {
1486+
Some((_, Node::Pat(_) | Node::Local(_) | Node::Param(_))) => (),
1487+
Some((_, Node::Arm(arm))) => {
1488+
break Some((
1489+
arm.span,
1490+
PatUsableLocation::Arm(
1491+
arm.guard.as_ref().map(|&(Guard::If(e) | Guard::IfLet(_, e))| e),
1492+
arm.body,
1493+
),
1494+
));
1495+
},
1496+
Some((id, Node::Stmt(_))) => {
1497+
if let Some((_, Node::Block(block))) = parents.next() {
1498+
let mut stmts = block.stmts.iter();
1499+
stmts.find(|s| s.hir_id == id);
1500+
break Some((block.span, PatUsableLocation::Local(stmts.as_slice(), block.expr)));
1501+
}
1502+
// Should be impossible. Statements can only have blocks as their parent.
1503+
break None;
1504+
},
1505+
Some((
1506+
_,
1507+
Node::Item(&Item {
1508+
span,
1509+
kind: ItemKind::Fn(.., body),
1510+
..
1511+
})
1512+
| Node::ImplItem(&ImplItem {
1513+
span,
1514+
kind: ImplItemKind::Fn(_, body),
1515+
..
1516+
})
1517+
| Node::Expr(&Expr {
1518+
span,
1519+
kind: ExprKind::Closure(_, _, body, _, _),
1520+
..
1521+
}),
1522+
)) => {
1523+
break Some((span, PatUsableLocation::Body(&map.body(body).value)));
1524+
},
1525+
Some((
1526+
_,
1527+
Node::TraitItem(&TraitItem {
1528+
span,
1529+
kind: TraitItemKind::Fn(_, ref kind),
1530+
..
1531+
}),
1532+
)) => {
1533+
break match *kind {
1534+
TraitFn::Required(_) => Some((span, PatUsableLocation::Decl)),
1535+
TraitFn::Provided(body) => Some((span, PatUsableLocation::Body(&map.body(body).value))),
1536+
};
1537+
},
1538+
_ => break None,
1539+
}
1540+
}
1541+
}
1542+
14721543
/// Returns Option<String> where String is a textual representation of the type encapsulated in the
14731544
/// slice iff the given expression is a slice of primitives (as defined in the
14741545
/// `is_recursively_primitive_type` function) and None otherwise.

clippy_utils/src/visitors.rs

+71-1
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,49 @@
1-
use crate::path_to_local_id;
1+
use crate::{path_to_local_id, PatUsableLocation};
22
use rustc_hir as hir;
33
use rustc_hir::intravisit::{self, walk_expr, NestedVisitorMap, Visitor};
44
use rustc_hir::{Arm, Body, Expr, HirId, Stmt};
55
use rustc_lint::LateContext;
66
use rustc_middle::hir::map::Map;
77

8+
/// A type which can be visited.
9+
pub trait Visitable<'tcx> {
10+
/// Calls the corresponding `visit_*` function on the visitor.
11+
fn visit<V: Visitor<'tcx>>(self, visitor: &mut V);
12+
}
13+
macro_rules! visitable_ref {
14+
($t:ident, $f:ident) => {
15+
impl Visitable<'tcx> for &'tcx $t<'tcx> {
16+
fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
17+
visitor.$f(self);
18+
}
19+
}
20+
};
21+
}
22+
visitable_ref!(Stmt, visit_stmt);
23+
visitable_ref!(Expr, visit_expr);
24+
impl Visitable<'tcx> for PatUsableLocation<'tcx> {
25+
fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
26+
match self {
27+
PatUsableLocation::Decl => (),
28+
PatUsableLocation::Arm(guard, body) => {
29+
if let Some(guard) = guard {
30+
visitor.visit_expr(guard);
31+
}
32+
visitor.visit_expr(body)
33+
},
34+
PatUsableLocation::Body(body) => visitor.visit_expr(body),
35+
PatUsableLocation::Local(stmts, expr) => {
36+
for stmt in stmts {
37+
visitor.visit_stmt(stmt);
38+
}
39+
if let Some(expr) = expr {
40+
visitor.visit_expr(expr);
41+
}
42+
},
43+
}
44+
}
45+
}
46+
847
/// returns `true` if expr contains match expr desugared from try
948
fn contains_try(expr: &hir::Expr<'_>) -> bool {
1049
struct TryFinder {
@@ -188,3 +227,34 @@ impl<'v> Visitor<'v> for LocalUsedVisitor<'v> {
188227
NestedVisitorMap::OnlyBodies(self.hir)
189228
}
190229
}
230+
231+
/// Calls the given function for each usage of the given local.
232+
pub fn foreach_local_usage<'tcx>(
233+
cx: &LateContext<'tcx>,
234+
local: HirId,
235+
visit: impl Visitable<'tcx>,
236+
f: impl FnMut(&'tcx Expr<'tcx>),
237+
) {
238+
struct V<'a, 'tcx, F> {
239+
cx: &'a LateContext<'tcx>,
240+
local: HirId,
241+
f: F,
242+
}
243+
impl<'tcx, F: FnMut(&'tcx Expr<'tcx>)> Visitor<'tcx> for V<'_, 'tcx, F> {
244+
type Map = Map<'tcx>;
245+
fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
246+
NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
247+
}
248+
249+
fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
250+
if path_to_local_id(e, self.local) {
251+
(self.f)(e);
252+
} else {
253+
walk_expr(self, e)
254+
}
255+
}
256+
}
257+
258+
let mut v = V { cx, local, f };
259+
visit.visit(&mut v);
260+
}

tests/ui/needless_borrow.fixed

-17
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ fn main() {
1818
let vec = Vec::new();
1919
let vec_val = g(&vec); // should not error, because `&Vec<T>` derefs to `&[T]`
2020
h(&"foo"); // should not error, because the `&&str` is required, due to `&Trait`
21-
if let Some(cake) = Some(&5) {}
2221
let garbl = match 42 {
2322
44 => &a,
2423
45 => {
@@ -43,19 +42,3 @@ trait Trait {}
4342
impl<'a> Trait for &'a str {}
4443

4544
fn h(_: &dyn Trait) {}
46-
#[warn(clippy::needless_borrow)]
47-
#[allow(dead_code)]
48-
fn issue_1432() {
49-
let mut v = Vec::<String>::new();
50-
let _ = v.iter_mut().filter(|&ref a| a.is_empty());
51-
let _ = v.iter().filter(|&a| a.is_empty());
52-
53-
let _ = v.iter().filter(|&a| a.is_empty());
54-
}
55-
56-
#[allow(dead_code)]
57-
#[warn(clippy::needless_borrow)]
58-
#[derive(Debug)]
59-
enum Foo<'a> {
60-
Str(&'a str),
61-
}

tests/ui/needless_borrow.rs

-17
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ fn main() {
1818
let vec = Vec::new();
1919
let vec_val = g(&vec); // should not error, because `&Vec<T>` derefs to `&[T]`
2020
h(&"foo"); // should not error, because the `&&str` is required, due to `&Trait`
21-
if let Some(ref cake) = Some(&5) {}
2221
let garbl = match 42 {
2322
44 => &a,
2423
45 => {
@@ -43,19 +42,3 @@ trait Trait {}
4342
impl<'a> Trait for &'a str {}
4443

4544
fn h(_: &dyn Trait) {}
46-
#[warn(clippy::needless_borrow)]
47-
#[allow(dead_code)]
48-
fn issue_1432() {
49-
let mut v = Vec::<String>::new();
50-
let _ = v.iter_mut().filter(|&ref a| a.is_empty());
51-
let _ = v.iter().filter(|&ref a| a.is_empty());
52-
53-
let _ = v.iter().filter(|&a| a.is_empty());
54-
}
55-
56-
#[allow(dead_code)]
57-
#[warn(clippy::needless_borrow)]
58-
#[derive(Debug)]
59-
enum Foo<'a> {
60-
Str(&'a str),
61-
}

tests/ui/needless_borrow.stderr

+2-14
Original file line numberDiff line numberDiff line change
@@ -6,23 +6,11 @@ LL | let c = x(&&a);
66
|
77
= note: `-D clippy::needless-borrow` implied by `-D warnings`
88

9-
error: this pattern creates a reference to a reference
10-
--> $DIR/needless_borrow.rs:21:17
11-
|
12-
LL | if let Some(ref cake) = Some(&5) {}
13-
| ^^^^^^^^ help: change this to: `cake`
14-
159
error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler
16-
--> $DIR/needless_borrow.rs:28:15
10+
--> $DIR/needless_borrow.rs:27:15
1711
|
1812
LL | 46 => &&a,
1913
| ^^^ help: change this to: `&a`
2014

21-
error: this pattern creates a reference to a reference
22-
--> $DIR/needless_borrow.rs:51:31
23-
|
24-
LL | let _ = v.iter().filter(|&ref a| a.is_empty());
25-
| ^^^^^ help: change this to: `a`
26-
27-
error: aborting due to 4 previous errors
15+
error: aborting due to 2 previous errors
2816

0 commit comments

Comments
 (0)