diff options
| -rw-r--r-- | clippy_lints/src/lib.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/pass_by_ref_or_value.rs | 91 | ||||
| -rw-r--r-- | clippy_utils/src/ty.rs | 32 | ||||
| -rw-r--r-- | tests/ui/trivially_copy_pass_by_ref.rs | 26 | ||||
| -rw-r--r-- | tests/ui/trivially_copy_pass_by_ref.stderr | 8 |
5 files changed, 117 insertions, 41 deletions
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index db52122aef6..832e9df9665 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -7,6 +7,7 @@ #![feature(let_chains)] #![feature(let_else)] #![feature(lint_reasons)] +#![feature(never_type)] #![feature(once_cell)] #![feature(rustc_private)] #![feature(stmt_expr_attributes)] diff --git a/clippy_lints/src/pass_by_ref_or_value.rs b/clippy_lints/src/pass_by_ref_or_value.rs index 05ab62786f4..6edf0e7a7b2 100644 --- a/clippy_lints/src/pass_by_ref_or_value.rs +++ b/clippy_lints/src/pass_by_ref_or_value.rs @@ -3,17 +3,19 @@ use std::iter; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; -use clippy_utils::ty::is_copy; +use clippy_utils::ty::{for_each_top_level_late_bound_region, is_copy}; use clippy_utils::{is_self, is_self_ty}; +use core::ops::ControlFlow; use if_chain::if_chain; use rustc_ast::attr; +use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::FnKind; use rustc_hir::{BindingAnnotation, Body, FnDecl, HirId, Impl, ItemKind, MutTy, Mutability, Node, PatKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; use rustc_middle::ty::layout::LayoutOf; +use rustc_middle::ty::{self, RegionKind}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::def_id::LocalDefId; use rustc_span::{sym, Span}; @@ -141,50 +143,62 @@ impl<'tcx> PassByRefOrValue { } let fn_sig = cx.tcx.fn_sig(def_id); - let fn_sig = cx.tcx.erase_late_bound_regions(fn_sig); - let fn_body = cx.enclosing_body.map(|id| cx.tcx.hir().body(id)); - for (index, (input, &ty)) in iter::zip(decl.inputs, fn_sig.inputs()).enumerate() { + // Gather all the lifetimes found in the output type which may affect whether + // `TRIVIALLY_COPY_PASS_BY_REF` should be linted. + let mut output_regions = FxHashSet::default(); + for_each_top_level_late_bound_region(fn_sig.skip_binder().output(), |region| -> ControlFlow<!> { + output_regions.insert(region); + ControlFlow::Continue(()) + }); + + for (index, (input, ty)) in iter::zip( + decl.inputs, + fn_sig.skip_binder().inputs().iter().map(|&ty| fn_sig.rebind(ty)), + ) + .enumerate() + { // All spans generated from a proc-macro invocation are the same... match span { - Some(s) if s == input.span => return, + Some(s) if s == input.span => continue, _ => (), } - match ty.kind() { - ty::Ref(input_lt, ty, Mutability::Not) => { - // 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().kind() { - ty::Ref(output_lt, _, _) => vec![output_lt], - ty::Adt(_, substs) => substs.regions().collect(), - _ => vec![], - }; + match *ty.skip_binder().kind() { + ty::Ref(lt, ty, Mutability::Not) => { + match lt.kind() { + RegionKind::ReLateBound(index, region) + if index.as_u32() == 0 && output_regions.contains(®ion) => + { + continue; + }, + // Early bound regions on functions are either from the containing item, are bounded by another + // lifetime, or are used as a bound for a type or lifetime. + RegionKind::ReEarlyBound(..) => continue, + _ => (), + } - if_chain! { - 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.ref_min_size; - if let hir::TyKind::Rptr(_, MutTy { ty: decl_ty, .. }) = input.kind; - then { - let value_type = if fn_body.and_then(|body| body.params.get(index)).map_or(false, is_self) { - "self".into() - } else { - snippet(cx, decl_ty.span, "_").into() - }; - span_lint_and_sugg( - cx, - TRIVIALLY_COPY_PASS_BY_REF, - input.span, - &format!("this argument ({} byte) is passed by reference, but would be more efficient if passed by value (limit: {} byte)", size, self.ref_min_size), - "consider passing by value instead", - value_type, - Applicability::Unspecified, - ); - } + let ty = cx.tcx.erase_late_bound_regions(fn_sig.rebind(ty)); + if is_copy(cx, ty) + && let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()) + && size <= self.ref_min_size + && let hir::TyKind::Rptr(_, MutTy { ty: decl_ty, .. }) = input.kind + { + let value_type = if fn_body.and_then(|body| body.params.get(index)).map_or(false, is_self) { + "self".into() + } else { + snippet(cx, decl_ty.span, "_").into() + }; + span_lint_and_sugg( + cx, + TRIVIALLY_COPY_PASS_BY_REF, + input.span, + &format!("this argument ({} byte) is passed by reference, but would be more efficient if passed by value (limit: {} byte)", size, self.ref_min_size), + "consider passing by value instead", + value_type, + Applicability::Unspecified, + ); } }, @@ -196,6 +210,7 @@ impl<'tcx> PassByRefOrValue { _ => continue, } } + let ty = cx.tcx.erase_late_bound_regions(ty); if_chain! { if is_copy(cx, ty); diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 227e97d37ec..4c079151f24 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -2,6 +2,7 @@ #![allow(clippy::module_name_repetitions)] +use core::ops::ControlFlow; use rustc_ast::ast::Mutability; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir as hir; @@ -13,8 +14,8 @@ use rustc_lint::LateContext; use rustc_middle::mir::interpret::{ConstValue, Scalar}; use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst}; use rustc_middle::ty::{ - self, AdtDef, Binder, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, Ty, TyCtxt, TypeFoldable, UintTy, - VariantDiscr, + self, AdtDef, Binder, BoundRegion, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, Region, RegionKind, Ty, + TyCtxt, TypeFoldable, TypeSuperFoldable, TypeVisitor, UintTy, VariantDiscr, }; use rustc_span::symbol::Ident; use rustc_span::{sym, Span, Symbol, DUMMY_SP}; @@ -667,3 +668,30 @@ pub fn is_c_void(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { false } } + +pub fn for_each_top_level_late_bound_region<B>( + ty: Ty<'_>, + f: impl FnMut(BoundRegion) -> ControlFlow<B>, +) -> ControlFlow<B> { + struct V<F> { + index: u32, + f: F, + } + impl<'tcx, B, F: FnMut(BoundRegion) -> ControlFlow<B>> TypeVisitor<'tcx> for V<F> { + type BreakTy = B; + fn visit_region(&mut self, r: Region<'tcx>) -> ControlFlow<Self::BreakTy> { + if let RegionKind::ReLateBound(idx, bound) = r.kind() && idx.as_u32() == self.index { + (self.f)(bound) + } else { + ControlFlow::Continue(()) + } + } + fn visit_binder<T: TypeFoldable<'tcx>>(&mut self, t: &Binder<'tcx, T>) -> ControlFlow<Self::BreakTy> { + self.index += 1; + let res = t.super_visit_with(self); + self.index -= 1; + res + } + } + ty.visit_with(&mut V { index: 0, f }) +} diff --git a/tests/ui/trivially_copy_pass_by_ref.rs b/tests/ui/trivially_copy_pass_by_ref.rs index ea3dce17081..24117e37ed1 100644 --- a/tests/ui/trivially_copy_pass_by_ref.rs +++ b/tests/ui/trivially_copy_pass_by_ref.rs @@ -113,6 +113,32 @@ mod issue5876 { } } +fn _ref_to_opt_ref_implicit(x: &u32) -> Option<&u32> { + Some(x) +} + +#[allow(clippy::needless_lifetimes)] +fn _ref_to_opt_ref_explicit<'a>(x: &'a u32) -> Option<&'a u32> { + Some(x) +} + +fn _with_constraint<'a, 'b: 'a>(x: &'b u32, y: &'a u32) -> &'a u32 { + if true { x } else { y } +} + +async fn _async_implicit(x: &u32) -> &u32 { + x +} + +#[allow(clippy::needless_lifetimes)] +async fn _async_explicit<'a>(x: &'a u32) -> &'a u32 { + x +} + +fn _unrelated_lifetimes<'a, 'b>(_x: &'a u32, y: &'b u32) -> &'b u32 { + y +} + 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 a88d35f3ea5..66ecb3d8e77 100644 --- a/tests/ui/trivially_copy_pass_by_ref.stderr +++ b/tests/ui/trivially_copy_pass_by_ref.stderr @@ -106,5 +106,11 @@ error: this argument (N byte) is passed by reference, but would be more efficien LL | fn foo(x: &i32) { | ^^^^ help: consider passing by value instead: `i32` -error: aborting due to 17 previous errors +error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte) + --> $DIR/trivially_copy_pass_by_ref.rs:138:37 + | +LL | fn _unrelated_lifetimes<'a, 'b>(_x: &'a u32, y: &'b u32) -> &'b u32 { + | ^^^^^^^ help: consider passing by value instead: `u32` + +error: aborting due to 18 previous errors |