Skip to content

Improve intra doc errors display #87285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 73 additions & 24 deletions src/librustdoc/passes/collect_intra_doc_links.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use rustc_resolve::ParentScope;
use rustc_session::lint::Lint;
use rustc_span::hygiene::{MacroKind, SyntaxContext};
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::DUMMY_SP;
use rustc_span::{BytePos, DUMMY_SP};
use smallvec::{smallvec, SmallVec};

use pulldown_cmark::LinkType;
Expand Down Expand Up @@ -1193,16 +1193,20 @@ impl LinkCollector<'_, '_> {
let report_mismatch = |specified: Disambiguator, resolved: Disambiguator| {
// The resolved item did not match the disambiguator; give a better error than 'not found'
let msg = format!("incompatible link kind for `{}`", path_str);
let callback = |diag: &mut DiagnosticBuilder<'_>, sp| {
let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option<rustc_span::Span>| {
let note = format!(
"this link resolved to {} {}, which is not {} {}",
resolved.article(),
resolved.descr(),
specified.article(),
specified.descr()
);
diag.note(&note);
suggest_disambiguator(resolved, diag, path_str, dox, sp, &ori_link.range);
if let Some(sp) = sp {
diag.span_label(sp, &note);
} else {
diag.note(&note);
}
suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp);
};
report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback);
};
Expand Down Expand Up @@ -1699,6 +1703,51 @@ impl Suggestion {
Self::RemoveDisambiguator => path_str.into(),
}
}

fn as_help_span(
&self,
path_str: &str,
ori_link: &str,
sp: rustc_span::Span,
) -> Vec<(rustc_span::Span, String)> {
let inner_sp = match ori_link.find('(') {
Some(index) => sp.with_hi(sp.lo() + BytePos(index as _)),
None => sp,
};
let inner_sp = match ori_link.find('!') {
Some(index) => inner_sp.with_hi(inner_sp.lo() + BytePos(index as _)),
None => inner_sp,
};
let inner_sp = match ori_link.find('@') {
Some(index) => inner_sp.with_lo(inner_sp.lo() + BytePos(index as u32 + 1)),
None => inner_sp,
};
match self {
Self::Prefix(prefix) => {
// FIXME: if this is an implied shortcut link, it's bad style to suggest `@`
let mut sugg = vec![(sp.with_hi(inner_sp.lo()), format!("{}@", prefix))];
if sp.hi() != inner_sp.hi() {
sugg.push((inner_sp.shrink_to_hi().with_hi(sp.hi()), String::new()));
}
sugg
}
Self::Function => {
let mut sugg = vec![(inner_sp.shrink_to_hi().with_hi(sp.hi()), "()".to_string())];
if sp.lo() != inner_sp.lo() {
sugg.push((inner_sp.shrink_to_lo().with_lo(sp.lo()), String::new()));
}
sugg
}
Self::Macro => {
let mut sugg = vec![(inner_sp.shrink_to_hi(), "!".to_string())];
Copy link
Contributor

@estebank estebank Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to account for the foo() -> foo! change, so the span covers the () and gets replaced by !:

Suggested change
let mut sugg = vec![(inner_sp.shrink_to_hi(), "!".to_string())];
let mut sugg = vec![(inner_sp.shrink_to_hi().with_hi(sp.hi()), "!".to_string())];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add a specific check for that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it is a valid link to have macro!(), so I'll keep as is. :)

if sp.lo() != inner_sp.lo() {
sugg.push((inner_sp.shrink_to_lo().with_lo(sp.lo()), String::new()));
}
sugg
}
Self::RemoveDisambiguator => return vec![(sp, path_str.into())],
}
}
}

