Auto merge of #75916 - jyn514:unify-error-reporting, r=euclio

Unify error reporting for intra-doc links

- Give a suggestion even if there is no span available
- Give a more accurate description of the change than 'use the
disambiguator'
- Write much less code

Closes #75836.
r? @euclio
cc @pickfire - this gets rid of 'disambiguator' like you suggested in https://github.com/rust-lang/rust/pull/75079#discussion_r464464195.

src/librustc_hir/def.rs

@@ -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::Def(kind, ..) => kind.ns(),
-            Res::PrimTy(..) | Res::SelfTy(..) | Res::ToolMod => ns == Namespace::TypeNS,
+            Res::PrimTy(..) | Res::SelfTy(..) | Res::ToolMod => Some(Namespace::TypeNS),
-            Res::SelfCtor(..) | Res::Local(..) => ns == Namespace::ValueNS,
+            Res::SelfCtor(..) | Res::Local(..) => Some(Namespace::ValueNS),
-            Res::NonMacroAttr(..) => ns == Namespace::MacroNS,
+            Res::NonMacroAttr(..) => Some(Namespace::MacroNS),
-            Res::Err => true,
+            Res::Err => None,
         }
     }
+
+    /// 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)
+    }
 }

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -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;
                         }
@@ -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;
                     }
@@ -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 {
+                    suggest_disambiguator(resolved, diag, path_str, &dox, sp, &link_range);
-                        diag.span_suggestion(
-                            sp,
-                            &help_msg,
-                            suggestion,
-                            Applicability::MaybeIncorrect,
-                        );
-                    } else {
-                        diag.help(&format!("{}: {}", help_msg, suggestion));
-                    }
                 });
             };
             if let Res::PrimTy(_) = res {
@@ -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 {
+        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",
@@ -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 `@`
+        (PREFIX, format!("{}@{}", prefix, path_str))
     }
 
     fn ns(self) -> Namespace {
@@ -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(),
@@ -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 {
@@ -1276,54 +1278,40 @@ 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 in candidates {
-
+            let disambiguator = Disambiguator::from_res(res);
-            for (res, ns) in candidates {
+            suggest_disambiguator(disambiguator, diag, path_str, dox, sp, &link_range);
-                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))
-                    }
-                };
-
-                if dox.bytes().nth(link_range.start) == Some(b'`') {
-                    suggestion = format!("`{}`", suggestion);
-                }
-
-                // 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,
-                );
-            }
         }
     });
 }
 
+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);
+
+    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(
     cx: &DocContext<'_>,
     item: &Item,

src/test/rustdoc-ui/intra-link-prim-conflict.rs

@@ -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
 }

src/test/rustdoc-ui/intra-link-prim-conflict.stderr

@@ -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]
+LL | /// [mod@char]
-   |      ^^^^^^^^^^^
+   |      ^^^^^^^^
-help: to link to the builtin type, prefix with the item type
+help: to link to the builtin type, prefix with the item kind
    |
 LL | /// [prim@char]
    |      ^^^^^^^^^
@@ -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]
+LL | /// [mod@char]
-   |      ^^^^^^^^^^^
+   |      ^^^^^^^^
-help: to link to the builtin type, prefix with the item type
+help: to link to the builtin type, prefix with the item kind
    |
 LL | /// [prim@char]
    |      ^^^^^^^^^
@@ -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
 

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

@@ -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.
    |      ^^^^^^^^^^^^^^^^^^
@@ -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.
    |      ^^^^^^^^^^^^^^^^
@@ -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.
    |      ^^^^^^^^^^^^^^^^^^^^^^^
@@ -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].
+LL | /// Ambiguous [mod@type_and_value].
-   |                ^^^^^^^^^^^^^^^^^^^^^
+   |                ^^^^^^^^^^^^^^^^^^
-help: to link to the constant, prefix with the item type
+help: to link to the constant, prefix with the item kind
    |
 LL | /// Ambiguous [const@type_and_value].
    |                ^^^^^^^^^^^^^^^^^^^^
