Skip to content

Commit 33442b1

Browse files
euclioeuclio
committed
overhaul intra-doc-link ambiguity warning
- Makes the warning part of the `intra_doc_link_resolution_failure` lint. - Tightens the span to just the ambiguous link. - Reports ambiguities across all three namespaces. - Uses structured suggestions for disambiguation. - Adds a test for the warnings.
1 parent 16e7e05 commit 33442b1

File tree

4 files changed

+266
-95
lines changed

4 files changed

+266
-95
lines changed

‎src/librustdoc/passes/collect_intra_doc_links.rs‎

Lines changed: 146 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
use rustc::lint as lint;
2-
use rustc::hir;
1+
use errors::Applicability;
32
use rustc::hir::def::Def;
43
use rustc::hir::def_id::DefId;
4+
use rustc::hir;
5+
use rustc::lint as lint;
56
use rustc::ty;
67
use syntax;
78
use syntax::ast::{self, Ident};
@@ -53,6 +54,13 @@ struct LinkCollector<'a, 'tcx> {
5354
is_nightly_build: bool,
5455
}
5556

57+
#[derive(Debug, Copy, Clone)]
58+
enum Namespace {
59+
Type,
60+
Value,
61+
Macro,
62+
}
63+
5664
impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
5765
fn new(cx: &'a DocContext<'tcx>) -> Self {
5866
LinkCollector {
@@ -345,57 +353,52 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
345353
}
346354
PathKind::Unknown => {
347355
// Try everything!
356+
let mut candidates = vec![];
357+
348358
if let Some(macro_def) = macro_resolve(cx, path_str) {
349-
if let Ok(type_def) =
350-
self.resolve(path_str, false, &current_item, parent_node)
351-
{
352-
let (type_kind, article, type_disambig)
353-
= type_ns_kind(type_def.0, path_str);
354-
ambiguity_error(cx, &item.attrs, path_str,
355-
article, type_kind, &type_disambig,
356-
"a", "macro", &format!("macro@{}", path_str));
357-
continue;
358-
} else if let Ok(value_def) =
359-
self.resolve(path_str, true, &current_item, parent_node)
360-
{
361-
let (value_kind, value_disambig)
362-
= value_ns_kind(value_def.0, path_str)
363-
.expect("struct and mod cases should have been \
364-
caught in previous branch");
365-
ambiguity_error(cx, &item.attrs, path_str,
366-
"a", value_kind, &value_disambig,
367-
"a", "macro", &format!("macro@{}", path_str));
368-
}
369-
(macro_def, None)
370-
} else if let Ok(type_def) =
359+
candidates.push(((macro_def, None), Namespace::Macro));
360+
}
361+
362+
if let Ok(type_def) =
371363
self.resolve(path_str, false, &current_item, parent_node)
372364
{
373-
// It is imperative we search for not-a-value first
374-
// Otherwise we will find struct ctors for when we are looking
375-
// for structs, and the link won't work if there is something in
376-
// both namespaces.
377-
if let Ok(value_def) =
378-
self.resolve(path_str, true, &current_item, parent_node)
379-
{
380-
let kind = value_ns_kind(value_def.0, path_str);
381-
if let Some((value_kind, value_disambig)) = kind {
382-
let (type_kind, article, type_disambig)
383-
= type_ns_kind(type_def.0, path_str);
384-
ambiguity_error(cx, &item.attrs, path_str,
385-
article, type_kind, &type_disambig,
386-
"a", value_kind, &value_disambig);
387-
continue;
388-
}
389-
}
390-
type_def
391-
} else if let Ok(value_def) =
365+
candidates.push((type_def, Namespace::Type));
366+
}
367+
368+
if let Ok(value_def) =
392369
self.resolve(path_str, true, &current_item, parent_node)
393370
{
394-
value_def
395-
} else {
371+
// Structs, variants, and mods exist in both namespaces, skip them.
372+
match value_def.0 {
373+
Def::StructCtor(..)
374+
| Def::Mod(..)
375+
| Def::Variant(..)
376+
| Def::VariantCtor(..)
377+
| Def::SelfCtor(..) => (),
378+
_ => candidates.push((value_def, Namespace::Value)),
379+
}
380+
}
381+
382+
if candidates.len() == 1 {
383+
candidates.remove(0).0
384+
} else if candidates.is_empty() {
396385
resolution_failure(cx, &item.attrs, path_str, &dox, link_range);
397386
// this could just be a normal link
398387
continue;
388+
} else {
389+
let candidates = candidates.into_iter().map(|((def, _), ns)| {
390+
(def, ns)
391+
}).collect::<Vec<_>>();
392+
393+
ambiguity_error(
394+
cx,
395+
&item.attrs,
396+
path_str,
397+
&dox,
398+
link_range,
399+
&candidates,
400+
);
401+
continue;
399402
}
400403
}
401404
PathKind::Macro => {
@@ -505,59 +508,108 @@ fn resolution_failure(
505508
diag.emit();
506509
}
507510

508-
fn ambiguity_error(cx: &DocContext<'_>, attrs: &Attributes,
509-
path_str: &str,
510-
article1: &str, kind1: &str, disambig1: &str,
511-
article2: &str, kind2: &str, disambig2: &str) {
511+
fn ambiguity_error(
512+
cx: &DocContext<'_>,
513+
attrs: &Attributes,
514+
path_str: &str,
515+
dox: &str,
516+
link_range: Option<Range<usize>>,
517+
candidates: &[(Def, Namespace)],
518+
) {
512519
let sp = span_of_attrs(attrs);
513-
cx.sess()
514-
.struct_span_warn(sp,
515-
&format!("`{}` is both {} {} and {} {}",
516-
path_str, article1, kind1,
517-
article2, kind2))
518-
.help(&format!("try `{}` if you want to select the {}, \
519-
or `{}` if you want to \
520-
select the {}",
521-
disambig1, kind1, disambig2,
522-
kind2))
523-
.emit();
524-
}
525520

526-
/// Given a def, returns its name and disambiguator
527-
/// for a value namespace.
528-
///
529-
/// Returns `None` for things which cannot be ambiguous since
530-
/// they exist in both namespaces (structs and modules).
531-
fn value_ns_kind(def: Def, path_str: &str) -> Option<(&'static str, String)> {
532-
match def {
533-
// Structs, variants, and mods exist in both namespaces; skip them.
534-
Def::StructCtor(..) | Def::Mod(..) | Def::Variant(..) |
535-
Def::VariantCtor(..) | Def::SelfCtor(..)
536-
=> None,
537-
Def::Fn(..)
538-
=> Some(("function", format!("{}()", path_str))),
539-
Def::Method(..)
540-
=> Some(("method", format!("{}()", path_str))),
541-
Def::Const(..)
542-
=> Some(("const", format!("const@{}", path_str))),
543-
Def::Static(..)
544-
=> Some(("static", format!("static@{}", path_str))),
545-
_ => Some(("value", format!("value@{}", path_str))),
521+
let mut msg = format!("`{}` is ", path_str);
522+
523+
match candidates {
524+
[(first_def, _), (second_def, _)] => {
525+
msg += &format!(
526+
"both {} {} and {} {}",
527+
first_def.article(),
528+
first_def.kind_name(),
529+
second_def.article(),
530+
second_def.kind_name(),
531+
);
532+
}
533+
_ => {
534+
let mut candidates = candidates.iter().peekable();
535+
while let Some((def, _)) = candidates.next() {
536+
if candidates.peek().is_some() {
537+
msg += &format!("{} {}, ", def.article(), def.kind_name());
538+
} else {
539+
msg += &format!("and {} {}", def.article(), def.kind_name());
540+
}
541+
}
542+
}
546543
}
547-
}
548544

549-
/// Given a def, returns its name, the article to be used, and a disambiguator
550-
/// for the type namespace.
551-
fn type_ns_kind(def: Def, path_str: &str) -> (&'static str, &'static str, String) {
552-
let (kind, article) = match def {
553-
// We can still have non-tuple structs.
554-
Def::Struct(..) => ("struct", "a"),
555-
Def::Enum(..) => ("enum", "an"),
556-
Def::Trait(..) => ("trait", "a"),
557-
Def::Union(..) => ("union", "a"),
558-
_ => ("type", "a"),
559-
};
560-
(kind, article, format!("{}@{}", kind, path_str))
545+
let mut diag = cx.tcx.struct_span_lint_hir(
546+
lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
547+
hir::CRATE_HIR_ID,
548+
sp,
549+
&msg,
550+
);
551+
552+
if let Some(link_range) = link_range {
553+
if let Some(sp) = super::source_span_for_markdown_range(cx, dox, &link_range, attrs) {
554+
diag.set_span(sp);
555+
diag.span_label(sp, "ambiguous link");
556+
557+
for (def, ns) in candidates {
558+
let (action, mut suggestion) = match def {
559+
Def::Method(..) | Def::Fn(..) => {
560+
("add parentheses", format!("{}()", path_str))
561+
}
562+
_ => {
563+
let type_ = match (def, ns) {
564+
(Def::Const(..), _) => "const",
565+
(Def::Static(..), _) => "static",
566+
(Def::Struct(..), _) => "struct",
567+
(Def::Enum(..), _) => "enum",
568+
(Def::Union(..), _) => "union",
569+
(Def::Trait(..), _) => "trait",
570+
(Def::Mod(..), _) => "module",
571+
(_, Namespace::Type) => "type",
572+
(_, Namespace::Value) => "value",
573+
(_, Namespace::Macro) => "macro",
574+
};
575+
576+
// FIXME: if this is an implied shortcut link, it's bad style to suggest `@`
577+
("prefix with the item type", format!("{}@{}", type_, path_str))
578+
}
579+
};
580+
581+
if dox.bytes().nth(link_range.start) == Some(b'`') {
582+
suggestion = format!("`{}`", suggestion);
583+
}
584+
585+
diag.span_suggestion(
586+
sp,
587+
&format!("to link to the {}, {}", def.kind_name(), action),
588+
suggestion,
589+
Applicability::MaybeIncorrect,
590+
);
591+
}
592+
} else {
593+
// blah blah blah\nblah\nblah [blah] blah blah\nblah blah
594+
// ^ ~~~~
595+
// | link_range
596+
// last_new_line_offset
597+
let last_new_line_offset = dox[..link_range.start].rfind('\n').map_or(0, |n| n + 1);
598+
let line = dox[last_new_line_offset..].lines().next().unwrap_or("");
599+
600+
// Print the line containing the `link_range` and manually mark it with '^'s.
601+
diag.note(&format!(
602+
"the link appears in this line:\n\n{line}\n\
603+
{indicator: <before$}{indicator:^<found$}",
604+
line=line,
605+
indicator="",
606+
before=link_range.start - last_new_line_offset,
607+
found=link_range.len(),
608+
));
609+
}
610+
}
611+
612+
diag.emit();
561613
}
562614

563615
/// Given an enum variant's def, return the def of its enum and the associated fragment.

‎src/test/rustdoc-ui/intra-links-ambiguity.rs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
#![deny(intra_doc_link_resolution_failure)]
2+
#![allow(non_camel_case_types)]
3+
#![allow(non_upper_case_globals)]
4+
5+
pub fn ambiguous() {}
6+
7+
pub struct ambiguous {}
8+
9+
#[macro_export]
10+
macro_rules! multi_conflict { () => {} }
11+
12+
#[allow(non_camel_case_types)]
13+
pub struct multi_conflict {}
14+
15+
pub fn multi_conflict() {}
16+
17+
pub mod type_and_value {}
18+
19+
pub const type_and_value: i32 = 0;
20+
21+
pub mod foo {
22+
pub enum bar {}
23+
24+
pub fn bar() {}
25+
}
26+
27+
/// [`ambiguous`] is ambiguous. //~ERROR `ambiguous`
28+
///
29+
/// [ambiguous] is ambiguous. //~ERROR ambiguous
30+
///
31+
/// [`multi_conflict`] is a three-way conflict. //~ERROR `multi_conflict`
32+
///
33+
/// Ambiguous [type_and_value]. //~ERROR type_and_value
34+
///
35+
/// Ambiguous non-implied shortcut link [`foo::bar`]. //~ERROR `foo::bar`
36+
pub struct Docs {}

‎src/test/rustdoc-ui/intra-links-ambiguity.stderr‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
error: `ambiguous` is both a struct and a function
2+
--> $DIR/intra-links-ambiguity.rs:27:6
3+
|
4+
LL | /// [`ambiguous`] is ambiguous.
5+
| ^^^^^^^^^^^ ambiguous link
6+
|
7+
note: lint level defined here
8+
--> $DIR/intra-links-ambiguity.rs:1:9
9+
|
10+
LL | #![deny(intra_doc_link_resolution_failure)]
11+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12+
help: to link to the struct, prefix with the item type
13+
|
14+
LL | /// [`struct@ambiguous`] is ambiguous.
15+
| ^^^^^^^^^^^^^^^^^^
16+
help: to link to the function, add parentheses
17+
|
18+
LL | /// [`ambiguous()`] is ambiguous.
19+
| ^^^^^^^^^^^^^
20+
21+
error: `ambiguous` is both a struct and a function
22+
--> $DIR/intra-links-ambiguity.rs:29:6
23+
|
24+
LL | /// [ambiguous] is ambiguous.
25+
| ^^^^^^^^^ ambiguous link
26+
help: to link to the struct, prefix with the item type
27+
|
28+
LL | /// [struct@ambiguous] is ambiguous.
29+
| ^^^^^^^^^^^^^^^^
30+
help: to link to the function, add parentheses
31+
|
32+
LL | /// [ambiguous()] is ambiguous.
33+
| ^^^^^^^^^^^
34+
35+
error: `multi_conflict` is a macro, a struct, and a function
36+
--> $DIR/intra-links-ambiguity.rs:31:6
37+
|
38+
LL | /// [`multi_conflict`] is a three-way conflict.
39+
| ^^^^^^^^^^^^^^^^ ambiguous link
40+
help: to link to the macro, prefix with the item type
41+
|
42+
LL | /// [`macro@multi_conflict`] is a three-way conflict.
43+
| ^^^^^^^^^^^^^^^^^^^^^^
44+
help: to link to the struct, prefix with the item type
45+
|
46+
LL | /// [`struct@multi_conflict`] is a three-way conflict.
47+
| ^^^^^^^^^^^^^^^^^^^^^^^
48+
help: to link to the function, add parentheses
49+
|
50+
LL | /// [`multi_conflict()`] is a three-way conflict.
51+
| ^^^^^^^^^^^^^^^^^^
52+
53+
error: `type_and_value` is both a module and a constant
54+
--> $DIR/intra-links-ambiguity.rs:33:16
55+
|
56+
LL | /// Ambiguous [type_and_value].
57+
| ^^^^^^^^^^^^^^ ambiguous link
58+
help: to link to the module, prefix with the item type
59+
|
60+
LL | /// Ambiguous [module@type_and_value].
61+
| ^^^^^^^^^^^^^^^^^^^^^
62+
help: to link to the constant, prefix with the item type
63+
|
64+
LL | /// Ambiguous [const@type_and_value].
65+
| ^^^^^^^^^^^^^^^^^^^^
66+
67+
error: `foo::bar` is both an enum and a function
68+
--> $DIR/intra-links-ambiguity.rs:35:42
69+
|
70+
LL | /// Ambiguous non-implied shortcut link [`foo::bar`].
71+
| ^^^^^^^^^^ ambiguous link
72+
help: to link to the enum, prefix with the item type
73+
|
74+
LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`].
75+
| ^^^^^^^^^^^^^^^
76+
help: to link to the function, add parentheses
77+
|
78+
LL | /// Ambiguous non-implied shortcut link [`foo::bar()`].
79+
| ^^^^^^^^^^^^
80+
81+
error: aborting due to 5 previous errors
82+

0 commit comments

Comments
 (0)