Improve needless_lifetimes

Author: smoelius

clippy_lints/src/lifetimes.rs

@@ -1,4 +1,4 @@
-use clippy_utils::diagnostics::span_lint;
+use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
 use clippy_utils::trait_ref_of_method;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir::intravisit::nested_filter::{self as hir_nested_filter, NestedFilter};
@@ -151,6 +151,7 @@ fn check_fn_inner<'tcx>(
         .params
         .iter()
         .filter(|param| matches!(param.kind, GenericParamKind::Type { .. }));
+
     for typ in types {
         for pred in generics.bounds_for_param(cx.tcx.hir().local_def_id(typ.hir_id)) {
             if pred.origin == PredicateOrigin::WhereClause {
@@ -187,15 +188,30 @@ fn check_fn_inner<'tcx>(
             }
         }
     }
-    if could_use_elision(cx, decl, body, trait_sig, generics.params) {
-        span_lint(
+
+    if let Some(elidable_lts) = could_use_elision(cx, decl, body, trait_sig, generics.params) {
+        let lts = elidable_lts
+            .iter()
+            // In principle, the result of the call to `Node::ident` could be `unwrap`ped, as `DefId` should refer to a
+            // `Node::GenericParam`.
+            .filter_map(|&(def_id, _)| cx.tcx.hir().get_by_def_id(def_id).ident())
+            .map(|ident| ident.to_string())
+            .collect::<Vec<_>>()
+            .join(", ");
+
+        span_lint_and_then(
             cx,
             NEEDLESS_LIFETIMES,
             span.with_hi(decl.output.span().hi()),
-            "explicit lifetimes given in parameter types where they could be elided \
-             (or replaced with `'_` if needed by type declaration)",
+            &format!("the following explicit lifetimes could be elided: {lts}"),
+            |diag| {
+                if let Some(span) = elidable_lts.iter().find_map(|&(_, span)| span) {
+                    diag.span_help(span, "replace with `'_` in generic arguments such as here");
+                }
+            },
         );
     }
+
     if report_extra_lifetimes {
         self::report_extra_lifetimes(cx, decl, generics);
     }
@@ -220,13 +236,14 @@ fn explicit_self_type<'tcx>(cx: &LateContext<'tcx>, func: &FnDecl<'tcx>, ident:
     }
 }
 
+#[expect(clippy::too_many_lines)]
 fn could_use_elision<'tcx>(
     cx: &LateContext<'tcx>,
     func: &'tcx FnDecl<'_>,
     body: Option<BodyId>,
     trait_sig: Option<&[Ident]>,
     named_generics: &'tcx [GenericParam<'_>],
-) -> bool {
+) -> Option<Vec<(LocalDefId, Option<Span>)>> {
     // There are two scenarios where elision works:
     // * no output references, all input references have different LT
     // * output references, exactly one input reference with same LT
@@ -253,15 +270,15 @@ fn could_use_elision<'tcx>(
     }
 
     if input_visitor.abort() || output_visitor.abort() {
-        return false;
+        return None;
     }
 
     let input_lts = input_visitor.lts;
     let output_lts = output_visitor.lts;
 
     if let Some(trait_sig) = trait_sig {
         if explicit_self_type(cx, func, trait_sig.first().copied()) {
-            return false;
+            return None;
         }
     }
 
@@ -270,22 +287,22 @@ fn could_use_elision<'tcx>(
 
         let first_ident = body.params.first().and_then(|param| param.pat.simple_ident());
         if explicit_self_type(cx, func, first_ident) {
-            return false;
+            return None;
         }
 
         let mut checker = BodyLifetimeChecker {
             lifetimes_used_in_body: false,
         };
         checker.visit_expr(body.value);
         if checker.lifetimes_used_in_body {
-            return false;
+            return None;
         }
     }
 
     // check for lifetimes from higher scopes
     for lt in input_lts.iter().chain(output_lts.iter()) {
         if !allowed_lts.contains(lt) {
-            return false;
+            return None;
         }
     }
 
