Merge pull request #2199 from sinkuu/needless_pass_by_value_method

 Extend needless_pass_by_value to methods

Author: oli-obk

clippy_lints/src/collapsible_if.rs

@@ -134,7 +134,7 @@ fn check_collapsible_no_if_let(cx: &EarlyContext, expr: &ast::Expr, check: &ast:
                 db.span_suggestion(expr.span,
                                    "try",
                                    format!("if {} {}",
-                                           lhs.and(rhs),
+                                           lhs.and(&rhs),
                                            snippet_block(cx, content.span, "..")));
             });
         }

clippy_lints/src/int_plus_one.rs

@@ -45,6 +45,7 @@ impl LintPass for IntPlusOne {
 // x + 1 <= y
 // x <= y - 1
 
+#[derive(Copy, Clone)]
 enum Side {
     LHS,
     RHS,

clippy_lints/src/needless_pass_by_value.rs

@@ -1,4 +1,5 @@
 use rustc::hir::*;
+use rustc::hir::map::*;
 use rustc::hir::intravisit::FnKind;
 use rustc::lint::*;
 use rustc::ty::{self, RegionKind, TypeFoldable};
@@ -22,13 +23,20 @@ use std::borrow::Cow;
 /// sometimes avoid
 /// unnecessary allocations.
 ///
-/// **Known problems:** Hopefully none.
+/// **Known problems:**
+/// * This lint suggests taking an argument by reference,
+/// however sometimes it is better to let users decide the argument type
+/// (by using `Borrow` trait, for example), depending on how the function is used.
 ///
 /// **Example:**
 /// ```rust
 /// fn foo(v: Vec<i32>) {
 ///     assert_eq!(v.len(), 42);
 /// }
+/// // should be
+/// fn foo(v: &[i32]) {
+///     assert_eq!(v.len(), 42);
+/// }
 /// ```
 declare_lint! {
     pub NEEDLESS_PASS_BY_VALUE,
@@ -73,9 +81,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                     }
                 }
             },
+            FnKind::Method(..) => (),
             _ => return,
         }
 
+        // Exclude non-inherent impls
+        if let Some(NodeItem(item)) = cx.tcx.hir.find(cx.tcx.hir.get_parent_node(node_id)) {
+            if matches!(item.node, ItemImpl(_, _, _, _, Some(_), _, _) | ItemDefaultImpl(..)) {
+                return;
+            }
+        }
+
         // Allow `Borrow` or functions to be taken by value
         let borrow_trait = need!(get_trait_def_id(cx, &paths::BORROW_TRAIT));
         let fn_traits = [
@@ -109,7 +125,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
         } = {
             let mut ctx = MovedVariablesCtxt::new(cx);
             let region_scope_tree = &cx.tcx.region_scope_tree(fn_def_id);
-            euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).consume_body(body);
+            euv::ExprUseVisitor::new(&mut ctx, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None)
+                .consume_body(body);
             ctx
         };
 
@@ -127,6 +144,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                 return;
             }
 
+            // Ignore `self`s.
+            if idx == 0 {
+                if let PatKind::Binding(_, _, name, ..) = arg.pat.node {
+                    if name.node.as_str() == "self" {
+                        continue;
+                    }
+                }
+            }
+
             // * Exclude a type that is specifically bounded by `Borrow`.
             // * Exclude a type whose reference also fulfills its bound.
             //   (e.g. `std::convert::AsRef`, `serde::Serialize`)
@@ -163,7 +189,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                     if mode == BindingAnnotation::Mutable || mode == BindingAnnotation::RefMut {
                         continue;
                     }
-    
+
                     // Dereference suggestion
                     let sugg = |db: &mut DiagnosticBuilder| {
                         let deref_span = spans_need_deref.get(&canonical_id);
@@ -181,7 +207,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                                 db.span_suggestion(input.span,
                                                 "consider changing the type to",
                                                 slice_ty);
-        
+
                                 for (span, suggestion) in clone_spans {
                                     db.span_suggestion(
                                         span,
@@ -193,18 +219,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                                         suggestion.into()
                                     );
                                 }
-        
+
                                 // cannot be destructured, no need for `*` suggestion
                                 assert!(deref_span.is_none());
                                 return;
                             }
                         }
-    
+
                         if match_type(cx, ty, &paths::STRING) {
                             if let Some(clone_spans) =
                                 get_spans(cx, Some(body.id()), idx, &[("clone", ".to_string()"), ("as_str", "")]) {
                                 db.span_suggestion(input.span, "consider changing the type to", "&str".to_string());
-    
+
                                 for (span, suggestion) in clone_spans {
                                     db.span_suggestion(
                                         span,
@@ -216,14 +242,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                                         suggestion.into(),
                                     );
                                 }
-    
+
                                 assert!(deref_span.is_none());
                                 return;
                             }
                         }
