Skip to content

Commit

Permalink
fix intra-link resolution spans in block comments
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@euclio
euclio committed Nov 19, 2018
1 parent 9e8a982 commit ea5843c
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 50 deletions.
2 changes: 0 additions & 2 deletions src/librustdoc/clean/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -708,8 +708,6 @@ impl<I: IntoIterator<Item=ast::NestedMetaItem>> 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.
Expand Down
81 changes: 57 additions & 24 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,19 @@ 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,
Expand All @@ -507,34 +520,50 @@ 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()
})
};

// Extract the specific span
if attrs.doc_strings.iter().all(|frag| match frag {
DocFragment::SugaredDoc(..) => true,
_ => false,
}) {
let source_dox = sp.to_src(cx);
let mut source_lines = source_dox.lines().peekable();
let mut md_lines = dox.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 source_offset = 0;

// Eat any source lines before the markdown starts (e.g., `/**` on its own line).
while let Some(source_line) = source_lines.peek() {
if source_line.contains(md_lines.peek().unwrap()) {
break;
}

// Include the newline.
source_offset += source_line.len() + 1;
source_lines.next().unwrap();
}

// The number of lines up to and including the resolution failure.
let num_lines = dox[..link_range.start].lines().count();

// Consume inner comment markers (e.g., `///` or ` *`).
for (source_line, md_line) in source_lines.zip(md_lines).take(num_lines) {
source_offset += if md_line.is_empty() {
// If there is no markdown on this line, then the whole line is a comment
// marker. We don't have to count the newline here because it's in the markdown
// too.
source_line.len()
} else {
source_line.find(md_line).unwrap()
};
}

let sp = sp.from_inner_byte_pos(
link_range.start + code_dox_len,
link_range.end + code_dox_len,
link_range.start + source_offset,
link_range.end + source_offset,
);

diag = cx.tcx.struct_span_lint_node(lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
Expand All @@ -548,6 +577,10 @@ fn resolution_failure(
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("");

Expand Down
23 changes: 23 additions & 0 deletions src/test/rustdoc-ui/intra-links-warning.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,26 @@ 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;
84 changes: 60 additions & 24 deletions src/test/rustdoc-ui/intra-links-warning.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,60 @@ 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: `[BarA]` cannot be resolved, ignoring it...
--> $DIR/intra-links-warning.rs:24:10
|
Expand All @@ -64,37 +118,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...
Expand Down

0 comments on commit ea5843c

Please sign in to comment.