Rollup merge of rust-lang#58824 - euclio:intra-link-ambiguity, r=petrochenkov

overhaul intra-doc-link ambiguity warning

Fixes rust-lang#52784.

- 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.

r? @QuietMisdreavus

Author: Mark-Simulacrum

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -1,7 +1,8 @@
-use rustc::lint as lint;
-use rustc::hir;
-use rustc::hir::def::Def;
+use errors::Applicability;
+use rustc::hir::def::{Def, Namespace::{self, *}, PerNS};
 use rustc::hir::def_id::DefId;
+use rustc::hir;
+use rustc::lint as lint;
 use rustc::ty;
 use syntax;
 use syntax::ast::{self, Ident};
@@ -35,39 +36,24 @@ pub fn collect_intra_doc_links(krate: Crate, cx: &DocContext<'_>) -> Crate {
     }
 }
 
-#[derive(Debug)]
-enum PathKind {
-    /// Either a value or type, but not a macro
-    Unknown,
-    /// Macro
-    Macro,
-    /// Values, functions, consts, statics (everything in the value namespace)
-    Value,
-    /// Types, traits (everything in the type namespace)
-    Type,
-}
-
 struct LinkCollector<'a, 'tcx> {
     cx: &'a DocContext<'tcx>,
     mod_ids: Vec<ast::NodeId>,
-    is_nightly_build: bool,
 }
 
 impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     fn new(cx: &'a DocContext<'tcx>) -> Self {
         LinkCollector {
             cx,
             mod_ids: Vec::new(),
-            is_nightly_build: UnstableFeatures::from_environment().is_nightly_build(),
         }
     }
 
-    /// Resolves a given string as a path, along with whether or not it is
-    /// in the value namespace. Also returns an optional URL fragment in the case
-    /// of variants and methods.
+    /// Resolves a string as a path within a particular namespace. Also returns an optional
+    /// URL fragment in the case of variants and methods.
     fn resolve(&self,
                path_str: &str,
-               is_val: bool,
+               ns: Namespace,
                current_item: &Option<String>,
                parent_id: Option<ast::NodeId>)
         -> Result<(Def, Option<String>), ()>
@@ -78,11 +64,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         // path.
         if let Some(id) = parent_id.or(self.mod_ids.last().cloned()) {
             // FIXME: `with_scope` requires the `NodeId` of a module.
-            let result = cx.enter_resolver(|resolver| resolver.with_scope(id,
-                |resolver| {
-                    resolver.resolve_str_path_error(DUMMY_SP,
-                                                    &path_str, is_val)
-            }));
+            let result = cx.enter_resolver(|resolver| {
+                resolver.with_scope(id, |resolver| {
+                    resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns == ValueNS)
+                })
+            });
 
             if let Ok(result) = result {
                 // In case this is a trait item, skip the
@@ -95,16 +81,16 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     _ => return Ok((result.def, None))
                 };
 
-                if value != is_val {
+                if value != (ns == ValueNS) {
                     return Err(())
                 }
-            } else if let Some(prim) = is_primitive(path_str, is_val) {
+            } else if let Some(prim) = is_primitive(path_str, ns) {
                 return Ok((prim, Some(path_str.to_owned())))
             } else {
                 // If resolution failed, it may still be a method
                 // because methods are not handled by the resolver
                 // If so, bail when we're not looking for a value.
-                if !is_val {
+                if ns != ValueNS {
                     return Err(())
                 }
             }
@@ -128,7 +114,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                     path = name.clone();
                 }
             }
