rustdoc: Migrate sidebar rendering to Askama

[clubby789]

cc #108757

Renders the sidebar for documentation using an Askama template

[rustbot]

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

[jsha]

Thanks so much for working on this! This is gonna be awesome. Some early feedback: This moves too much of the logic into the template itself. I would rather see most of the logic continue to be on the Rust side, filling out fields of a Template struct that can then be straightforwardly interpolated into the template using very little flow control and very few method calls. Essentially, the Template struct should be an abstraction layer between "what we want to express" and "what gets rendered as HTML." I think this will have the happy side benefit of making the code easier to understand than it is today, since the Template struct can express intention more clearly without the HTML getting in the way.

As some additional justification for why it's valuable to keep the logic on the Rust side:

• Tooling like jump-to-definition, hover to see types, rustfmt, etc, is not available when viewing an HTML template
• Error messages for mistakes in Rust-inside-HTML-template are harder to understand
• Indentation is hard to manage when going back and forth between HTML code and Rust code within the same HTML template.

I gave an example below of how I think we can add fields to the Sidebar struct such that the HTML template becomes much clearer. I can go through and provide some additional thoughts as I have time, if that would be helpful.

In the STYLE.md doc I tried to express some of these preferences but it's a little easier to see them concretely in action.

[jsha]

I would love to see this simplified as:

{%- if !title.is_empty() %-}
<h2 class="location">
<a href="#">{{ title_prefix }}{{ title}}</a>
</h2>
{%- endif -%}

This would necessitate giving Sidebar a title: &str and title_prefix: &str field, and moving the it.is_foo() logic, plus the "Crate " / "Module " / "" logic back into the Rust code.

[jsha]

As another example, this could be simplified with the addition of two fields to Sidebar:

is_crate_page: bool,
crate_version: &'str, // only present on crate pages

Then the template would become:

