rustdoc: migrate `item_struct` to an Askama template #111430

nicklimmm · 2023-05-10T15:06:28Z

rustbot · 2023-05-10T15:06:35Z

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

src/librustdoc/html/templates/item_struct.html

GuillaumeGomez · 2023-05-22T15:25:11Z

src/librustdoc/html/render/print_item.rs

+        }
+
+        fn document_non_exhaustive_header(&self) -> &str {
+            document_non_exhaustive_header(self.it)


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

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.

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?

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.

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?

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

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?

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.

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)

src/librustdoc/html/render/print_item.rs

bors · 2023-05-27T02:16:40Z

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

…it, r=GuillaumeGomez rustdoc: Add `ItemTemplate` trait and related functions to avoid repetitively wrapping existing functions Context: rust-lang#111430 (comment) This trait will be used extensively in performing migrations to Askama templates (tracking issue: rust-lang#108868)

nicklimmm · 2023-05-29T13:27:15Z

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

GuillaumeGomez · 2023-05-29T18:50:33Z

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

nicklimmm · 2023-05-30T00:11:26Z

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 · 2023-06-12T14:30:55Z

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

src/librustdoc/html/render/print_item.rs

GuillaumeGomez · 2023-06-27T08:57:39Z

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

nicklimmm · 2023-08-05T08:37:07Z

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

Any updates on this?

GuillaumeGomez · 2023-10-26T08:53:38Z

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

rust-log-analyzer · 2023-10-27T18:19:44Z

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 · 2023-11-23T20:01:58Z

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

Dylan-DPC · 2024-02-09T17:20:29Z

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

nicklimmm · 2024-02-13T15:34:36Z

@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 · 2024-04-14T17:33:36Z

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.

Dylan-DPC · 2024-07-28T06:22:02Z

@nicklimmm any updates on this? thanks

Dylan-DPC · 2024-09-24T08:58:48Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

rustbot assigned GuillaumeGomez May 10, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 10, 2023

This comment has been minimized.

Sign in to view

nicklimmm force-pushed the askama-migration-item-struct branch from f3d8a0f to 9608e56 Compare May 11, 2023 06:28

This comment has been minimized.

Sign in to view

nicklimmm force-pushed the askama-migration-item-struct branch from d3a92fc to 19c7c91 Compare May 12, 2023 07:47

nicklimmm marked this pull request as ready for review May 12, 2023 11:14

GuillaumeGomez reviewed May 22, 2023

View reviewed changes

src/librustdoc/html/templates/item_struct.html Outdated Show resolved Hide resolved

GuillaumeGomez reviewed May 22, 2023

View reviewed changes

src/librustdoc/html/render/print_item.rs Outdated Show resolved Hide resolved

nicklimmm force-pushed the askama-migration-item-struct branch from 19c7c91 to 62f204f Compare May 24, 2023 10:15

nicklimmm mentioned this pull request May 25, 2023

rustdoc: Add ItemTemplate trait and related functions to avoid repetitively wrapping existing functions #111946

Merged

nicklimmm mentioned this pull request May 27, 2023

rustdoc: remove .borrow() calls in Askama templates for ItemTemplate implementors #112021

Closed

2 tasks

nicklimmm force-pushed the askama-migration-item-struct branch from 746033d to a33bea0 Compare May 28, 2023 23:53

nicklimmm force-pushed the askama-migration-item-struct branch from af46634 to 4d043bb Compare June 12, 2023 05:09

This comment has been minimized.

Sign in to view

nicklimmm force-pushed the askama-migration-item-struct branch from 070da09 to 6a33b54 Compare June 12, 2023 14:53