Auto merge of #136244 - yotamofek:pr/rustdoc-join-iter, r=<try>

[WIP: perf] attempt to prevent regressions in that originally happened in #135494

This implements something similar to [`Itertools::format`](https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.format), but on `Fn`s returning iterators instead of directly on iterators, to allow implementing `Display` without the use of a `Cell` (to handle the possibility of `fmt` being called multiple times while receiving `&self`).

This is WIP, I just want to get a perf run first to see if the regression I saw in #135494 is fixed

Author: bors

src/librustdoc/clean/cfg.rs

@@ -14,6 +14,7 @@ use rustc_span::Span;
 use rustc_span::symbol::{Symbol, sym};
 
 use crate::html::escape::Escape;
+use crate::join::Join as _;
 
 #[cfg(test)]
 mod tests;
@@ -396,6 +397,8 @@ impl Display<'_> {
         sub_cfgs: &[Cfg],
         separator: &str,
     ) -> fmt::Result {
+        use fmt::Display as _;
+
         let short_longhand = self.1.is_long() && {
             let all_crate_features =
                 sub_cfgs.iter().all(|sub_cfg| matches!(sub_cfg, Cfg::Cfg(sym::feature, Some(_))));
@@ -414,20 +417,27 @@ impl Display<'_> {
             }
         };
 
-        for (i, sub_cfg) in sub_cfgs.iter().enumerate() {
-            if i != 0 {
-                fmt.write_str(separator)?;
-            }
-            if let (true, Cfg::Cfg(_, Some(feat))) = (short_longhand, sub_cfg) {
-                if self.1.is_html() {
-                    write!(fmt, "<code>{feat}</code>")?;
-                } else {
-                    write!(fmt, "`{feat}`")?;
-                }
-            } else {
-                write_with_opt_paren(fmt, !sub_cfg.is_all(), Display(sub_cfg, self.1))?;
-            }
-        }
+        (|| {
+            sub_cfgs.iter().map(|sub_cfg| {
+                fmt::from_fn(move |fmt| {
+                    if let Cfg::Cfg(_, Some(feat)) = sub_cfg
+                        && short_longhand
+                    {
+                        if self.1.is_html() {
+                            write!(fmt, "<code>{feat}</code>")?;
+                        } else {
+                            write!(fmt, "`{feat}`")?;
+                        }
+                    } else {
+                        write_with_opt_paren(fmt, !sub_cfg.is_all(), Display(sub_cfg, self.1))?;
+                    }
+                    Ok(())
+                })
+            })
+        })
+        .join(separator)
+        .fmt(fmt)?;
+
         Ok(())
     }
 }
@@ -439,11 +449,20 @@ impl fmt::Display for Display<'_> {
                 Cfg::Any(ref sub_cfgs) => {
                     let separator =
                         if sub_cfgs.iter().all(Cfg::is_simple) { " nor " } else { ", nor " };
-                    for (i, sub_cfg) in sub_cfgs.iter().enumerate() {
-                        fmt.write_str(if i == 0 { "neither " } else { separator })?;
-                        write_with_opt_paren(fmt, !sub_cfg.is_all(), Display(sub_cfg, self.1))?;
-                    }
-                    Ok(())
+                    fmt.write_str("neither ")?;
+                    (|| {
+                        sub_cfgs.iter().map(|sub_cfg| {
+                            fmt::from_fn(|fmt| {
+                                write_with_opt_paren(
+                                    fmt,
+                                    !sub_cfg.is_all(),
+                                    Display(sub_cfg, self.1),
+                                )
+                            })
+                        })
+                    })
+                    .join(separator)
+                    .fmt(fmt)
                 }
                 ref simple @ Cfg::Cfg(..) => write!(fmt, "non-{}", Display(simple, self.1)),
                 ref c => write!(fmt, "not ({})", Display(c, self.1)),

src/librustdoc/html/format.rs

@@ -8,12 +8,10 @@
 //! not be used external to this module.
 
 use std::borrow::Cow;
-use std::cell::Cell;
 use std::cmp::Ordering;
 use std::fmt::{self, Display, Write};
 use std::iter::{self, once};
 
-use itertools::Itertools;
 use rustc_abi::ExternAbi;
 use rustc_attr_parsing::{ConstStability, StabilityLevel, StableSince};
 use rustc_data_structures::captures::Captures;
@@ -35,6 +33,7 @@ use crate::formats::cache::Cache;
 use crate::formats::item_type::ItemType;
 use crate::html::escape::{Escape, EscapeBodyText};
 use crate::html::render::Context;
+use crate::join::Join as _;
 use crate::passes::collect_intra_doc_links::UrlFragment;
 
 pub(crate) trait Print {
@@ -146,36 +145,18 @@ impl Buffer {
     }
 }
 
