rustdoc: migrate item_struct to an Askama template

[nicklimmm]

Part of #108868

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[GuillaumeGomez]

This wrapping is really not great. Isn't it possible to call this function directly from the askama?

[nicklimmm]

Yes, it is possible.

One thing that we can try to explore is implementing something like this PR (ItemTemplate trait, item_template_* functions) to prevent multiple wrappings across multiple items. This might be useful for future migrations.

[GuillaumeGomez]

Indeed!

[nicklimmm]

I'll try to come up with a solution for that and publish another PR once it's ready since it will involve other items.

What do you think?

[GuillaumeGomez]

Sounds acceptable. If you don't mind: let's wait for this other PR to be merged so it can be used in this one to make the diff smaller.

[nicklimmm]

I'm thinking of using a derive macro something like this,

Usage

// Marker trait; The trait requirements are from the Askama Template derive macro
trait ItemTemplate: askama::Template + fmt::Display {}

#[derive(ItemTemplate)]
#[template(path = "item_struct.html")]
struct ItemStruct {
    // cx, it, and other fields
}

impl ItemStruct {
    // implementation of custom methods here
}

Macro Expansion

impl ItemTemplate for ItemStruct {}

impl ItemStruct {
    // implementation of common methods here, e.g. document
}

Haven't tested this yet, just dumping my ideas here first. Any opinions?

[nicklimmm]

At first, I tried to use plain trait but faced some issues,

• Can't return impl Traits in trait definition: this is used for default method implementation of common methods
• The previous PR uses borrow_mut() to extract cx and it out of the struct: this will create a good amount of boilerplate code

[GuillaumeGomez]

I'm curious to see that. The only problem with proc-macro is that it would require to create a new rustdoc-proc-macro crate. Maybe let's go with the boilerplate code first and merge this first iteration and then we can think about how to reduce it later on?

[nicklimmm]

Agreed. Tried to implement one and it looked overkill (at least for now).

Let's see how it goes and will create another PR for that.

[nicklimmm]

• Can't return impl Traits in trait definition: this is used for default method implementation of common methods

FYI: "Return position impl Trait in traits" RFC is now entering its final comment period (comment)

[bors]

