Use more appropriate return type for resolve_associated_item

Previously, the types looked like this:

- None means this is not an associated item (but may be a variant field)
- Some(Err) means this is known to be an error. I think the only way that can happen is if it resolved and but you had your own anchor.
- Some(Ok(_, None)) was impossible.

Now, this returns a nested Option and does the error handling and
fiddling with the side channel in the caller. As a side-effect, it also
removes duplicate error handling.

This has one small change in behavior, which is that
`resolve_primitive_associated_item` now goes through `variant_field` if
it fails to resolve something.  This is not ideal, but since it will be
quickly rejected anyway, I think the performance hit is worth the
cleanup.

This also fixes a bug where struct fields would forget to set the side
channel, adds a test for the bug, and ignores `private_intra_doc_links`
in rustc_resolve (since it's always documented with
--document-private-items).

Author: jyn514

compiler/rustc_resolve/src/lib.rs

@@ -18,6 +18,7 @@
 #![feature(nll)]
 #![cfg_attr(bootstrap, feature(or_patterns))]
 #![recursion_limit = "256"]
+#![allow(rustdoc::private_intra_doc_links)]
 
 pub use rustc_hir::def::{Namespace, PerNS};
 

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -368,54 +368,28 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
     }
 
     /// Given a primitive type, try to resolve an associated item.
-    ///
-    /// HACK(jynelson): `item_str` is passed in instead of derived from `item_name` so the
-    /// lifetimes on `&'path` will work.
     fn resolve_primitive_associated_item(
         &self,
         prim_ty: PrimitiveType,
         ns: Namespace,
-        module_id: DefId,
         item_name: Symbol,
-    ) -> Result<(Res, Option<String>), ErrorKind<'path>> {
+    ) -> Option<(Res, String, Option<(DefKind, DefId)>)> {
         let tcx = self.cx.tcx;
 
-        prim_ty
-            .impls(tcx)
-            .into_iter()
-            .find_map(|&impl_| {
-                tcx.associated_items(impl_)
-                    .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_)
-                    .map(|item| {
-                        let kind = item.kind;
-                        self.kind_side_channel.set(Some((kind.as_def_kind(), item.def_id)));
-                        match kind {
-                            ty::AssocKind::Fn => "method",
-                            ty::AssocKind::Const => "associatedconstant",
-                            ty::AssocKind::Type => "associatedtype",
-                        }
-                    })
-                    .map(|out| {
-                        (
-                            Res::Primitive(prim_ty),
-                            Some(format!("{}#{}.{}", prim_ty.as_str(), out, item_name)),
-                        )
-                    })
-            })
-            .ok_or_else(|| {
-                debug!(
-                    "returning primitive error for {}::{} in {} namespace",
-                    prim_ty.as_str(),
-                    item_name,
-                    ns.descr()
-                );
-                ResolutionFailure::NotResolved {
-                    module_id,
-                    partial_res: Some(Res::Primitive(prim_ty)),
-                    unresolved: item_name.to_string().into(),
-                }
-                .into()
-            })
+        prim_ty.impls(tcx).into_iter().find_map(|&impl_| {
+            tcx.associated_items(impl_)
+                .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_)
+                .map(|item| {
+                    let kind = item.kind;
+                    let out = match kind {
+                        ty::AssocKind::Fn => "method",
+                        ty::AssocKind::Const => "associatedconstant",
+                        ty::AssocKind::Type => "associatedtype",
+                    };
+                    let fragment = format!("{}#{}.{}", prim_ty.as_str(), out, item_name);
+                    (Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id)))
+                })
+        })
     }
 
     /// Resolves a string as a macro.