-pub(crate) fn comma_sep<T: Display>(
-    items: impl Iterator<Item = T>,
-    space_after_comma: bool,
-) -> impl Display {
-    let items = Cell::new(Some(items));
-    fmt::from_fn(move |f| {
-        for (i, item) in items.take().unwrap().enumerate() {
-            if i != 0 {
-                write!(f, ",{}", if space_after_comma { " " } else { "" })?;
-            }
-            item.fmt(f)?;
-        }
-        Ok(())
-    })
-}
-
 pub(crate) fn print_generic_bounds<'a, 'tcx: 'a>(
     bounds: &'a [clean::GenericBound],
     cx: &'a Context<'tcx>,
 ) -> impl Display + 'a + Captures<'tcx> {
     fmt::from_fn(move |f| {
-        let mut bounds_dup = FxHashSet::default();
+        (|| {
+            let mut bounds_dup = FxHashSet::default();
 
-        for (i, bound) in bounds.iter().filter(|b| bounds_dup.insert(*b)).enumerate() {
-            if i > 0 {
-                f.write_str(" + ")?;
-            }
-            bound.print(cx).fmt(f)?;
-        }
-        Ok(())
+            bounds.iter().filter(move |b| bounds_dup.insert(*b)).map(|bound| bound.print(cx))
+        })
+        .join(" + ")
+        .fmt(f)
     })
 }
 
@@ -190,12 +171,7 @@ impl clean::GenericParamDef {
 
                 if !outlives.is_empty() {
                     f.write_str(": ")?;
-                    for (i, lt) in outlives.iter().enumerate() {
-                        if i != 0 {
-                            f.write_str(" + ")?;
-                        }
-                        write!(f, "{}", lt.print())?;
-                    }
+                    write!(f, "{}", (|| outlives.iter().map(|lt| lt.print())).join(" + "))?;
                 }
 
                 Ok(())
@@ -245,10 +221,11 @@ impl clean::Generics {
                 return Ok(());
             }
 
+            let real_params = (|| real_params.clone().map(|g| g.print(cx))).join(", ");
             if f.alternate() {
-                write!(f, "<{:#}>", comma_sep(real_params.map(|g| g.print(cx)), true))
+                write!(f, "<{:#}>", real_params)
             } else {
-                write!(f, "&lt;{}&gt;", comma_sep(real_params.map(|g| g.print(cx)), true))
+                write!(f, "&lt;{}&gt;", real_params)
             }
         })
     }
