Lint needless take-by-value

CHANGELOG.md

@@ -383,6 +383,7 @@ All notable changes to this project will be documented in this file.
 [`needless_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes
 [`needless_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop
 [`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return
+[`needless_take_by_value`]: https://github.com/Manishearth/rust-clippy/wiki#needless_take_by_value
 [`needless_update`]: https://github.com/Manishearth/rust-clippy/wiki#needless_update
 [`neg_multiply`]: https://github.com/Manishearth/rust-clippy/wiki#neg_multiply
 [`never_loop`]: https://github.com/Manishearth/rust-clippy/wiki#never_loop

README.md

@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 191 lints included in this crate:
+There are 192 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -289,6 +289,7 @@ name
 [needless_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes)                               | warn    | using explicit lifetimes for references in function arguments when elision rules would allow omitting them
 [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop)                             | warn    | for-looping over a range of indices where an iterator over items would do
 [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return)                                     | warn    | using a return statement like `return expr;` where an expression would suffice
+[needless_take_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_take_by_value)                       | warn    | taking arguments by value, but only using them by reference
 [needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update)                                     | warn    | using `Foo { ..base }` when there are no missing fields
 [neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply)                                           | warn    | multiplying integers with -1
 [never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop)                                               | warn    | any loop with an unconditional `break` statement

clippy_lints/src/consts.rs

@@ -216,12 +216,12 @@ fn constant_negate(o: Constant) -> Option<Constant> {
     use self::Constant::*;
     match o {
         Int(value) => (-value).ok().map(Int),
-        Float(is, ty) => Some(Float(neg_float_str(is), ty)),
+        Float(is, ty) => Some(Float(neg_float_str(&is), ty)),
         _ => None,
     }
 }
 
-fn neg_float_str(s: String) -> String {
+fn neg_float_str(s: &str) -> String {
     if s.starts_with('-') {
         s[1..].to_owned()
     } else {

clippy_lints/src/lib.rs

@@ -107,6 +107,7 @@ pub mod mut_reference;
 pub mod mutex_atomic;
 pub mod needless_bool;
 pub mod needless_borrow;
+pub mod needless_take_by_value;
 pub mod needless_update;
 pub mod neg_multiply;
 pub mod new_without_default;
@@ -299,6 +300,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount);
     reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold));
     reg.register_late_lint_pass(box should_assert_eq::ShouldAssertEq);
+    reg.register_late_lint_pass(box needless_take_by_value::NeedlessTakeByValue);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -455,6 +457,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         needless_bool::BOOL_COMPARISON,
         needless_bool::NEEDLESS_BOOL,
         needless_borrow::NEEDLESS_BORROW,
+        needless_take_by_value::NEEDLESS_TAKE_BY_VALUE,
         needless_update::NEEDLESS_UPDATE,
         neg_multiply::NEG_MULTIPLY,
         new_without_default::NEW_WITHOUT_DEFAULT,

clippy_lints/src/needless_take_by_value.rs

@@ -0,0 +1,178 @@
+use rustc::hir::*;
+use rustc::hir::intravisit::FnKind;
+use rustc::hir::def_id::DefId;
+use rustc::lint::*;
+use rustc::ty;
+use rustc::middle::expr_use_visitor as euv;
+use rustc::middle::mem_categorization as mc;
+use syntax::ast::NodeId;
+use syntax_pos::Span;
+use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, snippet, span_lint_and_then, paths};
+use std::collections::HashSet;
+
+/// **What it does:** Checks for functions taking arguments by value, but only using them by
+/// reference.
+///
+/// **Why is this bad?**
+///
+/// **Known problems:** Hopefully none.
+///
+/// **Example:**
+/// ```rust
+/// fn foo(v: Vec<i32>) {
+///     assert_eq!(v.len(), 42);
+/// }
+/// ```
+declare_lint! {
+    pub NEEDLESS_TAKE_BY_VALUE,
+    Warn,
+    "taking arguments by value, but only using them by reference"
+}
+
+pub struct NeedlessTakeByValue;
+
+impl LintPass for NeedlessTakeByValue {
+    fn get_lints(&self) -> LintArray {
+        lint_array![NEEDLESS_TAKE_BY_VALUE]
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'a, 'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl,
+        body: &'tcx Body,
+        span: Span,
+        node_id: NodeId
+    ) {
+        if in_macro(cx, span) {
+            return;
+        }
+
+        if let FnKind::ItemFn(..) = kind {
+        } else {
+            return;
+        }
+
+        // These are usually took by value and only used by reference
+        let fn_trait = cx.tcx.lang_items.fn_trait().expect("failed to find `Fn` trait");
+        let asref_trait = get_trait_def_id(cx, &paths::ASREF_TRAIT).expect("failed to find `AsRef` trait");
+        let borrow_trait = get_trait_def_id(cx, &paths::BORROW_TRAIT).expect("failed to find `Borrow` trait");
+
+        // Collect moved variables from the function body
+        let moved_vars = {
+            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
+        };
+
+        let fn_def_id = cx.tcx.hir.local_def_id(node_id);
+        let param_env = ty::ParameterEnvironment::for_item(cx.tcx, node_id);
+        let fn_sig = cx.tcx.item_type(fn_def_id).fn_sig();
+        let fn_sig = cx.tcx.liberate_late_bound_regions(param_env.free_id_outlive, fn_sig);
+
+        for ((input, ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) {
+            if_let_chain! {[
+                !is_self(arg),
+                !ty.is_mutable_pointer(),
+                !is_copy(cx, ty, node_id),
+                !implements_trait(cx, ty, fn_trait, &[], Some(node_id)),
+                !implements_trait(cx, ty, asref_trait, &[], Some(node_id)),
+                !implements_trait(cx, ty, borrow_trait, &[], Some(node_id)),
+
+                let PatKind::Binding(mode, defid, ..) = arg.pat.node,
+                !moved_vars.contains(&defid),
+            ], {
+                // Note: `toplevel_ref_arg` warns if `BindByRef`
+                let m = match mode {
+                    BindingMode::BindByRef(m) | BindingMode::BindByValue(m) => m,
+                };
+                if m == Mutability::MutMutable {
+                    continue;
+                }
+
+                span_lint_and_then(cx,
+                          NEEDLESS_TAKE_BY_VALUE,
+                          input.span,
+                          "this function taking a value by value, but only using them by reference",
+                          |db| {
+                              db.span_suggestion(input.span,
+                                                 "consider taking a reference instead",
+                                                 format!("&{}", snippet(cx, input.span, "_")));
+                          });
+            }}
+        }
+    }
+}
+
+struct MovedVariablesCtxt<'a, 'tcx: 'a> {
+    cx: &'a LateContext<'a, 'tcx>,
+    moved_vars: HashSet<DefId>,
+}
+
+impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> {
+    fn new(cx: &'a LateContext<'a, 'tcx>) -> Self {
+        MovedVariablesCtxt {
+            cx: cx,
+            moved_vars: HashSet::new(),
+        }
+    }
+
+    fn consume_common(
+        &mut self,
+        _consume_id: NodeId,
+        _consume_span: Span,
+        cmt: mc::cmt<'tcx>,
+        mode: euv::ConsumeMode
+    ) {
+        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);
+            }
+        }}
+
+    }
+}
+
+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_id, consume_span, cmt, mode);
+    }
+
+    fn matched_pat(&mut self, _matched_pat: &Pat, _cmt: mc::cmt, _mode: euv::MatchMode) {}
+
+    fn consume_pat(&mut self, consume_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) {
+        self.consume_common(consume_pat.id, consume_pat.span, cmt, mode);
+    }
+
+    fn borrow(
+        &mut self,
+        _borrow_id: NodeId,
+        _borrow_span: Span,
+        _cmt: mc::cmt<'tcx>,
+        _loan_region: &'tcx ty::Region,
+        _bk: ty::BorrowKind,
+        _loan_cause: euv::LoanCause
+    ) {
+    }
+
+    fn mutate(
+        &mut self,
+        _assignment_id: NodeId,
+        _assignment_span: Span,
+        _assignee_cmt: mc::cmt<'tcx>,
+        _mode: euv::MutateMode
+    ) {
+    }
+
+    fn decl_without_init(&mut self, _id: NodeId, _span: Span) {}
+}