@@ -301,47 +318,62 @@ fn could_use_elision<'tcx>(
         for lt in input_visitor.nested_elision_site_lts {
             if let RefLt::Named(def_id) = lt {
                 if allowed_lts.contains(&cx.tcx.item_name(def_id.to_def_id())) {
-                    return false;
+                    return None;
                 }
             }
         }
         for lt in output_visitor.nested_elision_site_lts {
             if let RefLt::Named(def_id) = lt {
                 if allowed_lts.contains(&cx.tcx.item_name(def_id.to_def_id())) {
-                    return false;
+                    return None;
                 }
             }
         }
     }
 
     // no input lifetimes? easy case!
     if input_lts.is_empty() {
-        false
+        None
     } else if output_lts.is_empty() {
         // no output lifetimes, check distinctness of input lifetimes
 
         // only unnamed and static, ok
         let unnamed_and_static = input_lts.iter().all(|lt| *lt == RefLt::Unnamed || *lt == RefLt::Static);
         if unnamed_and_static {
-            return false;
+            return None;
+        }
+        // we have no output reference, so we can elide explicit lifetimes that occur at most once
+        let elidable_lts = named_lifetime_occurrences(&input_lts)
+            .into_iter()
+            .filter_map(|(def_id, occurrences)| {
+                if occurrences <= 1 {
+                    Some((def_id, input_visitor.sample_generic_arg_span.get(&def_id).copied()))
+                } else {
+                    None
+                }
+            })
+            .collect::<Vec<_>>();
+        if elidable_lts.is_empty() {
+            None
+        } else {
+            Some(elidable_lts)
         }
-        // we have no output reference, so we only need all distinct lifetimes
-        input_lts.len() == unique_lifetimes(&input_lts)
     } else {
         // we have output references, so we need one input reference,
         // and all output lifetimes must be the same
-        if unique_lifetimes(&output_lts) > 1 {
-            return false;
-        }
         if input_lts.len() == 1 {
             match (&input_lts[0], &output_lts[0]) {
-                (&RefLt::Named(n1), &RefLt::Named(n2)) if n1 == n2 => true,
-                (&RefLt::Named(_), &RefLt::Unnamed) => true,
-                _ => false, /* already elided, different named lifetimes
-                             * or something static going on */
+                (&RefLt::Named(n1), &RefLt::Named(n2)) if n1 == n2 => {
+                    Some(vec![(n1, input_visitor.sample_generic_arg_span.get(&n1).copied())])
+                },
+                (&RefLt::Named(n), &RefLt::Unnamed) => {
+                    Some(vec![(n, input_visitor.sample_generic_arg_span.get(&n).copied())])
+                },
+                _ => None, /* already elided, different named lifetimes
+                            * or something static going on */
             }
         } else {
-            false
+            None
         }
     }
 }
@@ -358,10 +390,24 @@ fn allowed_lts_from(tcx: TyCtxt<'_>, named_generics: &[GenericParam<'_>]) -> FxH
     allowed_lts
 }
 
-/// Number of unique lifetimes in the given vector.
+/// Number of times each named lifetime occurs in the given slice. Returns a vector to preserve
+/// relative order.
 #[must_use]
-fn unique_lifetimes(lts: &[RefLt]) -> usize {
-    lts.iter().collect::<FxHashSet<_>>().len()
+fn named_lifetime_occurrences(lts: &[RefLt]) -> Vec<(LocalDefId, usize)> {
+    let mut occurrences = Vec::new();
+    for lt in lts {
+        if let &RefLt::Named(curr_def_id) = lt {
+            if let Some(i) = occurrences
+                .iter()
+                .position(|&(prev_def_id, _)| prev_def_id == curr_def_id)
+            {
+                occurrences[i].1 += 1;
+            } else {
+                occurrences.push((curr_def_id, 1));
+            }
+        }
+    }
+    occurrences
 }
 
 const CLOSURE_TRAIT_BOUNDS: [LangItem; 3] = [LangItem::Fn, LangItem::FnMut, LangItem::FnOnce];
@@ -370,6 +416,7 @@ const CLOSURE_TRAIT_BOUNDS: [LangItem; 3] = [LangItem::Fn, LangItem::FnMut, Lang
 struct RefVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     lts: Vec<RefLt>,
+    sample_generic_arg_span: FxHashMap<LocalDefId, Span>,
     nested_elision_site_lts: Vec<RefLt>,
     unelided_trait_object_lifetime: bool,
 }
@@ -379,6 +426,7 @@ impl<'a, 'tcx> RefVisitor<'a, 'tcx> {
         Self {
             cx,
             lts: Vec::new(),
+            sample_generic_arg_span: FxHashMap::default(),
             nested_elision_site_lts: Vec::new(),
             unelided_trait_object_lifetime: false,
         }
@@ -472,6 +520,22 @@ impl<'a, 'tcx> Visitor<'tcx> for RefVisitor<'a, 'tcx> {
             _ => walk_ty(self, ty),
         }
     }
+
+    fn visit_generic_arg(&mut self, generic_arg: &'tcx GenericArg<'tcx>) {
+        if let GenericArg::Lifetime(l) = generic_arg
+            && let LifetimeName::Param(def_id, _) = l.name
+        {
+            self.sample_generic_arg_span.entry(def_id).or_insert(l.span);
+        }
+        // Replace with `walk_generic_arg` if/when https://github.com/rust-lang/rust/pull/103692 lands.
+        // walk_generic_arg(self, generic_arg);
+        match generic_arg {
+            GenericArg::Lifetime(lt) => self.visit_lifetime(lt),
+            GenericArg::Type(ty) => self.visit_ty(ty),
+            GenericArg::Const(ct) => self.visit_anon_const(&ct.value),
+            GenericArg::Infer(inf) => self.visit_infer(inf),
+        }
+    }
 }
 
 /// Are any lifetimes mentioned in the `where` clause? If so, we don't try to

tests/ui/needless_lifetimes.rs

@@ -419,4 +419,12 @@ mod issue7296 {
     }
 }
 
+mod false_negative {
+    #![allow(unused)]
+
+    fn foo<'a>(x: &'a u8, y: &'_ u8) {}
+
+    fn bar<'a>(x: &'a u8, y: &'_ u8, z: &'_ u8) {}
+}
+
 fn main() {}