Skip to content

Do more lazy-formatting in librustdoc 🦥 #136828

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 15, 2025
2 changes: 1 addition & 1 deletion src/librustdoc/clean/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use rustc_session::parse::ParseSess;
use rustc_span::Span;
use rustc_span::symbol::{Symbol, sym};

use crate::display::Joined as _;
use crate::html::escape::Escape;
use crate::joined::Joined as _;

#[cfg(test)]
mod tests;
Expand Down
18 changes: 18 additions & 0 deletions src/librustdoc/joined.rs → src/librustdoc/display.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//! Various utilities for working with [`fmt::Display`] implementations.

use std::fmt::{self, Display, Formatter};

pub(crate) trait Joined: IntoIterator {
Expand Down Expand Up @@ -27,3 +29,19 @@ where
Ok(())
}
}

pub(crate) trait MaybeDisplay {
/// For a given `Option<T: Display>`, returns a `Display` implementation that will display `t` if `Some(t)`, or nothing if `None`.
fn maybe_display(self) -> impl Display;
}

impl<T: Display> MaybeDisplay for Option<T> {
fn maybe_display(self) -> impl Display {
fmt::from_fn(move |f| {
if let Some(t) = self.as_ref() {
t.fmt(f)?;
}
Ok(())
})
}
}
47 changes: 27 additions & 20 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ use super::url_parts_builder::{UrlPartsBuilder, estimate_item_path_byte_length};
use crate::clean::types::ExternalLocation;
use crate::clean::utils::find_nearest_parent_module;
use crate::clean::{self, ExternalCrate, PrimitiveType};
use crate::display::Joined as _;
use crate::formats::cache::Cache;
use crate::formats::item_type::ItemType;
use crate::html::escape::{Escape, EscapeBodyText};
use crate::html::render::Context;
use crate::joined::Joined as _;
use crate::passes::collect_intra_doc_links::UrlFragment;

