Do not propose to elide lifetimes if this causes an ambiguity (rust-lang#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 rust-lang#13923

changelog: [`needless_lifetimes`]: remove false positives by checking
that lifetimes can indeed be elided

Author: xFrednet

clippy_lints/src/lifetimes.rs

@@ -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()

tests/ui/needless_lifetimes.fixed

@@ -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() {}

tests/ui/needless_lifetimes.rs

@@ -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() {}

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