rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.49k stars 1.55k forks source link

Consider splitting `needless_lifetimes` into cases that do and do not require `'_` #13514

Open dtolnay opened 1 month ago

dtolnay commented 1 month ago

Description

I think the suggested code changes from needless_lifetimes are consistently good improvements when they look like this, involving only deleting characters:

warning: the following explicit lifetimes could be elided: 'a
   --> src/de.rs:339:11
    |
339 | impl<'de, 'a> EnumAccess<'de> for &'a mut Deserializer<'de> {
    |           ^^                       ^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
    |
339 - impl<'de, 'a> EnumAccess<'de> for &'a mut Deserializer<'de> {
339 + impl<'de> EnumAccess<'de> for &mut Deserializer<'de> {

and a mixed bag when they look like this:

warning: the following explicit lifetimes could be elided: 'a
   --> src/de.rs:197:19
    |
197 |         impl<'de, 'a> de::SeqAccess<'de> for SeqAccess<'a, 'de> {
    |                   ^^                                   ^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
    |
197 -         impl<'de, 'a> de::SeqAccess<'de> for SeqAccess<'a, 'de> {
197 +         impl<'de> de::SeqAccess<'de> for SeqAccess<'_, 'de> {
    |

I would love to be able to deny the first category and allow the second category.

Another possible way to draw a distinction is whether the lint pertains to an already-named lifetime. In SeqAccess<'a, 'de> a name has already been chosen for that 'a lifetime in the location where the type is defined, so the impl isn't responsible for inventing a whole new name. Whereas in &'a mut Deserializer<'de>, the impl is needlessly inventing a brand new name for this 'a lifetime. I think this "already-named" framing is slightly different than the "does it require '_" framing, considering cases like impl Trait for dyn Trait + '_. I am not sure which of the two would make the best split.

Version

rustc 1.83.0-nightly (9096f4faf 2024-10-05) binary: rustc commit-hash: 9096f4fafa2ac2d771f866337b4ee7064cde8575 commit-date: 2024-10-05 host: aarch64-unknown-linux-gnu release: 1.83.0-nightly LLVM version: 19.1.0

Additional Labels

No response

ojeda commented 1 month ago

Yeah, it may be a good idea to split some of these -- in the PR, @Alexendoo seemed to have similar thoughts:

The change seems like a natural extension but it does make me wonder if we should split the lint into cases that need '_ and ones that don't, but that can be separate since we already suggest '_ sometimes

alice-i-cecile commented 1 month ago

Seconding this request. The introduction of '_ lifetimes is a lateral move at best IMO.

We tested this lint in the Bevy repo here: https://github.com/bevyengine/bevy/pull/15907#issuecomment-2412254361. As discussed, the loss of encoded information with the choice of lifetime names is quite painful.

orlp commented 1 week ago

I'd love if it this was implemented, clippy just suggested this monstrosity to me:

error: the following explicit lifetimes could be elided: 'k
   --> crates/polars-utils/src/idx_map/bytes_idx_map.rs:136:10
    |
136 | impl<'a, 'k, V> VacantEntry<'a, 'k, V> {
    |          ^^                     ^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
    = note: `-D clippy::needless-lifetimes` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_lifetimes)]`
help: elide the lifetimes
    |
136 - impl<'a, 'k, V> VacantEntry<'a, 'k, V> {
136 + impl<'a, V> VacantEntry<'a, '_, V> {
ojeda commented 1 week ago

For Linux, we would have needed this: https://lore.kernel.org/rust-for-linux/20241012231300.397010-1-ojeda@kernel.org/

In the end, we decided to allow the lint instead for the time being: https://lore.kernel.org/rust-for-linux/20241116181538.369355-1-ojeda@kernel.org/