Skip to content

Commit 627d24c

Browse files
sinkuusinkuu
committed
Fix suggestion for let <pat> 
1 parent 263e60c commit 627d24c

File tree

3 files changed

+109
-69
lines changed

3 files changed

+109
-69
lines changed

‎clippy_lints/src/needless_pass_by_value.rs

Lines changed: 89 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use rustc::middle::expr_use_visitor as euv;
88
use rustc::middle::mem_categorization as mc;
99
use syntax::ast::NodeId;
1010
use syntax_pos::Span;
11+
use syntax::errors::DiagnosticBuilder;
1112
use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then,
1213
paths};
1314
use std::collections::{HashSet, HashMap};
@@ -59,7 +60,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
5960
return;
6061
}
6162

62-
// These are usually passed by value and only used by reference
63+
// Allows these to be passed by value.
6364
let fn_trait = cx.tcx.lang_items.fn_trait().expect("failed to find `Fn` trait");
6465
let asref_trait = get_trait_def_id(cx, &paths::ASREF_TRAIT).expect("failed to find `AsRef` trait");
6566
let borrow_trait = get_trait_def_id(cx, &paths::BORROW_TRAIT).expect("failed to find `Borrow` trait");
@@ -71,8 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
7172
.collect()
7273
};
7374

74-
// Collect moved variables and non-moving usages at `match`es from the function body
75-
let MovedVariablesCtxt { moved_vars, non_moving_matches, .. } = {
75+
// Collect moved variables and spans which will need dereferencings from the function body.
76+
let MovedVariablesCtxt { moved_vars, spans_need_deref, .. } = {
7677
let mut ctx = MovedVariablesCtxt::new(cx);
7778
let infcx = cx.tcx.borrowck_fake_infer_ctxt(body.id());
7879
{
@@ -119,11 +120,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
119120
continue;
120121
}
121122

122-
span_lint_and_then(cx,
123-
NEEDLESS_PASS_BY_VALUE,
124-
input.span,
125-
"this argument is passed by value, but not consumed in the function body",
126-
|db| {
123+
// Suggestion logic
124+
let sugg = |db: &mut DiagnosticBuilder| {
127125
if_let_chain! {[
128126
match_type(cx, ty, &paths::VEC),
129127
let TyPath(QPath::Resolved(_, ref path)) = input.node,
@@ -148,24 +146,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
148146
format!("&{}", snippet(cx, input.span, "_")));
149147
}
150148

151-
// For non-moving consumption at `match`es,
152-
// suggests adding `*` to dereference the added reference.
153-
// e.g. `match x { Some(_) => 1, None => 2 }`
154-
// -> `match *x { .. }`
155-
if let Some(matches) = non_moving_matches.get(&defid) {
156-
for (i, m) in matches.iter().enumerate() {
157-
if let ExprMatch(ref e, ..) = cx.tcx.hir.expect_expr(*m).node {
158-
db.span_suggestion(e.span,
159-
if i == 0 {
160-
"...and dereference it here"
161-
} else {
162-
"...and here"
163-
},
164-
format!("*{}", snippet(cx, e.span, "<expr>")));
165-
}
149+
// Suggests adding `*` to dereference the added reference.
150+
if let Some(spans) = spans_need_deref.get(&defid) {
151+
let mut spans: Vec<_> = spans.iter().cloned().collect();
152+
spans.sort();
153+
for (i, span) in spans.into_iter().enumerate() {
154+
db.span_suggestion(span,
155+
if i == 0 {
156+
"...and dereference it here"
157+
} else {
158+
"...and here"
159+
},
160+
format!("*{}", snippet(cx, span, "<expr>")));
166161
}
167162
}
168-
});
163+
};
164+
165+
span_lint_and_then(cx,
166+
NEEDLESS_PASS_BY_VALUE,
167+
input.span,
168+
"this argument is passed by value, but not consumed in the function body",
169+
sugg);
169170
}}
170171
}
171172
}
@@ -174,15 +175,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
174175
struct MovedVariablesCtxt<'a, 'tcx: 'a> {
175176
cx: &'a LateContext<'a, 'tcx>,
176177
moved_vars: HashSet<DefId>,
177-
non_moving_matches: HashMap<DefId, HashSet<NodeId>>,
178+
/// Spans which need to be prefixed with `*` for dereferencing the suggested additional
179+
/// reference.
180+
spans_need_deref: HashMap<DefId, HashSet<Span>>,
178181
}
179182

180183
impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
181184
fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
182185
MovedVariablesCtxt {
183186
cx: cx,
184187
moved_vars: HashSet::new(),
185-
non_moving_matches: HashMap::new(),
188+
spans_need_deref: HashMap::new(),
186189
}
187190
}
188191

