From d63b3f9bbb66b6ec4c7eea42078fca23730761c1 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 4 Apr 2021 09:21:22 -0400 Subject: [PATCH 1/3] Remove duplicate unwrap_or_else --- .../passes/collect_intra_doc_links.rs | 90 ++++++++++--------- 1 file changed, 47 insertions(+), 43 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 437f42b26dd11..295f1087d63ae 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -377,7 +377,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ns: Namespace, module_id: DefId, item_name: Symbol, - item_str: &'path str, ) -> Result<(Res, Option), ErrorKind<'path>> { let tcx = self.cx.tcx; @@ -399,7 +398,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .map(|out| { ( Res::Primitive(prim_ty), - Some(format!("{}#{}.{}", prim_ty.as_str(), out, item_str)), + Some(format!("{}#{}.{}", prim_ty.as_str(), out, item_name)), ) }) }) @@ -413,7 +412,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ResolutionFailure::NotResolved { module_id, partial_res: Some(Res::Primitive(prim_ty)), - unresolved: item_str.into(), + unresolved: item_name.to_string().into(), } .into() }) @@ -490,8 +489,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id: DefId, extra_fragment: &Option, ) -> Result<(Res, Option), ErrorKind<'path>> { - let tcx = self.cx.tcx; - if let Some(res) = self.resolve_path(path_str, ns, module_id) { match res { // FIXME(#76467): make this fallthrough to lookup the associated @@ -534,29 +531,44 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { } })?; - // FIXME: are these both necessary? - let ty_res = if let Some(ty_res) = resolve_primitive(&path_root, TypeNS) + // FIXME(#83862): this arbitrarily gives precedence to primitives over modules to support + // links to primitives when `#[doc(primitive)]` is present. It should give an ambiguity + // error instead and special case *only* modules with `#[doc(primitive)]`, not all + // primitives. + resolve_primitive(&path_root, TypeNS) .or_else(|| self.resolve_path(&path_root, TypeNS, module_id)) - { - ty_res - } else { - // FIXME: this is duplicated on the end of this function. - return if ns == Namespace::ValueNS { - self.variant_field(path_str, module_id) - } else { - Err(ResolutionFailure::NotResolved { - module_id, - partial_res: None, - unresolved: path_root.into(), + .and_then(|ty_res| { + self.resolve_associated_item(ty_res, item_name, ns, module_id, extra_fragment) + }) + .unwrap_or_else(|| { + if ns == Namespace::ValueNS { + self.variant_field(path_str, module_id) + } else { + Err(ResolutionFailure::NotResolved { + module_id, + partial_res: None, + unresolved: path_root.into(), + } + .into()) } - .into()) - }; - }; + }) + } + + fn resolve_associated_item( + &mut self, + root_res: Res, + item_name: Symbol, + ns: Namespace, + module_id: DefId, + extra_fragment: &Option, + // lol this is so bad + ) -> Option), ErrorKind<'static>>> { + let tcx = self.cx.tcx; - let res = match ty_res { - Res::Primitive(prim) => Some( - self.resolve_primitive_associated_item(prim, ns, module_id, item_name, item_str), - ), + match root_res { + Res::Primitive(prim) => { + Some(self.resolve_primitive_associated_item(prim, ns, module_id, item_name)) + } Res::Def( DefKind::Struct | DefKind::Union @@ -600,13 +612,15 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ty::AssocKind::Type => "associatedtype", }; Some(if extra_fragment.is_some() { - Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res))) + 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((ty_res, Some(format!("{}.{}", out, item_str)))) + Ok((root_res, Some(format!("{}.{}", out, item_name)))) }) } else if ns == Namespace::ValueNS { debug!("looking for variants or fields named {} for {:?}", item_name, did); @@ -637,7 +651,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { )) } else { Ok(( - ty_res, + root_res, Some(format!( "{}.{}", if def.is_enum() { "variant" } else { "structfield" }, @@ -670,26 +684,16 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }; if extra_fragment.is_some() { - Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(ty_res))) + 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_str)))) + Ok((res, Some(format!("{}.{}", kind, item_name)))) } }), _ => None, - }; - res.unwrap_or_else(|| { - if ns == Namespace::ValueNS { - self.variant_field(path_str, module_id) - } else { - Err(ResolutionFailure::NotResolved { - module_id, - partial_res: Some(ty_res), - unresolved: item_str.into(), - } - .into()) - } - }) + } } /// Used for reporting better errors. From ac04dbd056a94d59699be3983e5404856a9add13 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 4 Apr 2021 09:31:10 -0400 Subject: [PATCH 2/3] Reduce indentation in `resolve_associated_item` --- .../passes/collect_intra_doc_links.rs | 80 ++++++++----------- 1 file changed, 35 insertions(+), 45 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 295f1087d63ae..54d5f1cabfce7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -611,7 +611,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ty::AssocKind::Const => "associatedconstant", ty::AssocKind::Type => "associatedtype", }; - Some(if extra_fragment.is_some() { + return Some(if extra_fragment.is_some() { Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict( root_res, ))) @@ -621,51 +621,41 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // 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)))) - }) - } else if ns == Namespace::ValueNS { - debug!("looking for variants or fields named {} for {:?}", item_name, did); - // FIXME(jynelson): why is this different from - // `variant_field`? - match tcx.type_of(did).kind() { - ty::Adt(def, _) => { - let field = if def.is_enum() { - def.all_fields().find(|item| item.ident.name == item_name) - } else { - def.non_enum_variant() - .fields - .iter() - .find(|item| item.ident.name == item_name) - }; - field.map(|item| { - if extra_fragment.is_some() { - let res = Res::Def( - if def.is_enum() { - DefKind::Variant - } else { - DefKind::Field - }, - item.did, - ); - Err(ErrorKind::AnchorFailure( - AnchorFailure::RustdocAnchorConflict(res), - )) - } else { - Ok(( - root_res, - Some(format!( - "{}.{}", - if def.is_enum() { "variant" } else { "structfield" }, - item.ident - )), - )) - } - }) - } - _ => None, - } - } else { - None + }); + } + + if ns != Namespace::ValueNS { + return None; } + debug!("looking for variants or fields named {} for {:?}", item_name, did); + // FIXME: this doesn't really belong in `associated_item` (maybe `variant_field` is better?) + // NOTE: it's different from variant_field because it resolves fields and variants, + // not variant fields (2 path segments, not 3). + let def = match tcx.type_of(did).kind() { + ty::Adt(def, _) => def, + _ => return None, + }; + let field = if def.is_enum() { + def.all_fields().find(|item| item.ident.name == item_name) + } 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 + )), + )) + }) } Res::Def(DefKind::Trait, did) => tcx .associated_items(did) From 3b7e654fad80e91064b26416a4334cd3e984a6ce Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sun, 4 Apr 2021 16:23:08 -0400 Subject: [PATCH 3/3] 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). --- compiler/rustc_resolve/src/lib.rs | 1 + .../passes/collect_intra_doc_links.rs | 139 +++++++----------- .../intra-doc/private.private.stderr | 18 ++- .../intra-doc/private.public.stderr | 18 ++- src/test/rustdoc-ui/intra-doc/private.rs | 10 +- src/test/rustdoc/intra-doc/private.rs | 15 +- 6 files changed, 103 insertions(+), 98 deletions(-) diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index d474e99021104..1c5f8996e1b45 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/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}; diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 54d5f1cabfce7..3501b7d86a46f 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/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), 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, - // lol this is so bad - ) -> Option), 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, } diff --git a/src/test/rustdoc-ui/intra-doc/private.private.stderr b/src/test/rustdoc-ui/intra-doc/private.private.stderr index cae5b1f20e6c3..392321f9c60db 100644 --- a/src/test/rustdoc-ui/intra-doc/private.private.stderr +++ b/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 diff --git a/src/test/rustdoc-ui/intra-doc/private.public.stderr b/src/test/rustdoc-ui/intra-doc/private.public.stderr index 05b202e37fbcb..5d1c34b9168d9 100644 --- a/src/test/rustdoc-ui/intra-doc/private.public.stderr +++ b/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 diff --git a/src/test/rustdoc-ui/intra-doc/private.rs b/src/test/rustdoc-ui/intra-doc/private.rs index 3782864305f1f..525332ddaac3b 100644 --- a/src/test/rustdoc-ui/intra-doc/private.rs +++ b/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() {} diff --git a/src/test/rustdoc/intra-doc/private.rs b/src/test/rustdoc/intra-doc/private.rs index f86ca44403d93..337102d6ab3fa 100644 --- a/src/test/rustdoc/intra-doc/private.rs +++ b/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() {} +}