rustdoc: Add item_template macro

[nicklimmm]

Closes #112021

This change removes the use of self.borrows() in Askama templates, removes code duplication from item_and_mut_cx(), and improved readability by eliminating the prefix item_template_ when calling from the template.

References:

• Discussion issue: rustdoc: remove .borrow() calls in Askama templates for ItemTemplate implementors #112021
• ItemTemplate PR: rustdoc: Add ItemTemplate trait and related functions to avoid repetitively wrapping existing functions #111946

r? @GuillaumeGomez

[GuillaumeGomez]

So overall it looks quite nice minus the few improvements I suggested. But one thing I'm considering here would be to directly use a macro.

Technical wondering: do we really need to use syn and quote here? The code is simple enough here that I'm not sure whether it's worth it or not. For quote it's almost for sure since all the generated code could be put in one format!(...).into().

Anyway, what do you think of this @rust-lang/rustdoc ?

[Manishearth]

Even extracting the struct name from the AST can be nontrivial so I do think we need syn and quote if we're using a proc macro.

However in this case I think a regular macro should be fine?

[nicklimmm]

I'm not really sure how to achieve similar results here using regular macros (macro_rules).

One problem using regular macros here is when we need to add extra fields to the struct other than it and cx

For example ItemStruct in a different PR:

#[derive(Template)]
#[template(path = "item_struct.html")]
struct ItemStruct<'a, 'cx> {
    cx: std::cell::RefCell<&'a mut Context<'cx>>,
    it: &'a clean::Item,
    s: &'a clean::Struct, // <-- extra here
    should_render_fields: bool, // <-- extra here
}

[GuillaumeGomez]

I suppose something like this would work:

macro_rules! item_template {
    (struct $name:ident {
cx: std::cell::RefCell<&'a mut Context<'cx>>,
    it: &'a clean::Item
$rest:tt*) => {{
       // implementation
    }}
}

[nicklimmm]

Managed to pull it off (along with method selection), and the usage looks something like this:

    item_template!(
        #[template(path = "item_union.html")]
        struct ItemUnion<'a, 'cx> {
            cx: RefCell<&'a mut Context<'cx>>,
            it: &'a clean::Item,
            s: &'a clean::Union,
        },
        methods = [document, document_type_layout, render_attributes_in_pre, render_assoc_items]
    );

I think I prefer using regular macros instead of derives to prevent adding dependencies and create a new crate just for this issue.

Note: will remove the derive macro once this is finalized

[GuillaumeGomez]

Looks great like this. 👍

[nicklimmm]

Looks great like this. 👍

Lemme clean things up and remove the draft status once it's done :)

[nicklimmm]

Bump @GuillaumeGomez whenever you are free :)

[GuillaumeGomez]

Suggested change

	($method:tt $($rest:tt)*) => {
	($method:ident $($rest:tt)*) => {

And very likely needs to add another case with:

    ($token:tt $($rest:tt)*) => {
        compile_error!(concat!("unexpected token: ", stringify($token));
    }

[nicklimmm]

Since we only expect idents for methods, seems like we can restrict them further by only allowing idents as input.

In this case, we need to modify the match rules in item_template_methods!()

Wdyt?

Ignore above: sounds better to catch everything with tt first, then match on ident on unknown method case

[GuillaumeGomez]

I really like the methods = part. It makes the macro easier to understand when using it/reading code. Well done! 👍

[GuillaumeGomez]

One other thing: can you put the macros at the top of the file (right after the imports) please?

[GuillaumeGomez]

Please just add an explanation on top of the macro on what it does and how to call it. Then squash your commit and it'll be time for r+. Nice work!

[GuillaumeGomez]

Thanks!

@bors r+ rollup

[bors]

📌 Commit e240dab has been approved by GuillaumeGomez

It is now in the queue for this repository.

[bors]

⌛ Testing commit e240dab with merge 7b6093e...

[bors]

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 7b6093e to master...

[rust-timer]

Finished benchmarking commit (7b6093e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	-	-	0

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: 647.255s -> 647.688s (0.07%)

[coffinmatician]

This PR introduces an ICE regression in geo v0.22.1 if you build with cargo build --release. You first need to update the Cargo.lock with cargo update (or see the version that I attached below).

Cargo.lock.txt

error[E0275]: overflow evaluating the requirement `[closure@src/algorithm/map_coords.rs:888:69: 888:72]: Fn<(geo_types::Coord<T>,)>`
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`geo`)
  = note: required for `&[closure@src/algorithm/map_coords.rs:888:69: 888:72]` to implement `Fn<(geo_types::Coord<T>,)>`
  = note: 128 redundant requirements hidden
  = note: required for `&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&...` to implement `Fn<(geo_types::Coord<T>,)>`
  = note: the full type name has been written to '/home/alex/Downloads/geo-0.22.1/target/release/deps/geo-31a6d43c26e8a8cc.long-type-15151749810108145118.txt'