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.

4 files changed, 261 insertions, 1 deletions

diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs
index 8b2eee34a97..f99bc259a49 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<F>,
 }
 
@@ -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,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);
 
@@ -656,6 +694,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 8196d608abd..77188254316 100644
--- a/tests/ui/needless_lifetimes.fixed
+++ b/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() {}
diff --git a/tests/ui/needless_lifetimes.rs b/tests/ui/needless_lifetimes.rs
index b55dd99c46d..c74121d0d7b 100644
--- a/tests/ui/needless_lifetimes.rs
+++ b/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() {}
diff --git a/tests/ui/needless_lifetimes.stderr b/tests/ui/needless_lifetimes.stderr
index e56c914cc86..5a739201d3d 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