From 3005a09fedd5c874c36ca1c788954e51e3fa0617 Mon Sep 17 00:00:00 2001 From: binarycat Date: Sat, 1 Feb 2025 13:35:06 -0600 Subject: [PATCH] rustdoc: improve diagnostics on raw doc fragments 1. rustdoc::bare_urls doesn't output invalid suggestions if source_span_for_markdown_range fails to find a span 2. source_span_for_markdown_range tries harder to return a span by applying an additional diagnostic fixes https://github.com/rust-lang/rust/issues/135851 --- compiler/rustc_resolve/src/rustdoc.rs | 51 +++++++++++++++++-- src/librustdoc/passes/lint/bare_urls.rs | 12 +++-- tests/rustdoc-ui/intra-doc/warning.stderr | 36 +++---------- tests/rustdoc-ui/lints/bare-urls-limit.rs | 12 +++++ tests/rustdoc-ui/lints/bare-urls-limit.stderr | 18 +++++++ tests/rustdoc-ui/lints/bare-urls.fixed | 10 ++++ tests/rustdoc-ui/lints/bare-urls.rs | 10 ++++ tests/rustdoc-ui/lints/bare-urls.stderr | 38 +++++++++++++- tests/rustdoc-ui/unescaped_backticks.stderr | 8 +-- 9 files changed, 154 insertions(+), 41 deletions(-) create mode 100644 tests/rustdoc-ui/lints/bare-urls-limit.rs create mode 100644 tests/rustdoc-ui/lints/bare-urls-limit.stderr diff --git a/compiler/rustc_resolve/src/rustdoc.rs b/compiler/rustc_resolve/src/rustdoc.rs index a32fe6990160..24ebb4ddfbc2 100644 --- a/compiler/rustc_resolve/src/rustdoc.rs +++ b/compiler/rustc_resolve/src/rustdoc.rs @@ -514,20 +514,30 @@ pub fn span_of_fragments(fragments: &[DocFragment]) -> Option { /// 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, fragments: &[DocFragment], ) -> Option { + 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(); diff --git a/src/librustdoc/passes/lint/bare_urls.rs b/src/librustdoc/passes/lint/bare_urls.rs index 1e07277d38e5..3b3ce3e92202 100644 --- a/src/librustdoc/passes/lint/bare_urls.rs +++ b/src/librustdoc/passes/lint/bare_urls.rs @@ -18,12 +18,15 @@ 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| { - 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()), @@ -31,6 +34,7 @@ pub(super) fn visit_item(cx: &DocContext<'_>, item: &Item, hir_id: HirId, dox: & ], Applicability::MachineApplicable, ); + } }); }; diff --git a/tests/rustdoc-ui/intra-doc/warning.stderr b/tests/rustdoc-ui/intra-doc/warning.stderr index 3a06f1787e09..9b3f6f822d75 100644 --- a/tests/rustdoc-ui/intra-doc/warning.stderr +++ b/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` diff --git a/tests/rustdoc-ui/lints/bare-urls-limit.rs b/tests/rustdoc-ui/lints/bare-urls-limit.rs new file mode 100644 index 000000000000..f64154b04969 --- /dev/null +++ b/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: \n\n"] +//~^ ERROR this URL is not a hyperlink +#[doc = "bad: https://example.com/"] +pub fn duplicate_raw() {} diff --git a/tests/rustdoc-ui/lints/bare-urls-limit.stderr b/tests/rustdoc-ui/lints/bare-urls-limit.stderr new file mode 100644 index 000000000000..9573665cb131 --- /dev/null +++ b/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: \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 + diff --git a/tests/rustdoc-ui/lints/bare-urls.fixed b/tests/rustdoc-ui/lints/bare-urls.fixed index 7938d715199d..a91573146b82 100644 --- a/tests/rustdoc-ui/lints/bare-urls.fixed +++ b/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: "] +//~^ ERROR this URL is not a hyperlink +pub fn f() {} + +/// +//~^ ERROR this URL is not a hyperlink +#[doc = ""] +//~^ ERROR this URL is not a hyperlink +pub fn mixed() {} + /// /// [a](http://a.com) /// [b] diff --git a/tests/rustdoc-ui/lints/bare-urls.rs b/tests/rustdoc-ui/lints/bare-urls.rs index 75f42b78ffbb..5b008cdafa23 100644 --- a/tests/rustdoc-ui/lints/bare-urls.rs +++ 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() {} + /// /// [a](http://a.com) /// [b] diff --git a/tests/rustdoc-ui/lints/bare-urls.stderr b/tests/rustdoc-ui/lints/bare-urls.stderr index ddfc387eaf66..e1108c7e7f84 100644 --- a/tests/rustdoc-ui/lints/bare-urls.stderr +++ b/tests/rustdoc-ui/lints/bare-urls.stderr @@ -207,5 +207,41 @@ help: use an automatic link instead LL | /// hey! | + + -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: "] + | + + + +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 | /// + | + + + +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 = ""] + | + + + +error: aborting due to 20 previous errors diff --git a/tests/rustdoc-ui/unescaped_backticks.stderr b/tests/rustdoc-ui/unescaped_backticks.stderr index d93aaf5f3ca9..1bcb88e108f2 100644 --- a/tests/rustdoc-ui/unescaped_backticks.stderr +++ b/tests/rustdoc-ui/unescaped_backticks.stderr @@ -628,10 +628,10 @@ 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 @@ -639,10 +639,10 @@ LL | #[doc = "`"] 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: \`