pub(crate) fn write_str(s: &mut String, f: fmt::Arguments<'_>) {
Expand Down Expand Up @@ -709,19 +709,22 @@ fn resolved_path(
if w.alternate() {
write!(w, "{}{:#}", last.name, last.args.print(cx))?;
} else {
let path = if use_absolute {
if let Ok((_, _, fqp)) = href(did, cx) {
format!(
"{path}::{anchor}",
path = join_with_double_colon(&fqp[..fqp.len() - 1]),
anchor = anchor(did, *fqp.last().unwrap(), cx)
)
let path = fmt::from_fn(|f| {
if use_absolute {
if let Ok((_, _, fqp)) = href(did, cx) {
write!(
f,
"{path}::{anchor}",
path = join_with_double_colon(&fqp[..fqp.len() - 1]),
anchor = anchor(did, *fqp.last().unwrap(), cx)
)
} else {
write!(f, "{}", last.name)
}
} else {
last.name.to_string()
write!(f, "{}", anchor(did, last.name, cx))
}
} else {
anchor(did, last.name, cx).to_string()
};
});
write!(w, "{path}{args}", args = last.args.print(cx))?;
}
Ok(())
Expand Down Expand Up @@ -749,16 +752,20 @@ fn primitive_link_fragment(
match m.primitive_locations.get(&prim) {
Some(&def_id) if def_id.is_local() => {
let len = cx.current.len();
let path = if len == 0 {
let cname_sym = ExternalCrate { crate_num: def_id.krate }.name(cx.tcx());
format!("{cname_sym}/")
} else {
"../".repeat(len - 1)
};
let path = fmt::from_fn(|f| {
if len == 0 {
let cname_sym = ExternalCrate { crate_num: def_id.krate }.name(cx.tcx());
write!(f, "{cname_sym}/")?;
} else {
for _ in 0..(len - 1) {
f.write_str("../")?;
}
}
Ok(())
});
write!(
f,
"<a class=\"primitive\" href=\"{}primitive.{}.html{fragment}\">",
path,
"<a class=\"primitive\" href=\"{path}primitive.{}.html{fragment}\">",
prim.as_sym()
)?;
needs_termination = true;
Expand Down
29 changes: 17 additions & 12 deletions src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use std::cell::RefCell;
use std::collections::BTreeMap;
use std::fmt::{self, Write as _};
use std::io;
use std::path::{Path, PathBuf};
use std::sync::mpsc::{Receiver, channel};
use std::{fmt, io};

use rinja::Template;
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap, FxIndexSet};
Expand Down Expand Up @@ -265,21 +266,25 @@ impl<'tcx> Context<'tcx> {
// preventing an infinite redirection loop in the generated
// documentation.

let mut path = String::new();
for name in &names[..names.len() - 1] {
path.push_str(name.as_str());
path.push('/');
}
path.push_str(&item_path(ty, names.last().unwrap().as_str()));
let path = fmt::from_fn(|f| {
for name in &names[..names.len() - 1] {
write!(f, "{name}/")?;
}
write!(f, "{}", item_path(ty, names.last().unwrap().as_str()))
});
match self.shared.redirections {
Some(ref redirections) => {
let mut current_path = String::new();
for name in &self.current {
current_path.push_str(name.as_str());
current_path.push('/');
}
current_path.push_str(&item_path(ty, names.last().unwrap().as_str()));
redirections.borrow_mut().insert(current_path, path);
let _ = write!(
current_path,
"{}",
item_path(ty, names.last().unwrap().as_str())
);
redirections.borrow_mut().insert(current_path, path.to_string());
}
None => {
return layout::redirect(&format!(
Expand Down Expand Up @@ -854,9 +859,9 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
if !buf.is_empty() {
let name = item.name.as_ref().unwrap();
let item_type = item.type_();
let file_name = &item_path(item_type, name.as_str());
let file_name = item_path(item_type, name.as_str()).to_string();
self.shared.ensure_dir(&self.dst)?;
let joint_dst = self.dst.join(file_name);
let joint_dst = self.dst.join(&file_name);
self.shared.fs.write(joint_dst, buf)?;

if !self.info.render_redirect_pages {
Expand All @@ -873,7 +878,7 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
format!("{crate_name}/{file_name}"),
);
} else {
let v = layout::redirect(file_name);
let v = layout::redirect(&file_name);
let redir_dst = self.dst.join(redir_name);
self.shared.fs.write(redir_dst, v)?;
}
Expand Down
96 changes: 65 additions & 31 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub(crate) use self::context::*;
pub(crate) use self::span_map::{LinkFromSrc, collect_spans_and_sources};
pub(crate) use self::write_shared::*;
use crate::clean::{self, ItemId, RenderedLink};
use crate::display::{Joined as _, MaybeDisplay as _};
use crate::error::Error;
use crate::formats::Impl;
use crate::formats::cache::Cache;
Expand Down Expand Up @@ -568,17 +569,27 @@ fn document_short<'a, 'cx: 'a>(
let (mut summary_html, has_more_content) =
MarkdownSummaryLine(&s, &item.links(cx)).into_string_with_has_more_content();

if has_more_content {
let link = format!(" <a{}>Read more</a>", assoc_href_attr(item, link, cx));
let link = if has_more_content {
let link = fmt::from_fn(|f| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: This can be made more consise with format_args!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work either...

write!(
f,
" <a{}>Read more</a>",
assoc_href_attr(item, link, cx).maybe_display()
)
});

if let Some(idx) = summary_html.rfind("</p>") {
summary_html.insert_str(idx, &link);
summary_html.insert_str(idx, &link.to_string());
None
} else {
summary_html.push_str(&link);
Some(link)
}
} else {
None
}
.maybe_display();

write!(f, "<div class='docblock'>{summary_html}</div>")?;
write!(f, "<div class='docblock'>{summary_html}{link}</div>")?;
}
Ok(())
})
Expand Down Expand Up @@ -788,13 +799,23 @@ pub(crate) fn render_impls(
}

/// Build a (possibly empty) `href` attribute (a key-value pair) for the given associated item.
fn assoc_href_attr(it: &clean::Item, link: AssocItemLink<'_>, cx: &Context<'_>) -> String {
fn assoc_href_attr<'a, 'tcx>(
it: &clean::Item,
link: AssocItemLink<'a>,
cx: &Context<'tcx>,
) -> Option<impl fmt::Display + 'a + Captures<'tcx>> {
let name = it.name.unwrap();
let item_type = it.type_();

enum Href<'a> {
AnchorId(&'a str),
Anchor(ItemType),
Url(String, ItemType),
}

let href = match link {
AssocItemLink::Anchor(Some(ref id)) => Some(format!("#{id}")),
AssocItemLink::Anchor(None) => Some(format!("#{item_type}.{name}")),
AssocItemLink::Anchor(Some(ref id)) => Href::AnchorId(id),
AssocItemLink::Anchor(None) => Href::Anchor(item_type),
AssocItemLink::GotoSource(did, provided_methods) => {
// We're creating a link from the implementation of an associated item to its
// declaration in the trait declaration.
Expand All @@ -814,7 +835,7 @@ fn assoc_href_attr(it: &clean::Item, link: AssocItemLink<'_>, cx: &Context<'_>)
};

match href(did.expect_def_id(), cx) {
Ok((url, ..)) => Some(format!("{url}#{item_type}.{name}")),
Ok((url, ..)) => Href::Url(url, item_type),
// The link is broken since it points to an external crate that wasn't documented.
// Do not create any link in such case. This is better than falling back to a
// dummy anchor like `#{item_type}.{name}` representing the `id` of *this* impl item
Expand All @@ -826,15 +847,25 @@ fn assoc_href_attr(it: &clean::Item, link: AssocItemLink<'_>, cx: &Context<'_>)
// those two items are distinct!
// In this scenario, the actual `id` of this impl item would be
// `#{item_type}.{name}-{n}` for some number `n` (a disambiguator).
Err(HrefError::DocumentationNotBuilt) => None,
Err(_) => Some(format!("#{item_type}.{name}")),
Err(HrefError::DocumentationNotBuilt) => return None,
Err(_) => Href::Anchor(item_type),
}
}
};

let href = fmt::from_fn(move |f| match &href {
Href::AnchorId(id) => write!(f, "#{id}"),
Href::Url(url, item_type) => {
write!(f, "{url}#{item_type}.{name}")
}
Href::Anchor(item_type) => {
write!(f, "#{item_type}.{name}")
}
});

// If there is no `href` for the reason explained above, simply do not render it which is valid:
// https://html.spec.whatwg.org/multipage/links.html#links-created-by-a-and-area-elements
href.map(|href| format!(" href=\"{href}\"")).unwrap_or_default()
Some(fmt::from_fn(move |f| write!(f, " href=\"{href}\"")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work either 😅

}

#[derive(Debug)]
Expand Down Expand Up @@ -865,7 +896,7 @@ fn assoc_const(
"{indent}{vis}const <a{href} class=\"constant\">{name}</a>{generics}: {ty}",
indent = " ".repeat(indent),
vis = visibility_print_with_space(it, cx),
href = assoc_href_attr(it, link, cx),
href = assoc_href_attr(it, link, cx).maybe_display(),
name = it.name.as_ref().unwrap(),
generics = generics.print(cx),
ty = ty.print(cx),
Expand Down Expand Up @@ -905,7 +936,7 @@ fn assoc_type(
"{indent}{vis}type <a{href} class=\"associatedtype\">{name}</a>{generics}",
indent = " ".repeat(indent),
vis = visibility_print_with_space(it, cx),
href = assoc_href_attr(it, link, cx),
href = assoc_href_attr(it, link, cx).maybe_display(),
name = it.name.as_ref().unwrap(),
generics = generics.print(cx),
),
Expand Down Expand Up @@ -948,7 +979,7 @@ fn assoc_method(
let asyncness = header.asyncness.print_with_space();
let safety = header.safety.print_with_space();
let abi = print_abi_with_space(header.abi).to_string();
let href = assoc_href_attr(meth, link, cx);
let href = assoc_href_attr(meth, link, cx).maybe_display();

// NOTE: `{:#}` does not print HTML formatting, `{}` does. So `g.print` can't be reused between the length calculation and `write!`.
let generics_len = format!("{:#}", g.print(cx)).len();
Expand All @@ -962,7 +993,7 @@ fn assoc_method(
+ name.as_str().len()
+ generics_len;

let notable_traits = notable_traits_button(&d.output, cx);
let notable_traits = notable_traits_button(&d.output, cx).maybe_display();

let (indent, indent_str, end_newline) = if parent == ItemType::Trait {
header_len += 4;
Expand Down Expand Up @@ -990,7 +1021,6 @@ fn assoc_method(
name = name,
generics = g.print(cx),
decl = d.full_print(header_len, indent, cx),
notable_traits = notable_traits.unwrap_or_default(),
where_clause = print_where_clause(g, cx, indent, end_newline),
),
);
Expand Down Expand Up @@ -1438,7 +1468,10 @@ fn should_render_item(item: &clean::Item, deref_mut_: bool, tcx: TyCtxt<'_>) ->
}
}

pub(crate) fn notable_traits_button(ty: &clean::Type, cx: &Context<'_>) -> Option<String> {
pub(crate) fn notable_traits_button<'a, 'tcx>(
ty: &'a clean::Type,
cx: &'a Context<'tcx>,
) -> Option<impl fmt::Display + 'a + Captures<'tcx>> {
let mut has_notable_trait = false;

if ty.is_unit() {
Expand Down Expand Up @@ -1480,15 +1513,16 @@ pub(crate) fn notable_traits_button(ty: &clean::Type, cx: &Context<'_>) -> Optio
}
}

if has_notable_trait {
has_notable_trait.then(|| {
cx.types_with_notable_traits.borrow_mut().insert(ty.clone());
Some(format!(
" <a href=\"#\" class=\"tooltip\" data-notable-ty=\"{ty}\">ⓘ</a>",
ty = Escape(&format!("{:#}", ty.print(cx))),
))
} else {
None
}
fmt::from_fn(|f| {
write!(
f,
" <a href=\"#\" class=\"tooltip\" data-notable-ty=\"{ty}\">ⓘ</a>",
ty = Escape(&format!("{:#}", ty.print(cx))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all needed in this PR, but I wounder if this temporary allocation here can also go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was wondering that too. I think it's unavoidable, since Escape needs to actually go over the formatted string and escape certain characters. The only way to create a "lazy" Escape construct would be to modify std's Formatter, which obviously is not an option...
But maybe I'm missing something.

)
})
})
}

fn notable_traits_decl(ty: &clean::Type, cx: &Context<'_>) -> (String, String) {
Expand Down Expand Up @@ -2117,11 +2151,11 @@ pub(crate) fn render_impl_summary(
) {
let inner_impl = i.inner_impl();
let id = cx.derive_id(get_id_for_impl(cx.tcx(), i.impl_item.item_id));
let aliases = if aliases.is_empty() {
String::new()
} else {
format!(" data-aliases=\"{}\"", aliases.join(","))
};
let aliases = (!aliases.is_empty())
.then_some(fmt::from_fn(|f| {
write!(f, " data-aliases=\"{}\"", fmt::from_fn(|f| aliases.iter().joined(",", f)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto wrt format_args!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, doesn't work...

error[E0716]: temporary value dropped while borrowed
    --> src/librustdoc/html/render/mod.rs:2111:20
     |
2111 |           .then_some(format_args!(
     |  ____________________-
2112 | |             " data-aliases=\"{}\"",
2113 | |             fmt::from_fn(|f| aliases.iter().joined(",", f))
2114 | |         ))
     | |         ^
     | |         |
     | |_________creates a temporary value which is freed while still in use
     |           in this macro invocation
2115 |           .maybe_display();
     |                           - temporary value is freed at the end of this statement
2116 |       write!(w, "<section id=\"{id}\" class=\"impl\"{aliases}>");
     |                                                     --------- borrow later used here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is apparently a known limitation. Sorry 😔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I can never remember exactly when it can be used and when not, so I was happy to double check 😊

}))
.maybe_display();
write_str(w, format_args!("<section id=\"{id}\" class=\"impl\"{aliases}>"));
render_rightside(w, cx, &i.impl_item, RenderMode::Normal);
write_str(
Expand Down
Loading
Loading