diff --git a/clippy_lints/src/trivially_copy_pass_by_ref.rs b/clippy_lints/src/trivially_copy_pass_by_ref.rs index 61a2a9ded44..2929752bbb2 100644 --- a/clippy_lints/src/trivially_copy_pass_by_ref.rs +++ b/clippy_lints/src/trivially_copy_pass_by_ref.rs @@ -18,12 +18,13 @@ use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use if_chain::if_chain; use crate::rustc::ty::TyKind; +use crate::rustc::ty::FnSig; use crate::rustc::session::config::Config as SessionConfig; use crate::rustc_target::spec::abi::Abi; use crate::rustc_target::abi::LayoutOf; use crate::syntax::ast::NodeId; use crate::syntax_pos::Span; -use crate::utils::{in_macro, is_copy, is_self, span_lint_and_sugg, snippet}; +use crate::utils::{in_macro, is_copy, is_self_ty, span_lint_and_sugg, snippet}; /// **What it does:** Checks for functions taking arguments by reference, where /// the argument type is `Copy` and small enough to be more efficient to always @@ -67,7 +68,7 @@ pub struct TriviallyCopyPassByRef { limit: u64, } -impl TriviallyCopyPassByRef { +impl<'a, 'tcx> TriviallyCopyPassByRef { pub fn new(limit: Option, target: &SessionConfig) -> Self { let limit = limit.unwrap_or_else(|| { let bit_width = target.usize_ty.bit_width().expect("usize should have a width") as u64; @@ -80,6 +81,83 @@ impl TriviallyCopyPassByRef { }); Self { limit } } + + fn check_trait_method( + &mut self, + cx: &LateContext<'_, 'tcx>, + item: &TraitItemRef + ) { + let method_def_id = cx.tcx.hir.local_def_id(item.id.node_id); + let method_sig = cx.tcx.fn_sig(method_def_id); + let method_sig = cx.tcx.erase_late_bound_regions(&method_sig); + + let decl = match cx.tcx.hir.fn_decl(item.id.node_id) { + Some(b) => b, + None => return, + }; + + self.check_poly_fn(cx, &decl, &method_sig, None); + } + + fn check_poly_fn( + &mut self, + cx: &LateContext<'_, 'tcx>, + decl: &FnDecl, + sig: &FnSig<'tcx>, + span: Option, + ) { + // Use lifetimes to determine if we're returning a reference to the + // argument. In that case we can't switch to pass-by-value as the + // argument will not live long enough. + let output_lts = match sig.output().sty { + TyKind::Ref(output_lt, _, _) => vec![output_lt], + TyKind::Adt(_, substs) => substs.regions().collect(), + _ => vec![], + }; + + for (input, &ty) in decl.inputs.iter().zip(sig.inputs()) { + // All spans generated from a proc-macro invocation are the same... + match span { + Some(s) if s == input.span => return, + _ => (), + } + + if_chain! { + if let TyKind::Ref(input_lt, ty, Mutability::MutImmutable) = ty.sty; + if !output_lts.contains(&input_lt); + if is_copy(cx, ty); + if let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()); + if size <= self.limit; + if let hir::TyKind::Rptr(_, MutTy { ty: ref decl_ty, .. }) = input.node; + then { + let value_type = if is_self_ty(decl_ty) { + "self".into() + } else { + snippet(cx, decl_ty.span, "_").into() + }; + span_lint_and_sugg( + cx, + TRIVIALLY_COPY_PASS_BY_REF, + input.span, + "this argument is passed by reference, but would be more efficient if passed by value", + "consider passing by value instead", + value_type); + } + } + } + } + + fn check_trait_items( + &mut self, + cx: &LateContext<'_, '_>, + trait_items: &[TraitItemRef] + ) { + for item in trait_items { + if let AssociatedItemKind::Method{..} = item.kind { + self.check_trait_method(cx, item); + } + } + } } impl LintPass for TriviallyCopyPassByRef { @@ -89,12 +167,21 @@ impl LintPass for TriviallyCopyPassByRef { } impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TriviallyCopyPassByRef { + fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) { + if in_macro(item.span) { + return; + } + if let ItemKind::Trait(_, _, _, _, ref trait_items) = item.node { + self.check_trait_items(cx, trait_items); + } + } + fn check_fn( &mut self, cx: &LateContext<'a, 'tcx>, kind: FnKind<'tcx>, decl: &'tcx FnDecl, - body: &'tcx Body, + _body: &'tcx Body, span: Span, node_id: NodeId, ) { @@ -131,43 +218,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TriviallyCopyPassByRef { let fn_sig = cx.tcx.fn_sig(fn_def_id); let fn_sig = cx.tcx.erase_late_bound_regions(&fn_sig); - // Use lifetimes to determine if we're returning a reference to the - // argument. In that case we can't switch to pass-by-value as the - // argument will not live long enough. - let output_lts = match fn_sig.output().sty { - TyKind::Ref(output_lt, _, _) => vec![output_lt], - TyKind::Adt(_, substs) => substs.regions().collect(), - _ => vec![], - }; - - for ((input, &ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) { - // All spans generated from a proc-macro invocation are the same... - if span == input.span { - return; - } - - if_chain! { - if let TyKind::Ref(input_lt, ty, Mutability::MutImmutable) = ty.sty; - if !output_lts.contains(&input_lt); - if is_copy(cx, ty); - if let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()); - if size <= self.limit; - if let hir::TyKind::Rptr(_, MutTy { ty: ref decl_ty, .. }) = input.node; - then { - let value_type = if is_self(arg) { - "self".into() - } else { - snippet(cx, decl_ty.span, "_").into() - }; - span_lint_and_sugg( - cx, - TRIVIALLY_COPY_PASS_BY_REF, - input.span, - "this argument is passed by reference, but would be more efficient if passed by value", - "consider passing by value instead", - value_type); - } - } - } + self.check_poly_fn(cx, decl, &fn_sig, Some(span)); } } diff --git a/tests/ui/trivially_copy_pass_by_ref.rs b/tests/ui/trivially_copy_pass_by_ref.rs index cebe15b2cc8..2a0dc22bfef 100644 --- a/tests/ui/trivially_copy_pass_by_ref.rs +++ b/tests/ui/trivially_copy_pass_by_ref.rs @@ -18,6 +18,11 @@ struct Foo(u32); #[derive(Copy, Clone)] struct Bar([u8; 24]); +#[derive(Copy, Clone)] +pub struct Color { + pub r: u8, pub g: u8, pub b: u8, pub a: u8, +} + struct FooRef<'a> { foo: &'a Foo, } @@ -80,6 +85,20 @@ impl Bar { } } +trait MyTrait { + fn trait_method(&self, _foo: &Foo); +} + +pub trait MyTrait2 { + fn trait_method2(&self, _color: &Color); +} + +impl MyTrait for Foo { + fn trait_method(&self, _foo: &Foo) { + unimplemented!() + } +} + fn main() { let (mut foo, bar) = (Foo(0), Bar([0; 24])); let (mut a, b, c, x, y, z) = (0, 0, Bar([0; 24]), 0, Foo(0), 0); diff --git a/tests/ui/trivially_copy_pass_by_ref.stderr b/tests/ui/trivially_copy_pass_by_ref.stderr index 2026d4c00d8..3fb577d3edb 100644 --- a/tests/ui/trivially_copy_pass_by_ref.stderr +++ b/tests/ui/trivially_copy_pass_by_ref.stderr @@ -1,82 +1,94 @@ error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:52:11 + --> $DIR/trivially_copy_pass_by_ref.rs:57:11 | -52 | fn bad(x: &u32, y: &Foo, z: &Baz) { +57 | fn bad(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `u32` | = note: `-D clippy::trivially-copy-pass-by-ref` implied by `-D warnings` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:52:20 + --> $DIR/trivially_copy_pass_by_ref.rs:57:20 | -52 | fn bad(x: &u32, y: &Foo, z: &Baz) { +57 | fn bad(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Foo` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:52:29 + --> $DIR/trivially_copy_pass_by_ref.rs:57:29 | -52 | fn bad(x: &u32, y: &Foo, z: &Baz) { +57 | fn bad(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Baz` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:62:12 + --> $DIR/trivially_copy_pass_by_ref.rs:67:12 | -62 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +67 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { | ^^^^^ help: consider passing by value instead: `self` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:62:22 + --> $DIR/trivially_copy_pass_by_ref.rs:67:22 | -62 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +67 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `u32` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:62:31 + --> $DIR/trivially_copy_pass_by_ref.rs:67:31 | -62 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +67 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Foo` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:62:40 + --> $DIR/trivially_copy_pass_by_ref.rs:67:40 | -62 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +67 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Baz` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:65:16 + --> $DIR/trivially_copy_pass_by_ref.rs:70:16 | -65 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +70 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `u32` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:65:25 + --> $DIR/trivially_copy_pass_by_ref.rs:70:25 | -65 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +70 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Foo` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:65:34 + --> $DIR/trivially_copy_pass_by_ref.rs:70:34 | -65 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +70 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Baz` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:79:16 + --> $DIR/trivially_copy_pass_by_ref.rs:84:16 | -79 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +84 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `u32` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:79:25 + --> $DIR/trivially_copy_pass_by_ref.rs:84:25 | -79 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +84 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Foo` error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:79:34 + --> $DIR/trivially_copy_pass_by_ref.rs:84:34 | -79 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +84 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Baz` -error: aborting due to 13 previous errors +error: this argument is passed by reference, but would be more efficient if passed by value + --> $DIR/trivially_copy_pass_by_ref.rs:89:34 + | +89 | fn trait_method(&self, _foo: &Foo); + | ^^^^ help: consider passing by value instead: `Foo` + +error: this argument is passed by reference, but would be more efficient if passed by value + --> $DIR/trivially_copy_pass_by_ref.rs:93:37 + | +93 | fn trait_method2(&self, _color: &Color); + | ^^^^^^ help: consider passing by value instead: `Color` + +error: aborting due to 15 previous errors