Do not propose to elide lifetimes if this causes an ambiguity (#13929)
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. I added a field to the `LifetimeChecker` and `Usage` to flag lifetimes that cannot be replaced by default ones, but it feels a bit hacky. Fix #13923 changelog: [`needless_lifetimes`]: remove false positives by checking that lifetimes can indeed be elided
This commit is contained in:
Fridtjof Stoldt
GitHub
commit
5c2601af15
4 changed files with 261 additions and 1 deletions
|
|
@ -488,11 +488,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> {
|
||||
|
|
@ -501,6 +503,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<F>,
|
||||
}
|
||||
|
||||
|
|
@ -525,6 +528,7 @@ where
|
|||
where_predicate_depth: 0,
|
||||
bounded_ty_depth: 0,
|
||||
generic_args_depth: 0,
|
||||
lifetime_elision_impossible: false,
|
||||
phantom: std::marker::PhantomData,
|
||||
}
|
||||
}
|
||||
|
|
@ -566,6 +570,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,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
@ -592,11 +597,44 @@ 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<bool>;
|
||||
|
||||
fn visit_lifetime(&mut self, lifetime: &Lifetime) -> Self::Result {
|
||||
ControlFlow::Break(lifetime.is_elided() || lifetime.is_anonymous())
|
||||
}
|
||||
}
|
||||
|
||||
if fd.lifetime_elision_allowed
|
||||
&& let Return(ret_ty) = fd.output
|
||||
&& walk_ty(&mut V, ret_ty).is_break()
|
||||
{
|
||||
// The first encountered input lifetime will either be one on `self`, or will be the only lifetime.
|
||||
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::<hir_nested_filter::None>::new(cx, generics);
|
||||
|
||||
|
|
@ -662,6 +700,7 @@ fn report_elidable_impl_lifetimes<'tcx>(
|
|||
Usage {
|
||||
lifetime,
|
||||
in_where_predicate: false,
|
||||
lifetime_elision_impossible: false,
|
||||
..
|
||||
},
|
||||
] = usages.as_slice()
|
||||
|
|
|
|||
|
|
@ -576,4 +576,85 @@ mod issue13749bis {
|
|||
impl<'a, T: 'a> Generic<T> {}
|
||||
}
|
||||
|
||||
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() {}
|
||||
|
|
|
|||
|
|
@ -576,4 +576,85 @@ mod issue13749bis {
|
|||
impl<'a, T: 'a> Generic<T> {}
|
||||
}
|
||||
|
||||
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() {}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue