Rollup merge of rust-lang#157540 - yotamofek:pr/rustdoc/render_all_impls-cleanup, r=GuillaumeGomez

Cleanup and optimize `render_impls`

- take ownership of the `Vec<&Impl>` instead of copying into another alloc
- reuse `ImplString` to do natural sort ordering
- lazy formatting

Somewhat of a follow-up to rust-lang#157233 and rust-lang#157179 (cc @nnethercote - thanks!)
This kinda undoes rust-lang@f7c8bc2 but IMHO it makes more sense to be explicit about negative impl ordering, and also seems kinda wasteful to "render" the negativity into a string and rely on however ASCII decided to order characters. I can also undo this part, I think this PR is still a positive change even without it.

r? @GuillaumeGomez

LLM disclosure:
I used LLM for reviewing my changes and making sure some assumptions I was making were correct (i.e. that `render_impls` will not render anything IFF the list of traits passed to it is empty)

Author: jhpratt

src/librustdoc/html/render/mod.rs

@@ -40,6 +40,7 @@ mod type_layout;
 mod write_shared;
 
 use std::borrow::Cow;
+use std::cmp::Ordering;
 use std::collections::VecDeque;
 use std::fmt::{self, Display as _, Write};
 use std::iter::Peekable;
@@ -79,7 +80,7 @@ use crate::html::format::{
 use crate::html::markdown::{
     HeadingOffset, IdMap, Markdown, MarkdownItemInfo, MarkdownSummaryLine, short_markdown_summary,
 };
-use crate::html::render::print_item::compare_names;
+use crate::html::render::print_item::ImplString;
 use crate::html::render::search_index::get_function_type_for_search;
 use crate::html::static_files::SCRAPE_EXAMPLES_HELP_MD;
 use crate::html::{highlight, sources};
@@ -954,46 +955,50 @@ fn impl_trait_key(cx: &Context<'_>, i: &Impl) -> Option<String> {
 
 // Render the list of items inside one of the sections "Trait Implementations",
 // "Auto Trait Implementations," "Blanket Trait Implementations" (on struct/enum pages).
-fn render_impls(
-    cx: &Context<'_>,
-    mut w: impl Write,
-    impls: &[&Impl],
-    containing_item: &clean::Item,
+fn render_impls<'a, 'cx>(
+    cx: &'a Context<'cx>,
+    mut impls: Vec<&'a Impl>,
+    containing_item: &'a clean::Item,
     toggle_open_by_default: bool,
-) -> fmt::Result {
+) -> impl fmt::Display + use<'a, 'cx> {
+    impls.sort_by_cached_key(|imp| {
+        let prefix = match imp.inner_impl().polarity {
+            ty::ImplPolarity::Positive | ty::ImplPolarity::Reservation => Ordering::Greater,
+            ty::ImplPolarity::Negative => Ordering::Less,
+        };
+        (prefix, ImplString::new_path(imp, cx))
+    });
     // Render each impl alongside its `impl_trait_key`, which is used as the primary sorting key
     // to match the impl order in the sidebar.
-    let mut keyed_rendered_impls = impls
-        .iter()
-        .map(|i| {
-            let did = i.trait_did().unwrap();
-            let provided_trait_methods = i.inner_impl().provided_trait_methods(cx.tcx());
-            let assoc_link = AssocItemLink::GotoSource(did.into(), &provided_trait_methods);
-            let imp = render_impl(
-                cx,
-                i,
-                containing_item,
-                assoc_link,
-                RenderMode::Normal,
-                None,
-                &[],
-                ImplRenderingParameters {
-                    show_def_docs: true,
-                    show_default_items: true,
-                    show_non_assoc_items: true,
-                    toggle_open_by_default,
-                },
-            );
-            (impl_trait_key(cx, i).unwrap(), imp.to_string())
-        })
-        .collect::<Vec<_>>();
 
-    // Sort and then remove the `impl_trait_key`s, which are no longer needed after sorting.
-    keyed_rendered_impls
-        .sort_by(|(k1, h1), (k2, h2)| compare_names(k1, k2).then_with(|| h1.cmp(h2)));
-    let joined: String = keyed_rendered_impls.into_iter().map(|a| a.1).collect();
-
-    w.write_str(&joined)
+    fmt::from_fn(move |f| {
+        impls
+            .iter()
+            .map(|i| {
+                fmt::from_fn(|f| {
+                    let did = i.trait_did().unwrap();
+                    let provided_trait_methods = i.inner_impl().provided_trait_methods(cx.tcx());
+                    let assoc_link = AssocItemLink::GotoSource(did.into(), &provided_trait_methods);
+                    render_impl(
+                        cx,
+                        i,
+                        containing_item,
+                        assoc_link,
+                        RenderMode::Normal,
+                        None,
+                        &[],
+                        ImplRenderingParameters {
+                            show_def_docs: true,
+                            show_default_items: true,
+                            show_non_assoc_items: true,
+                            toggle_open_by_default,
+                        },
+                    )
+                    .fmt(f)
+                })
+            })
+            .joined("", f)
+    })
 }
 
 /// Build a (possibly empty) `href` attribute (a key-value pair) for the given associated item.
@@ -1415,16 +1420,12 @@ fn render_all_impls(
     mut w: impl Write,
     cx: &Context<'_>,
     containing_item: &clean::Item,
-    concrete_impls: &[&Impl],
-    auto_trait_impls: &[&Impl],
-    blanket_impls: &[&Impl],
+    concrete_impls: Vec<&Impl>,
+    auto_trait_impls: Vec<&Impl>,
+    blanket_impls: Vec<&Impl>,
 ) -> fmt::Result {
-    let impls = {
-        let mut buf = String::new();
-        render_impls(cx, &mut buf, concrete_impls, containing_item, true)?;
-        buf
-    };
-    if !impls.is_empty() {
+    if !concrete_impls.is_empty() {
+        let impls = render_impls(cx, concrete_impls, containing_item, true);
         write!(
             w,
             "{}<div id=\"trait-implementations-list\">{impls}</div>",
@@ -1433,25 +1434,24 @@ fn render_all_impls(
     }
 
     if !auto_trait_impls.is_empty() {
+        let impls = render_impls(cx, auto_trait_impls, containing_item, false);
         // FIXME: Change the ID to `auto-trait-implementations-list`!
         write!(
             w,
-            "{}<div id=\"synthetic-implementations-list\">",
+            "{}<div id=\"synthetic-implementations-list\">{impls}</div>",
             write_impl_section_heading("Auto Trait Implementations", "synthetic-implementations",)
         )?;
-        render_impls(cx, &mut w, auto_trait_impls, containing_item, false)?;
-        w.write_str("</div>")?;
     }
 
     if !blanket_impls.is_empty() {
+        let impls = render_impls(cx, blanket_impls, containing_item, false);
         write!(
             w,
-            "{}<div id=\"blanket-implementations-list\">",
+            "{}<div id=\"blanket-implementations-list\">{impls}</div>",
             write_impl_section_heading("Blanket Implementations", "blanket-implementations")
         )?;
-        render_impls(cx, &mut w, blanket_impls, containing_item, false)?;
-        w.write_str("</div>")?;
     }
+
     Ok(())
 }
 
@@ -1588,14 +1588,7 @@ fn render_assoc_items_inner(
         let (blanket_impls, concrete_impls): (Vec<&Impl>, _) =
             trait_impls.into_iter().partition(|t| t.inner_impl().kind.is_blanket());
 
-        render_all_impls(
-            w,
-            cx,
-            containing_item,
-            &concrete_impls,
-            &auto_trait_impls,
-            &blanket_impls,
-        )?;
+        render_all_impls(w, cx, containing_item, concrete_impls, auto_trait_impls, blanket_impls)?;
     }
     Ok(())
 }

src/librustdoc/html/render/print_item.rs

@@ -32,8 +32,8 @@ use crate::formats::item_type::ItemType;
 use crate::html::escape::{Escape, EscapeBodyTextWithWbr};
 use crate::html::format::{
     Ending, PrintWithSpace, full_print_fn_decl, print_abi_with_space, print_constness_with_space,
-    print_generic_bound, print_generics, print_impl, print_import, print_type, print_where_clause,
-    visibility_print_with_space,
+    print_generic_bound, print_generics, print_impl, print_import, print_path, print_type,
+    print_where_clause, visibility_print_with_space,
 };
 use crate::html::markdown::{HeadingOffset, MarkdownSummaryLine};
 use crate::html::render::sidebar::filters;
@@ -1016,9 +1016,9 @@ fn item_trait(cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) -> impl fmt:
             let (mut synthetic, mut concrete): (Vec<&&Impl>, Vec<&&Impl>) =
                 local.iter().partition(|i| i.inner_impl().kind.is_auto());
 
-            synthetic.sort_by_cached_key(|i| ImplString::new(i, cx));
-            concrete.sort_by_cached_key(|i| ImplString::new(i, cx));
-            foreign.sort_by_cached_key(|i| ImplString::new(i, cx));
+            synthetic.sort_by_cached_key(|i| ImplString::new_impl(i, cx));
+            concrete.sort_by_cached_key(|i| ImplString::new_impl(i, cx));
+            foreign.sort_by_cached_key(|i| ImplString::new_impl(i, cx));
 
             if !foreign.is_empty() {
                 write!(
@@ -1969,7 +1969,7 @@ fn item_primitive(cx: &Context<'_>, it: &clean::Item) -> impl fmt::Display {
             let (concrete, synthetic, blanket_impl) =
                 get_filtered_impls_for_reference(&cx.shared, it);
 
-            render_all_impls(w, cx, it, &concrete, &synthetic, &blanket_impl)
+            render_all_impls(w, cx, it, concrete, synthetic, blanket_impl)
         }
     })
 }
@@ -2346,16 +2346,21 @@ where
 }
 
 #[derive(PartialEq, Eq)]
-struct ImplString {
+pub(super) struct ImplString {
     // Plain text (not HTML text) because this is only used for sorting purposes, and the plain
     // text is much shorter and thus faster to compare.
     cmp_text: String,
 }
 
 impl ImplString {
-    fn new(i: &Impl, cx: &Context<'_>) -> ImplString {
+    fn new_impl(i: &Impl, cx: &Context<'_>) -> Self {
         let impl_ = i.inner_impl();
-        ImplString { cmp_text: format!("{:#}", print_impl(impl_, false, cx)) }
+        Self { cmp_text: format!("{:#}", print_impl(impl_, false, cx)) }
+    }
+
+    pub(super) fn new_path(i: &Impl, cx: &Context<'_>) -> Option<Self> {
+        let path = i.inner_impl().trait_.as_ref()?;
+        Some(Self { cmp_text: format!("{:#}", print_path(path, cx)) })
     }
 }
 

tests/rustdoc-html/macro/const-rendering-macros-33302.rs

@@ -1,5 +1,5 @@
 // https://github.com/rust-lang/rust/issues/33302
-#![crate_name="issue_33302"]
+#![crate_name = "issue_33302"]
 
 // Ensure constant and array length values are not taken from source
 // code, which wreaks havoc with macros.
@@ -25,11 +25,12 @@ macro_rules! make {
         }
 
         //@ has issue_33302/struct.S.html \
-        //        '//*[@class="impl"]' 'impl T<[i32; 16]> for S'
-        //@ has - '//*[@id="associatedconstant.C"]' 'const C: [i32; 16]'
+        //        '//*[@class="impl"]' 'impl T<(i32, i32)> for S'
+        //@ has - '//*[@id="associatedconstant.C"]' 'const C: (i32, i32)'
         //@ has - '//*[@id="associatedconstant.D"]' 'const D: i32'
-        impl T<[i32; ($n * $n)]> for S {
-            const C: [i32; ($n * $n)] = [0; ($n * $n)];
+        impl T<(i32, i32)> for S {
+            const C: (i32, i32) = ($n, $n);
+            const D: i32 = ($n / $n);
         }
 
         //@ has issue_33302/struct.S.html \
@@ -41,12 +42,11 @@ macro_rules! make {
         }
 
         //@ has issue_33302/struct.S.html \
-        //        '//*[@class="impl"]' 'impl T<(i32, i32)> for S'
-        //@ has - '//*[@id="associatedconstant.C-2"]' 'const C: (i32, i32)'
+        //        '//*[@class="impl"]' 'impl T<[i32; 16]> for S'
+        //@ has - '//*[@id="associatedconstant.C-2"]' 'const C: [i32; 16]'
         //@ has - '//*[@id="associatedconstant.D-2"]' 'const D: i32'
-        impl T<(i32, i32)> for S {
-            const C: (i32, i32) = ($n, $n);
-            const D: i32 = ($n / $n);
+        impl T<[i32; ($n * $n)]> for S {
+            const C: [i32; ($n * $n)] = [0; ($n * $n)];
         }
     };
 }