Skip to content

Commit 6c83d88

Browse files
sinkuusinkuu
committed
Suggestion for let <pat> 
1 parent 263e60c commit 6c83d88

File tree

3 files changed

+107
-69
lines changed

3 files changed

+107
-69
lines changed

‎clippy_lints/src/needless_pass_by_value.rs

Lines changed: 87 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,25 @@ 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+
for (i, span) in spans.iter().cloned().enumerate() {
152+
db.span_suggestion(span,
153+
if i == 0 {
154+
"...and dereference it here"
155+
} else {
156+
"...and here"
157+
},
158+
format!("*{}", snippet(cx, span, "<expr>")));
166159
}
167160
}
168-
});
161+
};
162+
163+
span_lint_and_then(cx,
164+
NEEDLESS_PASS_BY_VALUE,
165+
input.span,
166+
"this argument is passed by value, but not consumed in the function body",
167+
sugg);
169168
}}
170169
}
171170
}
@@ -174,15 +173,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
174173
struct MovedVariablesCtxt<'a, 'tcx: 'a> {
175174
cx: &'a LateContext<'a, 'tcx>,
176175
moved_vars: HashSet<DefId>,
177-
non_moving_matches: HashMap<DefId, HashSet<NodeId>>,
176+
/// Spans which need to be prefixed with `*` for dereferencing the suggested additional
177+
/// reference.
178+
spans_need_deref: HashMap<DefId, HashSet<Span>>,
178179
}
179180

180181
impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
181182
fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
182183
MovedVariablesCtxt {
183184
cx: cx,
184185
moved_vars: HashSet::new(),
185-
non_moving_matches: HashMap::new(),
186+
spans_need_deref: HashMap::new(),
186187
}
187188
}
188189

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

201253
impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
@@ -209,27 +261,7 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
209261
if let euv::MatchMode::MovingMatch = mode {
210262
self.move_common(matched_pat.id, matched_pat.span, cmt);
211263
} 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-
}}
264+
self.non_moving_pat(matched_pat, cmt);
233265
}
234266
}
235267

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

242274
fn borrow(
243275
&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
276+
_: NodeId,
277+
_: Span,
278+
_: mc::cmt<'tcx>,
279+
_: &'tcx ty::Region,
280+
_: ty::BorrowKind,
281+
_: euv::LoanCause
250282
) {
251283
}
252284

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-
}
285+
fn mutate(&mut self, _: NodeId, _: Span, _: mc::cmt<'tcx>, _: euv::MutateMode) {}
261286

262-
fn decl_without_init(&mut self, _id: NodeId, _span: Span) {}
287+
fn decl_without_init(&mut self, _: NodeId, _: Span) {}
263288
}
264289

265290

‎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)