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

Add support for range params to rustdoc #80220

Closed
wants to merge 2 commits into from

Conversation

CraftSpider
Copy link
Contributor

Fixes #79497

There's future work to use rustc_hir_pretty for more of what the function does, but this fixes the ICE.

@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 Dec 20, 2020
@CraftSpider
Copy link
Contributor Author

Forgot the review.
r? @jyn514

@rust-log-analyzer

This comment has been minimized.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 20, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 20, 2020

Also assigning @petrochenkov for the change to rustc_hir_pretty (but the change LGTM).

@jyn514 jyn514 assigned petrochenkov and jyn514 and unassigned jyn514 Dec 20, 2020
@jyn514 jyn514 added the A-hir Area: The high-level intermediate representation (HIR) label Dec 20, 2020
"tried to get argument name from PatKind::Range, \
which is not allowed in function arguments"
),
PatKind::Range(..) => rustc_hir_pretty::pat_to_string(p),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to degrade to _ here than pull HIR pretty-printing, especially given that this case is improbable in practice and only full ranges can be accepted in this position without errors.
This is also true for the PatKind::Lit case above (also warn!ing in that case is not rustdoc's job).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Degrade as in print out _ instead of the actual range, I'm assuming you mean?
I used the pretty-print primarily due to @jyn514's advice that the function should actually, in future work, shift to using it for more of the cases, and rely less on re-implementing logic for it. And I don't think there's any harm to printing the true value, as this case is rare and thus it's not like it should hurt average performance. So if anything, I think the Lit case should also fall back to pretty-print as well.

Copy link
Contributor

@petrochenkov petrochenkov Dec 28, 2020

Choose a reason for hiding this comment

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

The general plan for HIR pretty-printing is to either remove or privatize it, some work in this direction was already done in #70344 so it's rarely used now.

rustdoc's goal on the other hand is to print human-readable function parameter names, not to print HIR as precisely as possible.
Literals or ranges don't contain parameter names, so they don't need to be printed.

I'll leave this to @jyn514 to decide.

Copy link
Member

Choose a reason for hiding this comment

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

The main goal here is to reduce rustc_hir's dependencies and its size such that it can start and finish earlier, thereby working towards #65031.

Rustdoc is built after all rustc crates are built, so it doesn't affect this goal one way or another.

The general plan for HIR pretty-printing is to either remove or privatize it

Can you expand on this? I haven't seen plans to remove it altogether - will it be replaced with something? In this particular case a wildcard seems fine, but I'd like to eventually get rid of this whole function and use param_to_string instead. Would it be better to use that here directly instead of adding more functions to rustc_hir_pretty?

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2020
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 28, 2020
@petrochenkov
Copy link
Contributor

r? @jyn514

@jyn514
Copy link
Member

jyn514 commented Jan 7, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jan 7, 2021

📌 Commit 04ccdf1 has been approved by jyn514

@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 Jan 7, 2021
@jyn514
Copy link
Member

jyn514 commented Jan 7, 2021

Actually I think #80799 makes more sense so rustc_hir_pretty doesn't have to be expanded.

@bors r-

@jyn514 jyn514 closed this Jan 7, 2021
@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 Jan 7, 2021
@CraftSpider CraftSpider deleted the 79497 branch January 14, 2021 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-hir Area: The high-level intermediate representation (HIR) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
7 participants