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.

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

Remove redundant member-constraint check #89229

Merged
merged 11 commits into from
Oct 19, 2021
Merged

Remove redundant member-constraint check #89229

merged 11 commits into from
Oct 19, 2021

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 24, 2021 •
edited
Loading

impl trait will, for each lifetime in the hidden type, register a "member constraint" that says the lifetime must be equal or outlive one of the lifetimes of the impl trait. These member constraints will be solved by borrowck

But, as you can see in the big red block of removed code, there was an ad-hoc check for member constraints happening at the site where they get registered. This check had some minor effects on diagnostics, but will fall down on its feet with my big type alias impl trait refactor. So we removed it and I pulled the removal out into a (hopefully) reviewable PR that works on master directly.

@rust-highfive
Copy link
Collaborator

r? @nagisa

(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 Sep 24, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 24, 2021

@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 Sep 24, 2021
@bors
Copy link
Contributor

bors commented Sep 24, 2021

⌛ Trying commit d559f5638f04ec9239daff40c99f915724165b69 with merge e5988663b958f804945421ea908975067378270f...

@bors
Copy link
Contributor

bors commented Sep 24, 2021

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

@rust-timer
Copy link
Collaborator

Queued e5988663b958f804945421ea908975067378270f with parent f06f9bb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e5988663b958f804945421ea908975067378270f): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 208.2% on full builds of wg-grammar)

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 24, 2021
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 25, 2021

@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 Sep 25, 2021
@bors
Copy link
Contributor

bors commented Sep 25, 2021

⌛ Trying commit dde6581f20262fc84caa19d66ea39973b791f2d6 with merge 7bd51ca41626fd972720681467f74ed48ca6976e...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 25, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Sep 25, 2021

⌛ Trying commit 13d3eef5abd280a0c7c19160597c579361e5fdfd with merge d554c07b2ff5aaea10eab8489f88180ba5baa6de...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 25, 2021

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

@rust-timer
Copy link
Collaborator

Queued d554c07b2ff5aaea10eab8489f88180ba5baa6de with parent 218a96c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d554c07b2ff5aaea10eab8489f88180ba5baa6de): comparison url.

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

  • Moderate improvement in instruction counts (up to -1.0% on full builds of wg-grammar)

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 S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Sep 25, 2021
@jackh726
Copy link
Member

@oli-obk I'll try to review this weekend!

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jackh726
jackh726 reviewed Oct 18, 2021
View reviewed changes
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.

I think this is fine...

I'm seeing some duplicate errors being emitted; it would be nice to not do that, but I'd be fine landing this regardless.

I'm going to go ahead and say r=me here with or without fixing duplicate errors being emitted.

src/test/ui/impl-trait/static-return-lifetime-infered.stderr
LL | fn iter_values_anon(&self) -> impl Iterator<Item=u32> + '_ {
| ++++

error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that this error gets duplicated. Any particular reason why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somewhat... there are three places that do impl trait checks: typeck, borrowck and wfcheck. I am unsure without running a trace where this specific one is duplicated.

But in any case: I know it disappears with my lazy TAIT branch, and I believe users don't see it anyway due to error deduplication.

@jackh726
Copy link
Member

Oh, another thing: Can you squash the last couple commits of just blessing tests?

@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 18, 2021

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Oct 18, 2021

📌 Commit 4413f8c 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2021
@bors
Copy link
Contributor

bors commented Oct 18, 2021

⌛ Testing commit 4413f8c with merge ec724ac...

@bors
Copy link
Contributor

bors commented Oct 19, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 19, 2021
@bors bors merged commit ec724ac into rust-lang:master Oct 19, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 19, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ec724ac): comparison url.

Summary: This benchmark run did not return any relevant changes.

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

@rustbot label: -perf-regression

@oli-obk oli-obk deleted the i_love_inferctxt branch October 19, 2021 06:19
This was referenced Oct 20, 2021
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 left review comments

@estebank estebank estebank left review comments

@nagisa nagisa nagisa left review comments

Assignees

@jackh726 jackh726

Labels
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
Milestone
1.58.0
Development

Successfully merging this pull request may close these issues.