Skip to content

Support -Z unpretty=thir-tree again#86251

Merged
bors merged 2 commits intorust-lang:masterfrom
syvb:thir-tree-again
Jul 28, 2021
Merged

Support -Z unpretty=thir-tree again#86251
bors merged 2 commits intorust-lang:masterfrom
syvb:thir-tree-again

Conversation

@syvb
Copy link
Contributor

@syvb syvb commented Jun 12, 2021 •
edited by LeSeulArtichaut
Loading

Currently -Z unpretty=thir-tree is broken after some THIR refactorings. This re-implements it, making it easier to debug THIR-related issues.

We have to do analyzes before getting the THIR, since trying to create THIR from invalid HIR can ICE. But doing those analyzes requires the THIR to be built and stolen. We work around this by creating a separate query to construct the THIR tree string representation.

Closes rust-lang/project-thir-unsafeck#8, fixes #85552.

@rust-highfive
Copy link
Contributor

rust-highfive commented Jun 12, 2021

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 Jun 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2021
@syvb syvb mentioned this pull request Jul 24, 2021
Closed
@bjorn3
Copy link
Member

bjorn3 commented Jul 24, 2021

Can you add a test? It seems like this implementation would still have the steal issue as the new query isn't forced before the thir is stolen and steals the thir itself.

@syvb syvb force-pushed the thir-tree-again branch from a9ef274 to e8165e7 Compare July 24, 2021 21:18
@syvb
Copy link
Contributor Author

syvb commented Jul 24, 2021

@bjorn3 I've added a test. Stealing isn't an issue since the thir-tree query just directly builds the THIR, without going through the query system for caching. It is less efficient that way, but efficiency isn't much of a concern for debugging options anyways.

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 24, 2021 •
edited
Loading

Building the THIR triggers typeck, which might need to build MIR for e.g. constants, which will steal the THIR for that constant. I think that in more complex cases your code can and will ICE.
Nevermind, I misread. Why do we need a query though?

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 25, 2021

r? @oli-obk

@syvb
Copy link
Contributor Author

syvb commented Jul 25, 2021

@LeSeulArtichaut thir_tree doesn't need to be a query, I just found that it was the most convenient way for rustc_driver to get the tree. Calling rustc_mir_build::thir_tree directly would create an unnecessary direct dependency on rustc_mir_build.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 26, 2021

One alternative would be to move the entire printing logic to rustc_mir_build and only have a dependency on that instead of on rustc_typeck. Either this PR or my alternative are fine by me. lmk what you prefer

@syvb
Copy link
Contributor Author

syvb commented Jul 26, 2021

@oli-obk Calling rustc_mir_build::thir::cx::thir_tree directly would still require the type checking to be done, and so the rustc_typeck dependency would just be moved from rustc_driver to rustc_mir_build. It doesn't really make much of a difference where the type checking is invoked from.

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 27, 2021

(This PR fixes #85552 as well)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 27, 2021

📌 Commit 51df26e has been approved by oli-obk

@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 27, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 27, 2021
@GuillaumeGomez
Rollup merge of rust-lang#86251 - Smittyvb:thir-tree-again, r=oli-obk
e5aca25 
Support -Z unpretty=thir-tree again

Currently `-Z unpretty=thir-tree` is broken after some THIR refactorings. This re-implements it, making it easier to debug THIR-related issues.

We have to do analyzes before getting the THIR, since trying to create THIR from invalid HIR can ICE. But doing those analyzes requires the THIR to be built and stolen. We work around this by creating a separate query to construct the THIR tree string representation.

Closes rust-lang/project-thir-unsafeck#8, fixes rust-lang#85552.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 27, 2021
Closed
@bors
Copy link
Collaborator

bors commented Jul 28, 2021

⌛ Testing commit 51df26e with merge 1e95f145157d48621d012352b3c46226f93548cb...

@rust-log-analyzer
Copy link
Collaborator

rust-log-analyzer commented Jul 28, 2021

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

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

@bors
Copy link
Collaborator

bors commented Jul 28, 2021

💔 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 28, 2021
@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jul 28, 2021

Is it me or are the logs unavailable? @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 Jul 28, 2021
@bors
Copy link
Collaborator

bors commented Jul 28, 2021

⌛ Testing commit 51df26e with merge eba3228...

@bors
Copy link
Collaborator

bors commented Jul 28, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing eba3228 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2021
@bors bors merged commit eba3228 into rust-lang:master Jul 28, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@oli-obk oli-obk

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

Milestone

1.56.0

Development

Successfully merging this pull request may close these issues.

ICE: glacier/fixed/19064.rs -Zunpretty=thir-tree Re-implement -Z unpretty=thir-tree