Rollup merge of rust-lang#136400 - lolbinarycat:rustdoc-link-lint-135851, r=GuillaumeGomez

Improve handling of rustdoc lints when used with raw doc fragments.

1. `rustdoc::bare_urls` no longer outputs incoherent suggestions if `source_span_for_markdown_range` returns None, instead outputting no suggestion
2. `source_span_for_markdown_range` has one more heuristic, so it will return `None` less often.
3. add ui test to make sure we don't emit nonsense suggestions.

fixes rust-lang#135851

Author: matthiaskrgr

compiler/rustc_resolve/src/rustdoc.rs

@@ -514,20 +514,30 @@ pub fn span_of_fragments(fragments: &[DocFragment]) -> Option<Span> {
 /// This method does not always work, because markdown bytes don't necessarily match source bytes,
 /// like if escapes are used in the string. In this case, it returns `None`.
 ///
-/// This method will return `Some` only if:
+/// `markdown` is typically the entire documentation for an item,
+/// after combining fragments.
+///
+/// This method will return `Some` only if one of the following is true:
 ///
 /// - The doc is made entirely from sugared doc comments, which cannot contain escapes
-/// - The doc is entirely from a single doc fragment, with a string literal, exactly equal
+/// - The doc is entirely from a single doc fragment with a string literal exactly equal to `markdown`.
 /// - The doc comes from `include_str!`
+/// - The doc includes exactly one substring matching `markdown[md_range]` which is contained in a single doc fragment.
+///
+/// This function is defined in the compiler so it can be used by
+/// both `rustdoc` and `clippy`.
 pub fn source_span_for_markdown_range(
     tcx: TyCtxt<'_>,
     markdown: &str,
     md_range: &Range<usize>,
     fragments: &[DocFragment],
 ) -> Option<Span> {
+    use rustc_span::BytePos;
+
+    let map = tcx.sess.source_map();
     if let &[fragment] = &fragments
         && fragment.kind == DocFragmentKind::RawDoc
-        && let Ok(snippet) = tcx.sess.source_map().span_to_snippet(fragment.span)
+        && let Ok(snippet) = map.span_to_snippet(fragment.span)
         && snippet.trim_end() == markdown.trim_end()
         && let Ok(md_range_lo) = u32::try_from(md_range.start)
         && let Ok(md_range_hi) = u32::try_from(md_range.end)
@@ -544,10 +554,43 @@ pub fn source_span_for_markdown_range(
     let is_all_sugared_doc = fragments.iter().all(|frag| frag.kind == DocFragmentKind::SugaredDoc);
 
     if !is_all_sugared_doc {
+        // This case ignores the markdown outside of the range so that it can
+        // work in cases where the markdown is made from several different
+        // doc fragments, but the target range does not span across multiple
+        // fragments.
+        let mut match_data = None;
+        let pat = &markdown[md_range.clone()];
+        // This heirustic doesn't make sense with a zero-sized range.
+        if pat.is_empty() {
+            return None;
+        }
+        for (i, fragment) in fragments.iter().enumerate() {
+            if let Ok(snippet) = map.span_to_snippet(fragment.span)
+                && let Some(match_start) = snippet.find(pat)
+            {
+                // If there is either a match in a previous fragment, or
+                // multiple matches in this fragment, there is ambiguity.
+                if match_data.is_none() && !snippet[match_start + 1..].contains(pat) {
+                    match_data = Some((i, match_start));
+                } else {
+                    // Heirustic produced ambiguity, return nothing.
+                    return None;
+                }
+            }
+        }
+        if let Some((i, match_start)) = match_data {
+            let sp = fragments[i].span;
+            // we need to calculate the span start,
+            // then use that in our calulations for the span end
+            let lo = sp.lo() + BytePos(match_start as u32);
+            return Some(
+                sp.with_lo(lo).with_hi(lo + BytePos((md_range.end - md_range.start) as u32)),
+            );
+        }
         return None;
     }
 
-    let snippet = tcx.sess.source_map().span_to_snippet(span_of_fragments(fragments)?).ok()?;
+    let snippet = map.span_to_snippet(span_of_fragments(fragments)?).ok()?;
 
     let starting_line = markdown[..md_range.start].matches('\n').count();
     let ending_line = starting_line + markdown[md_range.start..md_range.end].matches('\n').count();

src/librustdoc/passes/lint/bare_urls.rs

@@ -18,19 +18,23 @@ use crate::html::markdown::main_body_opts;
 
 pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: &str) {
     let report_diag = |cx: &DocContext<'_>, msg: &'static str, range: Range<usize>| {
-        let sp = source_span_for_markdown_range(cx.tcx, dox, &range, &item.attrs.doc_strings)
-            .unwrap_or_else(|| item.attr_span(cx.tcx));
+        let maybe_sp = source_span_for_markdown_range(cx.tcx, dox, &range, &item.attrs.doc_strings);
+        let sp = maybe_sp.unwrap_or_else(|| item.attr_span(cx.tcx));
         cx.tcx.node_span_lint(crate::lint::BARE_URLS, hir_id, sp, |lint| {
             lint.primary_message(msg)
-                .note("bare URLs are not automatically turned into clickable links")
-                .multipart_suggestion(
+                .note("bare URLs are not automatically turned into clickable links");
+            // The fallback of using the attribute span is suitable for
+            // highlighting where the error is, but not for placing the < and >
+            if let Some(sp) = maybe_sp {
+                lint.multipart_suggestion(
                     "use an automatic link instead",
                     vec![
                         (sp.shrink_to_lo(), "<".to_string()),
                         (sp.shrink_to_hi(), ">".to_string()),
                     ],
                     Applicability::MachineApplicable,
                 );
+            }
         });
     };
 

tests/rustdoc-ui/intra-doc/warning.stderr

@@ -69,29 +69,19 @@ LL | bar [BarC] bar
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
 
 warning: unresolved link to `BarD`
-  --> $DIR/warning.rs:45:9
+  --> $DIR/warning.rs:45:20
    |
 LL | #[doc = "Foo\nbar [BarD] bar\nbaz"]
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                    ^^^^ no item named `BarD` in scope
    |
-   = note: the link appears in this line:
-           
-           bar [BarD] bar
-                ^^^^
-   = note: no item named `BarD` in scope
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
 
 warning: unresolved link to `BarF`
-  --> $DIR/warning.rs:54:4
+  --> $DIR/warning.rs:54:15
    |
 LL | f!("Foo\nbar [BarF] bar\nbaz");
-   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |               ^^^^ no item named `BarF` in scope
    |
-   = note: the link appears in this line:
-           
-           bar [BarF] bar
-                ^^^^
-   = note: no item named `BarF` in scope
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
    = note: this warning originates in the macro `f` (in Nightly builds, run with -Z macro-backtrace for more info)
 
@@ -112,29 +102,19 @@ LL |  * time to introduce a link [error]
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
 
 warning: unresolved link to `error`
-  --> $DIR/warning.rs:68:9
+  --> $DIR/warning.rs:68:23
    |
 LL | #[doc = "single line [error]"]
-   |         ^^^^^^^^^^^^^^^^^^^^^
+   |                       ^^^^^ no item named `error` in scope
    |
-   = note: the link appears in this line:
-           
-           single line [error]
-                        ^^^^^
-   = note: no item named `error` in scope
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
 
 warning: unresolved link to `error`
-  --> $DIR/warning.rs:71:9
+  --> $DIR/warning.rs:71:41
    |
 LL | #[doc = "single line with \"escaping\" [error]"]
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                                         ^^^^^ no item named `error` in scope
    |
-   = note: the link appears in this line:
-           
-           single line with "escaping" [error]
-                                        ^^^^^
-   = note: no item named `error` in scope
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
 
 warning: unresolved link to `error`

tests/rustdoc-ui/lints/bare-urls-limit.rs

@@ -0,0 +1,12 @@
+//@ check-fail
+
+#![deny(rustdoc::bare_urls)]
+
+// examples of bare urls that are beyond our ability to generate suggestions for
+
+// this falls through every heuristic in `source_span_for_markdown_range`,
+// and thus does not get any suggestion.
+#[doc = "good: <https://example.com/> \n\n"]
+//~^ ERROR this URL is not a hyperlink
+#[doc = "bad: https://example.com/"]
+pub fn duplicate_raw() {}

tests/rustdoc-ui/lints/bare-urls-limit.stderr

@@ -0,0 +1,18 @@
+error: this URL is not a hyperlink
+  --> $DIR/bare-urls-limit.rs:9:9
+   |
+LL |   #[doc = "good: <https://example.com/> \n\n"]
+   |  _________^
+LL | |
+LL | | #[doc = "bad: https://example.com/"]
+   | |___________________________________^
+   |
+   = note: bare URLs are not automatically turned into clickable links
+note: the lint level is defined here
+  --> $DIR/bare-urls-limit.rs:3:9
+   |
+LL | #![deny(rustdoc::bare_urls)]
+   |         ^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 1 previous error
+

tests/rustdoc-ui/lints/bare-urls.fixed

@@ -38,6 +38,16 @@
 //~^ ERROR this URL is not a hyperlink
 pub fn c() {}
 
+#[doc = "here's a thing: <https://example.com/>"]
+//~^ ERROR this URL is not a hyperlink
+pub fn f() {}
+
+/// <https://example.com/sugar>
+//~^ ERROR this URL is not a hyperlink
+#[doc = "<https://example.com/raw>"]
+//~^ ERROR this URL is not a hyperlink
+pub fn mixed() {}
+
 /// <https://somewhere.com>
 /// [a](http://a.com)
 /// [b]

tests/rustdoc-ui/lints/bare-urls.rs

@@ -38,6 +38,16 @@
 //~^ ERROR this URL is not a hyperlink
 pub fn c() {}
 
+#[doc = "here's a thing: https://example.com/"]
+//~^ ERROR this URL is not a hyperlink
+pub fn f() {}
+
+/// https://example.com/sugar
+//~^ ERROR this URL is not a hyperlink
+#[doc = "https://example.com/raw"]
+//~^ ERROR this URL is not a hyperlink
+pub fn mixed() {}
+
 /// <https://somewhere.com>
 /// [a](http://a.com)
 /// [b]

tests/rustdoc-ui/lints/bare-urls.stderr

@@ -207,5 +207,41 @@ help: use an automatic link instead
 LL | /// hey! <https://somewhere.com/a?hello=12&bye=11#xyz>
    |          +                                           +
 
-error: aborting due to 17 previous errors
+error: this URL is not a hyperlink
+  --> $DIR/bare-urls.rs:41:26
+   |
+LL | #[doc = "here's a thing: https://example.com/"]
+   |                          ^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: bare URLs are not automatically turned into clickable links
+help: use an automatic link instead
+   |
+LL | #[doc = "here's a thing: <https://example.com/>"]
+   |                          +                    +
+
+error: this URL is not a hyperlink
+  --> $DIR/bare-urls.rs:45:5
+   |
+LL | /// https://example.com/sugar
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: bare URLs are not automatically turned into clickable links
+help: use an automatic link instead
+   |
+LL | /// <https://example.com/sugar>
+   |     +                         +
+
+error: this URL is not a hyperlink
+  --> $DIR/bare-urls.rs:47:10
+   |
+LL | #[doc = "https://example.com/raw"]
+   |          ^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: bare URLs are not automatically turned into clickable links
+help: use an automatic link instead
+   |
+LL | #[doc = "<https://example.com/raw>"]
+   |          +                       +
+
+error: aborting due to 20 previous errors
 

tests/rustdoc-ui/unescaped_backticks.stderr

@@ -628,21 +628,21 @@ LL | /// or even to add a number `n` to 42 (`add(42, n)\`)!
    |                                                   +
 
 error: unescaped backtick
-  --> $DIR/unescaped_backticks.rs:108:9
+  --> $DIR/unescaped_backticks.rs:108:10
    |
 LL | #[doc = "`"]
-   |         ^^^
+   |          ^
    |
    = help: the opening or closing backtick of an inline code may be missing
    = help: if you meant to use a literal backtick, escape it
             change: `
            to this: \`
 
 error: unescaped backtick
-  --> $DIR/unescaped_backticks.rs:115:9
+  --> $DIR/unescaped_backticks.rs:115:26
    |
 LL | #[doc = concat!("\\", "`")]
-   |         ^^^^^^^^^^^^^^^^^^^^
+   |                          ^
    |
    = help: the opening backtick of an inline code may be missing
             change: \`