Extend `needless_lifetimes` to suggest eliding `impl` lifetimes #13286

smoelius · 2024-08-18T19:39:33Z

Example:

error: the following explicit lifetimes could be elided: 'a
  --> tests/ui/needless_lifetimes.rs:332:10
   |
LL |     impl<'a> Foo for Baz<'a> {}
   |          ^^              ^^
   |
help: elide the lifetimes
   |
LL -     impl<'a> Foo for Baz<'a> {}
LL +     impl Foo for Baz<'_> {}

The main change is in how impl lifetime uses are tracked. Previously, a hashmap was created, and lifetimes were removed from the hashmap as their uses were discovered. However, the uses are needed to generate elision suggestions. So, now, uses are added to the hashmap as they are discovered.

The PR is currently organized as six commits, which I think are self-explanatory:

Extend needless_lifetimes to suggest eliding impl lifetimes
Reorder functions [not strictly necessary, but IMHO, the code is better structured as a result]
Fix lifetime tests
Fix non-lifetime tests
Fix clippy_lints and clippy_utils
Fix typo in needless_lifetimes test

r? @Alexendoo (I think you are needless_lifetimes' primary author? Sorry if I have this wrong.)

changelog: Extend needless_lifetimes to suggest eliding impl lifetimes

bors · 2024-09-12T16:30:13Z

☔ The latest upstream changes (presumably #13390) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-09-22T19:11:58Z

☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts.

smoelius · 2024-09-22T23:43:49Z

Hey, @Alexendoo. Short of a full review, could I ask what your thoughts are on this PR?

Also, I would understand if your plate is too full. Would you like be to ask rustbot to select another reviewer?

bors · 2024-09-25T03:08:35Z

☔ The latest upstream changes (presumably #13442) made this pull request unmergeable. Please resolve the merge conflicts.

Alexendoo · 2024-09-25T14:25:14Z

Sorry yeah been busy, I think I've reviewed some stuff for this lint but it's been a while and I've forgotten most of it 😅

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

Some of the added extra_unused_lifetimes cases in https://github.com/rust-lang/rust-clippy/actions/runs/10985315434#summary-30497090664 look incorrect though e.g. https://docs.rs/rayon/1.10.0/src/rayon/iter/copied.rs.html#28

rust-lang#13286 (comment)

smoelius · 2024-09-25T22:12:23Z

Sorry yeah been busy, I think I've reviewed some stuff for this lint but it's been a while and I've forgotten most of it 😅

NP. Thanks very much for your comments.

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

👍

Some of the added extra_unused_lifetimes cases in https://github.com/rust-lang/rust-clippy/actions/runs/10985315434#summary-30497090664 look incorrect though e.g. https://docs.rs/rayon/1.10.0/src/rayon/iter/copied.rs.html#28

I was using GenericArg when I should have been using GenericArgs. The problem is fixed, and I added the case you pointed out as a test case.

Alexendoo · 2024-09-29T12:53:10Z

clippy_lints/src/lifetimes.rs

-    let mut checker = LifetimeChecker::<hir_nested_filter::None>::new(cx, hs);
+    let mut checker = LifetimeChecker::<hir_nested_filter::None>::new(cx, def_ids);


We could collect into the HashMap LifetimeChecker wants directly rather than creating a temporary Vec

Maybe pass the Generics into LifetimeChecker::new and have the collect done in there? (Maybe this is what you were suggesting.)

I was just thinking of passing in FxHashMap<LocalDefId, Vec<Usage>> but yeah yours is better since there's two places the collect is being done already

rust-lang#13286 (comment)

Alexendoo · 2024-10-01T00:40:13Z

Thanks!

@bors r+

bors · 2024-10-01T00:40:15Z

📌 Commit 54e4761 has been approved by Alexendoo

It is now in the queue for this repository.

bors · 2024-10-01T00:41:24Z

⌛ Testing commit 54e4761 with merge db1bda3...

bors · 2024-10-01T00:51:23Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing db1bda3 to master...

smoelius · 2024-10-01T00:52:28Z

Thank you!

Tracking lifetime support in impls was added in recent nightly clippy. rust-lang/rust-clippy#13286

I'm not entirely sure I like the non-uniformity of giving a name to the lifetime in the type only if it is used in the body, but I do appreciate reducing the generic parameter count in many impl blocks, and all of the actual cases seem fine. This additional lint came from <rust-lang/rust-clippy#13286> “Extend needless_lifetimes to suggest eliding impl lifetimes”.

In nightly Clippy (i.e. Rust 1.83.0), the `needless_lifetimes` lint has been extended [1] to suggest eliding `impl` lifetimes, e.g. error: the following explicit lifetimes could be elided: 'a --> rust/kernel/list.rs:647:6 | 647 | impl<'a, T: ?Sized + ListItem<ID>, const ID: u64> FusedIterator for Iter<'a, T, ID> {} | ^^ ^^ | = 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 | 647 - impl<'a, T: ?Sized + ListItem<ID>, const ID: u64> FusedIterator for Iter<'a, T, ID> {} 647 + impl<T: ?Sized + ListItem<ID>, const ID: u64> FusedIterator for Iter<'_, T, ID> {} Thus clean them. Link: rust-lang/rust-clippy#13286 [1] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

rustbot assigned Alexendoo Aug 18, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 18, 2024

smoelius force-pushed the elidable-impl-lifetimes branch 2 times, most recently from 07b5310 to c64794a Compare September 12, 2024 17:05

smoelius force-pushed the elidable-impl-lifetimes branch from c64794a to 05cdfad Compare September 22, 2024 23:40

smoelius added 7 commits September 25, 2024 13:48

Extend needless_lifetimes to suggest eliding impl lifetimes

55fcd46

Reorder functions

0a11c5c

Fix lifetime tests

f2495a1

Fix non-lifetime tests

8775850

Fix clippy_lints and clippy_utils

66f1f54

Fix typo in needless_lifetimes test

5c2f6db

Fix bug found during review

51b5272

rust-lang#13286 (comment)

smoelius force-pushed the elidable-impl-lifetimes branch from 05cdfad to 51b5272 Compare September 25, 2024 22:02

Adjust comment

b3cf448

Alexendoo reviewed Sep 29, 2024

View reviewed changes

Address review comment

54e4761

rust-lang#13286 (comment)

Alexendoo approved these changes Oct 1, 2024

View reviewed changes

bors merged commit db1bda3 into rust-lang:master Oct 1, 2024
8 checks passed

smoelius deleted the elidable-impl-lifetimes branch October 1, 2024 00:52

This was referenced Oct 1, 2024

Make trivial-copy-size-limit target independent #13319

Open

Initial impl of repr_packed_without_abi #13398

Open

zombie_processes: consider wait() calls in nested bodies #13462

Merged

y21 mentioned this pull request Oct 4, 2024

lint suggestion: needless_lifetimes in impl<'a> #12424

Closed

dburgener added a commit to dburgener/lalrpop that referenced this pull request Oct 4, 2024

Elide needless lifetimes in impls

c5068c2

Tracking lifetime support in impls was added in recent nightly clippy. rust-lang/rust-clippy#13286

dburgener mentioned this pull request Oct 4, 2024

Elide needless lifetimes in impls lalrpop/lalrpop#967

Merged

github-merge-queue bot pushed a commit to lalrpop/lalrpop that referenced this pull request Oct 4, 2024

Elide needless lifetimes in impls (#967)

8a131a9

Tracking lifetime support in impls was added in recent nightly clippy. rust-lang/rust-clippy#13286

ojeda mentioned this pull request Oct 12, 2024

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

Open

DJMcNab mentioned this pull request Nov 1, 2024

Eliminate needless lifetimes linebender/xilem#723

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `needless_lifetimes` to suggest eliding `impl` lifetimes #13286

Extend `needless_lifetimes` to suggest eliding `impl` lifetimes #13286

smoelius commented Aug 18, 2024

bors commented Sep 12, 2024

bors commented Sep 22, 2024

smoelius commented Sep 22, 2024

bors commented Sep 25, 2024

Alexendoo commented Sep 25, 2024

smoelius commented Sep 25, 2024

Alexendoo Sep 29, 2024

smoelius Sep 29, 2024

Alexendoo Sep 29, 2024 •

edited

Loading

Alexendoo commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 1, 2024

smoelius commented Oct 1, 2024

		let mut checker = LifetimeChecker::<hir_nested_filter::None>::new(cx, hs);
		let mut checker = LifetimeChecker::<hir_nested_filter::None>::new(cx, def_ids);

Extend needless_lifetimes to suggest eliding impl lifetimes #13286

Extend needless_lifetimes to suggest eliding impl lifetimes #13286

Conversation

smoelius commented Aug 18, 2024

bors commented Sep 12, 2024

bors commented Sep 22, 2024

smoelius commented Sep 22, 2024

bors commented Sep 25, 2024

Alexendoo commented Sep 25, 2024

smoelius commented Sep 25, 2024

Alexendoo Sep 29, 2024

Choose a reason for hiding this comment

smoelius Sep 29, 2024

Choose a reason for hiding this comment

Alexendoo Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Alexendoo commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 1, 2024

smoelius commented Oct 1, 2024

Extend `needless_lifetimes` to suggest eliding `impl` lifetimes #13286

Extend `needless_lifetimes` to suggest eliding `impl` lifetimes #13286

Alexendoo Sep 29, 2024 •

edited

Loading