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

Render more readable macro matcher tokens in rustdoc #92908

Merged
merged 8 commits into from
Jan 30, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Jan 14, 2022

Follow-up to #92334.

This PR lifts some of the token rendering logic from https://github.com/dtolnay/prettyplease into rustdoc so that even the matchers for which a source code snippet is not available (because they are macro-generated, or any other reason) follow some baseline good assumptions about where the tokens in the macro matcher are appropriate to space.

The below screenshots show an example of the difference using one of the gnarliest macros I could find. Some things to notice:

  • In the before, notice how a couple places break in between $(....)*, which is just about the worst possible place that it could break.

  • In the before, the lines that wrapped are weirdly indented by 1 space of indentation relative to column 0. In the after, we use the typical way of block indenting in Rust syntax which is put the open/close delimiters on their own line and indent their contents by 4 spaces relative to the previous line (so 8 spaces relative to column 0, because the matcher itself is indented by 4 relative to the macro_rules header).

  • In the after, macro_rules metavariables like $tokens:tt are kept together, which is how just about everybody writing Rust today writes them.

Before

Screenshot from 2022-01-14 13-05-53

After

Screenshot from 2022-01-14 13-06-04

r? @camelid

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 14, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2022
src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
Comment on lines 683 to 687
"as" | "box" | "break" | "const" | "continue" | "crate" | "else" | "enum" | "extern"
| "for" | "if" | "impl" | "in" | "let" | "loop" | "macro" | "match" | "mod" | "move"
| "mut" | "ref" | "return" | "static" | "struct" | "trait" | "type" | "unsafe" | "use"
| "where" | "while" | "yield" => true,
_ => false,
Copy link
Member

Choose a reason for hiding this comment

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

This seems really brittle. Is there some way we can call a function in rustc to get this information? Symbol::is_used_keyword_conditional looks promising.

Copy link
Member Author

Choose a reason for hiding this comment

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

rustc_span has functions for evaluating whether a Symbol is a reserved word, or is a keyword on a particular edition.

This logic isn't that though. It is quite specific to rendering spacing in stringified token streams. I would not expect to find a function for that in rustc, because here in rustdoc is the first place that cares about nicely stringifying token streams in a way that human users of the language are expected to regularly encounter.

I pushed a commit that inverts the list though, since among keywords there are fewer that do not need a space than need a space. Let me know you like that better.

@camelid camelid 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 Jan 14, 2022
@camelid
Copy link
Member

camelid commented Jan 15, 2022

I'll take another look at this soon. Thanks for the very clear and helpful comments. ❤️

cc @GuillaumeGomez in case you want to review as well

@camelid camelid 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 Jan 15, 2022
src/librustdoc/clean/render_macro_matchers.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/render_macro_matchers.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/render_macro_matchers.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

Could you add another "more complete" HTML test please?

@dtolnay
Copy link
Member Author

dtolnay commented Jan 17, 2022

Let me do that after #92914 has landed and I've rebased.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 18, 2022
…omez

htmldocck: Add support for `/text()` in `@snapshot`

This allows just testing the text, in cases where the HTML tags don't
matter.

See rust-lang#92908 (comment) for an example of when this would be useful.

r? `@GuillaumeGomez`
@dtolnay
Copy link
Member Author

dtolnay commented Jan 18, 2022

Rebased on #92914 and added a more exhaustive test in 039a058.

@GuillaumeGomez
Copy link
Member

Looks good to me, thanks! Let's wait for @camelid approval as well.

@camelid
Copy link
Member

camelid commented Jan 25, 2022

I feel a little uncomfortable about having so much custom code for a fairly niche use case (macro-generated macros), but I won't block it. I guess we could always delete this code if we had to since it's not part of our stability guarantee.

@camelid camelid removed their assignment Jan 25, 2022
@dtolnay
Copy link
Member Author

dtolnay commented Jan 25, 2022

I don't agree that this is niche -- doesn't this affect all standard library macros according to #86208 / #92334 (review)? That seems front and center for a large fraction of the macros that users of Rust are going to be looking at.


Screenshot from 2022-01-25 12-12-09

@camelid
Copy link
Member

camelid commented Jan 25, 2022

I thought your previous PR (#92334) already fixed most cases?

@dtolnay
Copy link
Member Author

dtolnay commented Jan 25, 2022

Your review comment on that PR says "it won't fix the rendering of macros in the standard library since the lack of sources there triggered the switch to pretty-printing in the first place". The screenshot above is from https://doc.rust-lang.org/nightly/std/macro.write.html so it confirms that the source-based rendering does not kick in for the standard macros. Maybe that can be solved a different way but for now #92334 by itself does not fix it.

@camelid
Copy link
Member

camelid commented Jan 25, 2022

Oops, it looks like I should've listened to past me ;)

I think this change is reasonable then. Sorry for the confusion!

@dtolnay dtolnay removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 27, 2022
@GuillaumeGomez
Copy link
Member

Thanks for working on this!

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 30, 2022

📌 Commit 039a058 has been approved by GuillaumeGomez

@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 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#92887 (Bootstrap compiler update)
 - rust-lang#92908 (Render more readable macro matcher tokens in rustdoc)
 - rust-lang#93183 (rustdoc: mobile nav fixes)
 - rust-lang#93192 (Add VS 2022 into error message)
 - rust-lang#93475 (Add test to ensure that theme is applied correctly when going back in history)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ba01337 into rust-lang:master Jan 30, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 30, 2022
@dtolnay dtolnay deleted the rustdoc branch January 31, 2022 08:04
@dtolnay dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) 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
Development

Successfully merging this pull request may close these issues.

6 participants