-    
+
                         let mut spans = vec![(input.span, format!("&{}", snippet(cx, input.span, "_")))];
-    
+
                         // Suggests adding `*` to dereference the added reference.
                         if let Some(deref_span) = deref_span {
                             spans.extend(
@@ -236,7 +262,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
                         }
                         multispan_sugg(db, "consider taking a reference instead".to_string(), spans);
                     };
-    
+
                     span_lint_and_then(
                         cx,
                         NEEDLESS_PASS_BY_VALUE,

clippy_lints/src/utils/sugg.rs

@@ -136,8 +136,8 @@ impl<'a> Sugg<'a> {
     }
 
     /// Convenience method to create the `<lhs> && <rhs>` suggestion.
-    pub fn and(self, rhs: Self) -> Sugg<'static> {
-        make_binop(ast::BinOpKind::And, &self, &rhs)
+    pub fn and(self, rhs: &Self) -> Sugg<'static> {
+        make_binop(ast::BinOpKind::And, &self, rhs)
     }
 
     /// Convenience method to create the `<lhs> as <rhs>` suggestion.
@@ -162,10 +162,10 @@ impl<'a> Sugg<'a> {
 
     /// Convenience method to create the `<lhs>..<rhs>` or `<lhs>...<rhs>`
     /// suggestion.
-    pub fn range(self, end: Self, limit: ast::RangeLimits) -> Sugg<'static> {
+    pub fn range(self, end: &Self, limit: ast::RangeLimits) -> Sugg<'static> {
         match limit {
-            ast::RangeLimits::HalfOpen => make_assoc(AssocOp::DotDot, &self, &end),
-            ast::RangeLimits::Closed => make_assoc(AssocOp::DotDotEq, &self, &end),
+            ast::RangeLimits::HalfOpen => make_assoc(AssocOp::DotDot, &self, end),
+            ast::RangeLimits::Closed => make_assoc(AssocOp::DotDotEq, &self, end),
         }
     }
 

tests/ui/methods.rs

@@ -1,9 +1,9 @@
 
 #![feature(const_fn)]
 
-
 #![warn(clippy, clippy_pedantic)]
-#![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default, new_without_default_derive, missing_docs_in_private_items)]
+#![allow(blacklisted_name, unused, print_stdout, non_ascii_literal, new_without_default,
+    new_without_default_derive, missing_docs_in_private_items, needless_pass_by_value)]
 
 use std::collections::BTreeMap;
 use std::collections::HashMap;

tests/ui/needless_pass_by_value.rs

@@ -65,7 +65,7 @@ fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) {
 
 trait Foo {}
 
-// `S: Serialize` can be passed by value
+// `S: Serialize` is allowed to be passed by value, since a caller can pass `&S` instead
 trait Serialize {}
 impl<'a, T> Serialize for &'a T where T: Serialize {}
 impl Serialize for i32 {}
@@ -79,4 +79,28 @@ fn issue_2114(s: String, t: String, u: Vec<i32>, v: Vec<i32>) {
     let _ = v.clone();
 }
 
+struct S<T, U>(T, U);
+
+impl<T: Serialize, U> S<T, U> {
+    fn foo(
+        self, // taking `self` by value is always allowed
+        s: String,
+        t: String,
+    ) -> usize {
+        s.len() + t.capacity()
+    }
+
+    fn bar(
+        _t: T, // Ok, since `&T: Serialize` too
+    ) {
+    }
+
+    fn baz(
+        &self,
+        _u: U,
+        _s: Self,
+    ) {
+    }
+}
+
 fn main() {}

tests/ui/needless_pass_by_value.stderr

@@ -104,3 +104,27 @@ help: change `v.clone()` to
 79 |     let _ = v.to_owned();
    |             ^^^^^^^^^^^^
 
+error: this argument is passed by value, but not consumed in the function body
+  --> $DIR/needless_pass_by_value.rs:87:12
+   |
+87 |         s: String,
+   |            ^^^^^^ help: consider changing the type to: `&str`
+
+error: this argument is passed by value, but not consumed in the function body
+  --> $DIR/needless_pass_by_value.rs:88:12
+   |
+88 |         t: String,
+   |            ^^^^^^ help: consider taking a reference instead: `&String`
+
+error: this argument is passed by value, but not consumed in the function body
+   --> $DIR/needless_pass_by_value.rs:100:13
+    |
+100 |         _u: U,
+    |             ^ help: consider taking a reference instead: `&U`
+
+error: this argument is passed by value, but not consumed in the function body
+   --> $DIR/needless_pass_by_value.rs:101:13
+    |
+101 |         _s: Self,
+    |             ^^^^ help: consider taking a reference instead: `&Self`
+