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

Support -Z unpretty=thir-tree again #86251

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Conversation

syvb
Copy link
Contributor

@syvb syvb commented Jun 12, 2021

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
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 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
@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
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

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

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

(This PR fixes #85552 as well)

@oli-obk
Copy link
Contributor

oli-obk commented Jul 27, 2021

@bors r+

@bors
Copy link
Contributor

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
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.
@bors
Copy link
Contributor

bors commented Jul 28, 2021

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

@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)

@bors
Copy link
Contributor

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

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
Contributor

bors commented Jul 28, 2021

⌛ Testing commit 51df26e with merge eba3228...

@bors
Copy link
Contributor

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
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.

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