mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Support non-moving usages at match

Browse Source 
This `match` is not moving the `String`:

```rust
fn foo(x: Option<String>) -> i32 {
    match x {
        Some(_) => 1,
        None => 2,
    }
}
```

With this change, it will be linted and suggested to add `*` to deref it.

```rust
fn foo(x: &Option<String>) -> i32 {
    match *x {
        Some(_) => 1,
        None => 2,
    }
}
```
This commit is contained in:
sinkuu 2017-02-20 16:45:37 +09:00
parent 0a6bc6031a
commit f1b0b774e7
5 changed files with 132 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
clippy_lints/src/array_indexing.rs
View File

@ -83,7 +83,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing {
.map(|end| constcx.eval(end, ExprTypeChecked))
.map(|v| v.ok());
if let Some((start, end)) = to_const_range(start, end, range.limits, size) {
if let Some((start, end)) = to_const_range(&start, &end, range.limits, size) {
if start > size || end > size {
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds");
}
@ -109,18 +109,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing {
/// Returns an option containing a tuple with the start and end (exclusive) of the range.
fn to_const_range(
start: Option<Option<ConstVal>>,
end: Option<Option<ConstVal>>,
start: &Option<Option<ConstVal>>,
end: &Option<Option<ConstVal>>,
limits: RangeLimits,
array_size: ConstInt
) -> Option<(ConstInt, ConstInt)> {
let start = match start {
let start = match *start {
Some(Some(ConstVal::Integral(x))) => x,
Some(_) => return None,
None => ConstInt::Infer(0),
};
let end = match end {
let end = match *end {
Some(Some(ConstVal::Integral(x))) => {
if limits == RangeLimits::Closed {
(x + ConstInt::Infer(1)).expect("such a big array is not realistic")

6
clippy_lints/src/consts.rs
View File

@ -203,9 +203,9 @@ pub fn lit_to_constant(lit: &LitKind) -> Constant {
}
}
fn constant_not(o: Constant) -> Option<Constant> {
fn constant_not(o: &Constant) -> Option<Constant> {
use self::Constant::*;
match o {
match *o {
Bool(b) => Some(Bool(!b)),
Int(value) => (!value).ok().map(Int),
_ => None,
@ -271,7 +271,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
},
ExprUnary(op, ref operand) => {
self.expr(operand).and_then(|o| match op {
UnNot => constant_not(o),
UnNot => constant_not(&o),
UnNeg => constant_negate(o),
UnDeref => Some(o),
})

109
clippy_lints/src/needless_pass_by_value.rs
View File

@ -10,7 +10,7 @@ use syntax::ast::NodeId;
use syntax_pos::Span;
use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then,
paths};
use std::collections::HashSet;
use std::collections::{HashSet, HashMap};
/// **What it does:** Checks for functions taking arguments by value, but not consuming them in its
/// body.
@ -71,15 +71,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
.collect()
};
// Collect moved variables from the function body
let moved_vars = {
// Collect moved variables and non-moving usages at `match`es from the function body
let MovedVariablesCtxt { moved_vars, non_moving_matches, .. } = {
let mut ctx = MovedVariablesCtxt::new(cx);
let infcx = cx.tcx.borrowck_fake_infer_ctxt(body.id());
{
let mut v = euv::ExprUseVisitor::new(&mut ctx, &infcx);
v.consume_body(body);
}
ctx.moved_vars
ctx
};
let fn_def_id = cx.tcx.hir.local_def_id(node_id);
@ -90,8 +90,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
for ((input, ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) {
// Determines whether `ty` implements `Borrow<U>` (U != ty) specifically.
// `implements_trait(.., borrow_trait, ..)` is useless
// due to the `Borrow<T> for T` blanket impl.
// This is needed due to the `Borrow<T> for T` blanket impl.
let implements_borrow_trait = preds.iter()
.filter_map(|pred| if let ty::Predicate::Trait(ref poly_trait_ref) = *pred {
Some(poly_trait_ref.skip_binder())
@ -99,7 +98,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
None
})
.filter(|tpred| tpred.def_id() == borrow_trait && &tpred.self_ty() == ty)
.any(|tpred| &tpred.input_types().nth(1).expect("Borrow trait must have an input") != ty);
.any(|tpred| &tpred.input_types().nth(1).expect("Borrow trait must have an parameter") != ty);
if_let_chain! {[
!is_self(arg),
@ -148,6 +147,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
"consider taking a reference instead",
format!("&{}", snippet(cx, input.span, "_")));
}
// For non-moving consumption at `match`es,
// suggests adding `*` to dereference the added reference.
// e.g. `match x { Some(_) => 1, None => 2 }`
// -> `match *x { .. }`
if let Some(matches) = non_moving_matches.get(&defid) {
for (i, m) in matches.iter().enumerate() {
if let ExprMatch(ref e, ..) = cx.tcx.hir.expect_expr(*m).node {
db.span_suggestion(e.span,
if i == 0 {
"...and dereference it here"
} else {
"...and here"
},
format!("*{}", snippet(cx, e.span, "<expr>")));
}
}
}
});
}}
}
@ -157,6 +174,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
struct MovedVariablesCtxt<'a, 'tcx: 'a> {
cx: &'a LateContext<'a, 'tcx>,
moved_vars: HashSet<DefId>,
non_moving_matches: HashMap<DefId, HashSet<NodeId>>,
}
impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
@ -164,43 +182,61 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
MovedVariablesCtxt {
cx: cx,
moved_vars: HashSet::new(),
non_moving_matches: HashMap::new(),
}
}
fn consume_common(&mut self, _span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) {
/*::utils::span_lint(self.cx,
NEEDLESS_PASS_BY_VALUE,
span,
&format!("consumed here, {:?} {:?}", mode, cmt.cat));*/
if_let_chain! {[
let euv::ConsumeMode::Move(_) = mode,
let mc::Categorization::Local(vid) = cmt.cat,
], {
if let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid) {
self.moved_vars.insert(def_id);
}
}}
fn move_common(&mut self, _consume_id: NodeId, _span: Span, cmt: mc::cmt<'tcx>) {
let cmt = unwrap_downcast_or_interior(cmt);
if_let_chain! {[
let mc::Categorization::Local(vid) = cmt.cat,
let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
], {
self.moved_vars.insert(def_id);
}}
}
}
impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
fn consume(&mut self, _consume_id: NodeId, consume_span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) {
self.consume_common(consume_span, cmt, mode);
fn consume(&mut self, consume_id: NodeId, consume_span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) {
if let euv::ConsumeMode::Move(_) = mode {
self.move_common(consume_id, consume_span, cmt);
}
}
fn matched_pat(&mut self, matched_pat: &Pat, mut cmt: mc::cmt<'tcx>, _mode: euv::MatchMode) {
if let mc::Categorization::Downcast(c, _) = cmt.cat.clone() {
cmt = c;
}
fn matched_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::MatchMode) {
if let euv::MatchMode::MovingMatch = mode {
self.move_common(matched_pat.id, matched_pat.span, cmt);
} else {
let cmt = unwrap_downcast_or_interior(cmt);
// if let euv::MatchMode::MovingMatch = mode {
self.consume_common(matched_pat.span, cmt, euv::ConsumeMode::Move(euv::MoveReason::PatBindingMove));
// }
if_let_chain! {[
let mc::Categorization::Local(vid) = cmt.cat,
let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid),
], {
// Find the `ExprMatch` which contains this pattern
let mut match_id = matched_pat.id;
loop {
match_id = self.cx.tcx.hir.get_parent_node(match_id);
if_let_chain! {[
let Some(map::Node::NodeExpr(e)) = self.cx.tcx.hir.find(match_id),
let ExprMatch(..) = e.node,
], {
break;
}}
}
self.non_moving_matches.entry(def_id).or_insert_with(HashSet::new)
.insert(match_id);
}}
}
}
fn consume_pat(&mut self, consume_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) {
self.consume_common(consume_pat.span, cmt, mode);
if let euv::ConsumeMode::Move(_) = mode {
self.move_common(consume_pat.id, consume_pat.span, cmt);
}
}
fn borrow(
@ -225,3 +261,16 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> {
fn decl_without_init(&mut self, _id: NodeId, _span: Span) {}
}
fn unwrap_downcast_or_interior(mut cmt: mc::cmt) -> mc::cmt {
loop {
match cmt.cat.clone() {
mc::Categorization::Downcast(c, _) |
mc::Categorization::Interior(c, _) => {
cmt = c;
},
_ => return cmt,
}
}
}

25
tests/ui/needless_pass_by_value.rs
View File

@ -2,9 +2,9 @@
#![plugin(clippy)]
#![deny(needless_pass_by_value)]
#![allow(dead_code)]
#![allow(dead_code, single_match)]
// `v` will be warned
// `v` should be warned
// `w`, `x` and `y` are allowed (moved or mutated)
fn foo<T: Default>(v: Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
assert_eq!(v.len(), 42);
@ -25,8 +25,8 @@ fn bar(x: String, y: Wrapper) {
assert_eq!(y.0.len(), 42);
}
// U implements `Borrow<U>`, but should be warned correctly
fn test_borrow_trait<T: std::borrow::Borrow<str>, U>(t: T, u: U) {
// U implements `Borrow<U>`, but warned correctly
println!("{}", t.borrow());
consume(&u);
}
@ -36,4 +36,23 @@ fn test_fn<F: Fn(i32) -> i32>(f: F) {
f(1);
}
// x should be warned, but y is ok
fn test_match(x: Option<Option<String>>, y: Option<Option<String>>) {
match x {
Some(Some(_)) => 1, // not moved
_ => 0,
};
match y {
Some(Some(s)) => consume(s), // moved
_ => (),
};
}
// x should be warned, but y is ok
fn test_destructure(x: Wrapper, y: Wrapper) {
let Wrapper(s) = y; // moved
assert_eq!(x.0.len(), s.len());
}
fn main() {}

26
tests/ui/needless_pass_by_value.stderr
View File

@ -31,13 +31,33 @@ help: consider taking a reference instead
| fn bar(x: String, y: &Wrapper) {
error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:28:63
--> $DIR/needless_pass_by_value.rs:29:63
|
28 | fn test_borrow_trait<T: std::borrow::Borrow<str>, U>(t: T, u: U) {
29 | fn test_borrow_trait<T: std::borrow::Borrow<str>, U>(t: T, u: U) {
| ^
|
help: consider taking a reference instead
| fn test_borrow_trait<T: std::borrow::Borrow<str>, U>(t: T, u: &U) {
error: aborting due to 4 previous errors
error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:40:18
|
40 | fn test_match(x: Option<Option<String>>, y: Option<Option<String>>) {
| ^^^^^^^^^^^^^^^^^^^^^^
|
help: consider taking a reference instead
| fn test_match(x: &Option<Option<String>>, y: Option<Option<String>>) {
help: ...and dereference it here
| match *x {
error: this argument is passed by value, but not consumed in the function body
--> $DIR/needless_pass_by_value.rs:53:24
|
53 | fn test_destructure(x: Wrapper, y: Wrapper) {
| ^^^^^^^
|
help: consider taking a reference instead
| fn test_destructure(x: &Wrapper, y: Wrapper) {
error: aborting due to 6 previous errors