Skip to content

Rewrite lifetimes lint pass #14715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Rewrite lifetimes lint pass #14715

wants to merge 4 commits into from

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Apr 30, 2025 •
edited by rustbot
Loading

Based on #14702

This should generally be a perf improvement. The old implementation allocated a lot of things and then did multiple passes over the data. This only does a single pass allocating significantly less. It also bails out earlier in most cases.

Some behaviour changes:

  • An ICE was fixed with debug assertions where removing multiple lifetime arguments could create overlapping spans. These spans are now merged into a single span.
  • The lint message has been changed in the first commit. Adding all the lifetime names to the message is pointless when the lint spans already highlights those.
  • The main lint span now no longer points to all uses. The suggestion already makes it clear what needs to change, no need to make the main lint span messy. It was especially bad when the signature was split over multiple lines.
  • The help message was change to be a little more direct about what gets removed, but I'm not really attached either way.
  • The lifetime in dyn Trait + 'a is now elided when possible instead of completely bailing out of the lint.
  • Lifetimes appearing in various spots no longer completely bails out. Instead this leaves just those lifetimes alone.
  • There is a regression for <'a, T: 'a> where 'a is only used for the type outlives constraint. Theses lifetimes can be removed, but I'd rather handle that at the same time as handling outlives constraints on lifetimes.

changelog: [elidable_lifetime_names]: Lint when dyn Trait + 'a can be elided.
changelog: [needless_lifetimes] [elidable_lifetime_names]: Lint more cases when only some lifetimes can be elided.

Jarcho added 4 commits April 28, 2025 11:22
@Jarcho
Remove redundant work in trait_ref_of_method.
0b01931
@Jarcho
Actually check for a problematic line comment in `with_leading_whites…
3ff1ad4 
…pace`
@Jarcho
needless_lifetimes and elidable_lifetime_names
f1e55c8 
* Change lint message to not mention the lifetime names
* Only point to the lifetime definition in the lint span.
@Jarcho
Rewritelifetimes lint pass. This mainly reduces the amount of work …
f980edd 
…done when no lint is produced, but has a few behavior changes.

* Input to output inference on `dyn Trait` lifetimes is handled with a sufficient MSRV.
* Elision on `dyn Trait` input lifetimes where the lifetime is used only once is suggested on all versions.
* Lifetimes that are only used as a constraint on a single type are no longer detected. This is a regression.
* A bunch of cases where the lints where prevented, but only a single lifetime was non-elidible now lint on remaining lifetimes.
@rustbot
Copy link
Collaborator

rustbot commented Apr 30, 2025

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 1, 2025

☔ The latest upstream changes (possibly 03a5b6b) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jarcho @rustbot @dswij