@@ -538,7 +512,21 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
         resolve_primitive(&path_root, TypeNS)
             .or_else(|| self.resolve_path(&path_root, TypeNS, module_id))
             .and_then(|ty_res| {
-                self.resolve_associated_item(ty_res, item_name, ns, module_id, extra_fragment)
+                let (res, fragment, side_channel) =
+                    self.resolve_associated_item(ty_res, item_name, ns, module_id)?;
+                let result = if extra_fragment.is_some() {
+                    let diag_res = side_channel.map_or(res, |(k, r)| Res::Def(k, r));
+                    Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(diag_res)))
+                } else {
+                    // HACK(jynelson): `clean` expects the type, not the associated item
+                    // but the disambiguator logic expects the associated item.
+                    // Store the kind in a side channel so that only the disambiguator logic looks at it.
+                    if let Some((kind, id)) = side_channel {
+                        self.kind_side_channel.set(Some((kind, id)));
+                    }
+                    Ok((res, Some(fragment)))
+                };
+                Some(result)
             })
             .unwrap_or_else(|| {
                 if ns == Namespace::ValueNS {
@@ -554,21 +542,21 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
             })
     }
 
+    /// Returns:
+    /// - None if no associated item was found
+    /// - Some((_, _, Some(_))) if an item was found and should go through a side channel
+    /// - Some((_, _, None)) otherwise
     fn resolve_associated_item(
         &mut self,
         root_res: Res,
         item_name: Symbol,
         ns: Namespace,
         module_id: DefId,
-        extra_fragment: &Option<String>,
-        // lol this is so bad
-    ) -> Option<Result<(Res, Option<String>), ErrorKind<'static>>> {
+    ) -> Option<(Res, String, Option<(DefKind, DefId)>)> {
         let tcx = self.cx.tcx;
 
         match root_res {
-            Res::Primitive(prim) => {
-                Some(self.resolve_primitive_associated_item(prim, ns, module_id, item_name))
-            }
+            Res::Primitive(prim) => self.resolve_primitive_associated_item(prim, ns, item_name),
             Res::Def(
                 DefKind::Struct
                 | DefKind::Union
@@ -611,17 +599,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                         ty::AssocKind::Const => "associatedconstant",
                         ty::AssocKind::Type => "associatedtype",
                     };
-                    return Some(if extra_fragment.is_some() {
-                        Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
-                            root_res,
-                        )))
-                    } else {
-                        // HACK(jynelson): `clean` expects the type, not the associated item
-                        // but the disambiguator logic expects the associated item.
-                        // Store the kind in a side channel so that only the disambiguator logic looks at it.
-                        self.kind_side_channel.set(Some((kind.as_def_kind(), id)));
-                        Ok((root_res, Some(format!("{}.{}", out, item_name))))
-                    });
+                    // HACK(jynelson): `clean` expects the type, not the associated item
+                    // but the disambiguator logic expects the associated item.
+                    // Store the kind in a side channel so that only the disambiguator logic looks at it.
+                    return Some((
+                        root_res,
+                        format!("{}.{}", out, item_name),
+                        Some((kind.as_def_kind(), id)),
+                    ));
                 }
 
                 if ns != Namespace::ValueNS {
@@ -640,22 +625,16 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                 } else {
                     def.non_enum_variant().fields.iter().find(|item| item.ident.name == item_name)
                 }?;
-                Some(if extra_fragment.is_some() {
-                    let res = Res::Def(
-                        if def.is_enum() { DefKind::Variant } else { DefKind::Field },
-                        field.did,
-                    );
-                    Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res)))
-                } else {
-                    Ok((
-                        root_res,
-                        Some(format!(
-                            "{}.{}",
-                            if def.is_enum() { "variant" } else { "structfield" },
-                            field.ident
-                        )),
-                    ))
-                })
+                let kind = if def.is_enum() { DefKind::Variant } else { DefKind::Field };
+                Some((
+                    root_res,
+                    format!(
+                        "{}.{}",
+                        if def.is_enum() { "variant" } else { "structfield" },
+                        field.ident
+                    ),
+                    Some((kind, field.did)),
+                ))
             }
             Res::Def(DefKind::Trait, did) => tcx
                 .associated_items(did)
@@ -673,14 +652,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
                         }
                     };
 
-                    if extra_fragment.is_some() {
-                        Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(
-                            root_res,
-                        )))
-                    } else {
-                        let res = Res::Def(item.kind.as_def_kind(), item.def_id);
-                        Ok((res, Some(format!("{}.{}", kind, item_name))))
-                    }
+                    let res = Res::Def(item.kind.as_def_kind(), item.def_id);
+                    (res, format!("{}.{}", kind, item_name), None)
                 }),
             _ => None,
         }

