Use rustc_lexer for rustdoc syntax highlighting#75775

matklad · 2020-08-21T16:06:27Z

r? @ghost

matklad · 2020-08-21T16:07:06Z

This is not ready for review yet, but

r? @GuillaumeGomez

just to give a heads up

src/librustdoc/html/highlight.rs

GuillaumeGomez · 2020-08-21T16:11:19Z

It looks like a nice start after a quick read. Don't forget to put back tests. :p

matklad · 2020-08-21T16:12:48Z

src/librustdoc/html/highlight.rs

This bit is now gone, and, for example, && will always produce two spans now. I wonder if that is important? We need a real parser to do this precisely, but an approximate gluing heuristic won't be hard to add, if we need that.

For now it's fine, but please open an issue about it so it's not forgotten.

Hmm, I wonder if this would fix rust-lang/docs.rs#484.

Good question, that'd be nice!

@matklad could you add a test/screenshot of how this highlights assert!(self.length < N && index <= self.length);?

Added the test. There are two spans for <= and one span for && but they all are ops, so the end result looks fine.

On a meta note, I would be surprised if this doesn't regress some corner cases, but I think it should be fine to fix them as they come up, until we complete #75981.

Looks great, thanks!

On a meta note, I would be surprised if this doesn't regress some corner cases, but I think it should be fine to fix them as they come up, until we complete #75981.

I agree, and I don't expect this to be any more broken than the existing code.

matklad · 2020-08-21T16:15:15Z

@GuillaumeGomez that particular test is deliberately removed, see the commit message in b48b66c.

I do plan to add more unit-tests here, using #75773

src/librustdoc/html/highlight.rs

matklad · 2020-08-27T11:00:50Z

This is ready for review @GuillaumeGomez !

src/librustdoc/Cargo.toml

src/librustdoc/html/highlight/sample.html

src/librustdoc/html/highlight/sample.rs

src/librustdoc/html/highlight/tests.rs

matklad · 2020-08-27T11:37:22Z

Moved the tests, will open the issue about more precise highlighting shortly

GuillaumeGomez · 2020-08-27T11:42:48Z

Looks good to me now! However, considering this is a big PR, I'd like to have another reviewer from @rust-lang/rustdoc to check if I didn't miss anything. :)

jyn514

Can you post a screenshot of what the sample file looks like when highlighted?

src/librustdoc/html/highlight.rs

src/librustdoc/test/fixtures/sample.html

matklad · 2020-08-27T12:57:10Z

@jyn514 thanks for the review, comments addressed (unless I've missed something)!

Filed #75981 for further improvements. FYI, here's the html bit of rust-analyzer's syntax highlighting. Here the bulk of actual highlighting.

Screenshot it #75775 (comment)

jyn514 · 2020-08-27T13:01:13Z

Looks great, thanks!

src/test/rustdoc/bad-codeblock-syntax.rs

ollie27 · 2020-08-27T13:41:44Z

src/librustdoc/test/tests.rs

 }
