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.

Author: samueltardieu

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

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