diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index db52122ae..832e9df96 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 05ab62786..5fa4fd748 100644 --- a/clippy_lints/src/pass_by_ref_or_value.rs +++ b/clippy_lints/src/pass_by_ref_or_value.rs @@ -3,17 +3,20 @@ 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::adjustment::{Adjust, PointerCast}; 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 +144,76 @@ 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 + { + if let Some(typeck) = cx.maybe_typeck_results() { + // Don't lint if an unsafe pointer is created. + // TODO: Limit the check only to unsafe pointers to the argument (or part of the argument) + // which escape the current function. + if typeck.node_types().iter().any(|(_, &ty)| ty.is_unsafe_ptr()) + || typeck + .adjustments() + .iter() + .flat_map(|(_, a)| a) + .any(|a| matches!(a.kind, Adjust::Pointer(PointerCast::UnsafeFnPointer))) + { + continue; + } } + 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 +225,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 227e97d37..4c079151f 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( + ty: Ty<'_>, + f: impl FnMut(BoundRegion) -> ControlFlow, +) -> ControlFlow { + struct V { + index: u32, + f: F, + } + impl<'tcx, B, F: FnMut(BoundRegion) -> ControlFlow> TypeVisitor<'tcx> for V { + type BreakTy = B; + fn visit_region(&mut self, r: Region<'tcx>) -> ControlFlow { + if let RegionKind::ReLateBound(idx, bound) = r.kind() && idx.as_u32() == self.index { + (self.f)(bound) + } else { + ControlFlow::Continue(()) + } + } + fn visit_binder>(&mut self, t: &Binder<'tcx, T>) -> ControlFlow { + 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 ea3dce170..8f78f16a0 100644 --- a/tests/ui/trivially_copy_pass_by_ref.rs +++ b/tests/ui/trivially_copy_pass_by_ref.rs @@ -113,6 +113,44 @@ 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 _return_ptr(x: &u32) -> *const u32 { + x +} + +fn _return_field_ptr(x: &(u32, u32)) -> *const u32 { + &x.0 +} + +fn _return_field_ptr_addr_of(x: &(u32, u32)) -> *const u32 { + core::ptr::addr_of!(x.0) +} + 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 a88d35f3e..66ecb3d8e 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