src/test/rustdoc-ui/intra-doc/private.private.stderr

@@ -1,19 +1,27 @@
 warning: public documentation for `DocMe` links to private item `DontDocMe`
-  --> $DIR/private.rs:5:11
+  --> $DIR/private.rs:7:11
    |
-LL | /// docs [DontDocMe] [DontDocMe::f]
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
    |           ^^^^^^^^^ this item is private
    |
    = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default
    = note: this link resolves only because you passed `--document-private-items`, but will break without
 
 warning: public documentation for `DocMe` links to private item `DontDocMe::f`
-  --> $DIR/private.rs:5:23
+  --> $DIR/private.rs:7:23
    |
-LL | /// docs [DontDocMe] [DontDocMe::f]
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
    |                       ^^^^^^^^^^^^ this item is private
    |
    = note: this link resolves only because you passed `--document-private-items`, but will break without
 
-warning: 2 warnings emitted
+warning: public documentation for `DocMe` links to private item `DontDocMe::x`
+  --> $DIR/private.rs:7:38
+   |
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
+   |                                      ^^^^^^^^^^^^ this item is private
+   |
+   = note: this link resolves only because you passed `--document-private-items`, but will break without
+
+warning: 3 warnings emitted
 

src/test/rustdoc-ui/intra-doc/private.public.stderr

@@ -1,19 +1,27 @@
 warning: public documentation for `DocMe` links to private item `DontDocMe`
-  --> $DIR/private.rs:5:11
+  --> $DIR/private.rs:7:11
    |
-LL | /// docs [DontDocMe] [DontDocMe::f]
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
    |           ^^^^^^^^^ this item is private
    |
    = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default
    = note: this link will resolve properly if you pass `--document-private-items`
 
 warning: public documentation for `DocMe` links to private item `DontDocMe::f`
-  --> $DIR/private.rs:5:23
+  --> $DIR/private.rs:7:23
    |
-LL | /// docs [DontDocMe] [DontDocMe::f]
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
    |                       ^^^^^^^^^^^^ this item is private
    |
    = note: this link will resolve properly if you pass `--document-private-items`
 
-warning: 2 warnings emitted
+warning: public documentation for `DocMe` links to private item `DontDocMe::x`
+  --> $DIR/private.rs:7:38
+   |
+LL | /// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
+   |                                      ^^^^^^^^^^^^ this item is private
+   |
+   = note: this link will resolve properly if you pass `--document-private-items`
+
+warning: 3 warnings emitted
 

src/test/rustdoc-ui/intra-doc/private.rs

@@ -2,12 +2,16 @@
 // revisions: public private
 // [private]compile-flags: --document-private-items
 
-/// docs [DontDocMe] [DontDocMe::f]
+// make sure to update `rustdoc/intra-doc/private.rs` if you update this file
+
+/// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
 //~^ WARNING public documentation for `DocMe` links to private item `DontDocMe`
+//~| WARNING public documentation for `DocMe` links to private item `DontDocMe::x`
 //~| WARNING public documentation for `DocMe` links to private item `DontDocMe::f`
-// FIXME: for [private] we should also make sure the link was actually generated
 pub struct DocMe;
-struct DontDocMe;
+struct DontDocMe {
+    x: usize,
+}
 
 impl DontDocMe {
     fn f() {}

src/test/rustdoc/intra-doc/private.rs

@@ -1,6 +1,17 @@
 #![crate_name = "private"]
 // compile-flags: --document-private-items
-/// docs [DontDocMe]
+
+// make sure to update `rustdoc-ui/intra-doc/private.rs` if you update this file
+
+/// docs [DontDocMe] [DontDocMe::f] [DontDocMe::x]
 // @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html"]' 'DontDocMe'
+// @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html#method.f"]' 'DontDocMe::f'
+// @has private/struct.DocMe.html '//*a[@href="../private/struct.DontDocMe.html#structfield.x"]' 'DontDocMe::x'
 pub struct DocMe;
-struct DontDocMe;
+struct DontDocMe {
+    x: usize,
+}
+
+impl DontDocMe {
+    fn f() {}
+}