Skip to content

rustdoc: Remove distinction between "regular" and "collapsed" docs #91072

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 17 additions & 40 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::cell::RefCell;
use std::default::Default;
use std::hash::{Hash, Hasher};
use std::iter::FromIterator;
use std::lazy::SyncOnceCell as OnceCell;
use std::path::PathBuf;
use std::rc::Rc;
Expand Down Expand Up @@ -121,12 +120,11 @@ crate struct Crate {
crate primitives: ThinVec<(DefId, PrimitiveType)>,
/// Only here so that they can be filtered through the rustdoc passes.
crate external_traits: Rc<RefCell<FxHashMap<DefId, TraitWithExtraInfo>>>,
crate collapsed: bool,
}

// `Crate` is frequently moved by-value. Make sure it doesn't unintentionally get bigger.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Crate, 104);
rustc_data_structures::static_assert_size!(Crate, 96);

impl Crate {
crate fn name(&self, tcx: TyCtxt<'_>) -> Symbol {
Expand Down Expand Up @@ -411,12 +409,6 @@ impl Item {
crate::passes::span_of_attrs(&self.attrs).unwrap_or_else(|| self.span(tcx).inner())
}

/// Finds the `doc` attribute as a NameValue and returns the corresponding
/// value found.
crate fn doc_value(&self) -> Option<String> {
self.attrs.doc_value()
}

/// Convenience wrapper around [`Self::from_def_id_and_parts`] which converts
/// `hir_id` to a [`DefId`]
pub fn from_hir_id_and_parts(
Expand Down Expand Up @@ -468,8 +460,8 @@ impl Item {

/// Finds all `doc` attributes as NameValues and returns their corresponding values, joined
/// with newlines.
crate fn collapsed_doc_value(&self) -> Option<String> {
self.attrs.collapsed_doc_value()
crate fn doc_value(&self) -> Option<String> {
self.attrs.doc_value()
}

crate fn links(&self, cx: &Context<'_>) -> Vec<RenderedLink> {
Expand Down Expand Up @@ -952,16 +944,14 @@ fn add_doc_fragment(out: &mut String, frag: &DocFragment) {
}
}

impl<'a> FromIterator<&'a DocFragment> for String {
fn from_iter<T>(iter: T) -> Self
where
T: IntoIterator<Item = &'a DocFragment>,
{
iter.into_iter().fold(String::new(), |mut acc, frag| {
add_doc_fragment(&mut acc, frag);
acc
})
/// Collapse a collection of [`DocFragment`]s into one string,
/// handling indentation and newlines as needed.
crate fn collapse_doc_fragments(doc_strings: &[DocFragment]) -> String {
let mut acc = String::new();
for frag in doc_strings {
add_doc_fragment(&mut acc, frag);
}
acc
}

/// A link that has not yet been rendered.
Expand Down Expand Up @@ -1074,27 +1064,10 @@ impl Attributes {
Attributes { doc_strings, other_attrs }
}

/// Finds the `doc` attribute as a NameValue and returns the corresponding
/// value found.
crate fn doc_value(&self) -> Option<String> {
let mut iter = self.doc_strings.iter();

let ori = iter.next()?;
let mut out = String::new();
add_doc_fragment(&mut out, ori);
for new_frag in iter {
if new_frag.kind != ori.kind || new_frag.parent_module != ori.parent_module {
break;
}
add_doc_fragment(&mut out, new_frag);
}
if out.is_empty() { None } else { Some(out) }
}

/// Return the doc-comments on this item, grouped by the module they came from.
///
/// The module can be different if this is a re-export with added documentation.
crate fn collapsed_doc_value_by_module_level(&self) -> FxHashMap<Option<DefId>, String> {
crate fn doc_value_by_module_level(&self) -> FxHashMap<Option<DefId>, String> {
let mut ret = FxHashMap::default();

for new_frag in self.doc_strings.iter() {
Expand All @@ -1106,8 +1079,12 @@ impl Attributes {

/// Finds all `doc` attributes as NameValues and returns their corresponding values, joined
/// with newlines.
crate fn collapsed_doc_value(&self) -> Option<String> {
if self.doc_strings.is_empty() { None } else { Some(self.doc_strings.iter().collect()) }
crate fn doc_value(&self) -> Option<String> {
Copy link
Member

Choose a reason for hiding this comment

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

not necessarily in this pr, but it would be nice to remove the Option here

Copy link
Member

Choose a reason for hiding this comment

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

I think we check the is_some to generate differently the doc block. From my point of view, this forces us to keep this check so might be better to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really understand why None is treated differently from Some("") though. What if someone writes #[doc = ""]? Won't that return Some("") instead of None? And why do we need to special-case the absence of doc attrs so much?

Copy link
Member

@jyn514 jyn514 Dec 18, 2021

Choose a reason for hiding this comment

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

Another difference between doc_value and collapsed_doc_value - doc_value returns None if the combined string is empty, collapsed_doc_value returns None if there are no lines. The difference matters if there's empty lines, like below:

///
///
///
pub fn foo() {}

Copy link
Member

Choose a reason for hiding this comment

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

@CraftSpider I see the following comment in rustdoc-json-types:

    /// The full markdown docstring of this item. Absent if there is no documentation at all,
    /// Some("") if there is some documentation but it is empty (EG `#[doc = ""]`).
    pub docs: Option<String>,

Is that an important distinction? Or can I just make that field a simple String?

if self.doc_strings.is_empty() {
None
} else {
Some(collapse_doc_fragments(&self.doc_strings))
}
}

crate fn get_doc_aliases(&self) -> Box<[String]> {
Expand Down
8 changes: 1 addition & 7 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,7 @@ crate fn krate(cx: &mut DocContext<'_>) -> Crate {
}));
}

Crate {
module,
externs,
primitives,
external_traits: cx.external_traits.clone(),
collapsed: false,
}
Crate { module, externs, primitives, external_traits: cx.external_traits.clone() }
}

fn external_generic_args(
Expand Down
3 changes: 0 additions & 3 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,9 +512,6 @@ crate fn run_global_ctxt(
let mut cache = ctxt.cache;
krate = tcx.sess.time("create_format_cache", || cache.populate(krate, tcx, &render_options));

// The main crate doc comments are always collapsed.
krate.collapsed = true;

(krate, render_options, cache)
}

Expand Down
4 changes: 1 addition & 3 deletions src/librustdoc/doctest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1137,9 +1137,7 @@ impl<'a, 'hir, 'tcx> HirCollector<'a, 'hir, 'tcx> {
}

attrs.unindent_doc_comments();
// The collapse-docs pass won't combine sugared/raw doc attributes, or included files with
// anything else, this will combine them for us.
if let Some(doc) = attrs.collapsed_doc_value() {
if let Some(doc) = attrs.doc_value() {
// Use the outermost invocation, so that doctest names come from where the docs were written.
let span = ast_attrs
.span()
Expand Down
9 changes: 0 additions & 9 deletions src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ crate struct SharedContext<'tcx> {
crate local_sources: FxHashMap<PathBuf, String>,
/// Show the memory layout of types in the docs.
pub(super) show_type_layout: bool,
/// Whether the collapsed pass ran
collapsed: bool,
/// The base-URL of the issue tracker for when an item has been tagged with
/// an issue number.
pub(super) issue_tracker_base_url: Option<String>,
Expand Down Expand Up @@ -142,12 +140,6 @@ impl SharedContext<'_> {
Ok(())
}

/// Returns the `collapsed_doc_value` of the given item if this is the main crate, otherwise
/// returns the `doc_value`.
crate fn maybe_collapsed_doc_value<'a>(&self, item: &'a clean::Item) -> Option<String> {
if self.collapsed { item.collapsed_doc_value() } else { item.doc_value() }
}

crate fn edition(&self) -> Edition {
self.tcx.sess.edition()
}
Expand Down Expand Up @@ -472,7 +464,6 @@ impl<'tcx> FormatRenderer<'tcx> for Context<'tcx> {
let (sender, receiver) = channel();
let mut scx = SharedContext {
tcx,
collapsed: krate.collapsed,
src_root,
local_sources,
issue_tracker_base_url,
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ fn document_full_inner(
is_collapsible: bool,
heading_offset: HeadingOffset,
) {
if let Some(s) = cx.shared.maybe_collapsed_doc_value(item) {
if let Some(s) = item.doc_value() {
debug!("Doc block: =====\n{}\n=====", s);
if is_collapsible {
w.write_str(
Expand Down Expand Up @@ -1628,7 +1628,7 @@ fn render_impl(
write!(w, "</summary>")
}

if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) {
if let Some(ref dox) = i.impl_item.doc_value() {
let mut ids = cx.id_map.borrow_mut();
write!(
w,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/json/conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl JsonRenderer<'_> {
.flatten()
.map(|clean::ItemLink { link, did, .. }| (link.clone(), from_item_id((*did).into())))
.collect();
let docs = item.attrs.collapsed_doc_value();
let docs = item.attrs.doc_value();
let attrs = item
.attrs
.other_attrs
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/bare_urls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl<'a, 'tcx> DocVisitor for BareUrlsLinter<'a, 'tcx> {
return;
}
};
let dox = item.attrs.collapsed_doc_value().unwrap_or_default();
let dox = item.attrs.doc_value().unwrap_or_default();
if !dox.is_empty() {
let report_diag = |cx: &DocContext<'_>, msg: &str, url: &str, range: Range<usize>| {
let sp = super::source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs)
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/calculate_doc_coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ impl<'a, 'b> DocVisitor for CoverageCalculator<'a, 'b> {
let mut tests = Tests { found_tests: 0 };

find_testable_code(
&i.attrs.collapsed_doc_value().unwrap_or_default(),
&i.attrs.doc_value().unwrap_or_default(),
&mut tests,
ErrorCodes::No,
false,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/check_code_block_syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl<'a, 'tcx> SyntaxChecker<'a, 'tcx> {

impl<'a, 'tcx> DocVisitor for SyntaxChecker<'a, 'tcx> {
fn visit_item(&mut self, item: &clean::Item) {
if let Some(dox) = &item.attrs.collapsed_doc_value() {
if let Some(dox) = &item.attrs.doc_value() {
let sp = item.attr_span(self.cx.tcx);
let extra = crate::html::markdown::ExtraInfo::new_did(
self.cx.tcx,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/check_doc_test_visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ crate fn check_doc_test_visibility(krate: Crate, cx: &mut DocContext<'_>) -> Cra

impl<'a, 'tcx> DocVisitor for DocTestVisibilityLinter<'a, 'tcx> {
fn visit_item(&mut self, item: &Item) {
let dox = item.attrs.collapsed_doc_value().unwrap_or_else(String::new);
let dox = item.attrs.doc_value().unwrap_or_else(String::new);

look_for_tests(self.cx, &dox, &item);

Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@ impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> {
// In the presence of re-exports, this is not the same as the module of the item.
// Rather than merging all documentation into one, resolve it one attribute at a time
// so we know which module it came from.
for (parent_module, doc) in item.attrs.collapsed_doc_value_by_module_level() {
for (parent_module, doc) in item.attrs.doc_value_by_module_level() {
debug!("combined_docs={}", doc);

let (krate, parent_node) = if let Some(id) = parent_module {
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/collect_intra_doc_links/early.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl IntraLinkCrateLoader {

// FIXME: this probably needs to consider inlining
let attrs = crate::clean::Attributes::from_ast(attrs, None);
for (parent_module, doc) in attrs.collapsed_doc_value_by_module_level() {
for (parent_module, doc) in attrs.doc_value_by_module_level() {
debug!(?doc);
for link in markdown_links(doc.as_str()) {
debug!(?link.link);
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/html_tags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ impl<'a, 'tcx> DocVisitor for InvalidHtmlTagsLinter<'a, 'tcx> {
return;
}
};
let dox = item.attrs.collapsed_doc_value().unwrap_or_default();
let dox = item.attrs.doc_value().unwrap_or_default();
if !dox.is_empty() {
let report_diag = |msg: &str, range: &Range<usize>| {
let sp = match super::source_span_for_markdown_range(tcx, &dox, range, &item.attrs)
Expand Down
5 changes: 4 additions & 1 deletion src/librustdoc/passes/unindent_comments/tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use super::*;

use crate::clean::types::collapse_doc_fragments;

use rustc_span::create_default_session_globals_then;
use rustc_span::source_map::DUMMY_SP;
use rustc_span::symbol::Symbol;
Expand All @@ -19,7 +22,7 @@ fn run_test(input: &str, expected: &str) {
create_default_session_globals_then(|| {
let mut s = create_doc_fragment(input);
unindent_fragments(&mut s);
assert_eq!(&s.iter().collect::<String>(), expected);
assert_eq!(collapse_doc_fragments(&s), expected);
});
}

Expand Down