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

Fix JSON reexport ICE #97599

Merged
merged 2 commits into from
Jun 2, 2022
Merged

Fix JSON reexport ICE #97599

merged 2 commits into from
Jun 2, 2022

Conversation

GuillaumeGomez
Copy link
Member

Fixes #97432.

The problem was that the ID was conflicting because the reexports have the same one. To fix it, I "extended" it by adding the Symbol into it as well.

r? @notriddle

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-json Area: Rustdoc JSON backend labels May 31, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2022
@Urgau
Copy link
Member

Urgau commented May 31, 2022 •
edited
Loading

This is more like a bandage than a real solution.

You should look at #93518 which is a completely different approach than this PR is taking, basically the idea is for the JSON output to not inline any item like the html output is doing, meaning that re-exports will not ICE anymore because they will still be different and thus not have the same id.

@notriddle
Copy link
Contributor

Does that PR fix this bug? If so, then I'd like to merge that one instead.

@CraftSpider
Copy link
Contributor

That PR needs updated and is not yet perfect, it still has some known failures. On the other hand as far as I can tell it does fix re-export issues generally. I've been slacking on doing the work to carry it over the finish-line, I'd be willing to help mentor someone else if they are willing to do some of the work to push it over the finish-line, alternatively I'll try to set aside some time this weekend to finish it up.

@GuillaumeGomez
Copy link
Member Author

I also took another approach at the start where I stopped inlining reexports but I found the result sub-optimal. However it'd be a better approach overall than my current PR.

What is missing in #93518?

@Urgau
Copy link
Member

Urgau commented Jun 1, 2022

What is missing in #93518?

It's currently blocked on fixing #93518 (comment) and applying this suggestion #93518 (comment). And maybe adding some more tests cases.

@GuillaumeGomez
Copy link
Member Author

I see. Considering none of these PRs are anywhere close to being merged, should we move forward with this one then? I can add a FIXME: remove this function once no more inlining for JSON format above the from_item_id_with_name function to make sure it won't remain.

My problem here is that we have an ICE, which should be avoided at all cost.

@CraftSpider
Copy link
Contributor

I'd support moving forward with this as a future FIXME

@GuillaumeGomez
Copy link
Member Author

Ok, updating the PR then. :)

@GuillaumeGomez
Copy link
Member Author

And done!

@GuillaumeGomez
Copy link
Member Author

@bors: r=CraftSpider rollup

@bors
Copy link
Contributor

bors commented Jun 1, 2022

📌 Commit 069ae16 has been approved by CraftSpider

@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 1, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 2, 2022
@Dylan-DPC
Rollup merge of rust-lang#97599 - GuillaumeGomez:reexport-json, r=Cra…
15c960b 
…ftSpider

Fix JSON reexport ICE

Fixes rust-lang#97432.

The problem was that the ID was conflicting because the reexports have the same one. To fix it, I "extended" it by adding the `Symbol` into it as well.

r? `@notriddle`
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 2, 2022
Closed
@Dylan-DPC
Copy link
Member

failed in rollup

@Dylan-DPC
Copy link
Member

@bors r-

@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 Jun 2, 2022
@GuillaumeGomez
Copy link
Member Author

Fixed the doc comment.

@bors: r=CraftSpider rollup

@bors
Copy link
Contributor

bors commented Jun 2, 2022

📌 Commit 5adca73 has been approved by CraftSpider

@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 Jun 2, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 2, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2022
@bors
Auto merge of rust-lang#97654 - Dylan-DPC:rollup-w6zrzxf, r=Dylan-DPC
44e9516 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97420 (Be a little nicer with casts when formatting `fn` pointers)
 - rust-lang#97450 ([RFC 2011] Basic compiler infrastructure)
 - rust-lang#97599 (Fix JSON reexport ICE)
 - rust-lang#97617 (Rustdoc anonymous reexports)
 - rust-lang#97636 (Revert rust-lang#96682.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f22f743 into rust-lang:master Jun 2, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 2, 2022
@GuillaumeGomez GuillaumeGomez deleted the reexport-json branch June 2, 2022 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CraftSpider CraftSpider CraftSpider approved these changes

Assignees

@notriddle notriddle

Labels
A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

ICE when running rustdoc in JSON mode on clap
8 participants
@GuillaumeGomez @Urgau @notriddle @CraftSpider @bors @Dylan-DPC @rust-highfive @rustbot