rustdoc: simplify highlight.rs #99337

jsha · 2022-07-16T16:42:39Z

Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters.

Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code.

I've tested a build of std docs before and after, and this does not change the generated HTML at all.

Followup from #91264 (comment)

r? @GuillaumeGomez

GuillaumeGomez · 2022-07-16T17:01:34Z

src/librustdoc/html/highlight.rs

+}
+
+/// Highlights `src` as an inline example, returning the HTML output.
+pub(crate) fn render_example_with_highlighting(


This new distinction is really nice!

GuillaumeGomez · 2022-07-16T17:04:42Z

src/librustdoc/html/highlight.rs

-    if let Some(class) = class {
-        write!(out, "<pre class=\"rust {}\">", class);
-    } else {
+    if class.is_empty() {


This seems less good. Having to check if a string is empty is easy to forget whereas you cannot make the mistake with Option. Does it make sense?

GuillaumeGomez · 2022-07-16T17:07:08Z

src/librustdoc/html/highlight.rs

@@ -229,7 +259,7 @@ fn get_real_ident_class(text: &str, edition: Edition, allow_path_keywords: bool)
    Some(match text {
        "ref" | "mut" => Class::RefKeyWord,
        "false" | "true" => Class::Bool,
-        _ if Symbol::intern(text).is_reserved(|| edition) => Class::KeyWord,
+        _ if Symbol::intern(text).is_reserved(|| Edition::Edition2021) => Class::KeyWord,


This is quite a big change. Until now the highlight depended on the edition provided to rustdoc. Even though it is a welcomed simplification, I'm not too sure if it's a good idea...

Another problem I have with this approach: we will need to not forget to update it whenever the edition is updated. To go around that limitation, you could add a current method on Edition which would return the current highest edition. To be discussed I suppose.

Also, it seems it's not taking into account the edition parameter that the users can use on codeblocks.

GuillaumeGomez · 2022-07-16T17:10:28Z

src/librustdoc/html/sources.rs

                    file_span,
                    cx,
                    &root_path,
-                    None,
+                    highlight::DecorationInfo::default(),


Even though it could be simplified into Default::default(), I think it's personally much better this way. :)

GuillaumeGomez · 2022-07-16T17:12:04Z

Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code.

I'm a bit worried about this. It's not a game changer but changes the way that some elements are (or not) highlighted. Maybe let's see what the others think?

cc @rust-lang/rustdoc

jsha · 2022-07-16T17:55:57Z

I mentioned the edition highlighting thing here: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/.60edition.60.20plumbed.20through.20highlighting.20code

In tracing through the code, the only thing the edition is used for is to decide what is a keyword for the purpose of highlighting. The only place that matters is in the transition to Rust 2018: dyn, async, await, and try became keywords. I think it is okay if those get highlighted as if they were keywords regardless of the edition.

GuillaumeGomez · 2022-07-16T18:14:30Z

My point is that all these keywords were valid identifiers in the 2015 edition. So even if it's start getting old now, it's a change if we're highlighting them by default all the time now. Although, play.rust-lang.org highlights all of them (except for try). Just want to confirm with everyone before moving forward. I'm personally neutral about this change: the simplification of code is a good plus and I don't think they were very much used as identifiers before in any case...

notriddle · 2022-07-17T04:54:46Z

I’m also neutral about this change. Does anything other than rustdoc track the edition when syntax highlighting? What do rust-analyzer and intellij-rust do?

jsha · 2022-07-27T06:22:19Z

What do rust-analyzer and intellij-rust do?

rust-analyzer highlights dyn as a keyword even when Cargo.toml marks a crate as "edition2015". I don't have intellij-rust installed.

GuillaumeGomez · 2022-07-27T09:00:30Z

Then I think it's fine to accept it. Just the code simplification makes it worth it.

@jsha Please update your PR to fix the CI and then here we go! :)

Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code.

GuillaumeGomez · 2022-08-10T09:07:52Z

Thanks for working on this!

@bors r+

bors · 2022-08-10T09:07:54Z

📌 Commit 5938fd7 has been approved by GuillaumeGomez

It is now in the queue for this repository.

…eGomez rustdoc: simplify highlight.rs Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code. I've tested a build of std docs before and after, and this does not change the generated HTML at all. Followup from rust-lang#91264 (comment) r? `@GuillaumeGomez`

…eGomez rustdoc: simplify highlight.rs Split render_with_highlighting, which took many optional parameters, into three functions for specific purposes, which each take a smaller number of mostly required parameters. Remove some plumbing to pass through an "edition" parameter, which was used solely to avoid highlighting some 2021 Edition keywords in non-2021 code. I've tested a build of std docs before and after, and this does not change the generated HTML at all. Followup from rust-lang#91264 (comment) r? ``@GuillaumeGomez``

…iaskrgr Rollup of 13 pull requests Successful merges: - rust-lang#93896 (rustdoc: make item-infos dimmer on dark theme) - rust-lang#99337 (rustdoc: simplify highlight.rs) - rust-lang#99421 (add crt-static for android) - rust-lang#99500 (Fix flags when using clang as linker for Fuchsia) - rust-lang#99511 (make raw_eq precondition more restrictive) - rust-lang#99992 (Add `x.sh` and `x.ps1` shell scripts) - rust-lang#100112 (Fix test: chunks_mut_are_send_and_sync) - rust-lang#100203 (provide correct size hint for unsupported platform `CommandArgs`) - rust-lang#100307 (Fix rust-lang#96847) - rust-lang#100350 (Stringify non-shorthand visibility correctly) - rust-lang#100374 (Improve crate selection on rustdoc search results page) - rust-lang#100392 (Simplify visitors) - rust-lang#100418 (Add stability attributes to BacktraceStatus variants) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 7 pull requests Successful merges: - rust-lang#92744 (Check if enum from foreign crate has any non exhaustive variants when attempting a cast) - rust-lang#99337 (rustdoc: simplify highlight.rs) - rust-lang#100007 (Never inline Windows dtor access) - rust-lang#100030 (cleanup code w/ pointers in std a little) - rust-lang#100192 ( Remove duplicated temporaries creating during box derefs elaboration) - rust-lang#100247 (Generalize trait object generic param check to aliases.) - rust-lang#100374 (Improve crate selection on rustdoc search results page) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned GuillaumeGomez Jul 16, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2022

GuillaumeGomez reviewed Jul 16, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

jsha force-pushed the simplify-highlight branch from d047b39 to e0068d3 Compare August 10, 2022 00:51

jsha force-pushed the simplify-highlight branch from e0068d3 to 5938fd7 Compare August 10, 2022 03:17

GuillaumeGomez approved these changes Aug 10, 2022

View reviewed changes

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 10, 2022

Dylan-DPC mentioned this pull request Aug 10, 2022

Rollup of 5 pull requests #100362

Closed

Dylan-DPC mentioned this pull request Aug 11, 2022

Rollup of 7 pull requests #100395

Merged

matthiaskrgr mentioned this pull request Aug 11, 2022

Rollup of 13 pull requests #100426

Merged

bors merged commit 9606408 into rust-lang:master Aug 12, 2022

rustbot added this to the 1.65.0 milestone Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: simplify highlight.rs #99337

rustdoc: simplify highlight.rs #99337

jsha commented Jul 16, 2022

GuillaumeGomez Jul 16, 2022

GuillaumeGomez Jul 16, 2022

GuillaumeGomez Jul 16, 2022

GuillaumeGomez Jul 16, 2022

GuillaumeGomez Jul 16, 2022

GuillaumeGomez commented Jul 16, 2022

This comment has been minimized.

jsha commented Jul 16, 2022

GuillaumeGomez commented Jul 16, 2022

notriddle commented Jul 17, 2022 •

edited

Loading

jsha commented Jul 27, 2022

GuillaumeGomez commented Jul 27, 2022

GuillaumeGomez commented Aug 10, 2022

bors commented Aug 10, 2022

rustdoc: simplify highlight.rs #99337

rustdoc: simplify highlight.rs #99337

Conversation

jsha commented Jul 16, 2022

GuillaumeGomez Jul 16, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jul 16, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jul 16, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jul 16, 2022

Choose a reason for hiding this comment

GuillaumeGomez Jul 16, 2022

Choose a reason for hiding this comment

GuillaumeGomez commented Jul 16, 2022

This comment has been minimized.

jsha commented Jul 16, 2022

GuillaumeGomez commented Jul 16, 2022

notriddle commented Jul 17, 2022 • edited Loading

jsha commented Jul 27, 2022

GuillaumeGomez commented Jul 27, 2022

GuillaumeGomez commented Aug 10, 2022

bors commented Aug 10, 2022

notriddle commented Jul 17, 2022 •

edited

Loading