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

Remove TypeckTables::empty(None) and make hir_owner non-optional. #73751

Merged
merged 7 commits into from
Jul 2, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 26, 2020

Each commit before the last one removes uses of TypeckTables::empty(None), replacing the empty tables with having Option around the &'tcx TypeckTables<'tcx> that HIR visitors kept track of.

The last commit removes the concept of "empty TypeckTables" altogether, guaranteeing that every TypeckTables corresponds to a HIR body owner.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2020
@eddyb
Copy link
Member Author

eddyb commented Jun 26, 2020

@bors try rollup=never @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 26, 2020

⌛ Trying commit e04c197165543ccd5d39c548a882a55edc68b2cd with merge 76da4feb08c1227c6fbff8370522c75e606b3195...

@bors
Copy link
Contributor

bors commented Jun 26, 2020

☀️ Try build successful - checks-azure
Build commit: 76da4feb08c1227c6fbff8370522c75e606b3195 (76da4feb08c1227c6fbff8370522c75e606b3195)

@rust-timer
Copy link
Collaborator

Queued 76da4feb08c1227c6fbff8370522c75e606b3195 with parent 1033351, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (76da4feb08c1227c6fbff8370522c75e606b3195): comparison url.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me with some comments about why we can be sure the options are Some

src/librustc_driver/pretty.rs Outdated Show resolved Hide resolved
src/librustc_passes/reachable.rs Outdated Show resolved Hide resolved
@eddyb eddyb force-pushed the no-empty-tables branch 2 times, most recently from b873d23 to 4118090 Compare June 30, 2020 16:28
@eddyb
Copy link
Member Author

eddyb commented Jun 30, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 30, 2020

📌 Commit 4118090 has been approved by nikomatsakis

@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 Jun 30, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 1, 2020
Remove `TypeckTables::empty(None)` and make hir_owner non-optional.

Each commit before the last one removes uses of `TypeckTables::empty(None)`, replacing the empty tables with having `Option` around the `&'tcx TypeckTables<'tcx>` that HIR visitors kept track of.

The last commit removes the concept of "empty `TypeckTables`" altogether, guaranteeing that every `TypeckTables` corresponds to a HIR body owner.

r? @nikomatsakis
@eddyb
Copy link
Member Author

eddyb commented Jul 1, 2020

@bors rollup=never (maybe it didn't trigger the previous time I did this)

@Manishearth
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented Jul 1, 2020

⌛ Testing commit 4118090 with merge 187f6164577d9bbc9428e2f1d2ecb91cd0fb7ec2...

@bors
Copy link
Contributor

bors commented Jul 1, 2020

💔 Test failed - checks-actions

@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 Jul 1, 2020
@eddyb
Copy link
Member Author

eddyb commented Jul 1, 2020

I just realized that the test that was failing to build (not to run, so I would've never seen) is the only test, not just one of many, and is responsible for running the actual clippy test suite. So now I have to go back and make sure this is doable at all wrt clippy.

@eddyb
Copy link
Member Author

eddyb commented Jul 1, 2020

Well, looks like this works for tracking down which cx.tables() calls are panicking:

./x.py test src/tools/clippy | rg "thread 'rustc' panicked at '`LateContext::tables` called outside of body'"

Deduplicated, it's only these cx.tables() calls that fail (the first one is the most common by far):

I anticipated qpath_res and had in mind adding a method to LateContext to gracefully handle situations outside the body (I've already done something similar elsewhere), I only didn't do it because I didn't realize I wasn't running clippy tests.

Since there's more than one example of cx.tables().qpath_res(...) failing I'll probably change all instances of it (even if most are expressions, which can't fail). Although I should check with clippy devs first.

The other ones are short-lived contexts that probably just need an Option around their tables fields.

@eddyb eddyb force-pushed the no-empty-tables branch from 4118090 to 5ec300f Compare July 1, 2020 15:49
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 1, 2020

📌 Commit 5ec300f1c05520d2f651bec7e37124d10fed448c has been approved by nikomatsakis

@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 Jul 1, 2020
@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit 5ec300f1c05520d2f651bec7e37124d10fed448c with merge 4c651407741c538614ac95257e82d6eefcbdca3b...

@bors

This comment has been minimized.

@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 Jul 2, 2020
@eddyb eddyb force-pushed the no-empty-tables branch from 5ec300f to 4b2d9e6 Compare July 2, 2020 13:51
@eddyb
Copy link
Member Author

eddyb commented Jul 2, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 2, 2020

📌 Commit 4b2d9e6 has been approved by nikomatsakis

@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 Jul 2, 2020
@bors
Copy link
Contributor

bors commented Jul 2, 2020

⌛ Testing commit 4b2d9e6 with merge 3503f56...

@bors
Copy link
Contributor

bors commented Jul 2, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 3503f56 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2020
@bors bors merged commit 3503f56 into rust-lang:master Jul 2, 2020
@eddyb eddyb deleted the no-empty-tables branch July 2, 2020 21:04
@nnethercote
Copy link
Contributor

This was a perf win on landing, mostly on unused-warnings. Nice work!

@cuviper cuviper added this to the 1.46 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants