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 union keyword highlighting in rustdoc HTML sources #87428

Merged
merged 2 commits into from
Sep 30, 2021
Merged

Fix union keyword highlighting in rustdoc HTML sources #87428

merged 2 commits into from
Sep 30, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 24, 2021 •
edited
Loading

I followed this logic: if I find an ident "union", I check if it followed by another ident or not. If it's the case, then I consider this is a keyword because it's declaring a union type.

To do so I created a new Iterator which allows to peek the next items without moving the current iterator position.

This is part of #85016. If the fix makes sense, I'll extend it to other weak keywords (the issue only mentions they exist but https://doc.rust-lang.org/nightly/reference/keywords.html#weak-keywords only talks about dyn and 'static so not sure if there is anything else to be done?).

cc @notriddle (you're one of the last ones who worked on this part of rustdoc so here you go 😉 )
r? @jyn514

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Jul 24, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 24, 2021

Why is rustdoc rewriting its own rust parsing? Can't we use rustc_parser somehow?

@jyn514 jyn514 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 Jul 24, 2021
@GuillaumeGomez
Copy link
Member Author

To be confirmed but iirc, last time we checked it was doing too much: unnecessary tokens like whitespaces for example were removed. However, we need them, which is why we have to rely on the rustc_lexer instead.

@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 Jul 26, 2021
@GuillaumeGomez
Copy link
Member Author

cc @camelid

@bors

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

estebank commented Aug 6, 2021

To be confirmed but iirc, last time we checked it was doing too much: unnecessary tokens like whitespaces for example were removed. However, we need them, which is why we have to rely on the rustc_lexer instead.

Could we modify the parser to not discard the whitespaces somehow as a new mode of operation?

@GuillaumeGomez
Copy link
Member Author

Again: to be checked. But that'll make the parsing slower overall if the compiler has to discard them every time. We could always add a parameter to allow to keep them around. Not sure if that's something the people in charge of the parser would want?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

estebank commented Aug 6, 2021

The two people most involved with the parser are @petrochenkov and myself. Personally, if there's no or low perf impact I'd prefer avoiding duplicated logic, and I'm expecting @petrochenkov's concern to be of maintainability. If it can be done cleanly, I would prefer going the single parser way.

@GuillaumeGomez
Copy link
Member Author

Alright, I'll try to do that as soon as I can!

@notriddle
Copy link
Contributor

Rust-Analyzer has a parser that preserves spaces, and already uses it for syntax highlighting.

But can we use it in rust-lang/rust right now?

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 6, 2021 •
edited
Loading

I'd prefer to use the existing compiler tools rather than add external ones.

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

jyn514 commented Aug 16, 2021

r? @estebank

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2021
@GuillaumeGomez GuillaumeGomez 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 Sep 29, 2021
@GuillaumeGomez
Copy link
Member Author

Did I miss anything?

@GuillaumeGomez
Copy link
Member Author

@bors: r=notriddle

@bors
Copy link
Contributor

bors commented Sep 29, 2021

📌 Commit ee38116 has been approved by notriddle

@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 Sep 29, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 29, 2021
@GuillaumeGomez
Rollup merge of rust-lang#87428 - GuillaumeGomez:union-highlighting, …
8dfbfe8 
…r=notriddle

Fix union keyword highlighting in rustdoc HTML sources

I followed this logic: if I find an ident "union", I check if it followed by another ident or not. If it's the case, then I consider this is a keyword because it's declaring a union type.

To do so I created a new Iterator which allows to peek the next items without moving the current iterator position.

This is part of rust-lang#85016. If the fix makes sense, I'll extend it to other weak keywords (the issue only mentions they exist but https://doc.rust-lang.org/nightly/reference/keywords.html#weak-keywords only talks about `dyn` and `'static` so not sure if there is anything else to be done?).

cc `@notriddle` (you're one of the last ones who worked on this part of rustdoc so here you go 😉 )
r? `@jyn514`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 29, 2021
Closed
@bors
Copy link
Contributor

bors commented Sep 29, 2021

⌛ Testing commit ee38116 with merge 874499884d0418da80234d5898c1fead0fa964fe...

@bors
Copy link
Contributor

bors commented Sep 29, 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 Sep 29, 2021
@ehuss
Copy link
Contributor

ehuss commented Sep 29, 2021

@bors retry
Issue with Windows LLDB

@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 Sep 29, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ehuss ehuss mentioned this pull request Sep 30, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2021
@bors
Auto merge of rust-lang#89386 - ehuss:rollup-idf4dmj, r=ehuss
30acf6d 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#87428 (Fix union keyword highlighting in rustdoc HTML sources)
 - rust-lang#88412 (Remove ignore-tidy-undocumented-unsafe from core::slice::sort)
 - rust-lang#89098 (Fix generics where bounds order)
 - rust-lang#89232 (Improve help for recursion limit errors)
 - rust-lang#89294 (:arrow_up: rust-analyzer)
 - rust-lang#89297 (Remove Never variant from clean::Type enum)
 - rust-lang#89311 (Add unit assignment to MIR for `asm!()`)
 - rust-lang#89313 (PassWrapper: handle function rename from upstream D36850)
 - rust-lang#89315 (Clarify that `CString::from_vec_unchecked` appends 0 byte.)
 - rust-lang#89335 (Optimize is_sorted for Range and RangeInclusive)
 - rust-lang#89366 (rustdoc: Remove lazy_static dependency)
 - rust-lang#89377 (Update cargo)
 - rust-lang#89378 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 42ea15b into rust-lang:master Sep 30, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 30, 2021
@GuillaumeGomez GuillaumeGomez deleted the union-highlighting branch September 30, 2021 08:03
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@estebank estebank estebank left review comments

@notriddle notriddle notriddle approved these changes

Assignees

@estebank estebank

Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.57.0
Development

Successfully merging this pull request may close these issues.
10 participants
@GuillaumeGomez @jyn514 @bors @estebank @rust-log-analyzer @notriddle @ehuss @JohnCSimon @rust-highfive @rustbot