Rustdoc: formatting to buffers #64044

Mark-Simulacrum · 2019-08-31T20:25:53Z

This should be reviewed commit-by-commit.

I've not attempted to fully flesh out what the end state of this PR could look like yet as I wanted to get it up for some early feedback (I already think this has some wins, too, so we could land it as-is).

The primary idea with Buffer is that it internally tracks whether we're printing to HTML or text, and the goal is that eventually instead of branch on fmt.alternate() anywhere, we'd call a helper like buf.nbsp() which would either return   or depending on the target we're printing to. Obviously, that's not included in this PR, in part because it was already getting quite big.

rust-highfive · 2019-08-31T21:39:09Z

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-08-31T20:26:31.6196785Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-31T20:26:31.6400507Z ##[command]git config gc.auto 0
2019-08-31T20:26:31.6475114Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-31T20:26:31.6543803Z ##[command]git config --get-all http.proxy
2019-08-31T20:26:31.6676754Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64044/merge:refs/remotes/pull/64044/merge
---
2019-08-31T20:36:12.2155265Z     Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
2019-08-31T20:36:12.7431996Z error: hidden lifetime parameters in types are deprecated
2019-08-31T20:36:12.7433105Z   --> src/librustdoc/html/format.rs:94:38
2019-08-31T20:36:12.7433577Z    |
2019-08-31T20:36:12.7434100Z 94 |     crate fn write_fmt(&mut self, v: fmt::Arguments) {
2019-08-31T20:36:12.7435030Z    |                                      ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
2019-08-31T20:36:12.7440361Z error: hidden lifetime parameters in types are deprecated
2019-08-31T20:36:12.7440919Z    --> src/librustdoc/html/format.rs:104:44
2019-08-31T20:36:12.7441347Z     |
2019-08-31T20:36:12.7441347Z     |
2019-08-31T20:36:12.7442073Z 104 |     crate fn with_formatter<T: FnOnce(&mut fmt::Formatter) -> fmt::Result>(&mut self, t: T) {
2019-08-31T20:36:12.7442737Z     |                                            ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
2019-08-31T20:36:12.7443133Z 
2019-08-31T20:36:12.9694425Z error[E0201]: duplicate definitions with name `insert_str`:
2019-08-31T20:36:12.9695555Z   --> src/librustdoc/html/format.rs:82:5
2019-08-31T20:36:12.9695804Z    |
2019-08-31T20:36:12.9696168Z 74 | /     crate fn insert_str(&mut self, idx: usize, s: &str) {
2019-08-31T20:36:12.9696538Z 75 | |         self.buffer.insert_str(idx, s);
2019-08-31T20:36:12.9696863Z 76 | |     }
2019-08-31T20:36:12.9697212Z    | |_____- previous definition of `insert_str` here
2019-08-31T20:36:12.9697432Z ...
2019-08-31T20:36:12.9697786Z 82 | /     crate fn insert_str(&mut self, idx: usize, s: &str) {
2019-08-31T20:36:12.9698117Z 83 | |         self.buffer.insert_str(idx, s);
2019-08-31T20:36:12.9698966Z    | |_____^ duplicate definition
2019-08-31T20:36:12.9699214Z 
2019-08-31T20:36:12.9699214Z 
2019-08-31T20:36:12.9710488Z error[E0201]: duplicate definitions with name `push_str`:
2019-08-31T20:36:12.9710745Z   --> src/librustdoc/html/format.rs:86:5
2019-08-31T20:36:12.9710951Z    |
2019-08-31T20:36:12.9711271Z 78 | /     crate fn push_str(&mut self, s: &str) {
2019-08-31T20:36:12.9711525Z 79 | |         self.buffer.push_str(s);
2019-08-31T20:36:12.9711781Z 80 | |     }
2019-08-31T20:36:12.9712209Z    | |_____- previous definition of `push_str` here
2019-08-31T20:36:12.9712405Z ...
2019-08-31T20:36:12.9712874Z 86 | /     crate fn push_str(&mut self, s: &str) {
2019-08-31T20:36:12.9713139Z 87 | |         self.buffer.push_str(s);
2019-08-31T20:36:12.9713645Z    | |_____^ duplicate definition
2019-08-31T20:36:12.9713679Z 
2019-08-31T20:36:12.9713886Z error: aborting due to 4 previous errors
2019-08-31T20:36:12.9713932Z 
---
2019-08-31T20:36:13.0042534Z == clock drift check ==
2019-08-31T20:36:13.0058871Z   local time: Sat Aug 31 20:36:13 UTC 2019
2019-08-31T20:36:13.0902224Z   network time: Sat, 31 Aug 2019 20:36:13 GMT
2019-08-31T20:36:13.0907243Z == end clock drift check ==
2019-08-31T20:36:15.2224675Z ##[error]Bash exited with code '1'.
2019-08-31T20:36:15.2262507Z ##[section]Starting: Checkout
2019-08-31T20:36:15.2264169Z ==============================================================================
2019-08-31T20:36:15.2264237Z Task         : Get sources
2019-08-31T20:36:15.2264278Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

matklad

Left a bunch of comments. Super +1 on removal of ? everywhere!

matklad · 2019-08-31T22:39:49Z

src/librustdoc/html/format.rs

+ self.buffer.push_str(s);
+ }
+
+ crate fn write_fmt(&mut self, v: fmt::Arguments) {


This probably wants a comment explaining that this works with write! macro, but doesn’t require ?

matklad · 2019-08-31T22:40:16Z

src/librustdoc/html/format.rs

+#[derive(Debug, Clone)]
+pub struct Buffer {
+ crate for_html: bool,
+ crate buffer: String,


Make private?

src/librustdoc/html/format.rs

matklad · 2019-08-31T22:46:08Z

src/librustdoc/html/layout.rs

@@ -26,15 +26,15 @@ pub struct Page<'a> {
 }

 pub fn render<T: fmt::Display, S: fmt::Display>(
- dst: &mut dyn io::Write,
+ dst: &mut Buffer,


Would be cool to rename all fmt, v, dst, buffer etc just to buf.

I'm trying to keep the diff as minimal as possible which means avoiding renaming for the sake of renaming.

matklad · 2019-08-31T22:48:54Z

src/librustdoc/html/layout.rs

@@ -11,6 +11,12 @@ pub struct Layout {
 pub favicon: String,
 pub external_html: ExternalHtml,
 pub krate: String,
+ /// The given user css file which allow to customize the generated
+ /// documentation theme.
+ pub css_file_extension: Option<PathBuf>,


Unrelated, but maybe extension_css_file?

css_file_extension seems good to me. Why would you prefer extension_css_file?

to not confuse with .css file extension.

matklad · 2019-08-31T22:51:37Z

src/librustdoc/html/render.rs

 for implementor in &all_implementors {
- try_err!(writeln!(&mut v, "{}", *implementor), &mydst);
+ v.push_str(&format!("{}", *implementor));


to_string instead of format?

cc https://internals.rust-lang.org/t/add-an-easier-way-to-append-t-display-to-string-to-stdlib/10607

matklad · 2019-08-31T22:53:00Z

src/librustdoc/html/render.rs

@@ -1247,19 +1241,18 @@ themePicker.onblur = handleThemeButtonsBlur;
 // identically even with rustdoc running in parallel.
 all_implementors.sort();

- let mut v = Vec::new();
- try_err!(writeln!(&mut v, "(function() {{var implementors = {{}};"), &mydst);
+ let mut v = String::from("(function() {{var implementors = {{}};\n");


It’s a bug I think? No need for double braces

Indeed, it was using writeln! before but doesn't make sense anymore to keep double braces.

matklad · 2019-08-31T22:58:50Z

src/librustdoc/html/render.rs

- fn print(self, buffer: &mut Buffer) {
- let cx = self.cx;
- let it = self.item;
+ fn print_sidebar(cx: &Context, it: &clean::Item, buffer: &mut Buffer) {


Let’s make buf the first argument consistently?

I could, but I think it's not too likely we want to do that in this PR since there are existing functions where the fmt::Formatter was in the middle of the function or so and moving that around is, while not generally error-prone due to types, quite noisy. I'd like to avoid doing it in this PR :)

matklad · 2019-08-31T23:01:02Z

src/librustdoc/html/format.rs

+ self.buffer.insert_str(idx, s);
+ }
+
+ crate fn push_str(&mut self, s: &str) {


There’s write_str already

This is intentional to keep the API both compatible with String and fmt::Formatter as we want to avoid noise in this PR at least.

Mark-Simulacrum · 2019-08-31T23:22:00Z

Pushed up fixup commits to address comments, will squash them in when we get to landing point (want to avoid rebasing so as to not lose review comments and/or disrupt in-progress reviews).

rust-highfive · 2019-09-01T00:42:42Z

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-08-31T23:21:33.4743852Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-31T23:21:33.4948105Z ##[command]git config gc.auto 0
2019-08-31T23:21:33.5047768Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-31T23:21:33.5100474Z ##[command]git config --get-all http.proxy
2019-08-31T23:21:33.5280624Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64044/merge:refs/remotes/pull/64044/merge
---
2019-08-31T23:31:27.0684399Z     Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
2019-08-31T23:31:27.5919299Z error: hidden lifetime parameters in types are deprecated
2019-08-31T23:31:27.5923999Z   --> src/librustdoc/html/format.rs:92:38
2019-08-31T23:31:27.5924464Z    |
2019-08-31T23:31:27.5925226Z 92 |     crate fn write_fmt(&mut self, v: fmt::Arguments) {
2019-08-31T23:31:27.5925812Z    |                                      ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
2019-08-31T23:31:27.5926377Z error: hidden lifetime parameters in types are deprecated
2019-08-31T23:31:27.5926738Z    --> src/librustdoc/html/format.rs:102:44
2019-08-31T23:31:27.5927442Z     |
2019-08-31T23:31:27.5927442Z     |
2019-08-31T23:31:27.5927869Z 102 |     crate fn with_formatter<T: FnOnce(&mut fmt::Formatter) -> fmt::Result>(&mut self, t: T) {
2019-08-31T23:31:27.5928728Z     |                                            ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
2019-08-31T23:31:30.2011722Z error: aborting due to 2 previous errors
2019-08-31T23:31:30.2012501Z 
2019-08-31T23:31:30.2391728Z error: Could not compile `rustdoc`.
2019-08-31T23:31:30.2392396Z 
---
2019-08-31T23:31:30.2496624Z == clock drift check ==
2019-08-31T23:31:30.2513033Z   local time: Sat Aug 31 23:31:30 UTC 2019
2019-08-31T23:31:30.4004304Z   network time: Sat, 31 Aug 2019 23:31:30 GMT
2019-08-31T23:31:30.4005195Z == end clock drift check ==
2019-08-31T23:31:32.4764949Z ##[error]Bash exited with code '1'.
2019-08-31T23:31:32.4802094Z ##[section]Starting: Checkout
2019-08-31T23:31:32.4804044Z ==============================================================================
2019-08-31T23:31:32.4804111Z Task         : Get sources
2019-08-31T23:31:32.4804153Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

GuillaumeGomez · 2019-09-02T12:06:05Z

Awesome! Doing most file writing into one place is a very good idea to improve performances. However, I'm very worried about memory consumption. Do you have a comparison before/after by any chance?

Mark-Simulacrum · 2019-09-02T12:11:01Z

We were always writing into Vec previously anyway, so this shouldn't change memory consumption at all.

Mark-Simulacrum · 2019-09-02T14:34:24Z

Rebased to squash in commits and hopefully fixed CI

GuillaumeGomez · 2019-09-02T21:05:51Z

Then all good for me (weird that I remembered that used more IO than vec storage but whatever...). Once @matklad is okay with, let r+ it!

Thanks a lot for doing it!

matklad · 2019-09-02T22:01:31Z

@bors r=GuillaumeGomez

bors · 2019-09-02T22:01:33Z

📌 Commit a9c6ef6 has been approved by GuillaumeGomez

…GuillaumeGomez Rustdoc: formatting to buffers This should be reviewed commit-by-commit. I've not attempted to fully flesh out what the end state of this PR could look like yet as I wanted to get it up for some early feedback (I already think this has some wins, too, so we could land it as-is). The primary idea with `Buffer` is that it internally tracks whether we're printing to HTML or text, and the goal is that eventually instead of branch on `fmt.alternate()` anywhere, we'd call a helper like `buf.nbsp()` which would either return ` ` or ` ` depending on the target we're printing to. Obviously, that's not included in this PR, in part because it was already getting quite big.

bors · 2019-09-03T10:17:31Z

⌛ Testing commit a9c6ef6 with merge 982018376261ae04983bab41ff19efff50984eb9...

Centril · 2019-09-03T10:21:30Z

Seems like this failed in #64115 (comment), @bors r-

Centril · 2019-09-03T10:22:39Z

(It could also be #64096)

Centril · 2019-09-03T10:23:20Z

@bors retry

Mark-Simulacrum · 2019-09-03T13:13:08Z

Almost certainly this PR, @bors r-

This avoids needlessly creating and threading the buffers through when we only use them once.

This means that callers can pass in a closure like `|buf| some_function(..., &mut buf)` and pass in arbitrary arguments to that function without complicating the trait definition. We also keep the impl for str and String, since it's useful to be able to just pass in "" or format!("{}"...) results in some cases. This changes Print's definition to take self, instead of &self, because otherwise FnOnce cannot be called directly. We could instead take FnMut or even Fn, but that seems like it'd merely complicate matters -- most of the time, the FnOnce does not constrain us at all anyway. If it does, a custom Print impl for &'_ SomeStruct is not all that painful.

Mark-Simulacrum · 2019-09-07T23:34:16Z

@bors r=GuillaumeGomez

bors · 2019-09-07T23:34:17Z

📌 Commit 02c5c5c has been approved by GuillaumeGomez

Fixed comments

bors · 2019-09-08T02:12:31Z

⌛ Testing commit 02c5c5c with merge 4a8ccdb...

Rustdoc: formatting to buffers This should be reviewed commit-by-commit. I've not attempted to fully flesh out what the end state of this PR could look like yet as I wanted to get it up for some early feedback (I already think this has some wins, too, so we could land it as-is). The primary idea with `Buffer` is that it internally tracks whether we're printing to HTML or text, and the goal is that eventually instead of branch on `fmt.alternate()` anywhere, we'd call a helper like `buf.nbsp()` which would either return ` ` or ` ` depending on the target we're printing to. Obviously, that's not included in this PR, in part because it was already getting quite big.

bors · 2019-09-08T06:06:33Z

☀️ Test successful - checks-azure
Approved by: GuillaumeGomez
Pushing 4a8ccdb to master...

rust-highfive assigned GuillaumeGomez Aug 31, 2019

matklad reviewed Aug 31, 2019

View reviewed changes

JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2019

Mark-Simulacrum force-pushed the rustdoc-clean-2 branch 2 times, most recently from 8a1d72b to a9c6ef6 Compare September 2, 2019 14:32

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 2, 2019

Centril mentioned this pull request Sep 3, 2019

Rollup of 9 pull requests #64115

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 3, 2019

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 3, 2019

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 3, 2019

Mark-Simulacrum added 17 commits September 7, 2019 19:31

Add buffer abstraction

f35eae9

Migrate top-level rendering to Buffer

eebd026

Create buffers in top-level rendering

9e52ba9

This avoids needlessly creating and threading the buffers through when we only use them once.

Remove needless clone of layout

3657bfc

Move constant parameters to render to Layout struct

57243b7

Replace writeln!/write! with push_str

75528f2

Remove unnecessary Buffer in layout::render

04dc72e

Move sidebar to Buffer-printing

f8bccb1

Delete Sidebar struct in favor of FnOnce impl

eefaee1

De-indent all fmt::Display impls for later replacement to functions

d5f1470

layout::render takes Print instead of fmt::Display

3f0e77f

Move Source to Buffer

bb40d5f

Settings to function

17bef30

AllTypes to function

a502e85

Item to function

c1c1e86

Move to buffers throughout print_item

02c5c5c

Mark-Simulacrum force-pushed the rustdoc-clean-2 branch from 8f399bd to 02c5c5c Compare September 7, 2019 23:33

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 7, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 8, 2019

bors merged commit 02c5c5c into rust-lang:master Sep 8, 2019

bors mentioned this pull request Sep 8, 2019

[rustdoc] Add sub settings #60778

Closed

jsha mentioned this pull request Mar 15, 2023

Tracking Issue for Rustdoc Askama Migration #108868

Open

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustdoc: formatting to buffers #64044

Rustdoc: formatting to buffers #64044

Mark-Simulacrum commented Aug 31, 2019 •

edited

Loading

rust-highfive commented Aug 31, 2019

matklad left a comment

matklad Aug 31, 2019

matklad Aug 31, 2019

matklad Aug 31, 2019

Mark-Simulacrum Aug 31, 2019

matklad Aug 31, 2019

GuillaumeGomez Sep 2, 2019

matklad Sep 2, 2019

matklad Aug 31, 2019

matklad Aug 31, 2019

matklad Aug 31, 2019

GuillaumeGomez Sep 2, 2019

matklad Aug 31, 2019

Mark-Simulacrum Aug 31, 2019

matklad Aug 31, 2019

Mark-Simulacrum Aug 31, 2019

Mark-Simulacrum commented Aug 31, 2019

rust-highfive commented Sep 1, 2019

GuillaumeGomez commented Sep 2, 2019

Mark-Simulacrum commented Sep 2, 2019

Mark-Simulacrum commented Sep 2, 2019

GuillaumeGomez commented Sep 2, 2019

matklad commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 3, 2019

Centril commented Sep 3, 2019

Centril commented Sep 3, 2019

Centril commented Sep 3, 2019

Mark-Simulacrum commented Sep 3, 2019

Mark-Simulacrum commented Sep 7, 2019

bors commented Sep 7, 2019

bors commented Sep 8, 2019

bors commented Sep 8, 2019

Rustdoc: formatting to buffers #64044

Rustdoc: formatting to buffers #64044

Conversation

Mark-Simulacrum commented Aug 31, 2019 • edited Loading

rust-highfive commented Aug 31, 2019

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 31, 2019

rust-highfive commented Sep 1, 2019

GuillaumeGomez commented Sep 2, 2019

Mark-Simulacrum commented Sep 2, 2019

Mark-Simulacrum commented Sep 2, 2019

GuillaumeGomez commented Sep 2, 2019

matklad commented Sep 2, 2019

bors commented Sep 2, 2019

bors commented Sep 3, 2019

Centril commented Sep 3, 2019

Centril commented Sep 3, 2019

Centril commented Sep 3, 2019

Mark-Simulacrum commented Sep 3, 2019

Mark-Simulacrum commented Sep 7, 2019

bors commented Sep 7, 2019

bors commented Sep 8, 2019

bors commented Sep 8, 2019

Mark-Simulacrum commented Aug 31, 2019 •

edited

Loading