+
+#[test]
+fn test_html_highlighting() {


It doesn't make sense to put this test here. Syntax highlighting doesn't have anything to do with running doctests which is what this file is a submodule of. I suggest keeping these tests in src/librustdoc/html/highlight/tests.rs.

I disagree with you for the location, however I think those tests should be put into a file on their own (like test/highlight.rs).

rustc_lexer is the lossless lexer, which is a better fit for approximate syntax highlighting. As a side-effect, we can now syntax-highlight even broken code.

src/librustdoc/html/highlight/fixtures/sample.rs

matklad · 2020-08-27T16:37:52Z

Move tests back to where they originally were, and submitted #75989 to not be confused with this next time.

It's a unit-test in a sense that it only checks syntax highlighting. However, the resulting HTML is written to disk and can be easily inspected in the browser. To update the test, run with `--bless` argument or set `UPDATE_EXPEC=1` env var

See rust-lang#75773 and rust-lang#75775

GuillaumeGomez · 2020-08-29T11:58:20Z

Thanks!

@bors: r+

bors · 2020-08-29T11:58:21Z

📌 Commit b4f4db9 has been approved by GuillaumeGomez

bors · 2020-08-29T22:42:14Z

⌛ Testing commit b4f4db9 with merge ced37a5...

bors · 2020-08-30T00:47:30Z

☀️ Test successful - checks-actions, checks-azure
Approved by: GuillaumeGomez
Pushing ced37a5 to master...

rust-highfive assigned GuillaumeGomez Aug 21, 2020

matklad commented Aug 21, 2020

View reviewed changes

src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved

matklad commented Aug 21, 2020

View reviewed changes

matklad force-pushed the rustc-lexer-rustdoc-highlight branch 2 times, most recently from ab1987c to 1d083ce Compare August 21, 2020 17:27

petrochenkov self-assigned this Aug 21, 2020

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2020

ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 22, 2020

tesuji reviewed Aug 22, 2020

View reviewed changes

src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved

petrochenkov removed their assignment Aug 22, 2020

matklad force-pushed the rustc-lexer-rustdoc-highlight branch 3 times, most recently from 3788b43 to 63f6533 Compare August 27, 2020 10:52

matklad changed the title ~~WIP: Use rustc_lexer for rustdoc syntax highlighting~~ Use rustc_lexer for rustdoc syntax highlighting Aug 27, 2020

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from 63f6533 to 957511e Compare August 27, 2020 10:59

GuillaumeGomez reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/Cargo.toml Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/html/highlight/sample.html Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/html/highlight/sample.rs Outdated Show resolved Hide resolved

GuillaumeGomez reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/html/highlight/tests.rs Outdated Show resolved Hide resolved

matklad force-pushed the rustc-lexer-rustdoc-highlight branch 2 times, most recently from 5f18bab to f54359d Compare August 27, 2020 11:34

GuillaumeGomez approved these changes Aug 27, 2020

View reviewed changes

jyn514 reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved

src/librustdoc/html/highlight.rs Outdated Show resolved Hide resolved

src/librustdoc/test/fixtures/sample.html Outdated Show resolved Hide resolved

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from f54359d to 22f709f Compare August 27, 2020 12:28

matklad mentioned this pull request Aug 27, 2020

Next-generation syntax highlighting for rustdoc #75981

Open

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from 22f709f to fe9d84d Compare August 27, 2020 12:49

ollie27 reviewed Aug 27, 2020

View reviewed changes

Remove dependency on StringReader from rustdoc highlighter

1f95a91

rustc_lexer is the lossless lexer, which is a better fit for approximate syntax highlighting. As a side-effect, we can now syntax-highlight even broken code.

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from fe9d84d to 21c4bd9 Compare August 27, 2020 16:01

euclio reviewed Aug 27, 2020

View reviewed changes

src/librustdoc/html/highlight/fixtures/sample.rs Outdated Show resolved Hide resolved

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from 21c4bd9 to 0b16ff3 Compare August 27, 2020 16:35

Add expect test for rustdoc html highlighting

b4f4db9

It's a unit-test in a sense that it only checks syntax highlighting. However, the resulting HTML is written to disk and can be easily inspected in the browser. To update the test, run with `--bless` argument or set `UPDATE_EXPEC=1` env var

matklad force-pushed the rustc-lexer-rustdoc-highlight branch from 0b16ff3 to b4f4db9 Compare August 27, 2020 16:52

matklad added a commit to matklad/rust that referenced this pull request Aug 29, 2020

Allow --bess ing expect-tests in tools

e6a2c82

See rust-lang#75773 and rust-lang#75775

matklad added the WG-parselib label Aug 29, 2020

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 Aug 29, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2020

bors merged commit ced37a5 into rust-lang:master Aug 30, 2020

jyn514 mentioned this pull request Nov 15, 2020

rustdoc: DOS line endings cause extra newlines to show up in source view #76361

Closed

jyn514 mentioned this pull request May 5, 2021

Remove unneeded call to with_default_session_globals in rustdoc highlight #84953

Merged

cuviper added this to the 1.48.0 milestone May 2, 2024

Uh oh!

Conversation

matklad commented Aug 21, 2020

Uh oh!

matklad commented Aug 21, 2020

Uh oh!

Uh oh!

GuillaumeGomez commented Aug 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Aug 21, 2020

Uh oh!

Uh oh!

matklad commented Aug 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matklad commented Aug 27, 2020

Uh oh!

GuillaumeGomez commented Aug 27, 2020

Uh oh!

jyn514 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matklad commented Aug 27, 2020

Uh oh!

jyn514 commented Aug 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matklad commented Aug 27, 2020

Uh oh!

GuillaumeGomez commented Aug 29, 2020

Uh oh!

bors commented Aug 29, 2020

Uh oh!

bors commented Aug 29, 2020

Uh oh!

bors commented Aug 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants