Add `REDUNDANT_LIFETIMES` lint to detect lifetimes which are semantically redund...
source link: https://github.com/rust-lang/rust/pull/118391
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
There already is a UNUSED_LIFETIMES lint which is fired when we detect where clause bounds like where 'a: 'static, however, it doesn't use the full power of lexical region resolution to detect failures.
Right now UNUSED_LIFETIMES is an Allow lint, though presumably we could bump it to warn? I can (somewhat) easily implement a structured suggestion so this can be rustfix'd automatically, since we can just walk through the HIR body, replacing instances of the redundant lifetime.
Fixes #118376
r? lcnr
added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Member
Author
|
The only wrinkle is that this approach may be detrimental to performance... @bors try @rust-timer queue |
This comment has been minimized.
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
This comment has been minimized.
This comment has been minimized.
Contributor
|
💔 Test failed - checks-actions |
added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
labels
Member
Author
|
@bors try |
Contributor
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (7a2c7d7): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 675.422s -> 674.91s (-0.08%) |
added perf-regression Performance regressions
and removed S-waiting-on-perf Status: Waiting on a perf run to be completed.
labels
few notes:
- I think we should not lint if the "victim" lifetime is elided. The current impl lints in this case but it should not imo:
struct Ty<T: static>(T);
fn test(_: Ty<&str>) {}-
There is no need to fork the inference context and do region resolution for each check, you can simply use
outlives_env.free_region_map.sub_free_regions()to check for equality. -
To avoid the perf impact, can you try to run the lint in
enter_wf_checking_ctxtwhere we've already got the OutlivesEnvironment?
Member
Author
|
I will try adding this into WF if perf still comes back bad... @bors try @rust-timer queue |
This comment has been minimized.
added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label
Contributor
|
@rustbot labels -I-lang-nominated We discussed this in the T-lang triage call today. The feeling was that we should do this, hence the proposed FCP merge above. |
removed the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Contributor
|
☔ The latest upstream changes (presumably #117213) made this pull request unmergeable. Please resolve the merge conflicts. |
Contributor
|
☔ The latest upstream changes (presumably #118553) made this pull request unmergeable. Please resolve the merge conflicts. |
removed the T-types Relevant to the types team, which will review and decide on the PR/issue. label
added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete.
and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
labels
Contributor
Is that about anyways, blocked on FCP @rust-lang/lang. Nominating again to get this FCP finished |
added the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised.
and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off.
labels
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
removed the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Member
Author
That comment is a bit out of date. It was referencing before this was split into REDUNDANT_LIFETIMES, though it's true for both lints. |
} |
||
for &victim in &lifetimes[(idx + 1)..] { |
||
// We only care about lifetimes that are "real", i.e. that have a def-id. |
"lifetimes that are present in the source code"?
Member
Author
'static is present in the source code, so "present in the source code" isn't necessarily correct.
let mut shadowed = FxHashSet::default(); |
||
for (idx, &candidate) in lifetimes.iter().enumerate() { |
||
// Don't suggest removing a lifetime twice. |
This comment should say "Don't suggest using a lifetime we already removed." and a new hashset contains check needs to be added for selecting the same victim twice.
I don't see any tests covering this hashset contains check.
Member
Author
A second check is not necessary due to the transitivity of being redundant (lifetime equality). If there's some other candidate down the road that also is equal to a given victim, then it must also be equal to the candidate lifetime here, and will be placed in the shadowed set.
Also, I don't agree that the reworded comment is necessarily clearer. We're really just suggesting to remove the lifetime twice.
I don't see any tests covering this hashset contains check.
There are no direct tests, but, for example, static_id_indirect below exercises this. Otherwise it would suggest that 'b is redundant twice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
No milestone
Successfully merging this pull request may close these issues.
we should lint on named lifetimes forced to be equal to another named lifetime
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK