Skip to content
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.

Sign up for GitHub

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

Unify error reporting for intra-doc links #75916

Merged
merged 7 commits into from
Aug 29, 2020
Merged

Unify error reporting for intra-doc links #75916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
18e7a1b
Add `Res::ns()` instead of `matches_ns()`
jyn514 Aug 25, 2020
56d3f39
Unify error reporting for intra-doc links
jyn514 Aug 25, 2020
05a040f
Remove unused `ns` parameter
jyn514 Aug 25, 2020
5ce4ee9
Add back `Res::matches_ns`, implemented in terms of `ns`
jyn514 Aug 25, 2020
e233d80
Remove outdated FIXME
jyn514 Aug 25, 2020
bb351de
Use map_or(true) instead of unwrap_or(true)
jyn514 Aug 26, 2020
d3c5817
Note that `from_res` panics on errors
jyn514 Aug 26, 2020
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
18 changes: 12 additions & 6 deletions src/librustc_hir/def.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,13 +451,19 @@ impl<Id> Res<Id> {
}
}

pub fn matches_ns(&self, ns: Namespace) -> bool {
/// Returns `None` if this is `Res::Err`
pub fn ns(&self) -> Option<Namespace> {
match self {
Res::Def(kind, ..) => kind.ns() == Some(ns),
Res::PrimTy(..) | Res::SelfTy(..) | Res::ToolMod => ns == Namespace::TypeNS,
Res::SelfCtor(..) | Res::Local(..) => ns == Namespace::ValueNS,
Res::NonMacroAttr(..) => ns == Namespace::MacroNS,
Res::Err => true,
Res::Def(kind, ..) => kind.ns(),
Res::PrimTy(..) | Res::SelfTy(..) | Res::ToolMod => Some(Namespace::TypeNS),
Res::SelfCtor(..) | Res::Local(..) => Some(Namespace::ValueNS),
Res::NonMacroAttr(..) => Some(Namespace::MacroNS),
Res::Err => None,
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// Always returns `true` if `self` is `Res::Err`
pub fn matches_ns(&self, ns: Namespace) -> bool {
self.ns().map_or(true, |actual_ns| actual_ns == ns)
}
}
124 changes: 56 additions & 68 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,16 +829,13 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
}
let candidates =
candidates.map(|candidate| candidate.map(|(res, _)| res));
let candidates = [TypeNS, ValueNS, MacroNS]
.iter()
.filter_map(|&ns| candidates[ns].map(|res| (res, ns)));
ambiguity_error(
cx,
&item,
path_str,
&dox,
link_range,
candidates.collect(),
candidates.present_items().collect(),
);
continue;
}
Expand Down Expand Up @@ -880,7 +877,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
fragment = Some(path.to_owned());
} else {
// `[char]` when a `char` module is in scope
let candidates = vec![(res, TypeNS), (prim, TypeNS)];
let candidates = vec![res, prim];
ambiguity_error(cx, &item, path_str, &dox, link_range, candidates);
continue;
}
Expand All @@ -898,20 +895,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
specified.article(),
specified.descr()
);
let suggestion = resolved.display_for(path_str);
let help_msg =
format!("to link to the {}, use its disambiguator", resolved.descr());
diag.note(&note);
if let Some(sp) = sp {
diag.span_suggestion(
sp,
&help_msg,
suggestion,
Applicability::MaybeIncorrect,
);
} else {
diag.help(&format!("{}: {}", help_msg, suggestion));
}
suggest_disambiguator(resolved, diag, path_str, &dox, sp, &link_range);
});
};
if let Res::PrimTy(_) = res {
Expand Down Expand Up @@ -1047,17 +1032,32 @@ impl Disambiguator {
}
}

fn display_for(self, path_str: &str) -> String {
/// WARNING: panics on `Res::Err`
fn from_res(res: Res) -> Self {
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
match res {
Res::Def(kind, _) => Disambiguator::Kind(kind),
Res::PrimTy(_) => Disambiguator::Primitive,
_ => Disambiguator::Namespace(res.ns().expect("can't call `from_res` on Res::err")),
}
}

/// Return (description of the change, suggestion)
fn display_for(self, path_str: &str) -> (&'static str, String) {
const PREFIX: &str = "prefix with the item kind";
const FUNCTION: &str = "add parentheses";
const MACRO: &str = "add an exclamation mark";

let kind = match self {
Disambiguator::Primitive => return format!("prim@{}", path_str),
Disambiguator::Primitive => return (PREFIX, format!("prim@{}", path_str)),
Disambiguator::Kind(kind) => kind,
Disambiguator::Namespace(_) => panic!("display_for cannot be used on namespaces"),
};
if kind == DefKind::Macro(MacroKind::Bang) {
return format!("{}!", path_str);
return (MACRO, format!("{}!", path_str));
} else if kind == DefKind::Fn || kind == DefKind::AssocFn {
return format!("{}()", path_str);
return (FUNCTION, format!("{}()", path_str));
}

let prefix = match kind {
DefKind::Struct => "struct",
DefKind::Enum => "enum",
Expand All @@ -1079,7 +1079,9 @@ impl Disambiguator {
Namespace::MacroNS => "macro",
},
};
format!("{}@{}", prefix, path_str)

// FIXME: if this is an implied shortcut link, it's bad style to suggest `@`
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
(PREFIX, format!("{}@{}", prefix, path_str))
}

fn ns(self) -> Namespace {
Expand Down Expand Up @@ -1247,12 +1249,12 @@ fn ambiguity_error(
path_str: &str,
dox: &str,
link_range: Option<Range<usize>>,
candidates: Vec<(Res, Namespace)>,
candidates: Vec<Res>,
) {
let mut msg = format!("`{}` is ", path_str);

match candidates.as_slice() {
[(first_def, _), (second_def, _)] => {
[first_def, second_def] => {
msg += &format!(
"both {} {} and {} {}",
first_def.article(),
Expand All @@ -1263,7 +1265,7 @@ fn ambiguity_error(
}
_ => {
let mut candidates = candidates.iter().peekable();
while let Some((res, _)) = candidates.next() {
while let Some(res) = candidates.next() {
if candidates.peek().is_some() {
msg += &format!("{} {}, ", res.article(), res.descr());
} else {
Expand All @@ -1276,52 +1278,38 @@ fn ambiguity_error(
report_diagnostic(cx, &msg, item, dox, link_range.clone(), |diag, sp| {
if let Some(sp) = sp {
diag.span_label(sp, "ambiguous link");
} else {
diag.note("ambiguous link");
}

let link_range = link_range.expect("must have a link range if we have a span");

for (res, ns) in candidates {
let (action, mut suggestion) = match res {
Res::Def(DefKind::AssocFn | DefKind::Fn, _) => {
("add parentheses", format!("{}()", path_str))
}
Res::Def(DefKind::Macro(MacroKind::Bang), _) => {
("add an exclamation mark", format!("{}!", path_str))
}
_ => {
let type_ = match (res, ns) {
(Res::PrimTy(_), _) => "prim",
(Res::Def(DefKind::Const, _), _) => "const",
(Res::Def(DefKind::Static, _), _) => "static",
(Res::Def(DefKind::Struct, _), _) => "struct",
(Res::Def(DefKind::Enum, _), _) => "enum",
(Res::Def(DefKind::Union, _), _) => "union",
(Res::Def(DefKind::Trait, _), _) => "trait",
(Res::Def(DefKind::Mod, _), _) => "module",
(_, TypeNS) => "type",
(_, ValueNS) => "value",
(Res::Def(DefKind::Macro(MacroKind::Derive), _), MacroNS) => "derive",
(_, MacroNS) => "macro",
};

// FIXME: if this is an implied shortcut link, it's bad style to suggest `@`
("prefix with the item type", format!("{}@{}", type_, path_str))
}
};
for res in candidates {
let disambiguator = Disambiguator::from_res(res);
suggest_disambiguator(disambiguator, diag, path_str, dox, sp, &link_range);
}
});
}

if dox.bytes().nth(link_range.start) == Some(b'`') {
suggestion = format!("`{}`", suggestion);
}
fn suggest_disambiguator(
disambiguator: Disambiguator,
diag: &mut DiagnosticBuilder<'_>,
path_str: &str,
dox: &str,
sp: Option<rustc_span::Span>,
link_range: &Option<Range<usize>>,
) {
let (action, mut suggestion) = disambiguator.display_for(path_str);
let help = format!("to link to the {}, {}", disambiguator.descr(), action);

// FIXME: Create a version of this suggestion for when we don't have the span.
diag.span_suggestion(
sp,
&format!("to link to the {}, {}", res.descr(), action),
suggestion,
Applicability::MaybeIncorrect,
);
}
if let Some(sp) = sp {
let link_range = link_range.as_ref().expect("must have a link range if we have a span");
if dox.bytes().nth(link_range.start) == Some(b'`') {
suggestion = format!("`{}`", suggestion);
}
});

diag.span_suggestion(sp, &help, suggestion, Applicability::MaybeIncorrect);
} else {
diag.help(&format!("{}: {}", help, suggestion));
}
}

fn privacy_error(
Expand Down
4 changes: 2 additions & 2 deletions src/test/rustdoc-ui/intra-link-prim-conflict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@

/// [struct@char]
//~^ ERROR incompatible link
//~| HELP use its disambiguator
//~| HELP prefix with the item kind
//~| NOTE resolved to a module
pub mod char {}

pub mod inner {
//! [struct@char]
//~^ ERROR incompatible link
//~| HELP use its disambiguator
//~| HELP prefix with the item kind
//~| NOTE resolved to a builtin type
}
28 changes: 18 additions & 10 deletions src/test/rustdoc-ui/intra-link-prim-conflict.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ note: the lint level is defined here
|
LL | #![deny(broken_intra_doc_links)]
| ^^^^^^^^^^^^^^^^^^^^^^
help: to link to the module, prefix with the item type
help: to link to the module, prefix with the item kind
|
LL | /// [module@char]
| ^^^^^^^^^^^
help: to link to the builtin type, prefix with the item type
LL | /// [mod@char]
| ^^^^^^^^
help: to link to the builtin type, prefix with the item kind
|
LL | /// [prim@char]
| ^^^^^^^^^
Expand All @@ -24,11 +24,11 @@ error: `char` is both a module and a builtin type
LL | /// [type@char]
| ^^^^^^^^^ ambiguous link
|
help: to link to the module, prefix with the item type
help: to link to the module, prefix with the item kind
|
LL | /// [module@char]
| ^^^^^^^^^^^
help: to link to the builtin type, prefix with the item type
LL | /// [mod@char]
| ^^^^^^^^
help: to link to the builtin type, prefix with the item kind
|
LL | /// [prim@char]
| ^^^^^^^^^
Expand All @@ -37,17 +37,25 @@ error: incompatible link kind for `char`
--> $DIR/intra-link-prim-conflict.rs:19:6
|
LL | /// [struct@char]
| ^^^^^^^^^^^ help: to link to the module, use its disambiguator: `mod@char`
| ^^^^^^^^^^^
|
= note: this link resolved to a module, which is not a struct
help: to link to the module, prefix with the item kind
|
LL | /// [mod@char]
| ^^^^^^^^

error: incompatible link kind for `char`
--> $DIR/intra-link-prim-conflict.rs:26:10
|
LL | //! [struct@char]
| ^^^^^^^^^^^ help: to link to the builtin type, use its disambiguator: `prim@char`
| ^^^^^^^^^^^
|
= note: this link resolved to a builtin type, which is not a struct
help: to link to the builtin type, prefix with the item kind
|
LL | //! [prim@char]
| ^^^^^^^^^

error: aborting due to 4 previous errors

16 changes: 8 additions & 8 deletions src/test/rustdoc-ui/intra-links-ambiguity.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ note: the lint level is defined here
|
LL | #![deny(broken_intra_doc_links)]
| ^^^^^^^^^^^^^^^^^^^^^^
help: to link to the struct, prefix with the item type
help: to link to the struct, prefix with the item kind
|
LL | /// [`struct@ambiguous`] is ambiguous.
| ^^^^^^^^^^^^^^^^^^
Expand All @@ -24,7 +24,7 @@ error: `ambiguous` is both a struct and a function
LL | /// [ambiguous] is ambiguous.
| ^^^^^^^^^ ambiguous link
|
help: to link to the struct, prefix with the item type
help: to link to the struct, prefix with the item kind
|
LL | /// [struct@ambiguous] is ambiguous.
| ^^^^^^^^^^^^^^^^
Expand All @@ -39,7 +39,7 @@ error: `multi_conflict` is a struct, a function, and a macro
LL | /// [`multi_conflict`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^ ambiguous link
|
help: to link to the struct, prefix with the item type
help: to link to the struct, prefix with the item kind
|
LL | /// [`struct@multi_conflict`] is a three-way conflict.
| ^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -58,11 +58,11 @@ error: `type_and_value` is both a module and a constant
LL | /// Ambiguous [type_and_value].
| ^^^^^^^^^^^^^^ ambiguous link
|
help: to link to the module, prefix with the item type
help: to link to the module, prefix with the item kind
|
LL | /// Ambiguous [module@type_and_value].
| ^^^^^^^^^^^^^^^^^^^^^
help: to link to the constant, prefix with the item type
LL | /// Ambiguous [mod@type_and_value].
| ^^^^^^^^^^^^^^^^^^
help: to link to the constant, prefix with the item kind
|
LL | /// Ambiguous [const@type_and_value].
| ^^^^^^^^^^^^^^^^^^^^
Expand All @@ -73,7 +73,7 @@ error: `foo::bar` is both an enum and a function
LL | /// Ambiguous non-implied shortcut link [`foo::bar`].
| ^^^^^^^^^^ ambiguous link
|
help: to link to the enum, prefix with the item type
help: to link to the enum, prefix with the item kind
|
LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`].
| ^^^^^^^^^^^^^^^
Expand Down
Loading