Implement suggestions for elided_named_lifetimes

[GrigorenkoPV]

A follow-up to #129207, as per #129207 (comment).

r? cjgillot

I will probably squash this a bit, but later.

@rustbot label +A-lint

[GrigorenkoPV]

Apparently this is in conflict with clippy's needless_lifetimes. For

fn foo<'a>(x: &'a str) -> &str

this suggests adding lifetime to the output, but clippy suggests removing it from the input. Applying both at the same time produces

fn foo(x: &str) -> &'a str

which is not correct.

I would say that needless_lifetimes here is probably a more sensible suggestion than elided_named_lifetimes.

Though, if one applies elided_named_lifetimes first, then needless_lifetimes is still able to tell that the lifetime can be removed (now from both input and output), and the result is not broken.

Idk how to fix this one. cc @rust-lang/clippy

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[GrigorenkoPV]

It seems like without suggestions, elided_named_lifetimes can be quite confusing: #129207 (comment), so I feel like it is a priority to get this PR out in a timely manner, but there is a blocker at the moment: #129840 (comment)

I see three possible ways of dealing with the issue.

1. Hack around the conflict now

   This is what I've done: 9e2d264. This commit simply disables the suggestions for non-'static cases¹.

   Pros: no stalling, adds some suggestions, there is no conflict, the hack is easy to revert once we resolve the issue.
   Cons: many suggestions are missing.

2. Ignore the conflict

   Just merge as is (removing the failing test), and deal with it later. The fallout of the conflict would probably be rather small².

   Pros: no stalling, all suggestions are added.
   Cons: potential conflicts with a clippy complexity lint.

3. Resolve this now

   Do not merge this PR until the issue is resolved.

   Pros: all suggestions are added, there is no conflict
   Cons: high risk of missing the 1.83 release window, which elided_named_lifetimes has landed into.

Footnotes

1. I've tried setting applicability for potentially conflicting cases to all other variants instead of MachineApplicable, but it didn't help. ↩

2. citation needed ↩

[cjgillot]

@bors r+ rollup

The easiest way to remove the conflict could be to uplift the needless_lifetimes lint to rustc. It may fit well with the current architecture of the code, but I haven't tried writing any code.

[bors]

📌 Commit 9e2d264 has been approved by cjgillot

It is now in the queue for this repository.

[GrigorenkoPV]

Okay, I've managed to swap the priorities between elided_named_lifetimes and cippy::needless_lifetimes. Now the latter does not emit when the former would.¹

Patch

diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs
index ae7e9659856..693f1b22587 100644
--- a/compiler/rustc_lint/src/lints.rs
+++ b/compiler/rustc_lint/src/lints.rs
@@ -2639,13 +2639,6 @@ fn decorate_lint(self, diag: &mut rustc_errors::Diag<'_, G>) {
         if let Some(declaration) = declaration {
             diag.span_label(declaration, fluent::lint_label_named);
         }
-        // FIXME(GrigorenkoPV): this `if` and `return` should be removed,
-        //  but currently this lint's suggestions can conflict with those of `clippy::needless_lifetimes`:
-        //  https://github.com/rust-lang/rust/pull/129840#issuecomment-2323349119
-        // HACK: `'static` suggestions will never sonflict, emit only those for now.
-        if name != rustc_span::symbol::kw::StaticLifetime {
-            return;
-        }
         match kind {
             MissingLifetimeKind::Underscore => diag.span_suggestion_verbose(
                 span,
diff --git a/src/tools/clippy/clippy_lints/src/lifetimes.rs b/src/tools/clippy/clippy_lints/src/lifetimes.rs
index ba34af9c100..793ef442c13 100644
--- a/src/tools/clippy/clippy_lints/src/lifetimes.rs
+++ b/src/tools/clippy/clippy_lints/src/lifetimes.rs
@@ -420,6 +420,9 @@ fn could_use_elision<'tcx>(
         .filter_map(|(def_id, occurrences)| {
             if occurrences == 1
                 && (input_lts.len() == 1 || !output_lts.iter().any(|lt| named_lifetime(lt) == Some(def_id)))
+                && !output_lts
+                    .iter()
+                    .any(|lt| suggested_by_elided_named_lifetimes(lt, def_id))
             {
                 Some(def_id)
             } else {
@@ -437,6 +440,10 @@ fn could_use_elision<'tcx>(
     Some((elidable_lts, usages))
 }
 
+fn suggested_by_elided_named_lifetimes(lt: &Lifetime, res: LocalDefId) -> bool {
+    lt.res == LifetimeName::Param(res) && matches!(lt.ident.name, kw::Empty | kw::UnderscoreLifetime)
+}
+
 fn allowed_lts_from(named_generics: &[GenericParam<'_>]) -> FxHashSet<LocalDefId> {
     named_generics
         .iter()

but the problem now is that

if one applies elided_named_lifetimes first, then needless_lifetimes is still able to tell that the lifetime can be removed (now from both input and output)

This does result in only correct code along the way, but it makes a new suggestion pop up after the previous is applied. The testsuite does not like that:

error: test got exit status: 1, but expected 0
   --> tests/ui/needless_lifetimes.fixed:266:30
    |
266 | fn named_input_elided_output<'a>(_arg: &'a str) -> &'a str {
    |                              ^^ after rustfix is applied, all errors should be gone, but weren't
    |

So, to reiterate my previous point:

I would say that needless_lifetimes here is probably a more sensible suggestion than elided_named_lifetimes.

We should probably emit needless_lifetimes first, cache the set of lifetime params that were suggested to be elided, and then just avoid those in elided_named_lifetimes.

The problem is that we emit elided_named_lifetimes immediately during lifetime resolution, while needless_lifetimes walks the tree after the fact.

I don't think we can move needless_lifetimes inside the resolution (it fundamentally works with already resolved lifetimes and walks the tree quite a lot). But maybe we can move elided_named_lifetimes outside of resolution and into the same pass as needless_lifetimes.²

The easiest way to remove the conflict could be to uplift the needless_lifetimes lint to rustc.

It seems like uplifting is unavoidable here, because we need some information to flow from needless_lifetimes to elided_named_lifetimes. But needless_lifetimes currently has some issues:

• It might be too aggressive for people's liking: Suggestion: Make needless_lifetimes ignore functions that are, or contain, unsafe. rust-clippy#9694
• Broken suggestions
  • Format error in suggestion for needless_lifetimes rust-clippy#12789
  • Sometimes not all occurrences are removed
    • needless_lifetimes suggestion forgets to remove some lifetimes rust-clippy#10495
    • needless_lifetimes wrongly detect elidable lifetime with Iterator rust-clippy#11291
• It can produce overlapping suggestions
  • debug ice: overlapping suggestions when needless_lifetimes removes back-to-back lifetimes  rust-clippy#11197
  • Separate lint needless_lifetimes for types with multiple lifetimes rust-clippy#12495
• False positives
  • needless_lifetimes removes lifetimes that are actually necessary rust-clippy#12908
• Some false negatives, but that is not an issue for uplifting. And there seems to be an upcoming PR to fix some of those Extend needless_lifetimes to suggest eliding impl lifetimes rust-clippy#13286

There was an attempt to fix those but it seemingly got abandoned right at the start: rust-lang/rust-clippy#13141 rust-lang/rust-clippy#12456

Soo, should we maybe just break needless_lifetimes even further and go with the option 2?

Footnotes

1. And unlike with the 9e2d264 hack, the suggestion is suppressed only when there would be a conflict, not when there potentially could be a conflict. ↩

2. It seems like we can easily detect the lifetimes that needless_lifetimes is interested in using matches!(lt.res, LifetimeName::Param(_) | LifetimeName::Static) && matches!(lt.ident.name, kw::Empty | kw::UnderscoreLifetime). Though, we do lose access to MissingLifetimeKind, which was quite handy for suggestions. But maybe we can propagate it or infer it from the context. Speaking of inferring from the context, moving elided_named_lifetimes to a LateLintPass will probably eliminate the need for the LifetimeRes::Static.suppress_elision_warning & MissingLifetime.id_for_lint hacks. ↩