clippy_lints/src/utils/paths.rs

@@ -1,7 +1,9 @@
 //! This module contains paths to types and functions Clippy needs to know about.
 
+pub const ASREF_TRAIT: [&'static str; 3] = ["core", "convert", "AsRef"];
 pub const BEGIN_PANIC: [&'static str; 3] = ["std", "panicking", "begin_panic"];
 pub const BINARY_HEAP: [&'static str; 3] = ["collections", "binary_heap", "BinaryHeap"];
+pub const BORROW_TRAIT: [&'static str; 3] = ["core", "borrow", "Borrow"];
 pub const BOX: [&'static str; 3] = ["std", "boxed", "Box"];
 pub const BOX_NEW: [&'static str; 4] = ["std", "boxed", "Box", "new"];
 pub const BTREEMAP: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"];

tests/issue-825.rs

@@ -4,7 +4,7 @@
 #![allow(warnings)]
 
 // this should compile in a reasonable amount of time
-fn rust_type_id(name: String) {
+fn rust_type_id(name: &str) {
     if "bool" == &name[..] || "uint" == &name[..] || "u8" == &name[..] || "u16" == &name[..] ||
        "u32" == &name[..] || "f32" == &name[..] || "f64" == &name[..] || "i8" == &name[..] ||
        "i16" == &name[..] || "i32" == &name[..] ||

tests/ui/absurd-extreme-comparisons.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(absurd_extreme_comparisons)]
-#![allow(unused, eq_op, no_effect, unnecessary_operation)]
+#![allow(unused, eq_op, no_effect, unnecessary_operation, needless_take_by_value)]
 
 fn main() {
     const Z: u32 = 0;

tests/ui/box_vec.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(clippy)]
-#![allow(boxed_local)]
+#![allow(boxed_local, needless_take_by_value)]
 #![allow(blacklisted_name)]
 
 macro_rules! boxit {

tests/ui/complex_types.rs

@@ -1,7 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 #![deny(clippy)]
-#![allow(unused)]
+#![allow(unused, needless_take_by_value)]
 #![feature(associated_consts, associated_type_defaults)]
 
 type Alias = Vec<Vec<Box<(u32, u32, u32, u32)>>>; // no warning here

tests/ui/dlist.rs

@@ -4,7 +4,7 @@
 
 #![plugin(clippy)]
 #![deny(clippy)]
-#![allow(dead_code)]
+#![allow(dead_code, needless_take_by_value)]
 
 extern crate collections;
 use collections::linked_list::LinkedList;

tests/ui/drop_forget_ref.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(drop_ref, forget_ref)]
-#![allow(toplevel_ref_arg, similar_names)]
+#![allow(toplevel_ref_arg, similar_names, needless_take_by_value)]
 
 use std::mem::{drop, forget};
 

tests/ui/entry.rs

@@ -1,6 +1,6 @@
 #![feature(plugin)]
 #![plugin(clippy)]
-#![allow(unused)]
+#![allow(unused, needless_take_by_value)]
 
 #![deny(map_entry)]
 

tests/ui/eta.rs

@@ -1,6 +1,6 @@
 #![feature(plugin)]
 #![plugin(clippy)]
-#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names)]
+#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names, needless_take_by_value)]
 #![deny(redundant_closure)]
 
 fn main() {

tests/ui/lifetimes.rs

@@ -2,7 +2,7 @@
 #![plugin(clippy)]
 
 #![deny(needless_lifetimes, unused_lifetimes)]
-#![allow(dead_code)]
+#![allow(dead_code, needless_take_by_value)]
 
 fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) { }
 

tests/ui/needless_take_by_value.rs

@@ -0,0 +1,26 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(needless_take_by_value)]
+#![allow(dead_code)]
+
+// `v` will 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);
+
+    consume(w);
+
+    x.push(T::default());
+
+    y
+}
+
+fn consume<T>(_: T) {}
+
+// ok
+fn test_fn<F: Fn(i32) -> i32>(f: F) {
+    f(1);
+}
+
+fn main() {}

tests/ui/needless_take_by_value.stderr

@@ -0,0 +1,16 @@
+error: this function taking a value by value, but only using them by reference
+ --> $DIR/needless_take_by_value.rs:9:23
+  |
+9 | fn foo<T: Default>(v: Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
+  |                       ^^^^^^
+  |
+note: lint level defined here
+ --> $DIR/needless_take_by_value.rs:4:9
+  |
+4 | #![deny(needless_take_by_value)]
+  |         ^^^^^^^^^^^^^^^^^^^^^^
+help: consider taking a reference instead
+  | fn foo<T: Default>(v: &Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
+
+error: aborting due to previous error
+

tests/ui/should_assert_eq.rs

@@ -1,6 +1,7 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
+#![allow(needless_take_by_value)]
 #![deny(should_assert_eq)]
 
 #[derive(PartialEq, Eq)]

tests/ui/should_assert_eq.stderr

@@ -1,28 +1,28 @@
 error: use `assert_eq` for better reporting
-  --> $DIR/should_assert_eq.rs:13:5
+  --> $DIR/should_assert_eq.rs:14:5
    |
-13 |     assert!(1 == 2);
+14 |     assert!(1 == 2);
    |     ^^^^^^^^^^^^^^^^
    |
 note: lint level defined here
-  --> $DIR/should_assert_eq.rs:4:9
+  --> $DIR/should_assert_eq.rs:5:9
    |
-4  | #![deny(should_assert_eq)]
+5  | #![deny(should_assert_eq)]
    |         ^^^^^^^^^^^^^^^^
    = note: this error originates in a macro outside of the current crate
 
 error: use `assert_eq` for better reporting
-  --> $DIR/should_assert_eq.rs:14:5
+  --> $DIR/should_assert_eq.rs:15:5
    |
-14 |     assert!(Debug(1) == Debug(2));
+15 |     assert!(Debug(1) == Debug(2));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in a macro outside of the current crate
 
 error: use `assert_eq` for better reporting
-  --> $DIR/should_assert_eq.rs:21:5
+  --> $DIR/should_assert_eq.rs:22:5
    |
-21 |     assert!(x == y);
+22 |     assert!(x == y);
    |     ^^^^^^^^^^^^^^^^
    |
    = note: this error originates in a macro outside of the current crate

tests/ui/unused_lt.rs

@@ -1,6 +1,6 @@
 #![feature(plugin)]
 #![plugin(clippy)]
-#![allow(unused, dead_code, needless_lifetimes)]
+#![allow(unused, dead_code, needless_lifetimes, needless_take_by_value)]
 #![deny(unused_lifetimes)]
 
 fn empty() {