@@ -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`].
    |                                          ^^^^^^^^^^^^^^^

src/test/rustdoc-ui/intra-links-disambiguator-mismatch.rs

@@ -14,55 +14,55 @@ trait T {}
 /// Link to [struct@S]
 //~^ ERROR incompatible link kind for `S`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [mod@S]
 //~^ ERROR incompatible link kind for `S`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [union@S]
 //~^ ERROR incompatible link kind for `S`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [trait@S]
 //~^ ERROR incompatible link kind for `S`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [struct@T]
 //~^ ERROR incompatible link kind for `T`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [derive@m]
 //~^ ERROR incompatible link kind for `m`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP add an exclamation mark
 
 /// Link to [const@s]
 //~^ ERROR incompatible link kind for `s`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [static@c]
 //~^ ERROR incompatible link kind for `c`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [fn@c]
 //~^ ERROR incompatible link kind for `c`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [c()]
 //~^ ERROR incompatible link kind for `c`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP prefix with the item kind
 
 /// Link to [const@f]
 //~^ ERROR incompatible link kind for `f`
 //~| NOTE this link resolved
-//~| HELP use its disambiguator
+//~| HELP add parentheses
 pub fn f() {}

src/test/rustdoc-ui/intra-links-disambiguator-mismatch.stderr

@@ -2,7 +2,7 @@ error: incompatible link kind for `S`
   --> $DIR/intra-links-disambiguator-mismatch.rs:14:14
    |
 LL | /// Link to [struct@S]
-   |              ^^^^^^^^ help: to link to the enum, use its disambiguator: `enum@S`
+   |              ^^^^^^^^
    |
 note: the lint level is defined here
   --> $DIR/intra-links-disambiguator-mismatch.rs:1:9
@@ -10,44 +10,64 @@ note: the lint level is defined here
 LL | #![deny(broken_intra_doc_links)]
    |         ^^^^^^^^^^^^^^^^^^^^^^
    = note: this link resolved to an enum, which is not a struct
+help: to link to the enum, prefix with the item kind
+   |
+LL | /// Link to [enum@S]
+   |              ^^^^^^
 
 error: incompatible link kind for `S`
   --> $DIR/intra-links-disambiguator-mismatch.rs:19:14
    |
 LL | /// Link to [mod@S]
-   |              ^^^^^ help: to link to the enum, use its disambiguator: `enum@S`
+   |              ^^^^^
    |
    = note: this link resolved to an enum, which is not a module
+help: to link to the enum, prefix with the item kind
+   |
+LL | /// Link to [enum@S]
+   |              ^^^^^^
 
 error: incompatible link kind for `S`
   --> $DIR/intra-links-disambiguator-mismatch.rs:24:14
    |
 LL | /// Link to [union@S]
-   |              ^^^^^^^ help: to link to the enum, use its disambiguator: `enum@S`
+   |              ^^^^^^^
    |
    = note: this link resolved to an enum, which is not a union
+help: to link to the enum, prefix with the item kind
+   |
+LL | /// Link to [enum@S]
+   |              ^^^^^^
 
 error: incompatible link kind for `S`
   --> $DIR/intra-links-disambiguator-mismatch.rs:29:14
    |
 LL | /// Link to [trait@S]
-   |              ^^^^^^^ help: to link to the enum, use its disambiguator: `enum@S`
+   |              ^^^^^^^
    |
    = note: this link resolved to an enum, which is not a trait
+help: to link to the enum, prefix with the item kind
+   |
+LL | /// Link to [enum@S]
+   |              ^^^^^^
 
 error: incompatible link kind for `T`
   --> $DIR/intra-links-disambiguator-mismatch.rs:34:14
    |
 LL | /// Link to [struct@T]
-   |              ^^^^^^^^ help: to link to the trait, use its disambiguator: `trait@T`
+   |              ^^^^^^^^
    |
    = note: this link resolved to a trait, which is not a struct
+help: to link to the trait, prefix with the item kind
+   |
+LL | /// Link to [trait@T]
+   |              ^^^^^^^
 
 error: incompatible link kind for `m`
   --> $DIR/intra-links-disambiguator-mismatch.rs:39:14
    |
 LL | /// Link to [derive@m]
-   |              ^^^^^^^^ help: to link to the macro, use its disambiguator: `m!`
+   |              ^^^^^^^^ help: to link to the macro, add an exclamation mark: `m!`
    |
    = note: this link resolved to a macro, which is not a derive macro
 
@@ -55,39 +75,55 @@ error: incompatible link kind for `s`
   --> $DIR/intra-links-disambiguator-mismatch.rs:44:14
    |
 LL | /// Link to [const@s]
-   |              ^^^^^^^ help: to link to the static, use its disambiguator: `static@s`
+   |              ^^^^^^^
    |
    = note: this link resolved to a static, which is not a constant
+help: to link to the static, prefix with the item kind
+   |
+LL | /// Link to [static@s]
+   |              ^^^^^^^^
 
 error: incompatible link kind for `c`
   --> $DIR/intra-links-disambiguator-mismatch.rs:49:14
    |
 LL | /// Link to [static@c]
-   |              ^^^^^^^^ help: to link to the constant, use its disambiguator: `const@c`
+   |              ^^^^^^^^
    |
    = note: this link resolved to a constant, which is not a static
+help: to link to the constant, prefix with the item kind
+   |
+LL | /// Link to [const@c]
+   |              ^^^^^^^
 
 error: incompatible link kind for `c`
   --> $DIR/intra-links-disambiguator-mismatch.rs:54:14
    |
 LL | /// Link to [fn@c]
-   |              ^^^^ help: to link to the constant, use its disambiguator: `const@c`
+   |              ^^^^
    |
    = note: this link resolved to a constant, which is not a function
+help: to link to the constant, prefix with the item kind
+   |
+LL | /// Link to [const@c]
+   |              ^^^^^^^
 
 error: incompatible link kind for `c`
   --> $DIR/intra-links-disambiguator-mismatch.rs:59:14
    |
 LL | /// Link to [c()]
-   |              ^^^ help: to link to the constant, use its disambiguator: `const@c`
+   |              ^^^
    |
    = note: this link resolved to a constant, which is not a function
+help: to link to the constant, prefix with the item kind
+   |
+LL | /// Link to [const@c]
+   |              ^^^^^^^
 
 error: incompatible link kind for `f`
   --> $DIR/intra-links-disambiguator-mismatch.rs:64:14
    |
 LL | /// Link to [const@f]
-   |              ^^^^^^^ help: to link to the function, use its disambiguator: `f()`
+   |              ^^^^^^^ help: to link to the function, add parentheses: `f()`
    |
    = note: this link resolved to a function, which is not a constant