Skip to content
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.

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

Lint elided lifetimes in path during lifetime resolution. #90446

Merged
merged 7 commits into from
Dec 2, 2021

Conversation

cjgillot
Copy link
Contributor

The lifetime elision lint is known to be brittle and can be redundant with later lifetime resolution errors. This PR aims to remove the redundancy by performing the lint after lifetime resolution.

This PR proposes to carry the information that an elision should be linted against by using a special LifetimeName. I am not certain this is the best solution, but it is certainly the easiest.

Fixes #60199
Fixes #55768
Fixes #63110
Fixes #71957

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Didn't give this a super close read yet, but had some thoughts.

compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late/diagnostics.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member

jackh726 commented Nov 1, 2021

r? @jackh726

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 11, 2021
@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 26, 2021
@bors
Copy link
Contributor

bors commented Nov 26, 2021

⌛ Trying commit dd28f96 with merge 15577f9bb6cc47d19e4ab586f20ba64f7fe26b01...

@bors
Copy link
Contributor

bors commented Nov 26, 2021

☀️ Try build successful - checks-actions
Build commit: 15577f9bb6cc47d19e4ab586f20ba64f7fe26b01 (15577f9bb6cc47d19e4ab586f20ba64f7fe26b01)

@rust-timer
Copy link
Collaborator

Queued 15577f9bb6cc47d19e4ab586f20ba64f7fe26b01 with parent 6d246f0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (15577f9bb6cc47d19e4ab586f20ba64f7fe26b01): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -3.9% on incr-unchanged builds of deep-vector)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking 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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 27, 2021
@cjgillot
Copy link
Contributor Author

@jackh726 the variant with Implicit(bool) is here: #91271

Finished benchmarking commit (3a16e8d): comparison url.

Summary: This change led to large relevant mixed results shrug in compiler performance.

* Large improvement in instruction counts (up to -3.9% on `incr-unchanged` builds of `deep-vector`)

* Moderate regression in instruction counts (up to 1.4% on `incr-unchanged` builds of `wg-grammar`)

@jackh726
Copy link
Member

I'm going to go ahead and say that I prefer #91271. Aside from wg-grammar opt, perf is pretty similar. I would attribute that to as maybe just noise. However, that PR looks much cleaner imo. That said, it does look like the "Probably a nicer way to do this would be to make new_implicit_lifetime just take a missing: bool arg" comment got missed in this PR but is present there.

r=me on the variant PR once fmt done + CI green. Or r=me on this PR with above comment addressed.

@jackh726 jackh726 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 Nov 30, 2021
@cjgillot
Copy link
Contributor Author

cjgillot commented Dec 1, 2021

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Dec 1, 2021

📌 Commit aa2450f has been approved by jackh726

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 1, 2021
@bors
Copy link
Contributor

bors commented Dec 1, 2021

⌛ Testing commit aa2450f with merge 76938d6...

@bors
Copy link
Contributor

bors commented Dec 2, 2021

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 76938d6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 2, 2021
@bors bors merged commit 76938d6 into rust-lang:master Dec 2, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 2, 2021
@bors bors mentioned this pull request Dec 2, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (76938d6): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

  • Large improvement in instruction counts (up to -1.4% on incr-unchanged builds of stm32f4)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L-elided_lifetimes_in_paths Lint: elided_lifetimes_in_paths merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants