From 48f95c164967539bf75dd9839a208c4a0ce6a615 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 16444c309237e436e4c9da725aa32fe781866099 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 3611a643b3e4522f6e4168c3d1a360c9a3884064 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. --- .../passes/collect_intra_doc_links.rs | 139 +++++++----------- 1 file changed, 56 insertions(+), 83 deletions(-) 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, }