fix(elidable_lifetime_names): avoid overlapping spans in suggestions (#15667)

Fixes rust-lang/rust-clippy#14157
Fixes rust-lang/rust-clippy#15666

changelog: [`elidable_lifetime_names`]: avoid overlapping spans in
suggestions

r? clippy

Author: samueltardieu

clippy_lints/src/lifetimes.rs

@@ -745,7 +745,7 @@ fn report_elidable_impl_lifetimes<'tcx>(
     impl_: &'tcx Impl<'_>,
     map: &FxIndexMap<LocalDefId, Vec<Usage>>,
 ) {
-    let single_usages = map
+    let (elidable_lts, usages): (Vec<_>, Vec<_>) = map
         .iter()
         .filter_map(|(def_id, usages)| {
             if let [
@@ -762,14 +762,12 @@ fn report_elidable_impl_lifetimes<'tcx>(
                 None
             }
         })
-        .collect::<Vec<_>>();
+        .unzip();
 
-    if single_usages.is_empty() {
+    if elidable_lts.is_empty() {
         return;
     }
 
-    let (elidable_lts, usages): (Vec<_>, Vec<_>) = single_usages.into_iter().unzip();
-
     report_elidable_lifetimes(cx, impl_.generics, &elidable_lts, &usages, true);
 }
 
@@ -795,9 +793,7 @@ fn report_elidable_lifetimes(
         // 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_node_by_def_id(def_id).ident())
-        .map(|ident| ident.to_string())
-        .collect::<Vec<_>>()
-        .join(", ");
+        .format(", ");
 
     let elidable_usages: Vec<ElidableUsage> = usages
         .iter()
@@ -860,36 +856,89 @@ fn elision_suggestions(
         .filter(|param| !param.is_elided_lifetime() && !param.is_impl_trait())
         .collect::<Vec<_>>();
 
-    let mut suggestions = if elidable_lts.len() == explicit_params.len() {
+    if !elidable_lts
+        .iter()
+        .all(|lt| explicit_params.iter().any(|param| param.def_id == *lt))
+    {
+        return None;
+    }
+
+    let mut suggestions = if elidable_lts.is_empty() {
+        vec![]
+    } else if elidable_lts.len() == explicit_params.len() {
         // if all the params are elided remove the whole generic block
         //
         // fn x<'a>() {}
         //     ^^^^
         vec![(generics.span, String::new())]
     } else {
-        elidable_lts
-            .iter()
-            .map(|&id| {
-                let pos = explicit_params.iter().position(|param| param.def_id == id)?;
-                let param = explicit_params.get(pos)?;
-
-                let span = if let Some(next) = explicit_params.get(pos + 1) {
-                    // fn x<'prev, 'a, 'next>() {}
-                    //             ^^^^
-                    param.span.until(next.span)
+        match &explicit_params[..] {
+            // no params, nothing to elide
+            [] => unreachable!("handled by `elidable_lts.is_empty()`"),
+            [param] => {
+                if elidable_lts.contains(&param.def_id) {
+                    unreachable!("handled by `elidable_lts.len() == explicit_params.len()`")
                 } else {
-                    // `pos` should be at least 1 here, because the param in position 0 would either have a `next`
-                    // param or would have taken the `elidable_lts.len() == explicit_params.len()` branch.
-                    let prev = explicit_params.get(pos - 1)?;
-
-                    // fn x<'prev, 'a>() {}
-                    //           ^^^^
-                    param.span.with_lo(prev.span.hi())
+                    unreachable!("handled by `elidable_lts.is_empty()`")
+                }
+            },
+            [_, _, ..] => {
+                // Given a list like `<'a, 'b, 'c, 'd, ..>`,
+                //
+                // If there is a cluster of elidable lifetimes at the beginning, say `'a` and `'b`, we should
+                // suggest removing them _and_ the trailing comma. The span for that is `a.span.until(c.span)`:
+                // <'a, 'b, 'c, 'd, ..> => <'a, 'b, 'c, 'd, ..>
+                //  ^^  ^^                  ^^^^^^^^
+                //
+                // And since we know that `'c` isn't elidable--otherwise it would've been in the cluster--we can go
+                // over all the lifetimes after it, and for each elidable one, add a suggestion spanning the
+                // lifetime itself and the comma before, because each individual suggestion is guaranteed to leave
+                // the list valid:
+                // <.., 'c, 'd, 'e, 'f, 'g, ..> => <.., 'c, 'd, 'e, 'f, 'g, ..>
+                //          ^^      ^^  ^^                ^^^^    ^^^^^^^^
+                //
+                // In case there is no such starting cluster, we only need to do the second part of the algorithm:
+                // <'a, 'b, 'c, 'd, 'e, 'f, 'g, ..> => <'a, 'b , 'c, 'd, 'e, 'f, 'g, ..>
+                //      ^^  ^^      ^^  ^^                ^^^^^^^^^    ^^^^^^^^
+
+                // Split off the starting cluster
+                // TODO: use `slice::split_once` once stabilized (github.com/rust-lang/rust/issues/112811):
+                // ```
+                // let Some(split) = explicit_params.split_once(|param| !elidable_lts.contains(&param.def_id)) else {
+                //     // there were no lifetime param that couldn't be elided
+                //     unreachable!("handled by `elidable_lts.len() == explicit_params.len()`")
+                // };
+                // match split { /* .. */ }
+                // ```
+                let Some(split_pos) = explicit_params
+                    .iter()
+                    .position(|param| !elidable_lts.contains(&param.def_id))
+                else {
+                    // there were no lifetime param that couldn't be elided
+                    unreachable!("handled by `elidable_lts.len() == explicit_params.len()`")
                 };
-
-                Some((span, String::new()))
-            })
-            .collect::<Option<Vec<_>>>()?
+                let split = explicit_params
+                    .split_at_checked(split_pos)
+                    .expect("got `split_pos` from `position` on the same Vec");
+
+                match split {
+                    ([..], []) => unreachable!("handled by `elidable_lts.len() == explicit_params.len()`"),
+                    ([], [_]) => unreachable!("handled by `explicit_params.len() == 1`"),
+                    (cluster, rest @ [rest_first, ..]) => {
+                        // the span for the cluster
+                        (cluster.first().map(|fw| fw.span.until(rest_first.span)).into_iter())
+                            // the span for the remaining lifetimes (calculations independent of the cluster)
+                            .chain(
+                                rest.array_windows()
+                                    .filter(|[_, curr]| elidable_lts.contains(&curr.def_id))
+                                    .map(|[prev, curr]| curr.span.with_lo(prev.span.hi())),
+                            )
+                            .map(|sp| (sp, String::new()))
+                            .collect()
+                    },
+                }
+            },
+        }
     };
 
     suggestions.extend(usages.iter().map(|&usage| {

tests/ui/crashes/ice-15666.fixed

@@ -0,0 +1,6 @@
+#![warn(clippy::elidable_lifetime_names)]
+
+struct UnitVariantAccess<'a, 'b, 's>(&'a &'b &'s ());
+trait Trait<'de> {}
+impl<'de> Trait<'de> for UnitVariantAccess<'_, 'de, '_> {}
+//~^ elidable_lifetime_names

tests/ui/crashes/ice-15666.rs

@@ -0,0 +1,6 @@
+#![warn(clippy::elidable_lifetime_names)]
+
+struct UnitVariantAccess<'a, 'b, 's>(&'a &'b &'s ());
+trait Trait<'de> {}
+impl<'de, 'a, 's> Trait<'de> for UnitVariantAccess<'a, 'de, 's> {}
+//~^ elidable_lifetime_names

tests/ui/crashes/ice-15666.stderr

@@ -0,0 +1,16 @@
+error: the following explicit lifetimes could be elided: 'a, 's
+  --> tests/ui/crashes/ice-15666.rs:5:11
+   |
+LL | impl<'de, 'a, 's> Trait<'de> for UnitVariantAccess<'a, 'de, 's> {}
+   |           ^^  ^^                                   ^^       ^^
+   |
+   = note: `-D clippy::elidable-lifetime-names` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::elidable_lifetime_names)]`
+help: elide the lifetimes
+   |
+LL - impl<'de, 'a, 's> Trait<'de> for UnitVariantAccess<'a, 'de, 's> {}
+LL + impl<'de> Trait<'de> for UnitVariantAccess<'_, 'de, '_> {}
+   |
+
+error: aborting due to 1 previous error
+

tests/ui/elidable_lifetime_names.fixed

@@ -192,3 +192,85 @@ mod issue13923 {
         x.b
     }
 }
+
+fn issue15666_original() {
+    struct UnitVariantAccess<'a, 'b, 's>(&'a &'b &'s ());
+
+    trait Trait<'de> {}
+
+    //~v elidable_lifetime_names
+    impl<'de> Trait<'de> for UnitVariantAccess<'_, 'de, '_> {}
+    //        ^^  ^^                                   ^^       ^^
+}
+
+#[allow(clippy::upper_case_acronyms)]
+fn issue15666() {
+    struct S1<'a>(&'a ());
+    struct S2<'a, 'b>(&'a &'b ());
+    struct S3<'a, 'b, 'c>(&'a &'b &'c ());
+
+    trait T {}
+    trait TA<'a> {}
+    trait TB<'b> {}
+    trait TC<'c> {}
+    trait TAB<'a, 'b> {}
+    trait TAC<'a, 'c> {}
+    trait TBC<'b, 'c> {}
+    trait TABC<'a, 'b, 'c> {}
+
+    // 1 lifetime
+
+    impl<'a> TA<'a> for S1<'a> {}
+
+    //~v elidable_lifetime_names
+    impl T for S1<'_> {}
+    //   ^^
+
+    // 2 lifetimes
+
+    impl<'a, 'b> TAB<'a, 'b> for S2<'a, 'b> {}
+
+    //~v elidable_lifetime_names
+    impl<'a> TA<'a> for S2<'a, '_> {}
+    //       ^^
+
+    //~v elidable_lifetime_names
+    impl<'b> TB<'b> for S2<'_, 'b> {}
+    //   ^^
+
+    //~v elidable_lifetime_names
+    impl T for S2<'_, '_> {}
+    //   ^^  ^^
+
+    // 3 lifetimes
+
+    impl<'a, 'b, 'c> TABC<'a, 'b, 'c> for S3<'a, 'b, 'c> {}
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b> TAB<'a, 'b> for S3<'a, 'b, '_> {}
+    //           ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'c> TAC<'a, 'c> for S3<'a, '_, 'c> {}
+    //       ^^
+
+    //~v elidable_lifetime_names
+    impl<'a> TA<'a> for S3<'a, '_, '_> {}
+    //       ^^  ^^
+
+    //~v elidable_lifetime_names
+    impl<'b, 'c> TBC<'b, 'c> for S3<'_, 'b, 'c> {}
+    //   ^^
+
+    //~v elidable_lifetime_names
+    impl<'b> TB<'b> for S3<'_, 'b, '_> {}
+    //   ^^      ^^
+
+    //~v elidable_lifetime_names
+    impl<'c> TC<'c> for S3<'_, '_, 'c> {}
+    //   ^^  ^^
+
+    //~v elidable_lifetime_names
+    impl T for S3<'_, '_, '_> {}
+    //   ^^  ^^  ^^
+}

tests/ui/elidable_lifetime_names.rs

@@ -192,3 +192,85 @@ mod issue13923 {
         x.b
     }
 }
+
+fn issue15666_original() {
+    struct UnitVariantAccess<'a, 'b, 's>(&'a &'b &'s ());
+
+    trait Trait<'de> {}
+
+    //~v elidable_lifetime_names
+    impl<'de, 'a, 's> Trait<'de> for UnitVariantAccess<'a, 'de, 's> {}
+    //        ^^  ^^                                   ^^       ^^
+}
+
+#[allow(clippy::upper_case_acronyms)]
+fn issue15666() {
+    struct S1<'a>(&'a ());
+    struct S2<'a, 'b>(&'a &'b ());
+    struct S3<'a, 'b, 'c>(&'a &'b &'c ());
+
+    trait T {}
+    trait TA<'a> {}
+    trait TB<'b> {}
+    trait TC<'c> {}
+    trait TAB<'a, 'b> {}
+    trait TAC<'a, 'c> {}
+    trait TBC<'b, 'c> {}
+    trait TABC<'a, 'b, 'c> {}
+
+    // 1 lifetime
+
+    impl<'a> TA<'a> for S1<'a> {}
+
+    //~v elidable_lifetime_names
+    impl<'a> T for S1<'a> {}
+    //   ^^
+
+    // 2 lifetimes
+
+    impl<'a, 'b> TAB<'a, 'b> for S2<'a, 'b> {}
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b> TA<'a> for S2<'a, 'b> {}
+    //       ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b> TB<'b> for S2<'a, 'b> {}
+    //   ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b> T for S2<'a, 'b> {}
+    //   ^^  ^^
+
+    // 3 lifetimes
+
+    impl<'a, 'b, 'c> TABC<'a, 'b, 'c> for S3<'a, 'b, 'c> {}
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TAB<'a, 'b> for S3<'a, 'b, 'c> {}
+    //           ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TAC<'a, 'c> for S3<'a, 'b, 'c> {}
+    //       ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TA<'a> for S3<'a, 'b, 'c> {}
+    //       ^^  ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TBC<'b, 'c> for S3<'a, 'b, 'c> {}
+    //   ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TB<'b> for S3<'a, 'b, 'c> {}
+    //   ^^      ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> TC<'c> for S3<'a, 'b, 'c> {}
+    //   ^^  ^^
+
+    //~v elidable_lifetime_names
+    impl<'a, 'b, 'c> T for S3<'a, 'b, 'c> {}
+    //   ^^  ^^  ^^
+}