From f89e2dd935d86bf2da3e4181e47ecaf56b5de026 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 16:50:20 -0800 Subject: [PATCH 1/9] Move anchor conflict check to call site I think it makes the code easier to understand. --- src/librustdoc/passes/collect_intra_doc_links.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 79530086282..40b3cb8641c 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -514,7 +514,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => assert_eq!(ns, ValueNS), Res::Def(DefKind::AssocTy, _) => assert_eq!(ns, TypeNS), Res::Def(DefKind::Variant, _) => { - return handle_variant(self.cx, res, extra_fragment); + if extra_fragment.is_some() { + // NOTE: `res` can never be a primitive since this match arm means + // `tcx.def_kind(res) == DefKind::Variant`. + return Err(ErrorKind::AnchorFailure( + AnchorFailure::RustdocAnchorConflict(res), + )); + } + return handle_variant(self.cx, res); } // Not a trait item; just return what we found. _ => return Ok((res, extra_fragment.clone())), @@ -2272,14 +2279,9 @@ fn privacy_error(cx: &DocContext<'_>, diag_info: &DiagnosticInfo<'_>, path_str: fn handle_variant( cx: &DocContext<'_>, res: Res, - extra_fragment: &Option, ) -> Result<(Res, Option), ErrorKind<'static>> { use rustc_middle::ty::DefIdTree; - if extra_fragment.is_some() { - // NOTE: `res` can never be a primitive since this function is only called when `tcx.def_kind(res) == DefKind::Variant`. - return Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res))); - } cx.tcx .parent(res.def_id(cx.tcx)) .map(|parent| { From 3b50a4e286e81426862867d8ce31ebbd9bcc930c Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 17:17:18 -0800 Subject: [PATCH 2/9] Use `DefId`s instead of names in `UrlFragment` This is the next step in computing more "semantic" information during intra-doc link collection and then doing rendering all at the end. --- src/librustdoc/clean/types.rs | 3 +- .../passes/collect_intra_doc_links.rs | 94 ++++++++++--------- 2 files changed, 50 insertions(+), 47 deletions(-) diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs index f0f61bb94c8..f278c6c17bb 100644 --- a/src/librustdoc/clean/types.rs +++ b/src/librustdoc/clean/types.rs @@ -1,6 +1,5 @@ use std::cell::RefCell; use std::default::Default; -use std::fmt::Write; use std::hash::Hash; use std::lazy::SyncOnceCell as OnceCell; use std::path::PathBuf; @@ -496,7 +495,7 @@ impl Item { if let Ok((mut href, ..)) = href(*did, cx) { debug!(?href); if let Some(ref fragment) = *fragment { - write!(href, "{}", fragment).unwrap() + fragment.render(&mut href, cx.tcx()).unwrap() } Some(RenderedLink { original_text: s.clone(), diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 40b3cb8641c..08193bd765a 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -13,7 +13,7 @@ use rustc_hir::def::{ PerNS, }; use rustc_hir::def_id::{CrateNum, DefId}; -use rustc_middle::ty::{Ty, TyCtxt}; +use rustc_middle::ty::{DefIdTree, Ty, TyCtxt}; use rustc_middle::{bug, span_bug, ty}; use rustc_resolve::ParentScope; use rustc_session::lint::Lint; @@ -27,6 +27,7 @@ use pulldown_cmark::LinkType; use std::borrow::Cow; use std::cell::Cell; use std::convert::{TryFrom, TryInto}; +use std::fmt::Write; use std::mem; use std::ops::Range; @@ -240,53 +241,61 @@ enum AnchorFailure { #[derive(Clone, Debug, Hash, PartialEq, Eq)] crate enum UrlFragment { - Method(Symbol), - TyMethod(Symbol), - AssociatedConstant(Symbol), - AssociatedType(Symbol), - - StructField(Symbol), - Variant(Symbol), - VariantField { variant: Symbol, field: Symbol }, - + Def(FragmentKind, DefId), UserWritten(String), } +#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] +crate enum FragmentKind { + Method, + TyMethod, + AssociatedConstant, + AssociatedType, + + StructField, + Variant, + VariantField, +} + impl UrlFragment { /// Create a fragment for an associated item. /// /// `is_prototype` is whether this associated item is a trait method /// without a default definition. - fn from_assoc_item(name: Symbol, kind: ty::AssocKind, is_prototype: bool) -> Self { + fn from_assoc_item(def_id: DefId, kind: ty::AssocKind, is_prototype: bool) -> Self { match kind { ty::AssocKind::Fn => { if is_prototype { - UrlFragment::TyMethod(name) + UrlFragment::Def(FragmentKind::TyMethod, def_id) } else { - UrlFragment::Method(name) + UrlFragment::Def(FragmentKind::Method, def_id) } } - ty::AssocKind::Const => UrlFragment::AssociatedConstant(name), - ty::AssocKind::Type => UrlFragment::AssociatedType(name), + ty::AssocKind::Const => UrlFragment::Def(FragmentKind::AssociatedConstant, def_id), + ty::AssocKind::Type => UrlFragment::Def(FragmentKind::AssociatedType, def_id), } } -} -/// Render the fragment, including the leading `#`. -impl std::fmt::Display for UrlFragment { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "#")?; - match self { - UrlFragment::Method(name) => write!(f, "method.{}", name), - UrlFragment::TyMethod(name) => write!(f, "tymethod.{}", name), - UrlFragment::AssociatedConstant(name) => write!(f, "associatedconstant.{}", name), - UrlFragment::AssociatedType(name) => write!(f, "associatedtype.{}", name), - UrlFragment::StructField(name) => write!(f, "structfield.{}", name), - UrlFragment::Variant(name) => write!(f, "variant.{}", name), - UrlFragment::VariantField { variant, field } => { - write!(f, "variant.{}.field.{}", variant, field) + /// Render the fragment, including the leading `#`. + crate fn render(&self, s: &mut String, tcx: TyCtxt<'_>) -> std::fmt::Result { + write!(s, "#")?; + match *self { + UrlFragment::Def(kind, def_id) => { + let name = tcx.item_name(def_id); + match kind { + FragmentKind::Method => write!(s, "method.{}", name), + FragmentKind::TyMethod => write!(s, "tymethod.{}", name), + FragmentKind::AssociatedConstant => write!(s, "associatedconstant.{}", name), + FragmentKind::AssociatedType => write!(s, "associatedtype.{}", name), + FragmentKind::StructField => write!(s, "structfield.{}", name), + FragmentKind::Variant => write!(s, "variant.{}", name), + FragmentKind::VariantField => { + let variant = tcx.item_name(tcx.parent(def_id).unwrap()); + write!(s, "variant.{}.field.{}", variant, name) + } + } } - UrlFragment::UserWritten(raw) => write!(f, "{}", raw), + UrlFragment::UserWritten(ref raw) => write!(s, "{}", raw), } } } @@ -387,13 +396,12 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { } match tcx.type_of(did).kind() { ty::Adt(def, _) if def.is_enum() => { - if def.all_fields().any(|item| item.ident.name == variant_field_name) { + if let Some(field) = + def.all_fields().find(|f| f.ident.name == variant_field_name) + { Ok(( ty_res, - Some(UrlFragment::VariantField { - variant: variant_name, - field: variant_field_name, - }), + Some(UrlFragment::Def(FragmentKind::VariantField, field.did)), )) } else { Err(ResolutionFailure::NotResolved { @@ -430,7 +438,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_) .map(|item| { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item_name, kind, false); + let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); (Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id))) }) }) @@ -683,7 +691,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { assoc_item.map(|item| { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item_name, kind, false); + let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); // 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. @@ -737,7 +745,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { if let Some(item) = assoc_item { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item_name, kind, false); + let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); // 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. @@ -774,7 +782,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .find(|item| item.ident.name == item_name)?; Some(( root_res, - UrlFragment::StructField(field.ident.name), + UrlFragment::Def(FragmentKind::StructField, field.did), Some((DefKind::Field, field.did)), )) } @@ -783,7 +791,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, did) .map(|item| { let fragment = UrlFragment::from_assoc_item( - item_name, + item.def_id, item.kind, !item.defaultness.has_value(), ); @@ -919,8 +927,6 @@ fn is_derive_trait_collision(ns: &PerNS DocVisitor for LinkCollector<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { - use rustc_middle::ty::DefIdTree; - let parent_node = item.def_id.as_def_id().and_then(|did| find_nearest_parent_module(self.cx.tcx, did)); if parent_node.is_some() { @@ -2280,14 +2286,12 @@ fn handle_variant( cx: &DocContext<'_>, res: Res, ) -> Result<(Res, Option), ErrorKind<'static>> { - use rustc_middle::ty::DefIdTree; - cx.tcx .parent(res.def_id(cx.tcx)) .map(|parent| { let parent_def = Res::Def(DefKind::Enum, parent); let variant = cx.tcx.expect_variant_res(res.as_hir_res().unwrap()); - (parent_def, Some(UrlFragment::Variant(variant.ident.name))) + (parent_def, Some(UrlFragment::Def(FragmentKind::Variant, variant.def_id))) }) .ok_or_else(|| ResolutionFailure::NoParentItem.into()) } From a324fc19d4b4bd6ecb34abf318aea2e65f98c241 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 17:27:28 -0800 Subject: [PATCH 3/9] Only check for conflicting anchors in one place This coalesces the checks into one place. --- .../passes/collect_intra_doc_links.rs | 58 +++++++++++-------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 08193bd765a..7c9de76a0b6 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -513,7 +513,31 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'path str, ns: Namespace, module_id: DefId, - extra_fragment: &Option, + user_fragment: &Option, + ) -> Result<(Res, Option), ErrorKind<'path>> { + let (res, rustdoc_fragment) = self.resolve_inner(path_str, ns, module_id)?; + let chosen_fragment = match (user_fragment, rustdoc_fragment) { + (Some(_), Some(r_frag)) => { + let diag_res = match r_frag { + UrlFragment::Def(_, did) => Res::Def(self.cx.tcx.def_kind(did), did), + // FIXME: eliminate this branch somehow + UrlFragment::UserWritten(_) => unreachable!(), + }; + let failure = AnchorFailure::RustdocAnchorConflict(diag_res); + return Err(ErrorKind::AnchorFailure(failure)); + } + (Some(u_frag), None) => Some(u_frag.clone()), + (None, Some(r_frag)) => Some(r_frag), + (None, None) => None, + }; + Ok((res, chosen_fragment)) + } + + fn resolve_inner<'path>( + &mut self, + path_str: &'path str, + ns: Namespace, + module_id: DefId, ) -> Result<(Res, Option), ErrorKind<'path>> { if let Some(res) = self.resolve_path(path_str, ns, module_id) { match res { @@ -522,17 +546,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => assert_eq!(ns, ValueNS), Res::Def(DefKind::AssocTy, _) => assert_eq!(ns, TypeNS), Res::Def(DefKind::Variant, _) => { - if extra_fragment.is_some() { - // NOTE: `res` can never be a primitive since this match arm means - // `tcx.def_kind(res) == DefKind::Variant`. - return Err(ErrorKind::AnchorFailure( - AnchorFailure::RustdocAnchorConflict(res), - )); - } return handle_variant(self.cx, res); } // Not a trait item; just return what we found. - _ => return Ok((res, extra_fragment.clone())), + _ => return Ok((res, None)), } } @@ -565,21 +582,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .and_then(|ty_res| { let (res, fragment, side_channel) = self.resolve_associated_item(ty_res, item_name, ns, module_id)?; - let result = if extra_fragment.is_some() { - // NOTE: can never be a primitive since `side_channel.is_none()` only when `res` - // is a trait (and the side channel DefId is always an associated item). - 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) + + // 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))); + } + Some(Ok((res, Some(fragment)))) }) .unwrap_or_else(|| { if ns == Namespace::ValueNS { From 7e3132adb375480aebff297217e3125165cc56b2 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 17:51:27 -0800 Subject: [PATCH 4/9] Extract function for reporting feature gate error --- .../passes/collect_intra_doc_links.rs | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 7c9de76a0b6..ddb9f53c695 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1370,22 +1370,7 @@ impl LinkCollector<'_, '_> { && item.def_id.is_local() && !self.cx.tcx.features().intra_doc_pointers { - let span = super::source_span_for_markdown_range( - self.cx.tcx, - dox, - &ori_link.range, - &item.attrs, - ) - .unwrap_or_else(|| item.attr_span(self.cx.tcx)); - - rustc_session::parse::feature_err( - &self.cx.tcx.sess.parse_sess, - sym::intra_doc_pointers, - span, - "linking to associated items of raw pointers is experimental", - ) - .note("rustdoc does not allow disambiguating between `*const` and `*mut`, and pointers are unstable until it does") - .emit(); + self.report_rawptr_assoc_feature_gate(dox, &ori_link, item); } } else { match disambiguator { @@ -1412,6 +1397,20 @@ impl LinkCollector<'_, '_> { } } + fn report_rawptr_assoc_feature_gate(&self, dox: &str, ori_link: &MarkdownLink, item: &Item) { + let span = + super::source_span_for_markdown_range(self.cx.tcx, dox, &ori_link.range, &item.attrs) + .unwrap_or_else(|| item.attr_span(self.cx.tcx)); + rustc_session::parse::feature_err( + &self.cx.tcx.sess.parse_sess, + sym::intra_doc_pointers, + span, + "linking to associated items of raw pointers is experimental", + ) + .note("rustdoc does not allow disambiguating between `*const` and `*mut`, and pointers are unstable until it does") + .emit(); + } + fn resolve_with_disambiguator_cached( &mut self, key: ResolutionInfo, From 1064003f8c39e138c3fefa9104fd74ffe81df3e8 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 18:12:26 -0800 Subject: [PATCH 5/9] Use fragment instead of side channel for prim. assoc. items I had the epiphany that now that fragments are "semantic" -- rather than just strings -- they fill the role that used to be handled by the side channel. I think I may be able to get rid of the other uses of the side channel using this technique too. --- src/librustdoc/passes/collect_intra_doc_links.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index ddb9f53c695..fe390f6354c 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1353,7 +1353,9 @@ impl LinkCollector<'_, '_> { match res { Res::Primitive(prim) => { - if let Some((kind, id)) = self.kind_side_channel.take() { + if let Some(UrlFragment::Def(_, id)) = fragment { + let kind = self.cx.tcx.def_kind(id); + // We're actually resolving an associated item of a primitive, so we need to // verify the disambiguator (if any) matches the type of the associated item. // This case should really follow the same flow as the `Res::Def` branch below, From fde94538bf893fe11148354bc2da0028207ea47c Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 18:27:06 -0800 Subject: [PATCH 6/9] Use fragment instead of side channel in another place --- src/librustdoc/passes/collect_intra_doc_links.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index fe390f6354c..6711028ab1b 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1309,7 +1309,11 @@ impl LinkCollector<'_, '_> { }; let verify = |kind: DefKind, id: DefId| { - let (kind, id) = self.kind_side_channel.take().unwrap_or((kind, id)); + let (kind, id) = if let Some(UrlFragment::Def(_, id)) = fragment { + (self.cx.tcx.def_kind(id), id) + } else { + (kind, id) + }; debug!("intra-doc link to {} resolved to {:?} (id: {:?})", path_str, res, id); // Disallow e.g. linking to enums with `struct@` From 54a14e844c6a5ed51741e039e6376822f88ac121 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 21:33:07 -0800 Subject: [PATCH 7/9] Remove the last use of the side channel --- .../passes/collect_intra_doc_links.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 6711028ab1b..097cb398950 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -827,19 +827,28 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ) -> Option { // resolve() can't be used for macro namespace let result = match ns { - Namespace::MacroNS => self.resolve_macro(path_str, module_id).map_err(ErrorKind::from), + Namespace::MacroNS => self + .resolve_macro(path_str, module_id) + .map(|res| (res, None)) + .map_err(ErrorKind::from), Namespace::TypeNS | Namespace::ValueNS => { - self.resolve(path_str, ns, module_id, extra_fragment).map(|(res, _)| res) + self.resolve(path_str, ns, module_id, extra_fragment) } }; let res = match result { - Ok(res) => Some(res), + Ok((res, frag)) => { + if let Some(UrlFragment::Def(_, id)) = frag { + Some(Res::Def(self.cx.tcx.def_kind(id), id)) + } else { + Some(res) + } + } Err(ErrorKind::Resolve(box kind)) => kind.full_res(), Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res))) => Some(res), Err(ErrorKind::AnchorFailure(AnchorFailure::MultipleAnchors)) => None, }; - self.kind_side_channel.take().map(|(kind, id)| Res::Def(kind, id)).or(res) + res } } From a69e15c50160dcd986b18588ac263837a750a53c Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Wed, 5 Jan 2022 21:37:00 -0800 Subject: [PATCH 8/9] Remove the side channel Hooray! It was no longer used, so it can just be deleted. --- .../passes/collect_intra_doc_links.rs | 55 ++++--------------- 1 file changed, 11 insertions(+), 44 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 097cb398950..7aa2f158810 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -25,7 +25,6 @@ use smallvec::{smallvec, SmallVec}; use pulldown_cmark::LinkType; use std::borrow::Cow; -use std::cell::Cell; use std::convert::{TryFrom, TryInto}; use std::fmt::Write; use std::mem; @@ -48,12 +47,8 @@ crate const COLLECT_INTRA_DOC_LINKS: Pass = Pass { }; fn collect_intra_doc_links(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - let mut collector = LinkCollector { - cx, - mod_ids: Vec::new(), - kind_side_channel: Cell::new(None), - visited_links: FxHashMap::default(), - }; + let mut collector = + LinkCollector { cx, mod_ids: Vec::new(), visited_links: FxHashMap::default() }; collector.visit_crate(&krate); krate } @@ -319,7 +314,6 @@ struct DiagnosticInfo<'a> { #[derive(Clone, Debug, Hash)] struct CachedLink { pub res: (Res, Option), - pub side_channel: Option<(DefKind, DefId)>, } struct LinkCollector<'a, 'tcx> { @@ -329,10 +323,6 @@ struct LinkCollector<'a, 'tcx> { /// The last module will be used if the parent scope of the current item is /// unknown. mod_ids: Vec, - /// This is used to store the kind of associated items, - /// because `clean` and the disambiguator code expect them to be different. - /// See the code for associated items on inherent impls for details. - kind_side_channel: Cell>, /// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link. /// The link will be `None` if it could not be resolved (i.e. the error was cached). visited_links: FxHashMap>, @@ -430,7 +420,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { prim_ty: PrimitiveType, ns: Namespace, item_name: Symbol, - ) -> Option<(Res, UrlFragment, Option<(DefKind, DefId)>)> { + ) -> Option<(Res, UrlFragment)> { let tcx = self.cx.tcx; prim_ty.impls(tcx).into_iter().find_map(|&impl_| { @@ -439,7 +429,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .map(|item| { let kind = item.kind; let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); - (Res::Primitive(prim_ty), fragment, Some((kind.as_def_kind(), item.def_id))) + (Res::Primitive(prim_ty), fragment) }) }) } @@ -580,15 +570,9 @@ 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| { - let (res, fragment, side_channel) = + let (res, fragment) = self.resolve_associated_item(ty_res, item_name, ns, module_id)?; - // 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))); - } Some(Ok((res, Some(fragment)))) }) .unwrap_or_else(|| { @@ -686,7 +670,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { item_name: Symbol, ns: Namespace, module_id: DefId, - ) -> Option<(Res, UrlFragment, Option<(DefKind, DefId)>)> { + ) -> Option<(Res, UrlFragment)> { let tcx = self.cx.tcx; match root_res { @@ -702,10 +686,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { assoc_item.map(|item| { let kind = item.kind; let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); - // 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. - (root_res, fragment, Some((kind.as_def_kind(), item.def_id))) + (root_res, fragment) }) }) } @@ -756,10 +737,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { if let Some(item) = assoc_item { let kind = item.kind; let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); - // 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, fragment, Some((kind.as_def_kind(), item.def_id)))); + return Some((root_res, fragment)); } if ns != Namespace::ValueNS { @@ -790,11 +768,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .fields .iter() .find(|item| item.ident.name == item_name)?; - Some(( - root_res, - UrlFragment::Def(FragmentKind::StructField, field.did), - Some((DefKind::Field, field.did)), - )) + Some((root_res, UrlFragment::Def(FragmentKind::StructField, field.did))) } Res::Def(DefKind::Trait, did) => tcx .associated_items(did) @@ -806,7 +780,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { !item.defaultness.has_value(), ); let res = Res::Def(item.kind.as_def_kind(), item.def_id); - (res, fragment, None) + (res, fragment) }), _ => None, } @@ -1436,7 +1410,6 @@ impl LinkCollector<'_, '_> { if let Some(ref cached) = self.visited_links.get(&key) { match cached { Some(cached) => { - self.kind_side_channel.set(cached.side_channel); return Some(cached.res.clone()); } None if cache_resolution_failure => return None, @@ -1453,13 +1426,7 @@ impl LinkCollector<'_, '_> { // Cache only if resolved successfully - don't silence duplicate errors if let Some(res) = res { // Store result for the actual namespace - self.visited_links.insert( - key, - Some(CachedLink { - res: res.clone(), - side_channel: self.kind_side_channel.clone().into_inner(), - }), - ); + self.visited_links.insert(key, Some(CachedLink { res: res.clone() })); Some(res) } else { From a626da4e7833598b26d89de01237150986582af4 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 11:39:39 -0800 Subject: [PATCH 9/9] Split out `ItemFragment` from `UrlFragment` This allows eliminating branches in the code where a user-written fragment is impossible. --- .../passes/collect_intra_doc_links.rs | 88 +++++++++++-------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 7aa2f158810..0069f438270 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -236,10 +236,23 @@ enum AnchorFailure { #[derive(Clone, Debug, Hash, PartialEq, Eq)] crate enum UrlFragment { - Def(FragmentKind, DefId), + Item(ItemFragment), UserWritten(String), } +impl UrlFragment { + /// Render the fragment, including the leading `#`. + crate fn render(&self, s: &mut String, tcx: TyCtxt<'_>) -> std::fmt::Result { + match self { + UrlFragment::Item(frag) => frag.render(s, tcx), + UrlFragment::UserWritten(raw) => write!(s, "#{}", raw), + } + } +} + +#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] +crate struct ItemFragment(FragmentKind, DefId); + #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] crate enum FragmentKind { Method, @@ -252,7 +265,7 @@ crate enum FragmentKind { VariantField, } -impl UrlFragment { +impl ItemFragment { /// Create a fragment for an associated item. /// /// `is_prototype` is whether this associated item is a trait method @@ -261,13 +274,13 @@ impl UrlFragment { match kind { ty::AssocKind::Fn => { if is_prototype { - UrlFragment::Def(FragmentKind::TyMethod, def_id) + ItemFragment(FragmentKind::TyMethod, def_id) } else { - UrlFragment::Def(FragmentKind::Method, def_id) + ItemFragment(FragmentKind::Method, def_id) } } - ty::AssocKind::Const => UrlFragment::Def(FragmentKind::AssociatedConstant, def_id), - ty::AssocKind::Type => UrlFragment::Def(FragmentKind::AssociatedType, def_id), + ty::AssocKind::Const => ItemFragment(FragmentKind::AssociatedConstant, def_id), + ty::AssocKind::Type => ItemFragment(FragmentKind::AssociatedType, def_id), } } @@ -275,7 +288,7 @@ impl UrlFragment { crate fn render(&self, s: &mut String, tcx: TyCtxt<'_>) -> std::fmt::Result { write!(s, "#")?; match *self { - UrlFragment::Def(kind, def_id) => { + ItemFragment(kind, def_id) => { let name = tcx.item_name(def_id); match kind { FragmentKind::Method => write!(s, "method.{}", name), @@ -290,7 +303,6 @@ impl UrlFragment { } } } - UrlFragment::UserWritten(ref raw) => write!(s, "{}", raw), } } } @@ -300,7 +312,7 @@ struct ResolutionInfo { module_id: DefId, dis: Option, path_str: String, - extra_fragment: Option, + extra_fragment: Option, } #[derive(Clone)] @@ -339,7 +351,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { &self, path_str: &'path str, module_id: DefId, - ) -> Result<(Res, Option), ErrorKind<'path>> { + ) -> Result<(Res, Option), ErrorKind<'path>> { let tcx = self.cx.tcx; let no_res = || ResolutionFailure::NotResolved { module_id, @@ -389,10 +401,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { if let Some(field) = def.all_fields().find(|f| f.ident.name == variant_field_name) { - Ok(( - ty_res, - Some(UrlFragment::Def(FragmentKind::VariantField, field.did)), - )) + Ok((ty_res, Some(ItemFragment(FragmentKind::VariantField, field.did)))) } else { Err(ResolutionFailure::NotResolved { module_id, @@ -420,7 +429,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { prim_ty: PrimitiveType, ns: Namespace, item_name: Symbol, - ) -> Option<(Res, UrlFragment)> { + ) -> Option<(Res, ItemFragment)> { let tcx = self.cx.tcx; prim_ty.impls(tcx).into_iter().find_map(|&impl_| { @@ -428,7 +437,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_) .map(|item| { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); + let fragment = ItemFragment::from_assoc_item(item.def_id, kind, false); (Res::Primitive(prim_ty), fragment) }) }) @@ -503,21 +512,19 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'path str, ns: Namespace, module_id: DefId, - user_fragment: &Option, + user_fragment: &Option, ) -> Result<(Res, Option), ErrorKind<'path>> { let (res, rustdoc_fragment) = self.resolve_inner(path_str, ns, module_id)?; let chosen_fragment = match (user_fragment, rustdoc_fragment) { (Some(_), Some(r_frag)) => { let diag_res = match r_frag { - UrlFragment::Def(_, did) => Res::Def(self.cx.tcx.def_kind(did), did), - // FIXME: eliminate this branch somehow - UrlFragment::UserWritten(_) => unreachable!(), + ItemFragment(_, did) => Res::Def(self.cx.tcx.def_kind(did), did), }; let failure = AnchorFailure::RustdocAnchorConflict(diag_res); return Err(ErrorKind::AnchorFailure(failure)); } - (Some(u_frag), None) => Some(u_frag.clone()), - (None, Some(r_frag)) => Some(r_frag), + (Some(u_frag), None) => Some(UrlFragment::UserWritten(u_frag.clone())), + (None, Some(r_frag)) => Some(UrlFragment::Item(r_frag)), (None, None) => None, }; Ok((res, chosen_fragment)) @@ -528,7 +535,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'path str, ns: Namespace, module_id: DefId, - ) -> Result<(Res, Option), ErrorKind<'path>> { + ) -> Result<(Res, Option), ErrorKind<'path>> { if let Some(res) = self.resolve_path(path_str, ns, module_id) { match res { // FIXME(#76467): make this fallthrough to lookup the associated @@ -670,7 +677,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { item_name: Symbol, ns: Namespace, module_id: DefId, - ) -> Option<(Res, UrlFragment)> { + ) -> Option<(Res, ItemFragment)> { let tcx = self.cx.tcx; match root_res { @@ -685,7 +692,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { assoc_item.map(|item| { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); + let fragment = ItemFragment::from_assoc_item(item.def_id, kind, false); (root_res, fragment) }) }) @@ -736,7 +743,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { if let Some(item) = assoc_item { let kind = item.kind; - let fragment = UrlFragment::from_assoc_item(item.def_id, kind, false); + let fragment = ItemFragment::from_assoc_item(item.def_id, kind, false); return Some((root_res, fragment)); } @@ -768,13 +775,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { .fields .iter() .find(|item| item.ident.name == item_name)?; - Some((root_res, UrlFragment::Def(FragmentKind::StructField, field.did))) + Some((root_res, ItemFragment(FragmentKind::StructField, field.did))) } Res::Def(DefKind::Trait, did) => tcx .associated_items(did) .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, did) .map(|item| { - let fragment = UrlFragment::from_assoc_item( + let fragment = ItemFragment::from_assoc_item( item.def_id, item.kind, !item.defaultness.has_value(), @@ -797,7 +804,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ns: Namespace, path_str: &str, module_id: DefId, - extra_fragment: &Option, + extra_fragment: &Option, ) -> Option { // resolve() can't be used for macro namespace let result = match ns { @@ -812,7 +819,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { let res = match result { Ok((res, frag)) => { - if let Some(UrlFragment::Def(_, id)) = frag { + if let Some(UrlFragment::Item(ItemFragment(_, id))) = frag { Some(Res::Def(self.cx.tcx.def_kind(id), id)) } else { Some(res) @@ -1039,7 +1046,7 @@ impl From for PreprocessingError<'_> { struct PreprocessingInfo { path_str: String, disambiguator: Option, - extra_fragment: Option, + extra_fragment: Option, link_text: String, } @@ -1125,7 +1132,7 @@ fn preprocess_link<'a>( Some(Ok(PreprocessingInfo { path_str, disambiguator, - extra_fragment: extra_fragment.map(|frag| UrlFragment::UserWritten(frag.to_owned())), + extra_fragment: extra_fragment.map(|frag| frag.to_owned()), link_text: link_text.to_owned(), })) } @@ -1292,7 +1299,7 @@ impl LinkCollector<'_, '_> { }; let verify = |kind: DefKind, id: DefId| { - let (kind, id) = if let Some(UrlFragment::Def(_, id)) = fragment { + let (kind, id) = if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment { (self.cx.tcx.def_kind(id), id) } else { (kind, id) @@ -1340,7 +1347,7 @@ impl LinkCollector<'_, '_> { match res { Res::Primitive(prim) => { - if let Some(UrlFragment::Def(_, id)) = fragment { + if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment { let kind = self.cx.tcx.def_kind(id); // We're actually resolving an associated item of a primitive, so we need to @@ -1488,7 +1495,7 @@ impl LinkCollector<'_, '_> { let mut candidates = PerNS { macro_ns: self .resolve_macro(path_str, base_node) - .map(|res| (res, extra_fragment.clone())), + .map(|res| (res, extra_fragment.clone().map(UrlFragment::UserWritten))), type_ns: match self.resolve(path_str, TypeNS, base_node, extra_fragment) { Ok(res) => { debug!("got res in TypeNS: {:?}", res); @@ -1520,7 +1527,10 @@ impl LinkCollector<'_, '_> { // Shouldn't happen but who knows? Ok((res, Some(fragment))) } - (fragment, None) | (None, fragment) => Ok((res, fragment)), + (fragment, None) => Ok((res, fragment)), + (None, fragment) => { + Ok((res, fragment.map(UrlFragment::UserWritten))) + } } } } @@ -1557,7 +1567,7 @@ impl LinkCollector<'_, '_> { } Some(MacroNS) => { match self.resolve_macro(path_str, base_node) { - Ok(res) => Some((res, extra_fragment.clone())), + Ok(res) => Some((res, extra_fragment.clone().map(UrlFragment::UserWritten))), Err(mut kind) => { // `resolve_macro` only looks in the macro namespace. Try to give a better error if possible. for ns in [TypeNS, ValueNS] { @@ -2276,13 +2286,13 @@ fn privacy_error(cx: &DocContext<'_>, diag_info: &DiagnosticInfo<'_>, path_str: fn handle_variant( cx: &DocContext<'_>, res: Res, -) -> Result<(Res, Option), ErrorKind<'static>> { +) -> Result<(Res, Option), ErrorKind<'static>> { cx.tcx .parent(res.def_id(cx.tcx)) .map(|parent| { let parent_def = Res::Def(DefKind::Enum, parent); let variant = cx.tcx.expect_variant_res(res.as_hir_res().unwrap()); - (parent_def, Some(UrlFragment::Def(FragmentKind::Variant, variant.def_id))) + (parent_def, Some(ItemFragment(FragmentKind::Variant, variant.def_id))) }) .ok_or_else(|| ResolutionFailure::NoParentItem.into()) }