/// Reports a diagnostic for an intra-doc link.
Expand Down Expand Up @@ -1732,7 +1781,16 @@ fn report_diagnostic(
tcx.struct_span_lint_hir(lint, hir_id, sp, |lint| {
let mut diag = lint.build(msg);

let span = super::source_span_for_markdown_range(tcx, dox, link_range, &item.attrs);
let span =
super::source_span_for_markdown_range(tcx, dox, link_range, &item.attrs).map(|sp| {
if dox.bytes().nth(link_range.start) == Some(b'`')
&& dox.bytes().nth(link_range.end - 1) == Some(b'`')
{
sp.with_lo(sp.lo() + BytePos(1)).with_hi(sp.hi() - BytePos(1))
} else {
sp
}
});

if let Some(sp) = span {
diag.set_span(sp);
Expand Down Expand Up @@ -1938,9 +1996,8 @@ fn resolution_failure(
disambiguator,
diag,
path_str,
diag_info.dox,
diag_info.ori_link,
sp,
&diag_info.link_range,
)
}

Expand Down Expand Up @@ -2007,7 +2064,7 @@ fn anchor_failure(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>, failure: A
if let Some((fragment_offset, _)) =
diag_info.ori_link.char_indices().filter(|(_, x)| *x == '#').nth(anchor_idx)
{
sp = sp.with_lo(sp.lo() + rustc_span::BytePos(fragment_offset as _));
sp = sp.with_lo(sp.lo() + BytePos(fragment_offset as _));
}
diag.span_label(sp, "invalid anchor");
}
Expand Down Expand Up @@ -2075,14 +2132,7 @@ fn ambiguity_error(

for res in candidates {
let disambiguator = Disambiguator::from_res(res);
suggest_disambiguator(
disambiguator,
diag,
path_str,
diag_info.dox,
sp,
&diag_info.link_range,
);
suggest_disambiguator(disambiguator, diag, path_str, diag_info.ori_link, sp);
}
});
}
Expand All @@ -2093,21 +2143,20 @@ fn suggest_disambiguator(
disambiguator: Disambiguator,
diag: &mut DiagnosticBuilder<'_>,
path_str: &str,
dox: &str,
ori_link: &str,
sp: Option<rustc_span::Span>,
link_range: &Range<usize>,
) {
let suggestion = disambiguator.suggestion();
let help = format!("to link to the {}, {}", disambiguator.descr(), suggestion.descr());

if let Some(sp) = sp {
let msg = if dox.bytes().nth(link_range.start) == Some(b'`') {
format!("`{}`", suggestion.as_help(path_str))
let mut spans = suggestion.as_help_span(path_str, ori_link, sp);
if spans.len() > 1 {
diag.multipart_suggestion(&help, spans, Applicability::MaybeIncorrect);
} else {
suggestion.as_help(path_str)
};

diag.span_suggestion(sp, &help, msg, Applicability::MaybeIncorrect);
let (sp, suggestion_text) = spans.pop().unwrap();
diag.span_suggestion_verbose(sp, &help, suggestion_text, Applicability::MaybeIncorrect);
}
Comment on lines +2154 to +2159
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you can just always call multipart_suggestion_verbose to get the same effect. Otherwise, it's fine to keep this as is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I added one, but in a PR that hasn't been merged yet. Disregard for now then.

} else {
diag.help(&format!("{}: {}", help, suggestion.as_help(path_str)));
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/rustdoc-ui/assoc-item-not-in-scope.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: unresolved link to `S::fmt`
--> $DIR/assoc-item-not-in-scope.rs:4:14
--> $DIR/assoc-item-not-in-scope.rs:4:15
|
LL | /// Link to [`S::fmt`]
| ^^^^^^^^ the struct `S` has no field or associated item named `fmt`
| ^^^^^^ the struct `S` has no field or associated item named `fmt`
|
note: the lint level is defined here
--> $DIR/assoc-item-not-in-scope.rs:1:9
Expand Down
38 changes: 19 additions & 19 deletions src/test/rustdoc-ui/intra-doc/ambiguity.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -12,26 +12,26 @@ LL | #![deny(rustdoc::broken_intra_doc_links)]
help: to link to the module, prefix with `mod@`
|
LL | /// [mod@true]
| ^^^^^^^^
| ^^^^
help: to link to the builtin type, prefix with `prim@`
|
LL | /// [prim@true]
| ^^^^^^^^^
| ^^^^^

error: `ambiguous` is both a struct and a function
--> $DIR/ambiguity.rs:27:6
--> $DIR/ambiguity.rs:27:7
|
LL | /// [`ambiguous`] is ambiguous.
| ^^^^^^^^^^^ ambiguous link
| ^^^^^^^^^ ambiguous link
|
help: to link to the struct, prefix with `struct@`
|
LL | /// [`struct@ambiguous`] is ambiguous.
| ^^^^^^^^^^^^^^^^^^
| ^^^^^^^
help: to link to the function, add parentheses
|
LL | /// [`ambiguous()`] is ambiguous.
| ^^^^^^^^^^^^^
| ^^

error: `ambiguous` is both a struct and a function
--> $DIR/ambiguity.rs:29:6
Expand All @@ -42,30 +42,30 @@ LL | /// [ambiguous] is ambiguous.
help: to link to the struct, prefix with `struct@`
|
LL | /// [struct@ambiguous] is ambiguous.
| ^^^^^^^^^^^^^^^^
| ^^^^^^^
help: to link to the function, add parentheses
|
LL | /// [ambiguous()] is ambiguous.
| ^^^^^^^^^^^
| ^^

error: `multi_conflict` is a struct, a function, and a macro
--> $DIR/ambiguity.rs:31:6
--> $DIR/ambiguity.rs:31:7
|
LL | /// [`multi_conflict`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^ ambiguous link
| ^^^^^^^^^^^^^^ ambiguous link
|
help: to link to the struct, prefix with `struct@`
|
LL | /// [`struct@multi_conflict`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^^^^^^^^
| ^^^^^^^
help: to link to the function, add parentheses
|
LL | /// [`multi_conflict()`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^^^
| ^^
help: to link to the macro, add an exclamation mark
|
LL | /// [`multi_conflict!`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^^
| ^

error: `type_and_value` is both a module and a constant
--> $DIR/ambiguity.rs:33:16
Expand All @@ -76,26 +76,26 @@ LL | /// Ambiguous [type_and_value].
help: to link to the module, prefix with `mod@`
|
LL | /// Ambiguous [mod@type_and_value].
| ^^^^^^^^^^^^^^^^^^
| ^^^^
help: to link to the constant, prefix with `const@`
|
LL | /// Ambiguous [const@type_and_value].
| ^^^^^^^^^^^^^^^^^^^^
| ^^^^^^

error: `foo::bar` is both an enum and a function
--> $DIR/ambiguity.rs:35:42
--> $DIR/ambiguity.rs:35:43
|
LL | /// Ambiguous non-implied shortcut link [`foo::bar`].
| ^^^^^^^^^^ ambiguous link
| ^^^^^^^^ ambiguous link
|
help: to link to the enum, prefix with `enum@`
|
LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`].
| ^^^^^^^^^^^^^^^
| ^^^^^
help: to link to the function, add parentheses
|
LL | /// Ambiguous non-implied shortcut link [`foo::bar()`].
| ^^^^^^^^^^^^
| ^^

error: aborting due to 6 previous errors

8 changes: 8 additions & 0 deletions src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#![deny(rustdoc::broken_intra_doc_links)]
//~^ NOTE lint level is defined
pub enum S {}
fn S() {}

#[macro_export]
macro_rules! m {
() => {};
}
Expand Down Expand Up @@ -41,6 +43,12 @@ trait T {}
//~| NOTE this link resolved
//~| HELP add an exclamation mark

/// Link to [m()]
//~^ ERROR unresolved link to `m`
//~| NOTE this link resolves to the macro `m`
//~| HELP add an exclamation mark
/// and to [m!()]

/// Link to [const@s]
//~^ ERROR incompatible link kind for `s`
//~| NOTE this link resolved
Expand Down
Loading