diff --git a/clippy_lints/src/collapsible_if.rs b/clippy_lints/src/collapsible_if.rs index 3ac19980a6d..fa0d7de6676 100644 --- a/clippy_lints/src/collapsible_if.rs +++ b/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, ".."))); }); } diff --git a/clippy_lints/src/int_plus_one.rs b/clippy_lints/src/int_plus_one.rs index 420427e7d0a..396b06524d0 100644 --- a/clippy_lints/src/int_plus_one.rs +++ b/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, diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index e00938fb3ef..ac965a59bd5 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/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) { /// 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, diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index d811de59844..3fd372052f6 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -136,8 +136,8 @@ impl<'a> Sugg<'a> { } /// Convenience method to create the ` && ` 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 ` as ` suggestion. @@ -162,10 +162,10 @@ impl<'a> Sugg<'a> { /// Convenience method to create the `..` or `...` /// 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), } } diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 6ecb3963154..c80f6acd06b 100644 --- a/tests/ui/methods.rs +++ b/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; diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index ac37b0bdda1..f4d490b214f 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/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, v: Vec) { let _ = v.clone(); } +struct S(T, U); + +impl S { + 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() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index f23b0714c59..a6c0c0454cb 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/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` +