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

resolve: Use NameBinding for local variables and generic parameters #89100

Merged
merged 1 commit into from
Oct 20, 2021
Merged

resolve: Use NameBinding for local variables and generic parameters #89100

merged 1 commit into from
Oct 20, 2021

Conversation

petrochenkov
Copy link
Contributor

NameBinding is a structure used for representing any name introduction (an item, or import, or even a built-in).
Except that local variables and generic parameters weren't represented as NameBindings, for this reason they requires separate paths in name resolution code in several places.

This PR introduces NameBindings for local variables as well and simplifies all the code working with them leaving only the NameBinding paths.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 19, 2021
@cjgillot cjgillot self-assigned this Oct 3, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2021

📌 Commit e4e3c9a has been approved by cjgillot

@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 17, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 17, 2021
@matthiaskrgr
Rollup merge of rust-lang#89100 - petrochenkov:localbind, r=cjgillot
14d1cd9 
resolve: Use `NameBinding` for local variables and generic parameters

`NameBinding` is a structure used for representing any name introduction (an item, or import, or even a built-in).
Except that local variables and generic parameters weren't represented as `NameBinding`s, for this reason they requires separate paths in name resolution code in several places.

This PR introduces `NameBinding`s for local variables as well and simplifies all the code working with them leaving only the `NameBinding` paths.
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 17, 2021
Closed
@ehuss
Copy link
Contributor

ehuss commented Oct 17, 2021

@bors r-

Failed in rollup #89992:

tidy error: /checkout/compiler/rustc_resolve/src/lib.rs: too many lines (3043) (add // ignore-tidy-filelength to the file to suppress this error)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 17, 2021
@jyn514
Copy link
Member

jyn514 commented Oct 18, 2021

@bors rollup=iffy

@petrochenkov
Copy link
Contributor Author

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Oct 18, 2021

📌 Commit c1e8fc8 has been approved by cjgillot

@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 Oct 18, 2021
@bors
Copy link
Contributor

bors commented Oct 19, 2021

⌛ Testing commit c1e8fc8 with merge 4a2bec300777fb1a2450c5dd87f7b00792b99969...

@bors
Copy link
Contributor

bors commented Oct 19, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 19, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@petrochenkov
Copy link
Contributor Author

@bors retry

@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 19, 2021
@ehuss
Copy link
Contributor

ehuss commented Oct 19, 2021

@bors retry

test-various hung after downloading the docker image, no clear reason.

@bors
Copy link
Contributor

bors commented Oct 20, 2021

⌛ Testing commit c1e8fc8 with merge 6162529...

@bors
Copy link
Contributor

bors commented Oct 20, 2021

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 6162529 to master...

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

Finished benchmarking commit (6162529): 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

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6162529): comparison url.

Summary: ERROR categorizing benchmark run!

@rustbot label: -perf-regression

@klensy klensy mentioned this pull request Oct 20, 2021
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6162529): comparison url.

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

  • Very large regression in instruction counts (up to 99.5% on incr-unchanged builds of style-servo)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Oct 20, 2021
@Mark-Simulacrum
Copy link
Member

This'll need some manual checking locally to verify the regression given the potential environment changes to the collector, I'll try to do that.

@Mark-Simulacrum
Copy link
Member

I'm pretty confident the regression is real. There's wall time regressions (including in e.g. self-profile results), so it seems to bear out in practice.

@petrochenkov I am inclined to revert this temporarily until there's a fix, unless the fix is simple and can be landed on the roughly same timeframe.

@petrochenkov
Copy link
Contributor Author

Wow, not something that I'd expect.
I suggest reverting, I'll try to investigate this but not soon.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Oct 21, 2021
@Mark-Simulacrum
Revert "Auto merge of rust-lang#89100 - petrochenkov:localbind, r=cjg…
7552af2 
…illot"

This reverts commit 6162529.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 21, 2021
Merged
@Mark-Simulacrum
Copy link
Member

Posted a revert #90130.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2021
@bors
Auto merge of rust-lang#90130 - Mark-Simulacrum:revert-namebinding, r…
547a6ff 
…=oli-obk

Revert "resolve: Use NameBinding for local variables and generic parameters"

This reverts commit 6162529, that is, PR rust-lang#89100.

Reverting per performance regression noted post-merge on that PR (rust-lang#89100 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@estebank estebank

@cjgillot cjgillot

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. 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.

None yet