{%- if is_crate_page -%}
<ul class="block"> {#- -#}
<li class="version">Version {{version}}</li> {#- -#}
<li><a id="all-types" href="all.html">All Items</a></li> {#- -#}
</ul> {#- -#}
{%- endif -%}

By the way, note that I use the double-sided whitespace-eliminating comment {#- -#} instead of the single-sided on {# -#}. That allows the comment to stand off from the HTML by a space, which I think is a little more readable.

The single-sided comment {# -#} is needed when an HTML tag with attributes is spread over multiple lines, in order to preserve one space between attributes. Otherwise the attributes would get jammed together.

[clubby789]

Hopefully closer to what you're looking for now. I've made a fair amount of use of macros (converted from regular functions in the original code), but it might be simpler/more readable to just inline the loops here.

[clubby789]

@rustbot ready
Given tests pass and the majority of the sidebar has been moved over, taking this off WIP

[jsha]

Looking great! Thanks for the revisions.

As you pointed out, it would be nicer to genericize the link blocks and get rid of the macros. I'm thinking something like:

struct<'a> LinkBlock<'a> {
heading: &'a str,
heading_href: &'a str,
links: Vec<Link<'a>>, // Or Iterator?
}
struct<'a> Link<'a> {
href: &'a str,
name: &'a str,
}

And then Sidebar would have a Vec<LinkBlock>.

I believe this would allow us to collapse the multiple different .html files for the different flavors of sidebar, in favor of a single sidebar.html that iterates across multiple LinkBlocks.

[clubby789]

I managed to move it all over to a single sidebar.html with LinkBlock and Link structs. Having only one layer of templates means I could remove all the |safe uses in the sidebar, which I was a bit worried about.
I also moved just about all of the sidebar-relevant code into a new module, as I was finding mod.rs a bit difficult to navigate with the current size.

[jsha]

Excellent, I love the latest refactoring. It adds a lot of clarity to both the template and the code.

Looks like you've got a couple of test cases failing:

command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/crate-version-escape" "/checkout/tests/rustdoc/crate-version-escape.rs"
stdout: none
--- stderr -------------------------------
5: @has check failed
`XPATH PATTERN` did not match
// @has 'foo/index.html' '//li[@class="version"]' 'Version <script>alert("hi")</script>'
Encountered 1 errors
------------------------------------------
---- [rustdoc] tests/rustdoc/crate-version.rs stdout ----
error: htmldocck failed!
status: exit status: 1
command: "/usr/bin/python3" "/checkout/src/etc/htmldocck.py" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc/crate-version" "/checkout/tests/rustdoc/crate-version.rs"
stdout: none
--- stderr -------------------------------
3: @has check failed
`XPATH PATTERN` did not match
// @has 'crate_version/index.html' '//*[@class="version"]' 'Version 1.3.37'
Encountered 1 errors
------------------------------------------
failures:
[rustdoc] tests/rustdoc/crate-version-escape.rs
[rustdoc] tests/rustdoc/crate-version.rs

Also one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them; if there are no diffs, the refactoring had no unintended side effects, at least over the std docs.

[clubby789]

Looks like you've got a couple of test cases failing:

Interesting, they pass locally. I can see the problem by eye, but odd that it wasn't picked up on my tests.

Also one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them

I've been attempting to do this, but having difficulty since the outputted HTML is all on one line and always seems to have slightly different URLs for resources. Are there any good tools for diffing in this situation?

Also looks like there have been some changes to doc layout since the current bootstrap compiler which are getting in the way of diffing 🙁

[clubby789]

Ah, upstream changed whitespace = "suppress" so the space after Version was squashed. Rebased and fixed

[jsha]

one thing I like to do when performing a refactoring like this is generate before-and-after output for ./x.py doc library/std and diff them
I've been attempting to do this, but having difficulty since the outputted HTML is all on one line and always seems to have slightly different URLs for resources. Are there any good tools for diffing in this situation?

Yeah, the all-one-line thing makes it hard. Usually I'll use plain diff to find files that differ, and then use vimdiff on a specific file to find differences, since it's good at showing intra-line diffs.

Sometimes I'll do sed -i s/>/\n/g to get things roughly on separate lines and then diff the result (of course this is not a valid HTML transformation but it's enough to see patterns). I've also tried using tidy on each side of the diff, but tidy is pretty aggressive in its transforms and sometimes changes so much that I can't tell what's going on.

When you say "always seems to have slightly different URLs for resources" - are you talking about the static files in rustdoc.static/? Those should be steady so long as the file contents aren't changing. If you make sure your branch is up to date with master, and clear out the generated files between runs, those URLs shouldn't change.

[jsha]

The latest test failures seem related to escaping of link fragments in href:

core/primitive.reference.html:1: broken link fragment `#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T` pointing to `core/primitive.reference.html`

Taking just the fragment, we have (old/new):

#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T
#impl-CoerceUnsized%3C%26'a+U%3E-for-%26'b+T

It looks like the ' is getting encoded using an HTML entity ('). That's not really necessary for us because we use double quotes (") to surround our attributes, so single quotes (') are not relevant. But Askama doesn't know that and is just trying to escape all possibly-relevant characters. I think using |safe for the LinkBlocks' href should fix it.

Note that URL escaping was recently touched in #107284 and #107655.

[jsha]

@bors r+ rollup

[bors]

📌 Commit 85ae75aba179e44de205556d7a0acee1e0bd1ef0 has been approved by jsha

It is now in the queue for this repository.

[GuillaumeGomez]

@bors r-

Just needs to remove unneeded -.

[clubby789]

Fixed, and also changed the indentation to be more in line with the style guide

[GuillaumeGomez]

It looks better indeed. You also removed some unneeded {# #}, well done. Thanks!

@bors r=jsha,GuillaumeGomez rollup

[bors]

📌 Commit 2f166d1 has been approved by jsha,GuillaumeGomez

It is now in the queue for this repository.