From edb73eb0f9070c7c8a77cf89de88019d4b2a6739 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 18 Dec 2021 11:05:14 -0600 Subject: [PATCH 1/2] Return a `String` from `collapsed_doc_value`, not an Option This has almost no effect in practice, since most places were using `unwrap_or_default`, and the rest were doing checks that *should* have been checking for an empty string. The only change in behavior is that the JSON backend no longer distinguishes "undocumented" and "documented with the empty string". This doesn't seem like a particularly useful distinction, but I can add it back for that code only if you think it's important. --- src/librustdoc/clean/types.rs | 27 +++++++------------ src/librustdoc/doctest.rs | 8 +++--- src/librustdoc/html/render/mod.rs | 13 +++++---- src/librustdoc/passes/bare_urls.rs | 2 +- .../passes/calculate_doc_coverage.rs | 2 +- .../passes/check_code_block_syntax.rs | 3 ++- .../passes/check_doc_test_visibility.rs | 4 +-- src/librustdoc/passes/html_tags.rs | 2 +- .../passes/petrochenkov-suggestions.txt | 3 +++ src/rustdoc-json-types/lib.rs | 7 +++-- 10 files changed, 34 insertions(+), 37 deletions(-) create mode 100644 src/librustdoc/passes/petrochenkov-suggestions.txt diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index 24e18bb3a51f4..1832a5e801a2f 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -468,7 +468,7 @@ impl Item { /// Finds all `doc` attributes as NameValues and returns their corresponding values, joined /// with newlines. - crate fn collapsed_doc_value(&self) -> Option { + crate fn collapsed_doc_value(&self) -> String { self.attrs.collapsed_doc_value() } @@ -956,16 +956,6 @@ fn add_doc_fragment(out: &mut String, frag: &DocFragment) { } } -/// 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. /// /// This link will be turned into a rendered link by [`Item::links`]. @@ -1104,12 +1094,15 @@ impl Attributes { /// Finds all `doc` attributes as NameValues and returns their corresponding values, joined /// with newlines. - crate fn collapsed_doc_value(&self) -> Option { - if self.doc_strings.is_empty() { - None - } else { - Some(collapse_doc_fragments(&self.doc_strings)) - } + /// + /// Unlike `doc_value`, this does not stop processing lines when switching between `#[doc]` and + /// sugared syntax. + crate fn collapsed_doc_value(&self) -> String { + let mut acc = String::new(); + for frag in &self.doc_strings { + add_doc_fragment(&mut acc, frag); + } + acc } crate fn get_doc_aliases(&self) -> Box<[Symbol]> { diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index ac24543929b66..f16f725256ab9 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -1123,9 +1123,9 @@ 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() { + // `doc_value()` doesn't combine sugared/raw doc attributes, this will combine them for us. + let docs = attrs.collapsed_doc_value(); + if !docs.is_empty() { // Use the outermost invocation, so that doctest names come from where the docs were written. let span = ast_attrs .span() @@ -1133,7 +1133,7 @@ impl<'a, 'hir, 'tcx> HirCollector<'a, 'hir, 'tcx> { .unwrap_or(DUMMY_SP); self.collector.set_position(span); markdown::find_testable_code( - &doc, + &docs, self.collector, self.codes, self.collector.enable_per_target_ignores, diff --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index eb606178d244d..2aa346bc22a54 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/src/librustdoc/html/render/mod.rs @@ -567,8 +567,10 @@ fn document_full_inner( is_collapsible: bool, heading_offset: HeadingOffset, ) { - if let Some(s) = item.collapsed_doc_value() { - debug!("Doc block: =====\n{}\n=====", s); + // NOTE: "collapsed" here is unrelated to `is_collapsible`, it just combines `#[doc]` and sugared attributes + let docs = item.collapsed_doc_value(); + if !docs.is_empty() { + debug!("Doc block: =====\n{}\n=====", docs); if is_collapsible { w.write_str( "
\ @@ -576,10 +578,10 @@ fn document_full_inner( Expand description\ ", ); - render_markdown(w, cx, &s, item.links(cx), heading_offset); + render_markdown(w, cx, &docs, item.links(cx), heading_offset); w.write_str("
"); } else { - render_markdown(w, cx, &s, item.links(cx), heading_offset); + render_markdown(w, cx, &docs, item.links(cx), heading_offset); } } @@ -1612,7 +1614,8 @@ fn render_impl( write!(w, "") } - if let Some(ref dox) = i.impl_item.collapsed_doc_value() { + let dox = i.impl_item.collapsed_doc_value(); + if !dox.is_empty() { let mut ids = cx.id_map.borrow_mut(); write!( w, diff --git a/src/librustdoc/passes/bare_urls.rs b/src/librustdoc/passes/bare_urls.rs index 3410f46e2a8d2..d88b06b7ef63e 100644 --- a/src/librustdoc/passes/bare_urls.rs +++ b/src/librustdoc/passes/bare_urls.rs @@ -68,7 +68,7 @@ impl<'a, 'tcx> DocVisitor for BareUrlsLinter<'a, 'tcx> { return; } }; - let dox = item.attrs.collapsed_doc_value().unwrap_or_default(); + let dox = item.attrs.collapsed_doc_value(); if !dox.is_empty() { let report_diag = |cx: &DocContext<'_>, msg: &str, url: &str, range: Range| { let sp = super::source_span_for_markdown_range(cx.tcx, &dox, &range, &item.attrs) diff --git a/src/librustdoc/passes/calculate_doc_coverage.rs b/src/librustdoc/passes/calculate_doc_coverage.rs index 6111c982de922..49d01854cff33 100644 --- a/src/librustdoc/passes/calculate_doc_coverage.rs +++ b/src/librustdoc/passes/calculate_doc_coverage.rs @@ -208,7 +208,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.collapsed_doc_value(), &mut tests, ErrorCodes::No, false, diff --git a/src/librustdoc/passes/check_code_block_syntax.rs b/src/librustdoc/passes/check_code_block_syntax.rs index a50bf558bf3d1..7d7e354a87b5e 100644 --- a/src/librustdoc/passes/check_code_block_syntax.rs +++ b/src/librustdoc/passes/check_code_block_syntax.rs @@ -145,7 +145,8 @@ 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() { + let dox = &item.attrs.collapsed_doc_value(); + if !dox.is_empty() { let sp = item.attr_span(self.cx.tcx); let extra = crate::html::markdown::ExtraInfo::new_did( self.cx.tcx, diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs index b86ec8abefaae..013ee65ef9caf 100644 --- a/src/librustdoc/passes/check_doc_test_visibility.rs +++ b/src/librustdoc/passes/check_doc_test_visibility.rs @@ -35,9 +35,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); - - look_for_tests(self.cx, &dox, &item); + look_for_tests(self.cx, &item.attrs.collapsed_doc_value(), &item); self.visit_item_recur(item) } diff --git a/src/librustdoc/passes/html_tags.rs b/src/librustdoc/passes/html_tags.rs index f7a9a0899e390..9c702d1c7190b 100644 --- a/src/librustdoc/passes/html_tags.rs +++ b/src/librustdoc/passes/html_tags.rs @@ -176,7 +176,7 @@ impl<'a, 'tcx> DocVisitor for InvalidHtmlTagsLinter<'a, 'tcx> { return; } }; - let dox = item.attrs.collapsed_doc_value().unwrap_or_default(); + let dox = item.attrs.collapsed_doc_value(); if !dox.is_empty() { let report_diag = |msg: &str, range: &Range| { let sp = match super::source_span_for_markdown_range(tcx, &dox, range, &item.attrs) diff --git a/src/librustdoc/passes/petrochenkov-suggestions.txt b/src/librustdoc/passes/petrochenkov-suggestions.txt new file mode 100644 index 0000000000000..698f0780761c6 --- /dev/null +++ b/src/librustdoc/passes/petrochenkov-suggestions.txt @@ -0,0 +1,3 @@ +- use rustc's map instead of separate traits in scope +- have a map from def id + link index to resolved DefId +- need to have all 3 namespaces in case the link doesn't resolve diff --git a/src/rustdoc-json-types/lib.rs b/src/rustdoc-json-types/lib.rs index 9466f84ffcd59..586de245535ad 100644 --- a/src/rustdoc-json-types/lib.rs +++ b/src/rustdoc-json-types/lib.rs @@ -68,9 +68,8 @@ pub struct Item { /// By default all documented items are public, but you can tell rustdoc to output private items /// so this field is needed to differentiate. pub visibility: Visibility, - /// 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, + /// The full markdown docstring of this item. + pub docs: String, /// This mapping resolves [intra-doc links](https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md) from the docstring to their IDs pub links: HashMap, /// Stringified versions of the attributes on this item (e.g. `"#[inline]"`) @@ -511,7 +510,7 @@ pub struct Static { } /// rustdoc format-version. -pub const FORMAT_VERSION: u32 = 9; +pub const FORMAT_VERSION: u32 = 10; #[cfg(test)] mod tests; From 112ed0bfc552e19458f2a9a2698070bbe7525865 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Mon, 20 Dec 2021 10:12:56 -0600 Subject: [PATCH 2/2] Add test for json output This also fixes a few bugs in jsondocck and makes some of its errors easier to read. --- src/rustdoc-json-types/lib.rs | 4 ++++ src/test/rustdoc-json/empty-lines.rs | 12 ++++++++++++ src/tools/compiletest/src/runtest.rs | 6 +++--- src/tools/jsondocck/src/main.rs | 9 +++++++-- 4 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 src/test/rustdoc-json/empty-lines.rs diff --git a/src/rustdoc-json-types/lib.rs b/src/rustdoc-json-types/lib.rs index 586de245535ad..33e9a3537efc4 100644 --- a/src/rustdoc-json-types/lib.rs +++ b/src/rustdoc-json-types/lib.rs @@ -69,6 +69,10 @@ pub struct Item { /// so this field is needed to differentiate. pub visibility: Visibility, /// The full markdown docstring of this item. + /// + /// This field has already been pre-parsed, including combining lines and de-indenting + /// whitespace. Each line is delimited by a newline, but the docs will not end with a trailing + /// newline (unless added explicitly by the user with `#[doc = "\n"]`). pub docs: String, /// This mapping resolves [intra-doc links](https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md) from the docstring to their IDs pub links: HashMap, diff --git a/src/test/rustdoc-json/empty-lines.rs b/src/test/rustdoc-json/empty-lines.rs new file mode 100644 index 0000000000000..4463e8e83ff75 --- /dev/null +++ b/src/test/rustdoc-json/empty-lines.rs @@ -0,0 +1,12 @@ +// @has empty_lines.json "$.index[*][?(@.name == 'foo')]" +// @has - "$.index[*][?(@.name == 'foo')].docs" \"\\n\\n\" +/// +/// +/// +// ^ note that the above line does *not* include a trailing new line in the docs +pub fn foo() {} + +// @has empty_lines.json "$.index[*][?(@.name == 'bar')].docs" "\"first line\\nsecond line \"" +#[doc = "\n first line"] +#[doc = "\n second line \n"] +pub fn bar() {} diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index f039ba59d231c..d410c380540c9 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -2401,8 +2401,6 @@ impl<'test> TestCx<'test> { } let root = self.config.find_rust_src_root().unwrap(); - let mut json_out = out_dir.join(self.testpaths.file.file_stem().unwrap()); - json_out.set_extension("json"); let res = self.cmd2procres( Command::new(self.config.jsondocck_path.as_ref().unwrap()) .arg("--doc-dir") @@ -2415,7 +2413,9 @@ impl<'test> TestCx<'test> { self.fatal_proc_rec("jsondocck failed!", &res) } - let mut json_out = out_dir.join(self.testpaths.file.file_stem().unwrap()); + let crate_name = + self.testpaths.file.file_stem().unwrap().to_str().unwrap().replace('-', "_"); + let mut json_out = out_dir.join(crate_name); json_out.set_extension("json"); let res = self.cmd2procres( Command::new(&self.config.docck_python) diff --git a/src/tools/jsondocck/src/main.rs b/src/tools/jsondocck/src/main.rs index b8ea10f3d2277..3fc8100f1c100 100644 --- a/src/tools/jsondocck/src/main.rs +++ b/src/tools/jsondocck/src/main.rs @@ -62,7 +62,10 @@ impl CommandKind { }; if !count { - print_err(&format!("Incorrect number of arguments to `@{}`", self), lineno); + print_err( + &format!("Incorrect number of arguments to `@{}` (got {})", self, args.len()), + lineno, + ); return false; } @@ -317,6 +320,8 @@ fn string_to_value<'a>(s: &str, cache: &'a Cache) -> Cow<'a, Value> { panic!("No variable: `{}`. Current state: `{:?}`", &s[1..], cache.variables) })) } else { - Cow::Owned(serde_json::from_str(s).unwrap()) + Cow::Owned(serde_json::from_str(s).unwrap_or_else(|err| { + panic!("failed to parse {:?} as a string: {}", s, err); + })) } }