Revert "Cleanup markdown span handling"

Joshua Nelson · jyn514 · commit 0f2571235b4a · 2020-12-26T12:11:44.000-05:00
This caused a diagnostic regression, originally it was:

```
warning: unresolved link to `std::process::Comman`
 --&gt; 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`
 --&gt; 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.
diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs
@@ -418,7 +418,7 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for LinkReplacer<'a, I> {
 struct HeadingLinks<'a, 'b, 'ids, I> {
     inner: I,
     toc: Option<&'b mut TocBuilder>,
-    buf: VecDeque<(Event<'a>, Range<usize>)>,
+    buf: VecDeque<Event<'a>>,
     id_map: &'ids mut IdMap,
 }
 
@@ -428,48 +428,48 @@ impl<'a, 'b, 'ids, I> HeadingLinks<'a, 'b, 'ids, I> {
     }
 }
 
-impl<'a, 'b, 'ids, I: Iterator<Item = (Event<'a>, Range<usize>)>> Iterator
-    for HeadingLinks<'a, 'b, 'ids, I>
-{
-    type Item = (Event<'a>, Range<usize>);
+impl<'a, 'b, 'ids, I: Iterator<Item = Event<'a>>> Iterator for HeadingLinks<'a, 'b, 'ids, I> {
+    type Item = Event<'a>;
 
     fn next(&mut self) -> Option<Self::Item> {
         if let Some(e) = self.buf.pop_front() {
             return Some(e);
         }
 
         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!("</a></h{}>", level).into()), 0..0));
+            self.buf.push_back(Event::Html(format!("</a></h{}>", level).into()));
 
             let start_tags = format!(
                 "<h{level} id=\"{id}\" class=\"section-header\">\
                     <a href=\"#{id}\">",
                 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<Item = (Event<'a>, Range<usize>)>> Iterator for Footnotes<'a, I> {
-    type Item = (Event<'a>, Range<usize>);
+impl<'a, I: Iterator<Item = Event<'a>>> Iterator for Footnotes<'a, I> {
+    type Item = Event<'a>;
 
     fn next(&mut self) -> Option<Self::Item> {
         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!(
                         "<sup id=\"fnref{0}\"><a href=\"#fn{0}\">{0}</a></sup>",
                         (*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<Item = (Event<'a>, Range<usize>)>> Iterator for Footnotes<'
                             ret.push_str("</li>");
                         }
                         ret.push_str("</ol></div>");
-                        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,16 +934,16 @@ 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);
 
         let mut toc = TocBuilder::new();
 
         {
             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<usize>)> {
+crate fn markdown_links(md: &str) -> Vec<(String, Option<Range<usize>>)> {
     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<usize>| {
-        // 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
@@ -180,7 +180,7 @@ struct DiagnosticInfo<'a> {
     item: &'a Item,
     dox: &'a str,
     ori_link: &'a str,
-    link_range: Range<usize>,
+    link_range: Option<Range<usize>>,
 }
 
 #[derive(Clone, Debug, Hash)]
@@ -920,7 +920,7 @@ impl LinkCollector<'_, '_> {
         parent_node: Option<DefId>,
         krate: CrateNum,
         ori_link: String,
-        link_range: Range<usize>,
+        link_range: Option<Range<usize>>,
     ) -> Option<ItemLink> {
         trace!("considering link '{}'", ori_link);
 
@@ -1566,7 +1566,7 @@ fn report_diagnostic(
     msg: &str,
     item: &Item,
     dox: &str,
-    link_range: &Range<usize>,
+    link_range: &Option<Range<usize>>,
     decorate: impl FnOnce(&mut DiagnosticBuilder<'_>, Option<rustc_span::Span>),
 ) {
     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: <before$}{indicator:^<found$}",
-                line = line,
-                indicator = "",
-                before = link_range.start - last_new_line_offset,
-                found = link_range.len(),
-            ));
+        let span = link_range
+            .as_ref()
+            .and_then(|range| super::source_span_for_markdown_range(cx, dox, range, attrs));
+
+        if let Some(link_range) = link_range {
+            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: <before$}{indicator:^<found$}",
+                    line = line,
+                    indicator = "",
+                    before = link_range.start - last_new_line_offset,
+                    found = link_range.len(),
+                ));
+            }
         }
 
         decorate(&mut diag, span);
@@ -1623,7 +1628,7 @@ fn resolution_failure(
     path_str: &str,
     disambiguator: Option<Disambiguator>,
     dox: &str,
-    link_range: Range<usize>,
+    link_range: Option<Range<usize>>,
     kinds: SmallVec<[ResolutionFailure<'_>; 3]>,
 ) {
     report_diagnostic(
@@ -1857,7 +1862,7 @@ fn anchor_failure(
     item: &Item,
     path_str: &str,
     dox: &str,
-    link_range: Range<usize>,
+    link_range: Option<Range<usize>>,
     failure: AnchorFailure,
 ) {
     let msg = match failure {
@@ -1882,7 +1887,7 @@ fn ambiguity_error(
     item: &Item,
     path_str: &str,
     dox: &str,
-    link_range: Range<usize>,
+    link_range: Option<Range<usize>>,
     candidates: Vec<Res>,
 ) {
     let mut msg = format!("`{}` is ", path_str);
@@ -1931,12 +1936,13 @@ fn suggest_disambiguator(
     path_str: &str,
     dox: &str,
     sp: Option<rustc_span::Span>,
-    link_range: &Range<usize>,
+    link_range: &Option<Range<usize>>,
 ) {
     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<usize>,
+    link_range: Option<Range<usize>>,
 ) {
     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
@@ -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
