From 58459abd0cbc998c6c3544ec43b94d4371d8782d Mon Sep 17 00:00:00 2001 From: Ryan Cumming Date: Mon, 23 Jul 2018 18:33:47 +1000 Subject: [PATCH 1/3] Allow pass by reference if we return a reference Currently this code will trigger `trivally_copy_pass_by_ref`: ``` struct OuterStruct { field: [u8; 8], } fn return_inner(outer: &OuterStruct) -> &[u8] { &outer.field } ``` If we change the `outer` to be pass-by-value it will not live long enough for us to return the reference. The above example is trivial but I've hit this in real code that either returns a reference to either the argument or in to `self`. This suppresses the `trivally_copy_pass_by_ref` lint if we return a reference and it has the same lifetime as the argument. This will likely miss complex cases with multiple lifetimes bounded by each other but it should cover the majority of cases with little effort. --- .../src/trivially_copy_pass_by_ref.rs | 11 +++- tests/ui/trivially_copy_pass_by_ref.rs | 11 ++++ tests/ui/trivially_copy_pass_by_ref.stderr | 52 +++++++++---------- 3 files changed, 47 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/trivially_copy_pass_by_ref.rs b/clippy_lints/src/trivially_copy_pass_by_ref.rs index b6fd8db51575..e0eb464596b5 100644 --- a/clippy_lints/src/trivially_copy_pass_by_ref.rs +++ b/clippy_lints/src/trivially_copy_pass_by_ref.rs @@ -115,6 +115,14 @@ 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_lt = if let TypeVariants::TyRef(output_lt, _, _) = fn_sig.output().sty { + Some(output_lt) + } else { + None + }; + 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 { @@ -122,7 +130,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TriviallyCopyPassByRef { } if_chain! { - if let TypeVariants::TyRef(_, ty, Mutability::MutImmutable) = ty.sty; + if let TypeVariants::TyRef(input_lt, ty, Mutability::MutImmutable) = ty.sty; + if Some(input_lt) != output_lt; if is_copy(cx, ty); if let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()); if size <= self.limit; diff --git a/tests/ui/trivially_copy_pass_by_ref.rs b/tests/ui/trivially_copy_pass_by_ref.rs index aba4aa5ea327..c6773add2443 100644 --- a/tests/ui/trivially_copy_pass_by_ref.rs +++ b/tests/ui/trivially_copy_pass_by_ref.rs @@ -11,6 +11,15 @@ type Baz = u32; fn good(a: &mut u32, b: u32, c: &Bar) { } +fn good_return_implicit_lt_ref(foo: &Foo) -> &u32 { + &foo.0 +} + +#[allow(needless_lifetimes)] +fn good_return_explicit_lt_ref<'a>(foo: &'a Foo) -> &'a u32 { + &foo.0 +} + fn bad(x: &u32, y: &Foo, z: &Baz) { } @@ -46,6 +55,8 @@ 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); good(&mut a, b, &c); + good_return_implicit_lt_ref(&y); + good_return_explicit_lt_ref(&y); bad(&x, &y, &z); foo.good(&mut a, b, &c); foo.good2(); diff --git a/tests/ui/trivially_copy_pass_by_ref.stderr b/tests/ui/trivially_copy_pass_by_ref.stderr index c6ab968a7c5f..db25cc5a0201 100644 --- a/tests/ui/trivially_copy_pass_by_ref.stderr +++ b/tests/ui/trivially_copy_pass_by_ref.stderr @@ -1,81 +1,81 @@ error: this argument is passed by reference, but would be more efficient if passed by value - --> $DIR/trivially_copy_pass_by_ref.rs:14:11 + --> $DIR/trivially_copy_pass_by_ref.rs:23:11 | -14 | fn bad(x: &u32, y: &Foo, z: &Baz) { +23 | fn bad(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `u32` | = note: `-D 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:14:20 + --> $DIR/trivially_copy_pass_by_ref.rs:23:20 | -14 | fn bad(x: &u32, y: &Foo, z: &Baz) { +23 | 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:14:29 + --> $DIR/trivially_copy_pass_by_ref.rs:23:29 | -14 | fn bad(x: &u32, y: &Foo, z: &Baz) { +23 | 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:24:12 + --> $DIR/trivially_copy_pass_by_ref.rs:33:12 | -24 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +33 | 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:24:22 + --> $DIR/trivially_copy_pass_by_ref.rs:33:22 | -24 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +33 | 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:24:31 + --> $DIR/trivially_copy_pass_by_ref.rs:33:31 | -24 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +33 | 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:24:40 + --> $DIR/trivially_copy_pass_by_ref.rs:33:40 | -24 | fn bad(&self, x: &u32, y: &Foo, z: &Baz) { +33 | 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:27:16 + --> $DIR/trivially_copy_pass_by_ref.rs:36:16 | -27 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +36 | 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:27:25 + --> $DIR/trivially_copy_pass_by_ref.rs:36:25 | -27 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +36 | 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:27:34 + --> $DIR/trivially_copy_pass_by_ref.rs:36:34 | -27 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +36 | 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:41:16 + --> $DIR/trivially_copy_pass_by_ref.rs:50:16 | -41 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +50 | 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:41:25 + --> $DIR/trivially_copy_pass_by_ref.rs:50:25 | -41 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +50 | 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:41:34 + --> $DIR/trivially_copy_pass_by_ref.rs:50:34 | -41 | fn bad2(x: &u32, y: &Foo, z: &Baz) { +50 | fn bad2(x: &u32, y: &Foo, z: &Baz) { | ^^^^ help: consider passing by value instead: `Baz` error: aborting due to 13 previous errors From 89a4558056914dc5e4e709a18039e843c8b5e566 Mon Sep 17 00:00:00 2001 From: Ryan Cumming Date: Mon, 23 Jul 2018 19:33:52 +1000 Subject: [PATCH 2/3] Add Known Problem for multiple lifetimes --- clippy_lints/src/trivially_copy_pass_by_ref.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/clippy_lints/src/trivially_copy_pass_by_ref.rs b/clippy_lints/src/trivially_copy_pass_by_ref.rs index e0eb464596b5..d27f4f061cbb 100644 --- a/clippy_lints/src/trivially_copy_pass_by_ref.rs +++ b/clippy_lints/src/trivially_copy_pass_by_ref.rs @@ -31,6 +31,12 @@ use crate::utils::{in_macro, is_copy, is_self, span_lint_and_sugg, snippet}; /// The configuration option `trivial_copy_size_limit` can be set to override /// this limit for a project. /// +/// This lint attempts to allow passing arguments by reference if a reference +/// to that argument is returned. This is implemented by comparing the lifetime +/// of the argument and return value for equality. However, this can cause +/// false positives in cases involving multiple lifetimes that are bounded by +/// each other. +/// /// **Example:** /// ```rust /// fn foo(v: &u32) { From 7c74c3e5080dd6312e552b77dc0e667140ee4897 Mon Sep 17 00:00:00 2001 From: Ryan Cumming Date: Mon, 23 Jul 2018 19:37:41 +1000 Subject: [PATCH 3/3] Wrap comment at 80 columns --- clippy_lints/src/trivially_copy_pass_by_ref.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/trivially_copy_pass_by_ref.rs b/clippy_lints/src/trivially_copy_pass_by_ref.rs index d27f4f061cbb..6a048b192138 100644 --- a/clippy_lints/src/trivially_copy_pass_by_ref.rs +++ b/clippy_lints/src/trivially_copy_pass_by_ref.rs @@ -121,8 +121,9 @@ 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. + // 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_lt = if let TypeVariants::TyRef(output_lt, _, _) = fn_sig.output().sty { Some(output_lt) } else {