@@ -270,10 +247,12 @@ pub(crate) fn print_where_clause<'a, 'tcx: 'a>(
     ending: Ending,
 ) -> impl Display + 'a + Captures<'tcx> {
     fmt::from_fn(move |f| {
-        let mut where_predicates = gens
-            .where_predicates
-            .iter()
-            .map(|pred| {
+        if gens.where_predicates.is_empty() {
+            return Ok(());
+        }
+
+        let where_predicates = || {
+            gens.where_predicates.iter().map(|pred| {
                 fmt::from_fn(move |f| {
                     if f.alternate() {
                         f.write_str(" ")?;
@@ -312,13 +291,9 @@ pub(crate) fn print_where_clause<'a, 'tcx: 'a>(
                     }
                 })
             })
-            .peekable();
-
-        if where_predicates.peek().is_none() {
-            return Ok(());
-        }
+        };
 
-        let where_preds = comma_sep(where_predicates, false);
+        let where_preds = where_predicates.join(",");
         let clause = if f.alternate() {
             if ending == Ending::Newline {
                 format!(" where{where_preds},")
@@ -415,12 +390,7 @@ impl clean::GenericBound {
                 } else {
                     f.write_str("use&lt;")?;
                 }
-                for (i, arg) in args.iter().enumerate() {
-                    if i > 0 {
-                        write!(f, ", ")?;
-                    }
-                    arg.fmt(f)?;
-                }
+                (|| args.iter()).join(", ").fmt(f)?;
                 if f.alternate() { f.write_str(">") } else { f.write_str("&gt;") }
             }
         })
@@ -524,11 +494,7 @@ pub(crate) enum HrefError {
 // Panics if `syms` is empty.
 pub(crate) fn join_with_double_colon(syms: &[Symbol]) -> String {
     let mut s = String::with_capacity(estimate_item_path_byte_length(syms.len()));
-    s.push_str(syms[0].as_str());
-    for sym in &syms[1..] {
-        s.push_str("::");
-        s.push_str(sym.as_str());
-    }
+    write!(s, "{}", (|| syms.iter()).join("::")).unwrap();
     s
 }
 
@@ -572,20 +538,20 @@ fn generate_macro_def_id_path(
     }
 
     if let Some(last) = path.last_mut() {
-        *last = Symbol::intern(&format!("macro.{}.html", last.as_str()));
+        *last = Symbol::intern(&format!("macro.{last}.html"));
     }
 
     let url = match cache.extern_locations[&def_id.krate] {
         ExternalLocation::Remote(ref s) => {
             // `ExternalLocation::Remote` always end with a `/`.
-            format!("{s}{path}", path = path.iter().map(|p| p.as_str()).join("/"))
+            format!("{s}{path}", path = (|| path.iter()).join("/"))
         }
         ExternalLocation::Local => {
             // `root_path` always end with a `/`.
             format!(
                 "{root_path}{path}",
                 root_path = root_path.unwrap_or(""),
-                path = path.iter().map(|p| p.as_str()).join("/")
+                path = (|| path.iter()).join("/")
             )
         }
         ExternalLocation::Unknown => {
@@ -682,9 +648,8 @@ fn make_href(
             url_parts.push("index.html");
         }
         _ => {
-            let prefix = shortty.as_str();
             let last = fqp.last().unwrap();
-            url_parts.push_fmt(format_args!("{prefix}.{last}.html"));
+            url_parts.push_fmt(format_args!("{shortty}.{last}.html"));
         }
     }
     Ok((url_parts.finish(), shortty, fqp.to_vec()))
@@ -950,12 +915,7 @@ fn tybounds<'a, 'tcx: 'a>(
     cx: &'a Context<'tcx>,
 ) -> impl Display + 'a + Captures<'tcx> {
     fmt::from_fn(move |f| {
-        for (i, bound) in bounds.iter().enumerate() {
-            if i > 0 {
-                write!(f, " + ")?;
-            }
-            bound.print(cx).fmt(f)?;
-        }
+        (|| bounds.iter().map(|bound| bound.print(cx))).join(" + ").fmt(f)?;
         if let Some(lt) = lt {
             // We don't need to check `alternate` since we can be certain that
             // the lifetime doesn't contain any characters which need escaping.
@@ -974,7 +934,7 @@ fn print_higher_ranked_params_with_space<'a, 'tcx: 'a>(
         if !params.is_empty() {
             f.write_str(keyword)?;
             f.write_str(if f.alternate() { "<" } else { "&lt;" })?;
-            comma_sep(params.iter().map(|lt| lt.print(cx)), true).fmt(f)?;
+            (|| params.iter().map(|lt| lt.print(cx))).join(", ").fmt(f)?;
             f.write_str(if f.alternate() { "> " } else { "&gt; " })?;
         }
         Ok(())
@@ -1025,9 +985,7 @@ fn fmt_type(
         clean::Primitive(clean::PrimitiveType::Never) => {
             primitive_link(f, PrimitiveType::Never, format_args!("!"), cx)
         }
-        clean::Primitive(prim) => {
-            primitive_link(f, prim, format_args!("{}", prim.as_sym().as_str()), cx)
-        }
+        clean::Primitive(prim) => primitive_link(f, prim, format_args!("{}", prim.as_sym()), cx),
         clean::BareFunction(ref decl) => {
             print_higher_ranked_params_with_space(&decl.generic_params, cx, "for").fmt(f)?;
             decl.safety.print_with_space().fmt(f)?;
@@ -1067,18 +1025,13 @@ fn fmt_type(
                     primitive_link(
                         f,
                         PrimitiveType::Tuple,
-                        format_args!("({})", generic_names.iter().map(|s| s.as_str()).join(", ")),
+                        format_args!("({})", (|| generic_names.iter()).join(", ")),
                         cx,
                     )
                 } else {
-                    write!(f, "(")?;
-                    for (i, item) in many.iter().enumerate() {
-                        if i != 0 {
-                            write!(f, ", ")?;
-                        }
-                        item.print(cx).fmt(f)?;
-                    }
-                    write!(f, ")")
+                    f.write_str("(")?;
+                    (|| many.iter().map(|item| item.print(cx))).join(", ").fmt(f)?;
+                    f.write_str(")")
                 }
             }
         },
@@ -1407,14 +1360,16 @@ impl clean::Arguments {
         cx: &'a Context<'tcx>,
     ) -> impl Display + 'a + Captures<'tcx> {
         fmt::from_fn(move |f| {
-            for (i, input) in self.values.iter().enumerate() {
-                write!(f, "{}: ", input.name)?;
-                input.type_.print(cx).fmt(f)?;
-                if i + 1 < self.values.len() {
-                    write!(f, ", ")?;
-                }
-            }
-            Ok(())
+            (|| {
+                self.values.iter().map(|input| {
+                    fmt::from_fn(|f| {
+                        write!(f, "{}: ", input.name)?;
+                        input.type_.print(cx).fmt(f)
+                    })
+                })
+            })
+            .join(", ")
+            .fmt(f)
         })
     }
 }
@@ -1723,12 +1678,7 @@ impl clean::ImportSource {
                 }
                 let name = self.path.last();
                 if let hir::def::Res::PrimTy(p) = self.path.res {
-                    primitive_link(
-                        f,
-                        PrimitiveType::from(p),
-                        format_args!("{}", name.as_str()),
-                        cx,
-                    )?;
+                    primitive_link(f, PrimitiveType::from(p), format_args!("{name}"), cx)?;
                 } else {
                     f.write_str(name.as_str())?;
                 }