Make unused_lifetimes lint warn-by-default

[Aaron1011]

There are no open issues related to this lint, and
it would be useful to have turned on.

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[Aaron1011]

See #41960

cc @rust-lang/lang for FCP

[Mark-Simulacrum]

I think it may make sense to temporarily switch this (or in a separate PR) to deny-by-default and run crater, to get a sense of the expected impact - it likely still makes sense to enable since it's just a lint, but having that information ahead of time may be helpful (and maybe we'll spot some bad pattern that should actually not be linted against...).

[joshtriplett]

👍 for doing a crater run to see the impact of this.

I do think we should make this warn-by-default, even if it produces a substantial number of warnings, but I agree that the crater results might turn up patterns that warrant further care.

[Aaron1011]

The impact from the Crater run was quite large - 3666 affected crates

I looked through a couple of the failures, and all of them looked correct to me. I think this PR is now ready for an FCP.

[nikomatsakis]

Nominating for brief discussion in @rust-lang/lang meeting -- how do we feel about the fallout?

It's worth noting that when we've landed things like this in the past, we've gotten pushback from folks with #![deny(warnings)] in CI and so forth. I think before we land this, we ought to at least do a bit of work at notifying people that we intend to do so (e.g., a rust-lang blog post).

[Mark-Simulacrum]

I found a few potential concerns:

On public traits (and other items?) the lifetime is part of the semver exposed surface area (right?), so users just dropping them are changing the public API of their crate. Other unused lints ignore public APIs, e.g. fields in structs. It's worth noting that trait Captures<'tcx> {} does not lint, you need to add fn foo(&self); to get the error.

#![deny(unused_lifetimes)]

pub trait Foo<'a> { //~ ERROR lifetime parameter `'a` never used
    fn foo(&self);
}

pub struct Bar {
    pub foo: u32,
}

(playground)

---

On GATs, I think the unused lifetime may get used downstream, so they may not be unused just because the definition makes no further mention of them:

#![feature(generic_associated_types)]
#![deny(unused_lifetimes)]

pub trait Foo {
    type Message<'a>; //~ ERROR
}

(playground)

[scottmcm]

Hmm, it looks like underscores don't disable the lint:

error: lifetime parameter `'_a` never used
 --> src/lib.rs:3:20
  |
3 | pub trait Captures<'_a> {
  |                   -^^^- help: elide the unused lifetime
  |

That seems like it might be a good thing to have available, like we do for unused_variables.

And having that to recommend would be better than "elide" since trying to use '_ doesn't work:

error[E0637]: `'_` cannot be used here
 --> src/lib.rs:3:20
  |
3 | pub trait Captures<'_> {
  |                    ^^ `'_` is a reserved lifetime name

error: lifetime parameter `'_` never used
 --> src/lib.rs:3:20
  |
3 | pub trait Captures<'_> {
  |                   -^^- help: elide the unused lifetime

[joshtriplett]

We discussed this in today's @rust-lang/rust meeting. We're in favor of making this warn by default, despite the large number of warnings. It'll catch a very common issue.

However, we do want to address the issues that @Mark-Simulacrum and @scottmcm raised above. So, we'll start an FCP to gauge consensus, but add some blocking concerns for those issues.

@rfcbot merge

[joshtriplett]

@rfcbot cancel

[rfcbot]

@joshtriplett proposal cancelled.

[joshtriplett]

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

Concerns:

• allow-underscores-to-silence-the-lint (Make unused_lifetimes lint warn-by-default #92386 (comment))
• lifetimes-are-semver-exposed-in-pub-traits (Make unused_lifetimes lint warn-by-default #92386 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[pnkfelix]

@rfcbot concern lifetimes-are-semver-exposed-in-pub-traits

See #92386 (comment) : we may want to revise how the lint is dealing with pub trait Trait<'a> { fn with_method(&self); }, since 1. that may provide utility that we are currently overlooking and 2. we don't want to encourage authors to silently break semver without due consideration.

[scottmcm]

First, I absolutely want to get this as warn-by-default. But

@rfcbot concern allow-underscores-to-silence-the-lint

Related to pnkfelix's concern, I think it's important to give a way to silence the lint other than allowing it. We compile let _foo = ...; and fn _bar() { ... } without linting, so it should also be ok to have '_quz.

I think that also helps provide hints/fixes compliant with felix's concern, too. It could always suggest removing an unused lifetime from a fn, since that can't be turbofished, but in other places -- especially on pub items -- it could instead give a machine-applicable suggestion to rename it with a _ in front, as that's non-breaking.

Precedent: let foo = 4; suggests "if this is intentional, prefix it with an underscore: _foo", not "remove it".

(Resolving this concern doesn't require fancy complicated suggestions, though. So long as names starting with underscores don't lint, I'll resolve it. Additional QoL stuff after that doesn't need to block FCP.)

[nikomatsakis]

@rfcbot reviewed

[eddyb]

Can (trait) impls be considered separately? Extraneous lifetimes in them (e.g. impl<'a> Foo for Bar {}) aren't API-visible at all, and the only reason they're not an error is a quirk of the implementation (and only possible for lifetimes because behavior cannot depend on them at all).

---

A quick grep through the regressed logs from the crater run seems to reveal 1989 (out of 3631 AFAICT) cases where impl<' shows up (and 1566 w/ a for on the same line, i.e. likely trait impl).

I was hoping these were a small portion but I suppose they weren't. At least they should be a mistake and removable in 100% of cases (except maybe from macros, but maybe we can detect that?), and wouldn't require a '_a silencing.

---

It could always suggest removing an unused lifetime from a fn, since that can't be turbofished

Just in case the implication there was that lifetimes in fns in general cannot be explicitly provided (or perhaps more likely, that someone else might accidentally see it and generalize beyond the "wholly unused" case):

(click to expand the side-tracked rant)

Common misconception (but understandable given the obscurity and arbitrariness of its source - still, a potential issue if library authors make theoretically-semver-breaking changes because the distinction isn't obvious), it's an accident of exposing an internal implementation detail to the user.

If the function definition doesn't allow some lifetime 'x to work as "late-bound" (i.e. the function remaining lifetime generic, a la for<'x> Fn(...) -> ..., even after generic parameter application), usually because of the use of 'x in trait bounds, you end up with a regular generic lifetime parameter.

And the future-incompatible cases where the code only compiles because the distinction is user-visible (such as when you have to pass one lifetime when two are declared) is still only a warning for now.

However, after writing all that, I do believe that a lifetime that isn't named even once, is going to be late-bound and therefore not explicitly specifiable. So the main place where it might be sketchy to remove unused fn lifetimes, would have to be the mixed case where you have other lifetimes that are specifiable and therefore would interact with the #42868 future-incompat warning.

[Mark-Simulacrum]

(Re-tagging with waiting-on-author since this is blocked on impl work to address outstanding concerns)

[Mark-Simulacrum]

The lang team discussed this in the 2022-02-08 meeting, and decided that we'd like to close this PR out for the time being. In the future, once the concerns here are addressed (likely in separate PRs), we would be open to reconsidering the enablement of the warnings by default here, but for now continuing to keep the PR open seems to add little benefit.

I have summarized (via links, primarily) the concerns and thoughts in a tracking issue #94038 so that they don't get lost.