diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 0f8e8894c04..7c1a61449a2 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/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,22 +36,9 @@ 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, - is_nightly_build: bool, } impl<'a, 'tcx> LinkCollector<'a, 'tcx> { @@ -58,16 +46,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { 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, parent_id: Option) -> Result<(Def, Option), ()> @@ -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,10 +265,6 @@ 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('/') { @@ -296,28 +278,28 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { 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[..] @@ -329,8 +311,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } match kind { - PathKind::Value => { - if let Ok(def) = self.resolve(path_str, true, ¤t_item, parent_node) { + Some(ns @ ValueNS) => { + if let Ok(def) = self.resolve(path_str, ns, ¤t_item, parent_node) { def } else { resolution_failure(cx, &item.attrs, path_str, &dox, link_range); @@ -340,8 +322,8 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { continue; } } - PathKind::Type => { - if let Ok(def) = self.resolve(path_str, false, ¤t_item, parent_node) { + Some(ns @ TypeNS) => { + if let Ok(def) = self.resolve(path_str, ns, ¤t_item, parent_node) { def } else { resolution_failure(cx, &item.attrs, path_str, &dox, link_range); @@ -349,62 +331,49 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { 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, ¤t_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, ¤t_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, ¤t_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, ¤t_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, ¤t_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, ¤t_item, parent_node) + .ok(), + value_ns: self + .resolve(path_str, ValueNS, ¤t_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 { @@ -511,59 +480,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>, + candidates: PerNS>, +) { 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::>(); + 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: Option { - if is_val { - None - } else { +fn is_primitive(path_str: &str, ns: Namespace) -> Option { + if ns == TypeNS { PRIMITIVES.iter().find(|x| x.0 == path_str).map(|x| x.1) + } else { + None } } diff --git a/src/test/rustdoc-ui/intra-links-ambiguity.rs b/src/test/rustdoc-ui/intra-links-ambiguity.rs new file mode 100644 index 00000000000..7316fcdad67 --- /dev/null +++ b/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 {} diff --git a/src/test/rustdoc-ui/intra-links-ambiguity.stderr b/src/test/rustdoc-ui/intra-links-ambiguity.stderr new file mode 100644 index 00000000000..5d66cc1364c --- /dev/null +++ b/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. + | ^^^^^^^^^^^ 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. + | ^^^^^^^^^^^^^^^^^^ +help: to link to the function, add parentheses + | +LL | /// [`ambiguous()`] is ambiguous. + | ^^^^^^^^^^^^^ + +error: `ambiguous` is both a struct and a function + --> $DIR/intra-links-ambiguity.rs:29:6 + | +LL | /// [ambiguous] is ambiguous. + | ^^^^^^^^^ ambiguous link +help: to link to the struct, prefix with the item type + | +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/intra-links-ambiguity.rs:31:6 + | +LL | /// [`multi_conflict`] is a three-way conflict. + | ^^^^^^^^^^^^^^^^ ambiguous link +help: to link to the struct, prefix with the item type + | +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/intra-links-ambiguity.rs:33:16 + | +LL | /// Ambiguous [type_and_value]. + | ^^^^^^^^^^^^^^ ambiguous link +help: to link to the module, prefix with the item type + | +LL | /// Ambiguous [module@type_and_value]. + | ^^^^^^^^^^^^^^^^^^^^^ +help: to link to the constant, prefix with the item type + | +LL | /// Ambiguous [const@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`]. + | ^^^^^^^^^^ ambiguous link +help: to link to the enum, prefix with the item type + | +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 5 previous errors + diff --git a/src/test/rustdoc/intra-links.rs b/src/test/rustdoc/intra-links.rs index 9139fc51b09..c356ab3a8ac 100644 --- a/src/test/rustdoc/intra-links.rs +++ b/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;