From 1455ca016ed9b649a0199543d20c9f2ee12d1f3d Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Thu, 2 Jan 2025 13:28:00 +0100 Subject: [PATCH] Do not propose to elide lifetimes if this causes an ambiguity Some lifetimes in function return types are not bound to concrete content and can be set arbitrarily. Clippy should not propose to replace them by the default `'_` lifetime if such a lifetime cannot be determined unambigously. --- clippy_lints/src/lifetimes.rs | 41 +++++++++++++++ tests/ui/needless_lifetimes.fixed | 82 ++++++++++++++++++++++++++++++ tests/ui/needless_lifetimes.rs | 82 ++++++++++++++++++++++++++++++ tests/ui/needless_lifetimes.stderr | 61 +++++++++++++++++++++- 4 files changed, 265 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 8b2eee34a972..f07fa9cf50c8 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -482,11 +482,13 @@ fn has_where_lifetimes<'tcx>(cx: &LateContext<'tcx>, generics: &'tcx Generics<'_ false } +#[allow(clippy::struct_excessive_bools)] struct Usage { lifetime: Lifetime, in_where_predicate: bool, in_bounded_ty: bool, in_generics_arg: bool, + lifetime_elision_impossible: bool, } struct LifetimeChecker<'cx, 'tcx, F> { @@ -495,6 +497,7 @@ struct LifetimeChecker<'cx, 'tcx, F> { where_predicate_depth: usize, bounded_ty_depth: usize, generic_args_depth: usize, + lifetime_elision_impossible: bool, phantom: std::marker::PhantomData, } @@ -519,6 +522,7 @@ where where_predicate_depth: 0, bounded_ty_depth: 0, generic_args_depth: 0, + lifetime_elision_impossible: false, phantom: std::marker::PhantomData, } } @@ -560,6 +564,7 @@ where in_where_predicate: self.where_predicate_depth != 0, in_bounded_ty: self.bounded_ty_depth != 0, in_generics_arg: self.generic_args_depth != 0, + lifetime_elision_impossible: self.lifetime_elision_impossible, }); } } @@ -586,11 +591,46 @@ where self.generic_args_depth -= 1; } + fn visit_fn_decl(&mut self, fd: &'tcx FnDecl<'tcx>) -> Self::Result { + self.lifetime_elision_impossible = !is_candidate_for_elision(fd); + walk_fn_decl(self, fd); + self.lifetime_elision_impossible = false; + } + fn nested_visit_map(&mut self) -> Self::Map { self.cx.tcx.hir() } } +/// Check if `fd` supports function elision with an anonymous (or elided) lifetime, +/// and has a lifetime somewhere in its output type. +fn is_candidate_for_elision(fd: &FnDecl<'_>) -> bool { + struct V; + + impl Visitor<'_> for V { + type Result = ControlFlow; + + fn visit_lifetime(&mut self, lifetime: &Lifetime) -> Self::Result { + ControlFlow::Break(lifetime.is_elided() || lifetime.is_anonymous()) + } + } + + if let Return(ret_ty) = fd.output + && walk_ty(&mut V, ret_ty).is_break() + { + // If lifetime elision is allowed, the first encountered input lifetime will either be one on + // `self`, or will be the only lifetime. + fd.lifetime_elision_allowed + && fd + .inputs + .iter() + .find_map(|ty| walk_ty(&mut V, ty).break_value()) + .unwrap() + } else { + false + } +} + fn report_extra_lifetimes<'tcx>(cx: &LateContext<'tcx>, func: &'tcx FnDecl<'_>, generics: &'tcx Generics<'_>) { let mut checker = LifetimeChecker::::new(cx, generics); @@ -656,6 +696,7 @@ fn report_elidable_impl_lifetimes<'tcx>( Usage { lifetime, in_where_predicate: false, + lifetime_elision_impossible: false, .. }, ] = usages.as_slice() diff --git a/tests/ui/needless_lifetimes.fixed b/tests/ui/needless_lifetimes.fixed index 8196d608abd2..bd4ff4dd0704 100644 --- a/tests/ui/needless_lifetimes.fixed +++ b/tests/ui/needless_lifetimes.fixed @@ -576,4 +576,86 @@ mod issue13749bis { impl<'a, T: 'a> Generic {} } +mod issue13923 { + struct Py<'py> { + data: &'py str, + } + + enum Content<'t, 'py> { + Py(Py<'py>), + T1(&'t str), + T2(&'t str), + } + + enum ContentString<'t> { + T1(&'t str), + T2(&'t str), + } + + impl<'t, 'py> ContentString<'t> { + // `'py` cannot be elided + fn map_content1(self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t> ContentString<'t> { + // `'py` can be elided because of `&self` + fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t> ContentString<'t> { + // `'py` can be elided because of `&'_ self` + fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t, 'py> ContentString<'t> { + // `'py` should not be elided as the default lifetime, even if working, could be named as `'t` + fn map_content4(self, f: impl FnOnce(&'t str) -> &'t str, o: &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(_) => Content::T2(o), + } + } + } + + impl<'t> ContentString<'t> { + // `'py` can be elided because of `&Self` + fn map_content5( + self: std::pin::Pin<&Self>, + f: impl FnOnce(&'t str) -> &'t str, + o: &'t str, + ) -> Content<'t, '_> { + match *self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(_) => Content::T2(o), + } + } + } + + struct Cx<'a, 'b> { + a: &'a u32, + b: &'b u32, + } + + // `'c` cannot be elided because we have several input lifetimes + fn one_explicit<'b>(x: Cx<'_, 'b>) -> &'b u32 { + x.b + } + +} + fn main() {} diff --git a/tests/ui/needless_lifetimes.rs b/tests/ui/needless_lifetimes.rs index b55dd99c46d0..04335b27e2db 100644 --- a/tests/ui/needless_lifetimes.rs +++ b/tests/ui/needless_lifetimes.rs @@ -576,4 +576,86 @@ mod issue13749bis { impl<'a, T: 'a> Generic {} } +mod issue13923 { + struct Py<'py> { + data: &'py str, + } + + enum Content<'t, 'py> { + Py(Py<'py>), + T1(&'t str), + T2(&'t str), + } + + enum ContentString<'t> { + T1(&'t str), + T2(&'t str), + } + + impl<'t, 'py> ContentString<'t> { + // `'py` cannot be elided + fn map_content1(self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t, 'py> ContentString<'t> { + // `'py` can be elided because of `&self` + fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t, 'py> ContentString<'t> { + // `'py` can be elided because of `&'_ self` + fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(content) => Content::T2(f(content)), + } + } + } + + impl<'t, 'py> ContentString<'t> { + // `'py` should not be elided as the default lifetime, even if working, could be named as `'t` + fn map_content4(self, f: impl FnOnce(&'t str) -> &'t str, o: &'t str) -> Content<'t, 'py> { + match self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(_) => Content::T2(o), + } + } + } + + impl<'t, 'py> ContentString<'t> { + // `'py` can be elided because of `&Self` + fn map_content5( + self: std::pin::Pin<&Self>, + f: impl FnOnce(&'t str) -> &'t str, + o: &'t str, + ) -> Content<'t, 'py> { + match *self { + Self::T1(content) => Content::T1(f(content)), + Self::T2(_) => Content::T2(o), + } + } + } + + struct Cx<'a, 'b> { + a: &'a u32, + b: &'b u32, + } + + // `'c` cannot be elided because we have several input lifetimes + fn one_explicit<'b>(x: Cx<'_, 'b>) -> &'b u32 { + &x.b + } + +} + fn main() {} diff --git a/tests/ui/needless_lifetimes.stderr b/tests/ui/needless_lifetimes.stderr index e56c914cc86d..5a739201d3d0 100644 --- a/tests/ui/needless_lifetimes.stderr +++ b/tests/ui/needless_lifetimes.stderr @@ -576,5 +576,64 @@ LL - fn one_input<'a>(x: &'a u8) -> &'a u8 { LL + fn one_input(x: &u8) -> &u8 { | -error: aborting due to 48 previous errors +error: the following explicit lifetimes could be elided: 'py + --> tests/ui/needless_lifetimes.rs:605:14 + | +LL | impl<'t, 'py> ContentString<'t> { + | ^^^ +LL | // `'py` can be elided because of `&self` +LL | fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + | ^^^ + | +help: elide the lifetimes + | +LL ~ impl<'t> ContentString<'t> { +LL | // `'py` can be elided because of `&self` +LL ~ fn map_content2(&self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> { + | + +error: the following explicit lifetimes could be elided: 'py + --> tests/ui/needless_lifetimes.rs:615:14 + | +LL | impl<'t, 'py> ContentString<'t> { + | ^^^ +LL | // `'py` can be elided because of `&'_ self` +LL | fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, 'py> { + | ^^^ + | +help: elide the lifetimes + | +LL ~ impl<'t> ContentString<'t> { +LL | // `'py` can be elided because of `&'_ self` +LL ~ fn map_content3(&'_ self, f: impl FnOnce(&'t str) -> &'t str) -> Content<'t, '_> { + | + +error: the following explicit lifetimes could be elided: 'py + --> tests/ui/needless_lifetimes.rs:635:14 + | +LL | impl<'t, 'py> ContentString<'t> { + | ^^^ +... +LL | ) -> Content<'t, 'py> { + | ^^^ + | +help: elide the lifetimes + | +LL ~ impl<'t> ContentString<'t> { +LL | // `'py` can be elided because of `&Self` +... +LL | o: &'t str, +LL ~ ) -> Content<'t, '_> { + | + +error: this expression creates a reference which is immediately dereferenced by the compiler + --> tests/ui/needless_lifetimes.rs:656:9 + | +LL | &x.b + | ^^^^ help: change this to: `x.b` + | + = note: `-D clippy::needless-borrow` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]` + +error: aborting due to 52 previous errors