From 56413ecffc22b9932f3c3893aa98a36e18818b42 Mon Sep 17 00:00:00 2001 From: Andy Russell Date: Mon, 10 Dec 2018 12:24:39 -0500 Subject: [PATCH] fix intra-link resolution spans in block comments This commit improves the calculation of code spans for intra-doc resolution failures. All sugared doc comments should now have the correct spans, including those where the comment is longer than the docs. It also fixes an issue where the spans were calculated incorrectly for certain unsugared doc comments. The diagnostic will now always use the span of the attributes, as originally intended. Fixes #55964. --- src/librustdoc/clean/mod.rs | 2 - .../passes/collect_intra_doc_links.rs | 109 ++++++++++++------ src/test/rustdoc-ui/.gitattributes | 1 + .../rustdoc-ui/intra-links-warning-crlf.rs | 23 ++++ .../intra-links-warning-crlf.stderr | 33 ++++++ src/test/rustdoc-ui/intra-links-warning.rs | 30 +++++ .../rustdoc-ui/intra-links-warning.stderr | 100 ++++++++++++---- 7 files changed, 238 insertions(+), 60 deletions(-) create mode 100644 src/test/rustdoc-ui/.gitattributes create mode 100644 src/test/rustdoc-ui/intra-links-warning-crlf.rs create mode 100644 src/test/rustdoc-ui/intra-links-warning-crlf.stderr diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 64f66d55fc63d..1e33ec8c37661 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -707,8 +707,6 @@ impl> NestedAttributesExt for I { /// kept separate because of issue #42760. #[derive(Clone, RustcEncodable, RustcDecodable, PartialEq, Eq, Debug, Hash)] pub enum DocFragment { - // FIXME #44229 (misdreavus): sugared and raw doc comments can be brought back together once - // hoedown is completely removed from rustdoc. /// A doc fragment created from a `///` or `//!` doc comment. SugaredDoc(usize, syntax_pos::Span, String), /// A doc fragment created from a "raw" `#[doc=""]` attribute. diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 426d3f3eeeaa2..29062ba58c2e7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -459,6 +459,19 @@ pub fn span_of_attrs(attrs: &Attributes) -> syntax_pos::Span { start.to(end) } +/// Reports a resolution failure diagnostic. +/// +/// Ideally we can report the diagnostic with the actual span in the source where the link failure +/// occurred. However, there's a mismatch between the span in the source code and the span in the +/// markdown, so we have to do a bit of work to figure out the correspondence. +/// +/// It's not too hard to find the span for sugared doc comments (`///` and `/**`), because the +/// source will match the markdown exactly, excluding the comment markers. However, it's much more +/// difficult to calculate the spans for unsugared docs, because we have to deal with escaping and +/// other source features. So, we attempt to find the exact source span of the resolution failure +/// in sugared docs, but use the span of the documentation attributes themselves for unsugared +/// docs. Because this span might be overly large, we display the markdown line containing the +/// failure as a note. fn resolution_failure( cx: &DocContext, attrs: &Attributes, @@ -469,47 +482,75 @@ fn resolution_failure( let sp = span_of_attrs(attrs); let msg = format!("`[{}]` cannot be resolved, ignoring it...", path_str); - let code_dox = sp.to_src(cx); - - let doc_comment_padding = 3; let mut diag = if let Some(link_range) = link_range { - // blah blah blah\nblah\nblah [blah] blah blah\nblah blah - // ^ ~~~~~~ - // | link_range - // last_new_line_offset - - let mut diag; - if dox.lines().count() == code_dox.lines().count() { - let line_offset = dox[..link_range.start].lines().count(); - // The span starts in the `///`, so we don't have to account for the leading whitespace. - let code_dox_len = if line_offset <= 1 { - doc_comment_padding - } else { - // The first `///`. - doc_comment_padding + - // Each subsequent leading whitespace and `///`. - code_dox.lines().skip(1).take(line_offset - 1).fold(0, |sum, line| { - sum + doc_comment_padding + line.len() - line.trim_start().len() - }) - }; + let src = cx.sess().source_map().span_to_snippet(sp); + let is_all_sugared_doc = attrs.doc_strings.iter().all(|frag| match frag { + DocFragment::SugaredDoc(..) => true, + _ => false, + }); + + if let (Ok(src), true) = (src, is_all_sugared_doc) { + // The number of markdown lines up to and including the resolution failure. + let num_lines = dox[..link_range.start].lines().count(); + + // We use `split_terminator('\n')` instead of `lines()` when counting bytes to ensure + // that DOS-style line endings do not cause the spans to be calculated incorrectly. + let mut src_lines = src.split_terminator('\n'); + let mut md_lines = dox.split_terminator('\n').take(num_lines).peekable(); + + // The number of bytes from the start of the source span to the resolution failure that + // are *not* part of the markdown, like comment markers. + let mut extra_src_bytes = 0; + + while let Some(md_line) = md_lines.next() { + loop { + let source_line = src_lines + .next() + .expect("could not find markdown line in source"); + + match source_line.find(md_line) { + Some(offset) => { + extra_src_bytes += if md_lines.peek().is_some() { + source_line.len() - md_line.len() + } else { + offset + }; + break; + } + None => { + // Since this is a source line that doesn't include a markdown line, + // we have to count the newline that we split from earlier. + extra_src_bytes += source_line.len() + 1; + } + } + } + } - // Extract the specific span. let sp = sp.from_inner_byte_pos( - link_range.start + code_dox_len, - link_range.end + code_dox_len, + link_range.start + extra_src_bytes, + link_range.end + extra_src_bytes, ); - diag = cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, - NodeId::from_u32(0), - sp, - &msg); + let mut diag = cx.tcx.struct_span_lint_node( + lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, + NodeId::from_u32(0), + sp, + &msg, + ); diag.span_label(sp, "cannot be resolved, ignoring"); + diag } else { - diag = cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, - NodeId::from_u32(0), - sp, - &msg); + let mut diag = cx.tcx.struct_span_lint_node( + lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, + NodeId::from_u32(0), + sp, + &msg, + ); + // 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(""); @@ -522,8 +563,8 @@ fn resolution_failure( before=link_range.start - last_new_line_offset, found=link_range.len(), )); + diag } - diag } else { cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE, NodeId::from_u32(0), diff --git a/src/test/rustdoc-ui/.gitattributes b/src/test/rustdoc-ui/.gitattributes new file mode 100644 index 0000000000000..2bcabdffb3d93 --- /dev/null +++ b/src/test/rustdoc-ui/.gitattributes @@ -0,0 +1 @@ +intra-links-warning-crlf.rs eol=crlf diff --git a/src/test/rustdoc-ui/intra-links-warning-crlf.rs b/src/test/rustdoc-ui/intra-links-warning-crlf.rs new file mode 100644 index 0000000000000..20f761fcf4f3b --- /dev/null +++ b/src/test/rustdoc-ui/intra-links-warning-crlf.rs @@ -0,0 +1,23 @@ +// ignore-tidy-cr + +// compile-pass + +// This file checks the spans of intra-link warnings in a file with CRLF line endings. The +// .gitattributes file in this directory should enforce it. + +/// [error] +pub struct A; + +/// +/// docs [error1] + +/// docs [error2] +/// +pub struct B; + +/** + * This is a multi-line comment. + * + * It also has an [error]. + */ +pub struct C; diff --git a/src/test/rustdoc-ui/intra-links-warning-crlf.stderr b/src/test/rustdoc-ui/intra-links-warning-crlf.stderr new file mode 100644 index 0000000000000..62537f2ce2dc9 --- /dev/null +++ b/src/test/rustdoc-ui/intra-links-warning-crlf.stderr @@ -0,0 +1,33 @@ +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning-crlf.rs:8:6 + | +LL | /// [error] + | ^^^^^ cannot be resolved, ignoring + | + = note: #[warn(intra_doc_link_resolution_failure)] on by default + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error1]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning-crlf.rs:12:11 + | +LL | /// docs [error1] + | ^^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error2]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning-crlf.rs:14:11 + | +LL | /// docs [error2] + | ^^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning-crlf.rs:21:20 + | +LL | * It also has an [error]. + | ^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + diff --git a/src/test/rustdoc-ui/intra-links-warning.rs b/src/test/rustdoc-ui/intra-links-warning.rs index d6bc275b57a6a..db2fd3211f871 100644 --- a/src/test/rustdoc-ui/intra-links-warning.rs +++ b/src/test/rustdoc-ui/intra-links-warning.rs @@ -55,3 +55,33 @@ macro_rules! f { } } f!("Foo\nbar [BarF] bar\nbaz"); + +/** # for example, + * + * time to introduce a link [error]*/ +pub struct A; + +/** + * # for example, + * + * time to introduce a link [error] + */ +pub struct B; + +#[doc = "single line [error]"] +pub struct C; + +#[doc = "single line with \"escaping\" [error]"] +pub struct D; + +/// Item docs. +#[doc="Hello there!"] +/// [error] +pub struct E; + +/// +/// docs [error1] + +/// docs [error2] +/// +pub struct F; diff --git a/src/test/rustdoc-ui/intra-links-warning.stderr b/src/test/rustdoc-ui/intra-links-warning.stderr index c05f99fadc9e4..ed31421851bed 100644 --- a/src/test/rustdoc-ui/intra-links-warning.stderr +++ b/src/test/rustdoc-ui/intra-links-warning.stderr @@ -55,6 +55,76 @@ LL | /// [Qux:Y] | = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:61:30 + | +LL | * time to introduce a link [error]*/ + | ^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:67:30 + | +LL | * time to introduce a link [error] + | ^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:71:1 + | +LL | #[doc = "single line [error]"] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: the link appears in this line: + + single line [error] + ^^^^^ + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:74:1 + | +LL | #[doc = "single line with /"escaping/" [error]"] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: the link appears in this line: + + single line with "escaping" [error] + ^^^^^ + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:77:1 + | +LL | / /// Item docs. +LL | | #[doc="Hello there!"] +LL | | /// [error] + | |___________^ + | + = note: the link appears in this line: + + [error] + ^^^^^ + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error1]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:83:11 + | +LL | /// docs [error1] + | ^^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + +warning: `[error2]` cannot be resolved, ignoring it... + --> $DIR/intra-links-warning.rs:85:11 + | +LL | /// docs [error2] + | ^^^^^^ cannot be resolved, ignoring + | + = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` + warning: `[BarA]` cannot be resolved, ignoring it... --> $DIR/intra-links-warning.rs:24:10 | @@ -64,37 +134,19 @@ LL | /// bar [BarA] bar = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` warning: `[BarB]` cannot be resolved, ignoring it... - --> $DIR/intra-links-warning.rs:28:1 + --> $DIR/intra-links-warning.rs:30:9 | -LL | / /** -LL | | * Foo -LL | | * bar [BarB] bar -LL | | * baz -LL | | */ - | |___^ +LL | * bar [BarB] bar + | ^^^^ cannot be resolved, ignoring | - = note: the link appears in this line: - - bar [BarB] bar - ^^^^ = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` warning: `[BarC]` cannot be resolved, ignoring it... - --> $DIR/intra-links-warning.rs:35:1 + --> $DIR/intra-links-warning.rs:37:6 | -LL | / /** Foo -LL | | -LL | | bar [BarC] bar -LL | | baz -... | -LL | | -LL | | */ - | |__^ +LL | bar [BarC] bar + | ^^^^ cannot be resolved, ignoring | - = note: the link appears in this line: - - bar [BarC] bar - ^^^^ = help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` warning: `[BarD]` cannot be resolved, ignoring it...