-            if let Some(prim) = is_primitive(&path, false) {
+            if let Some(prim) = is_primitive(&path, TypeNS) {
                 let did = primitive_impl(cx, &path).ok_or(())?;
                 return cx.tcx.associated_items(did)
                     .find(|item| item.ident.name == item_name)
@@ -152,8 +138,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                                      .find(|item| item.ident.name == item_name);
                     if let Some(item) = item {
                         let out = match item.kind {
-                            ty::AssociatedKind::Method if is_val => "method",
-                            ty::AssociatedKind::Const if is_val => "associatedconstant",
+                            ty::AssociatedKind::Method if ns == ValueNS => "method",
+                            ty::AssociatedKind::Const if ns == ValueNS => "associatedconstant",
                             _ => return Err(())
                         };
                         Ok((ty.def, Some(format!("{}.{}", out, item_name))))
@@ -190,9 +176,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                                  .find(|item| item.ident.name == item_name);
                     if let Some(item) = item {
                         let kind = match item.kind {
-                            ty::AssociatedKind::Const if is_val => "associatedconstant",
-                            ty::AssociatedKind::Type if !is_val => "associatedtype",
-                            ty::AssociatedKind::Method if is_val => {
+                            ty::AssociatedKind::Const if ns == ValueNS => "associatedconstant",
+                            ty::AssociatedKind::Type if ns == TypeNS => "associatedtype",
+                            ty::AssociatedKind::Method if ns == ValueNS => {
                                 if item.defaultness.has_value() {
                                     "method"
                                 } else {
@@ -279,39 +265,35 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
 
         look_for_tests(&cx, &dox, &item, true);
 
-        if !self.is_nightly_build {
-            return None;
-        }
-
         for (ori_link, link_range) in markdown_links(&dox) {
             // Bail early for real links.
             if ori_link.contains('/') {
                 continue;
             }
             let link = ori_link.replace("`", "");
             let (def, fragment) = {
-                let mut kind = PathKind::Unknown;
+                let mut kind = None;
                 let path_str = if let Some(prefix) =
                     ["struct@", "enum@", "type@",
                      "trait@", "union@"].iter()
                                       .find(|p| link.starts_with(**p)) {
-                    kind = PathKind::Type;
+                    kind = Some(TypeNS);
                     link.trim_start_matches(prefix)
                 } else if let Some(prefix) =
                     ["const@", "static@",
                      "value@", "function@", "mod@",
                      "fn@", "module@", "method@"]
                         .iter().find(|p| link.starts_with(**p)) {
-                    kind = PathKind::Value;
+                    kind = Some(ValueNS);
                     link.trim_start_matches(prefix)
                 } else if link.ends_with("()") {
-                    kind = PathKind::Value;
+                    kind = Some(ValueNS);
                     link.trim_end_matches("()")
                 } else if link.starts_with("macro@") {
-                    kind = PathKind::Macro;
+                    kind = Some(MacroNS);
                     link.trim_start_matches("macro@")
                 } else if link.ends_with('!') {
-                    kind = PathKind::Macro;
+                    kind = Some(MacroNS);
                     link.trim_end_matches('!')
                 } else {
                     &link[..]
@@ -323,8 +305,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                 }
 
                 match kind {
-                    PathKind::Value => {
-                        if let Ok(def) = self.resolve(path_str, true, &current_item, parent_node) {
+                    Some(ns @ ValueNS) => {
+                        if let Ok(def) = self.resolve(path_str, ns, &current_item, parent_node) {
                             def
                         } else {
                             resolution_failure(cx, &item.attrs, path_str, &dox, link_range);
@@ -334,71 +316,58 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> {
                             continue;
                         }
                     }
-                    PathKind::Type => {
-                        if let Ok(def) = self.resolve(path_str, false, &current_item, parent_node) {
+                    Some(ns @ TypeNS) => {
+                        if let Ok(def) = self.resolve(path_str, ns, &current_item, parent_node) {
                             def
                         } else {
                             resolution_failure(cx, &item.attrs, path_str, &dox, link_range);
                             // This could just be a normal link.
                             continue;
                         }
                     }
-                    PathKind::Unknown => {
+                    None => {
                         // Try everything!
-                        if let Some(macro_def) = macro_resolve(cx, path_str) {
-                            if let Ok(type_def) =
-                                self.resolve(path_str, false, &current_item, parent_node)
-                            {
-                                let (type_kind, article, type_disambig)
-                                    = type_ns_kind(type_def.0, path_str);
-                                ambiguity_error(cx, &item.attrs, path_str,
-                                                article, type_kind, &type_disambig,
-                                                "a", "macro", &format!("macro@{}", path_str));
-                                continue;
-                            } else if let Ok(value_def) =
-                                self.resolve(path_str, true, &current_item, parent_node)
-                            {
-                                let (value_kind, value_disambig)
-                                    = value_ns_kind(value_def.0, path_str)
-                                        .expect("struct and mod cases should have been \
-                                                 caught in previous branch");
-                                ambiguity_error(cx, &item.attrs, path_str,
-                                                "a", value_kind, &value_disambig,
-                                                "a", "macro", &format!("macro@{}", path_str));
-                            }
-                            (macro_def, None)
-                        } else if let Ok(type_def) =
-                            self.resolve(path_str, false, &current_item, parent_node)
-                        {
-                            // It is imperative we search for not-a-value first
-                            // Otherwise we will find struct ctors for when we are looking
-                            // for structs, and the link won't work if there is something in
-                            // both namespaces.
-                            if let Ok(value_def) =
-                                self.resolve(path_str, true, &current_item, parent_node)
-                            {
-                                let kind = value_ns_kind(value_def.0, path_str);
-                                if let Some((value_kind, value_disambig)) = kind {
-                                    let (type_kind, article, type_disambig)
-                                        = type_ns_kind(type_def.0, path_str);
-                                    ambiguity_error(cx, &item.attrs, path_str,
-                                                    article, type_kind, &type_disambig,
-                                                    "a", value_kind, &value_disambig);
-                                    continue;
-                                }
-                            }
-                            type_def
-                        } else if let Ok(value_def) =
-                            self.resolve(path_str, true, &current_item, parent_node)
-                        {
-                            value_def
-                        } else {
+                        let candidates = PerNS {
+                            macro_ns: macro_resolve(cx, path_str).map(|def| (def, None)),
+                            type_ns: self
+                                .resolve(path_str, TypeNS, &current_item, parent_node)
+                                .ok(),
+                            value_ns: self
+                                .resolve(path_str, ValueNS, &current_item, parent_node)
+                                .ok()
+                                .and_then(|(def, fragment)| {
+                                    // Constructors are picked up in the type namespace.
+                                    match def {
+                                        Def::StructCtor(..)
+                                        | Def::VariantCtor(..)
+                                        | Def::SelfCtor(..) => None,
+                                        _ => Some((def, fragment))
+                                    }
+                                }),
+                        };
+
+                        if candidates.is_empty() {
                             resolution_failure(cx, &item.attrs, path_str, &dox, link_range);
                             // this could just be a normal link
                             continue;
                         }
+
+                        let is_unambiguous = candidates.clone().present_items().count() == 1;
+                        if is_unambiguous {
+                            candidates.present_items().next().unwrap()
+                        } else {
+                            ambiguity_error(
+                                cx,
+                                &item.attrs,
+                                path_str,
+                                &dox,
+                                link_range,
+                                candidates.map(|candidate| candidate.map(|(def, _)| def)),
+                            );
+                            continue;
+                        }
                     }
-                    PathKind::Macro => {
+                    Some(MacroNS) => {
                         if let Some(def) = macro_resolve(cx, path_str) {
                             (def, None)
                         } else {
@@ -505,59 +474,114 @@ fn resolution_failure(
     diag.emit();
 }
 
-fn ambiguity_error(cx: &DocContext<'_>, attrs: &Attributes,
-                   path_str: &str,
-                   article1: &str, kind1: &str, disambig1: &str,
-                   article2: &str, kind2: &str, disambig2: &str) {
+fn ambiguity_error(
+    cx: &DocContext<'_>,
+    attrs: &Attributes,
+    path_str: &str,
+    dox: &str,
+    link_range: Option<Range<usize>>,
+    candidates: PerNS<Option<Def>>,
+) {
     let sp = span_of_attrs(attrs);
-    cx.sess()
-      .struct_span_warn(sp,
-                        &format!("`{}` is both {} {} and {} {}",
-                                 path_str, article1, kind1,
-                                 article2, kind2))
-      .help(&format!("try `{}` if you want to select the {}, \
-                      or `{}` if you want to \
-                      select the {}",
-                      disambig1, kind1, disambig2,
-                      kind2))
-      .emit();
-}
 
-/// Given a def, returns its name and disambiguator
-/// for a value namespace.
-///
-/// Returns `None` for things which cannot be ambiguous since
-/// they exist in both namespaces (structs and modules).
-fn value_ns_kind(def: Def, path_str: &str) -> Option<(&'static str, String)> {
-    match def {
-        // Structs, variants, and mods exist in both namespaces; skip them.
-        Def::StructCtor(..) | Def::Mod(..) | Def::Variant(..) |
-        Def::VariantCtor(..) | Def::SelfCtor(..)
-            => None,
-        Def::Fn(..)
-            => Some(("function", format!("{}()", path_str))),
-        Def::Method(..)
-            => Some(("method", format!("{}()", path_str))),
-        Def::Const(..)
-            => Some(("const", format!("const@{}", path_str))),
-        Def::Static(..)
-            => Some(("static", format!("static@{}", path_str))),
-        _ => Some(("value", format!("value@{}", path_str))),
+    let mut msg = format!("`{}` is ", path_str);
+
+    let candidates = [TypeNS, ValueNS, MacroNS].iter().filter_map(|&ns| {
+        candidates[ns].map(|def| (def, ns))
+    }).collect::<Vec<_>>();
+    match candidates.as_slice() {
+        [(first_def, _), (second_def, _)] => {
+            msg += &format!(
+                "both {} {} and {} {}",
+                first_def.article(),
+                first_def.kind_name(),
+                second_def.article(),
+                second_def.kind_name(),
+            );
+        }
+        _ => {
+            let mut candidates = candidates.iter().peekable();
+            while let Some((def, _)) = candidates.next() {
+                if candidates.peek().is_some() {
+                    msg += &format!("{} {}, ", def.article(), def.kind_name());
+                } else {
+                    msg += &format!("and {} {}", def.article(), def.kind_name());
+                }
+            }
+        }
     }
-}
 
-/// Given a def, returns its name, the article to be used, and a disambiguator
-/// for the type namespace.
-fn type_ns_kind(def: Def, path_str: &str) -> (&'static str, &'static str, String) {
-    let (kind, article) = match def {
-        // We can still have non-tuple structs.
-        Def::Struct(..) => ("struct", "a"),
-        Def::Enum(..) => ("enum", "an"),
-        Def::Trait(..) => ("trait", "a"),
-        Def::Union(..) => ("union", "a"),
-        _ => ("type", "a"),
-    };
-    (kind, article, format!("{}@{}", kind, path_str))
+    let mut diag = cx.tcx.struct_span_lint_hir(
+        lint::builtin::INTRA_DOC_LINK_RESOLUTION_FAILURE,
+        hir::CRATE_HIR_ID,
+        sp,
+        &msg,
+    );
+
+    if let Some(link_range) = link_range {
+        if let Some(sp) = super::source_span_for_markdown_range(cx, dox, &link_range, attrs) {
+            diag.set_span(sp);
+            diag.span_label(sp, "ambiguous link");
+
+            for (def, ns) in candidates {
+                let (action, mut suggestion) = match def {
+                    Def::Method(..) | Def::Fn(..) => {
+                        ("add parentheses", format!("{}()", path_str))
+                    }
+                    Def::Macro(..) => {
+                        ("add an exclamation mark", format!("{}!", path_str))
+                    }
+                    _ => {
+                        let type_ = match (def, ns) {
+                            (Def::Const(..), _) => "const",
+                            (Def::Static(..), _) => "static",
+                            (Def::Struct(..), _) => "struct",
+                            (Def::Enum(..), _) => "enum",
+                            (Def::Union(..), _) => "union",
+                            (Def::Trait(..), _) => "trait",
+                            (Def::Mod(..), _) => "module",
+                            (_, TypeNS) => "type",
+                            (_, ValueNS) => "value",
+                            (_, 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);
+                }
+
+                diag.span_suggestion(
+                    sp,
+                    &format!("to link to the {}, {}", def.kind_name(), action),
+                    suggestion,
+                    Applicability::MaybeIncorrect,
+                );
+            }
+        } else {
+            // 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("");
+
+            // Print the line containing the `link_range` and manually mark it with '^'s.
+            diag.note(&format!(
+                "the link appears in this line:\n\n{line}\n\
+                 {indicator: <before$}{indicator:^<found$}",
+                line=line,
+                indicator="",
+                before=link_range.start - last_new_line_offset,
+                found=link_range.len(),
+            ));
+        }
+    }
+
+    diag.emit();
 }
 
 /// Given an enum variant's def, return the def of its enum and the associated fragment.
@@ -594,11 +618,11 @@ const PRIMITIVES: &[(&str, Def)] = &[
     ("char",  Def::PrimTy(hir::PrimTy::Char)),
 ];
 
-fn is_primitive(path_str: &str, is_val: bool) -> Option<Def> {
-    if is_val {
-        None
-    } else {
+fn is_primitive(path_str: &str, ns: Namespace) -> Option<Def> {
+    if ns == TypeNS {
         PRIMITIVES.iter().find(|x| x.0 == path_str).map(|x| x.1)
+    } else {
+        None
     }
 }
 

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

@@ -0,0 +1,36 @@
+#![deny(intra_doc_link_resolution_failure)]
+#![allow(non_camel_case_types)]
+#![allow(non_upper_case_globals)]
+
+pub fn ambiguous() {}
+
+pub struct ambiguous {}
+
+#[macro_export]
+macro_rules! multi_conflict { () => {} }
+
+#[allow(non_camel_case_types)]
+pub struct multi_conflict {}
+
+pub fn multi_conflict() {}
+
+pub mod type_and_value {}
+
+pub const type_and_value: i32 = 0;
+
+pub mod foo {
+    pub enum bar {}
+
+    pub fn bar() {}
+}
+
+/// [`ambiguous`] is ambiguous. //~ERROR `ambiguous`
+///
+/// [ambiguous] is ambiguous. //~ERROR ambiguous
+///
+/// [`multi_conflict`] is a three-way conflict. //~ERROR `multi_conflict`
+///
+/// Ambiguous [type_and_value]. //~ERROR type_and_value
+///
+/// Ambiguous non-implied shortcut link [`foo::bar`]. //~ERROR `foo::bar`
+pub struct Docs {}

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

@@ -0,0 +1,82 @@
+error: `ambiguous` is both a struct and a function
+  --> $DIR/intra-links-ambiguity.rs:27:6
+   |
+LL | /// [`ambiguous`] is ambiguous. //~ERROR `ambiguous`
+   |      ^^^^^^^^^^^ ambiguous link
+   |
+note: lint level defined here
+  --> $DIR/intra-links-ambiguity.rs:1:9
+   |
+LL | #![deny(intra_doc_link_resolution_failure)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: to link to the struct, prefix with the item type
+   |
+LL | /// [`struct@ambiguous`] is ambiguous. //~ERROR `ambiguous`
+   |      ^^^^^^^^^^^^^^^^^^
+help: to link to the function, add parentheses
+   |
+LL | /// [`ambiguous()`] is ambiguous. //~ERROR `ambiguous`
+   |      ^^^^^^^^^^^^^
+
+error: `ambiguous` is both a struct and a function
+  --> $DIR/intra-links-ambiguity.rs:29:6
+   |
+LL | /// [ambiguous] is ambiguous. //~ERROR ambiguous
+   |      ^^^^^^^^^ ambiguous link
+help: to link to the struct, prefix with the item type
+   |
+LL | /// [struct@ambiguous] is ambiguous. //~ERROR ambiguous
+   |      ^^^^^^^^^^^^^^^^
+help: to link to the function, add parentheses
+   |
+LL | /// [ambiguous()] is ambiguous. //~ERROR ambiguous
+   |      ^^^^^^^^^^^
+
+error: `multi_conflict` is a struct, a function, and a macro
+  --> $DIR/intra-links-ambiguity.rs:31:6
+   |
+LL | /// [`multi_conflict`] is a three-way conflict. //~ERROR `multi_conflict`
+   |      ^^^^^^^^^^^^^^^^ ambiguous link
+help: to link to the struct, prefix with the item type
+   |
+LL | /// [`struct@multi_conflict`] is a three-way conflict. //~ERROR `multi_conflict`
+   |      ^^^^^^^^^^^^^^^^^^^^^^^
+help: to link to the function, add parentheses
+   |
+LL | /// [`multi_conflict()`] is a three-way conflict. //~ERROR `multi_conflict`
+   |      ^^^^^^^^^^^^^^^^^^
+help: to link to the macro, add an exclamation mark
+   |
+LL | /// [`multi_conflict!`] is a three-way conflict. //~ERROR `multi_conflict`
+   |      ^^^^^^^^^^^^^^^^^
+
+error: `type_and_value` is both a module and a constant
+  --> $DIR/intra-links-ambiguity.rs:33:16
+   |
+LL | /// Ambiguous [type_and_value]. //~ERROR type_and_value
+   |                ^^^^^^^^^^^^^^ ambiguous link
+help: to link to the module, prefix with the item type
+   |
+LL | /// Ambiguous [module@type_and_value]. //~ERROR type_and_value
+   |                ^^^^^^^^^^^^^^^^^^^^^
+help: to link to the constant, prefix with the item type
+   |
+LL | /// Ambiguous [const@type_and_value]. //~ERROR type_and_value
+   |                ^^^^^^^^^^^^^^^^^^^^
+
+error: `foo::bar` is both an enum and a function
+  --> $DIR/intra-links-ambiguity.rs:35:42
+   |
+LL | /// Ambiguous non-implied shortcut link [`foo::bar`]. //~ERROR `foo::bar`
+   |                                          ^^^^^^^^^^ ambiguous link
+help: to link to the enum, prefix with the item type
+   |
+LL | /// Ambiguous non-implied shortcut link [`enum@foo::bar`]. //~ERROR `foo::bar`
+   |                                          ^^^^^^^^^^^^^^^
+help: to link to the function, add parentheses
+   |
+LL | /// Ambiguous non-implied shortcut link [`foo::bar()`]. //~ERROR `foo::bar`
+   |                                          ^^^^^^^^^^^^
+
+error: aborting due to 5 previous errors
+

src/test/rustdoc/intra-links.rs

@@ -22,6 +22,7 @@
 //! * [`ThisType::this_method`](ThisType::this_method)
 //! * [`ThisEnum`](ThisEnum)
 //! * [`ThisEnum::ThisVariant`](ThisEnum::ThisVariant)
+//! * [`ThisEnum::ThisVariantCtor`](ThisEnum::ThisVariantCtor)
 //! * [`ThisTrait`](ThisTrait)
 //! * [`ThisTrait::this_associated_method`](ThisTrait::this_associated_method)
 //! * [`ThisTrait::ThisAssociatedType`](ThisTrait::ThisAssociatedType)
@@ -50,7 +51,7 @@ pub struct ThisType;
 impl ThisType {
     pub fn this_method() {}
 }
-pub enum ThisEnum { ThisVariant, }
+pub enum ThisEnum { ThisVariant, ThisVariantCtor(u32), }
 pub trait ThisTrait {
     type ThisAssociatedType;
     const THIS_ASSOCIATED_CONST: u8;