From 0f2571235b4ac60190b9d2dfe745367671c27c0f Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 26 Dec 2020 09:47:11 -0500 Subject: [PATCH] Revert "Cleanup markdown span handling" This caused a diagnostic regression, originally it was: ``` warning: unresolved link to `std::process::Comman` --> link.rs:3:10 | 3 | //! [a]: std::process::Comman | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` but after that PR rustdoc now displays ``` warning: unresolved link to `std::process::Comman` --> link.rs:1:14 | 1 | //! Links to [a] [link][a] | ^^^ no item named `Comman` in module `process` | = note: `#[warn(broken_intra_doc_links)]` on by default ``` which IMO is much less clear. --- src/librustdoc/html/markdown.rs | 126 +++++++++--------- .../passes/collect_intra_doc_links.rs | 62 +++++---- src/test/rustdoc-ui/reference-links.rs | 7 + src/test/rustdoc-ui/reference-links.stderr | 20 +++ 4 files changed, 126 insertions(+), 89 deletions(-) create mode 100644 src/test/rustdoc-ui/reference-links.rs create mode 100644 src/test/rustdoc-ui/reference-links.stderr diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 2ae28dcd0c478..015e8885c1b1b 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -418,7 +418,7 @@ impl<'a, I: Iterator>> Iterator for LinkReplacer<'a, I> { struct HeadingLinks<'a, 'b, 'ids, I> { inner: I, toc: Option<&'b mut TocBuilder>, - buf: VecDeque<(Event<'a>, Range)>, + buf: VecDeque>, id_map: &'ids mut IdMap, } @@ -428,10 +428,8 @@ impl<'a, 'b, 'ids, I> HeadingLinks<'a, 'b, 'ids, I> { } } -impl<'a, 'b, 'ids, I: Iterator, Range)>> Iterator - for HeadingLinks<'a, 'b, 'ids, I> -{ - type Item = (Event<'a>, Range); +impl<'a, 'b, 'ids, I: Iterator>> Iterator for HeadingLinks<'a, 'b, 'ids, I> { + type Item = Event<'a>; fn next(&mut self) -> Option { if let Some(e) = self.buf.pop_front() { @@ -439,29 +437,31 @@ impl<'a, 'b, 'ids, I: Iterator, Range)>> Iterator } let event = self.inner.next(); - if let Some((Event::Start(Tag::Heading(level)), _)) = event { + if let Some(Event::Start(Tag::Heading(level))) = event { let mut id = String::new(); for event in &mut self.inner { - match &event.0 { + match &event { Event::End(Tag::Heading(..)) => break, - Event::Start(Tag::Link(_, _, _)) | Event::End(Tag::Link(..)) => {} Event::Text(text) | Event::Code(text) => { id.extend(text.chars().filter_map(slugify)); - self.buf.push_back(event); } - _ => self.buf.push_back(event), + _ => {} + } + match event { + Event::Start(Tag::Link(_, _, _)) | Event::End(Tag::Link(..)) => {} + event => self.buf.push_back(event), } } let id = self.id_map.derive(id); if let Some(ref mut builder) = self.toc { let mut html_header = String::new(); - html::push_html(&mut html_header, self.buf.iter().map(|(ev, _)| ev.clone())); + html::push_html(&mut html_header, self.buf.iter().cloned()); let sec = builder.push(level as u32, html_header, id.clone()); - self.buf.push_front((Event::Html(format!("{} ", sec).into()), 0..0)); + self.buf.push_front(Event::Html(format!("{} ", sec).into())); } - self.buf.push_back((Event::Html(format!("", level).into()), 0..0)); + self.buf.push_back(Event::Html(format!("", level).into())); let start_tags = format!( "\ @@ -469,7 +469,7 @@ impl<'a, 'b, 'ids, I: Iterator, Range)>> Iterator id = id, level = level ); - return Some((Event::Html(start_tags.into()), 0..0)); + return Some(Event::Html(start_tags.into())); } event } @@ -560,23 +560,23 @@ impl<'a, I> Footnotes<'a, I> { } } -impl<'a, I: Iterator, Range)>> Iterator for Footnotes<'a, I> { - type Item = (Event<'a>, Range); +impl<'a, I: Iterator>> Iterator for Footnotes<'a, I> { + type Item = Event<'a>; fn next(&mut self) -> Option { loop { match self.inner.next() { - Some((Event::FootnoteReference(ref reference), range)) => { + Some(Event::FootnoteReference(ref reference)) => { let entry = self.get_entry(&reference); let reference = format!( "{0}", (*entry).1 ); - return Some((Event::Html(reference.into()), range)); + return Some(Event::Html(reference.into())); } - Some((Event::Start(Tag::FootnoteDefinition(def)), _)) => { + Some(Event::Start(Tag::FootnoteDefinition(def))) => { let mut content = Vec::new(); - for (event, _) in &mut self.inner { + for event in &mut self.inner { if let Event::End(Tag::FootnoteDefinition(..)) = event { break; } @@ -607,7 +607,7 @@ impl<'a, I: Iterator, Range)>> Iterator for Footnotes<' ret.push_str(""); } ret.push_str(""); - return Some((Event::Html(ret.into()), 0..0)); + return Some(Event::Html(ret.into())); } else { return None; } @@ -917,14 +917,13 @@ impl Markdown<'_> { }; let p = Parser::new_with_broken_link_callback(md, opts(), Some(&mut replacer)); - let p = p.into_offset_iter(); let mut s = String::with_capacity(md.len() * 3 / 2); let p = HeadingLinks::new(p, None, &mut ids); - let p = Footnotes::new(p); - let p = LinkReplacer::new(p.map(|(ev, _)| ev), links); + let p = LinkReplacer::new(p, links); let p = CodeBlocks::new(p, codes, edition, playground); + let p = Footnotes::new(p); html::push_html(&mut s, p); s @@ -935,7 +934,7 @@ impl MarkdownWithToc<'_> { crate fn into_string(self) -> String { let MarkdownWithToc(md, mut ids, codes, edition, playground) = self; - let p = Parser::new_ext(md, opts()).into_offset_iter(); + let p = Parser::new_ext(md, opts()); let mut s = String::with_capacity(md.len() * 3 / 2); @@ -943,8 +942,8 @@ impl MarkdownWithToc<'_> { { let p = HeadingLinks::new(p, Some(&mut toc), &mut ids); + let p = CodeBlocks::new(p, codes, edition, playground); let p = Footnotes::new(p); - let p = CodeBlocks::new(p.map(|(ev, _)| ev), codes, edition, playground); html::push_html(&mut s, p); } @@ -960,19 +959,19 @@ impl MarkdownHtml<'_> { if md.is_empty() { return String::new(); } - let p = Parser::new_ext(md, opts()).into_offset_iter(); + let p = Parser::new_ext(md, opts()); // Treat inline HTML as plain text. - let p = p.map(|event| match event.0 { - Event::Html(text) => (Event::Text(text), event.1), + let p = p.map(|event| match event { + Event::Html(text) => Event::Text(text), _ => event, }); let mut s = String::with_capacity(md.len() * 3 / 2); let p = HeadingLinks::new(p, None, &mut ids); + let p = CodeBlocks::new(p, codes, edition, playground); let p = Footnotes::new(p); - let p = CodeBlocks::new(p.map(|(ev, _)| ev), codes, edition, playground); html::push_html(&mut s, p); s @@ -1125,45 +1124,50 @@ crate fn plain_text_summary(md: &str) -> String { s } -crate fn markdown_links(md: &str) -> Vec<(String, Range)> { +crate fn markdown_links(md: &str) -> Vec<(String, Option>)> { if md.is_empty() { return vec![]; } let mut links = vec![]; - // Used to avoid mutable borrow issues in the `push` closure - // Probably it would be more efficient to use a `RefCell` but it doesn't seem worth the churn. let mut shortcut_links = vec![]; - let span_for_link = |link: &str, span: Range| { - // Pulldown includes the `[]` as well as the URL. Only highlight the relevant span. - // NOTE: uses `rfind` in case the title and url are the same: `[Ok][Ok]` - match md[span.clone()].rfind(link) { - Some(start) => { - let start = span.start + start; - start..start + link.len() + { + let locate = |s: &str| unsafe { + let s_start = s.as_ptr(); + let s_end = s_start.add(s.len()); + let md_start = md.as_ptr(); + let md_end = md_start.add(md.len()); + if md_start <= s_start && s_end <= md_end { + let start = s_start.offset_from(md_start) as usize; + let end = s_end.offset_from(md_start) as usize; + Some(start..end) + } else { + None + } + }; + + let mut push = |link: BrokenLink<'_>| { + // FIXME: use `link.span` instead of `locate` + // (doing it now includes the `[]` as well as the text) + shortcut_links.push((link.reference.to_owned(), locate(link.reference))); + None + }; + let p = Parser::new_with_broken_link_callback(md, opts(), Some(&mut push)); + + // There's no need to thread an IdMap through to here because + // the IDs generated aren't going to be emitted anywhere. + let mut ids = IdMap::new(); + let iter = Footnotes::new(HeadingLinks::new(p, None, &mut ids)); + + for ev in iter { + if let Event::Start(Tag::Link(_, dest, _)) = ev { + debug!("found link: {}", dest); + links.push(match dest { + CowStr::Borrowed(s) => (s.to_owned(), locate(s)), + s @ (CowStr::Boxed(..) | CowStr::Inlined(..)) => (s.into_string(), None), + }); } - // This can happen for things other than intra-doc links, like `#1` expanded to `https://github.com/rust-lang/rust/issues/1`. - None => span, - } - }; - let mut push = |link: BrokenLink<'_>| { - let span = span_for_link(link.reference, link.span); - shortcut_links.push((link.reference.to_owned(), span)); - None - }; - let p = Parser::new_with_broken_link_callback(md, opts(), Some(&mut push)); - - // There's no need to thread an IdMap through to here because - // the IDs generated aren't going to be emitted anywhere. - let mut ids = IdMap::new(); - let iter = Footnotes::new(HeadingLinks::new(p.into_offset_iter(), None, &mut ids)); - - for ev in iter { - if let Event::Start(Tag::Link(_, dest, _)) = ev.0 { - debug!("found link: {}", dest); - let span = span_for_link(&dest, ev.1); - links.push((dest.into_string(), span)); } } diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index a8adfe08b2561..ea5bf94689bc7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -180,7 +180,7 @@ struct DiagnosticInfo<'a> { item: &'a Item, dox: &'a str, ori_link: &'a str, - link_range: Range, + link_range: Option>, } #[derive(Clone, Debug, Hash)] @@ -920,7 +920,7 @@ impl LinkCollector<'_, '_> { parent_node: Option, krate: CrateNum, ori_link: String, - link_range: Range, + link_range: Option>, ) -> Option { trace!("considering link '{}'", ori_link); @@ -1566,7 +1566,7 @@ fn report_diagnostic( msg: &str, item: &Item, dox: &str, - link_range: &Range, + link_range: &Option>, decorate: impl FnOnce(&mut DiagnosticBuilder<'_>, Option), ) { let hir_id = match cx.as_local_hir_id(item.def_id) { @@ -1584,26 +1584,31 @@ fn report_diagnostic( cx.tcx.struct_span_lint_hir(lint, hir_id, sp, |lint| { let mut diag = lint.build(msg); - let span = super::source_span_for_markdown_range(cx, dox, link_range, attrs); - if let Some(sp) = span { - diag.set_span(sp); - } else { - // blah blah blah\nblah\nblah [blah] blah blah\nblah blah - // ^ ~~~~ - // | link_range - // last_new_line_offset - let last_new_line_offset = dox[..link_range.start].rfind('\n').map_or(0, |n| n + 1); - let line = dox[last_new_line_offset..].lines().next().unwrap_or(""); - - // Print the line containing the `link_range` and manually mark it with '^'s. - diag.note(&format!( - "the link appears in this line:\n\n{line}\n\ - {indicator: , dox: &str, - link_range: Range, + link_range: Option>, kinds: SmallVec<[ResolutionFailure<'_>; 3]>, ) { report_diagnostic( @@ -1857,7 +1862,7 @@ fn anchor_failure( item: &Item, path_str: &str, dox: &str, - link_range: Range, + link_range: Option>, failure: AnchorFailure, ) { let msg = match failure { @@ -1882,7 +1887,7 @@ fn ambiguity_error( item: &Item, path_str: &str, dox: &str, - link_range: Range, + link_range: Option>, candidates: Vec, ) { let mut msg = format!("`{}` is ", path_str); @@ -1931,12 +1936,13 @@ fn suggest_disambiguator( path_str: &str, dox: &str, sp: Option, - link_range: &Range, + link_range: &Option>, ) { let suggestion = disambiguator.suggestion(); let help = format!("to link to the {}, {}", disambiguator.descr(), suggestion.descr()); if let Some(sp) = sp { + let link_range = link_range.as_ref().expect("must have a link range if we have a span"); let msg = if dox.bytes().nth(link_range.start) == Some(b'`') { format!("`{}`", suggestion.as_help(path_str)) } else { @@ -1955,7 +1961,7 @@ fn privacy_error( item: &Item, path_str: &str, dox: &str, - link_range: Range, + link_range: Option>, ) { let sym; let item_name = match item.name { diff --git a/src/test/rustdoc-ui/reference-links.rs b/src/test/rustdoc-ui/reference-links.rs new file mode 100644 index 0000000000000..7c1a79722c993 --- /dev/null +++ b/src/test/rustdoc-ui/reference-links.rs @@ -0,0 +1,7 @@ +// Test that errors point to the reference, not to the title text. +#![deny(broken_intra_doc_links)] +//! Links to [a] [link][a] +//! +//! [a]: std::process::Comman +//~^ ERROR unresolved +//~| ERROR unresolved diff --git a/src/test/rustdoc-ui/reference-links.stderr b/src/test/rustdoc-ui/reference-links.stderr new file mode 100644 index 0000000000000..6ba73fbdb006d --- /dev/null +++ b/src/test/rustdoc-ui/reference-links.stderr @@ -0,0 +1,20 @@ +error: unresolved link to `std::process::Comman` + --> $DIR/reference-links.rs:5:10 + | +LL | //! [a]: std::process::Comman + | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` + | +note: the lint level is defined here + --> $DIR/reference-links.rs:2:9 + | +LL | #![deny(broken_intra_doc_links)] + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: unresolved link to `std::process::Comman` + --> $DIR/reference-links.rs:5:10 + | +LL | //! [a]: std::process::Comman + | ^^^^^^^^^^^^^^^^^^^^ no item named `Comman` in module `process` + +error: aborting due to 2 previous errors +