☔ The latest upstream changes (presumably #112013) made this pull request unmergeable. Please resolve the merge conflicts.

[nicklimmm]

@GuillaumeGomez Shall we proceed with this implementation first while waiting for comments on the .borrow() removal issue?

[GuillaumeGomez]

I'd rather do things somewhat right instead of migrating code later on.

[nicklimmm]

I'd rather do things somewhat right instead of migrating code later on.

Let's wait for some activity in the issue, then we can see how to proceed.

[bors]

☔ The latest upstream changes (presumably #112543) made this pull request unmergeable. Please resolve the merge conflicts.

[rust-timer]

Finished benchmarking commit (dca3ec2ac6d301df9ca730a5873f15fad7157b04): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.3%, 0.3%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 656.011s -> 655.869s (-0.02%)

[nicklimmm]

Hmm, it worsens the perf. I have no idea which part of the code contributes to the regression. Do we have profilers to point out the cause of the regression (esp for rustdoc)?

[GuillaumeGomez]

There is tcx.time() but I'm not sure how useful it'd be in this case. So after that remains the usual tools like perf and similar ones...

[nicklimmm]

Since the perf results are not that great, shall we close this? I have no idea how to improve it for now.

[GuillaumeGomez]

I was planning to discuss about the askama migration in the next rustdoc meeting to see what to do.

[nicklimmm]

I was planning to discuss about the askama migration in the next rustdoc meeting to see what to do.

Any updates on this?

[GuillaumeGomez]

Forgot to post, sorry. Here's the conclusion on zulip.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=nicklimmm
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_7121c787-a94a-47fb-b94c-21278221d037
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=askama-migration-item-struct
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_7121c787-a94a-47fb-b94c-21278221d037
GITHUB_REF=refs/pull/111430/merge
GITHUB_REF_NAME=111430/merge
GITHUB_REF_PROTECTED=false
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=2b5272242084c4bce1ad0212726977b6c6fe9d59
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_7121c787-a94a-47fb-b94c-21278221d037
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_7121c787-a94a-47fb-b94c-21278221d037
GITHUB_TRIGGERING_ACTOR=nicklimmm
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/111430/merge
GITHUB_WORKFLOW_SHA=2b5272242084c4bce1ad0212726977b6c6fe9d59
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
    Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
error: multiple unused formatting arguments
    --> src/librustdoc/html/render/print_item.rs:1784:24
     |
1780 |             "{attrs}{vis}const {name}: {typ}",
     |             --------------------------------- multiple missing formatting specifiers
...
1784 |             generics = c.generics.print(cx),
     |                        ^^^^^^^^^^^^^^^^^^^^ named argument never used
1785 |             typ = c.type_.print(cx),
1786 |             where_clause = print_where_clause(&c.generics, cx, 0, Ending::NoNewline),
     |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ named argument never used
error[E0061]: this method takes 3 arguments but 2 arguments were supplied
    --> src/librustdoc/html/render/mod.rs:1083:21
     |
1083 |         for a in it.attributes(tcx, false) {
---
718  |         tcx: TyCtxt<'_>,
     |         ---------------
719  |         cache: &Cache,
     |         -------------
720  |         keep_as_is: bool,
help: provide the argument
     |
     |
1083 |         for a in it.attributes(tcx, /* &Cache */, false) {

error[E0425]: cannot find function `render_struct_fields` in this scope
    --> src/librustdoc/html/render/print_item.rs:1290:21
     |
     |
1290 |                       render_struct_fields(
     |                       ^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `render_enum_fields`
...
1539 | / fn render_enum_fields(
1540 | |     mut w: &mut Buffer,
1541 | |     cx: &mut Context<'_>,
1542 | |     g: Option<&clean::Generics>,
1600 | |     }
1601 | | }
1601 | | }
     | |_- similarly named function `render_enum_fields` defined here
error[E0425]: cannot find function `item_fields` in this scope
    --> src/librustdoc/html/render/print_item.rs:1301:17
     |
     |
1301 |                 item_fields(w, cx, it, fields, None);

error[E0425]: cannot find function `render_struct_fields` in this scope
    --> src/librustdoc/html/render/print_item.rs:1309:21
     |
     |
1309 |                       render_struct_fields(
     |                       ^^^^^^^^^^^^^^^^^^^^ help: a function with a similar name exists: `render_enum_fields`
...
1539 | / fn render_enum_fields(
1540 | |     mut w: &mut Buffer,
1541 | |     cx: &mut Context<'_>,
1542 | |     g: Option<&clean::Generics>,
1600 | |     }
1601 | | }
1601 | | }
     | |_- similarly named function `render_enum_fields` defined here
error[E0425]: cannot find function `item_fields` in this scope
    --> src/librustdoc/html/render/print_item.rs:1320:17
     |
     |
1320 |                 item_fields(w, cx, it, fields, None);

error[E0061]: this function takes 7 arguments but 8 arguments were supplied
    --> src/librustdoc/html/render/print_item.rs:1585:25
     |
     |
1585 |                         render_struct(w, v, None, None, &s.fields, TAB, false, cx);
     |                                       |
     |                                       unexpected argument of type `&mut html::format::Buffer`
     |                                       help: remove the extra argument
     |
     |
note: function defined here
    --> src/librustdoc/html/render/print_item.rs:2141:4
     |
2141 | fn render_struct<'a, 'cx: 'a>(
2142 |     it: &'a clean::Item,
     |     -------------------
     |     -------------------
2143 |     g: Option<&'a clean::Generics>,
     |     ------------------------------
2144 |     ty: Option<CtorKind>,
     |     --------------------
2145 |     fields: &'a [clean::Item],
     |     -------------------------
2146 |     tab: &'a str,
2147 |     structhead: bool,
     |     ----------------
2148 |     cx: &'a Context<'cx>,
     |     --------------------

[bors]

☔ The latest upstream changes (presumably #118192) made this pull request unmergeable. Please resolve the merge conflicts.

[Dylan-DPC]

@nicklimmm if you can check the CI failures and resolve the conflicts, we can push this forward for a review

[nicklimmm]

@nicklimmm if you can check the CI failures and resolve the conflicts, we can push this forward for a review

Will catch up on this soon, as I'm quite busy with other stuff.

[JohnCSimon]

Ping from triage:

@nicklimmm if you can check the CI failures and resolve the conflicts, we can push this forward for a review

echoing this - seems you're close

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.