@@ -196,6 +199,57 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
196199
self.moved_vars.insert(def_id);
197200
}}
198201
}
202+
203+
fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>) {
204+
let cmt = unwrap_downcast_or_interior(cmt);
205+
206+
if_let_chain! {[
207+
let mc::Categorization::Local(vid) = cmt.cat,
208+
let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
209+
], {
210+
let mut id = matched_pat.id;
211+
loop {
212+
let parent = self.cx.tcx.hir.get_parent_node(id);
213+
if id == parent {
214+
// no parent
215+
return;
216+
}
217+
id = parent;
218+
219+
if let Some(node) = self.cx.tcx.hir.find(id) {
220+
match node {
221+
map::Node::NodeExpr(e) => {
222+
// `match` and `if let`
223+
if let ExprMatch(ref c, ..) = e.node {
224+
self.spans_need_deref
225+
.entry(def_id)
226+
.or_insert_with(HashSet::new)
227+
.insert(c.span);
228+
}
229+
}
230+
231+
map::Node::NodeStmt(s) => {
232+
// `let <pat> = x;`
233+
if_let_chain! {[
234+
let StmtDecl(ref decl, _) = s.node,
235+
let DeclLocal(ref local) = decl.node,
236+
], {
237+
self.spans_need_deref
238+
.entry(def_id)
239+
.or_insert_with(HashSet::new)
240+
.insert(local.init
241+
.as_ref()
242+
.map(|e| e.span)
243+
.expect("`let` stmt without init aren't caught by match_pat"));
244+
}}
245+
}
246+
247+
_ => {}
248+
}
249+
}
250+
}
251+
}}
252+
}
199253
}
200254

201255
impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
@@ -209,27 +263,7 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
209263
if let euv::MatchMode::MovingMatch = mode {
210264
self.move_common(matched_pat.id, matched_pat.span, cmt);
211265
} else {
212-
let cmt = unwrap_downcast_or_interior(cmt);
213-
214-
if_let_chain! {[
215-
let mc::Categorization::Local(vid) = cmt.cat,
216-
let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
217-
], {
218-
// Find the `ExprMatch` which contains this pattern
219-
let mut match_id = matched_pat.id;
220-
loop {
221-
match_id = self.cx.tcx.hir.get_parent_node(match_id);
222-
if_let_chain! {[
223-
let Some(map::Node::NodeExpr(e)) = self.cx.tcx.hir.find(match_id),
224-
let ExprMatch(..) = e.node,
225-
], {
226-
break;
227-
}}
228-
}
229-
230-
self.non_moving_matches.entry(def_id).or_insert_with(HashSet::new)
231-
.insert(match_id);
232-
}}
266+
self.non_moving_pat(matched_pat, cmt);
233267
}
234268
}
235269

@@ -241,25 +275,18 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
241275

242276
fn borrow(
243277
&mut self,
244-
_borrow_id: NodeId,
245-
_borrow_span: Span,
246-
_cmt: mc::cmt<'tcx>,
247-
_loan_region: &'tcx ty::Region,
248-
_bk: ty::BorrowKind,
249-
_loan_cause: euv::LoanCause
278+
_: NodeId,
279+
_: Span,
280+
_: mc::cmt<'tcx>,
281+
_: &'tcx ty::Region,
282+
_: ty::BorrowKind,
283+
_: euv::LoanCause
250284
) {
251285
}
252286

253-
fn mutate(
254-
&mut self,
255-
_assignment_id: NodeId,
256-
_assignment_span: Span,
257-
_assignee_cmt: mc::cmt<'tcx>,
258-
_mode: euv::MutateMode
259-
) {
260-
}
287+
fn mutate(&mut self, _: NodeId, _: Span, _: mc::cmt<'tcx>, _: euv::MutateMode) {}
261288

262-
fn decl_without_init(&mut self, _id: NodeId, _span: Span) {}
289+
fn decl_without_init(&mut self, _: NodeId, _: Span) {}
263290
}
264291

265292

‎tests/ui/needless_pass_by_value.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
#![plugin(clippy)]
33

44
#![deny(needless_pass_by_value)]
5-
#![allow(dead_code, single_match)]
5+
#![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names)]
66

77
// `v` should be warned
88
// `w`, `x` and `y` are allowed (moved or mutated)
@@ -49,10 +49,12 @@ fn test_match(x: Option<Option<String>>, y: Option<Option<String>>) {
4949
};
5050
}
5151

52-
// x should be warned, but y is ok
53-
fn test_destructure(x: Wrapper, y: Wrapper) {
54-
let Wrapper(s) = y; // moved
52+
// x and y should be warned, but z is ok
53+
fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
54+
let Wrapper(s) = z; // moved
55+
let Wrapper(ref t) = y; // not moved
5556
assert_eq!(x.0.len(), s.len());
57+
println!("{}", t);
5658
}
5759

5860
fn main() {}

‎tests/ui/needless_pass_by_value.stderr

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,22 @@ help: ...and dereference it here
5353
error: this argument is passed by value, but not consumed in the function body
5454
--> $DIR/needless_pass_by_value.rs:53:24
5555
|
56-
53 | fn test_destructure(x: Wrapper, y: Wrapper) {
56+
53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
5757
| ^^^^^^^
5858
|
5959
help: consider taking a reference instead
60-
| fn test_destructure(x: &Wrapper, y: Wrapper) {
60+
| fn test_destructure(x: &Wrapper, y: Wrapper, z: Wrapper) {
6161

62-
error: aborting due to 6 previous errors
62+
error: this argument is passed by value, but not consumed in the function body
63+
--> $DIR/needless_pass_by_value.rs:53:36
64+
|
65+
53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
66+
| ^^^^^^^
67+
|
68+
help: consider taking a reference instead
69+
| fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) {
70+
help: ...and dereference it here
71+
| let Wrapper(ref t) = *y; // not moved
72+
73+
error: aborting due to 7 previous errors
6374

0 commit comments

Comments
 (0)