From 51eda747b1dd5ac9a6afe6006fb26b5cd11b9d0f Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 22 Apr 2022 20:37:54 +0300 Subject: [PATCH 1/9] rustdoc: Remove `ResolutionFailure::NoParentItem` It's a bug and not an error --- .../passes/collect_intra_doc_links.rs | 57 +++---------------- 1 file changed, 8 insertions(+), 49 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 22b2f8c0c8ec3..1c5477b8d0d84 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -16,7 +16,7 @@ use rustc_resolve::ParentScope; use rustc_session::lint::Lint; use rustc_span::hygiene::MacroKind; use rustc_span::symbol::{sym, Ident, Symbol}; -use rustc_span::{BytePos, DUMMY_SP}; +use rustc_span::BytePos; use smallvec::{smallvec, SmallVec}; use std::borrow::Cow; @@ -97,14 +97,6 @@ impl Res { } } - fn as_hir_res(self) -> Option { - match self { - Res::Def(kind, id) => Some(rustc_hir::def::Res::Def(kind, id)), - // FIXME: maybe this should handle the subset of PrimitiveType that fits into hir::PrimTy? - Res::Primitive(_) => None, - } - } - /// Used for error reporting. fn disambiguator_suggestion(self) -> Suggestion { let kind = match self { @@ -189,9 +181,6 @@ enum ResolutionFailure<'a> { /// In `[std::io::Error::x]`, `x` would be unresolved. unresolved: Cow<'a, str>, }, - /// This happens when rustdoc can't determine the parent scope for an item. - /// It is always a bug in rustdoc. - NoParentItem, /// This link has malformed generic parameters; e.g., the angle brackets are unbalanced. MalformedGenerics(MalformedGenerics), /// Used to communicate that this should be ignored, but shouldn't be reported to the user. @@ -616,8 +605,12 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // item a separate function. 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); + Res::Def(DefKind::Variant, def_id) => { + let enum_def_id = self.cx.tcx.parent(def_id).expect("variant has no parent"); + return Ok(( + Res::Def(DefKind::Enum, enum_def_id), + Some(ItemFragment(FragmentKind::Variant, def_id)), + )); } // Not a trait item; just return what we found. _ => return Ok((res, None)), @@ -1268,19 +1261,7 @@ impl LinkCollector<'_, '_> { // parent_node first. let base_node = if item.is_mod() && inner_docs { self.mod_ids.last().copied() } else { parent_node }; - - let Some(module_id) = base_node else { - // This is a bug. - debug!("attempting to resolve item without parent module: {}", path_str); - resolution_failure( - self, - diag_info, - path_str, - disambiguator, - smallvec![ResolutionFailure::NoParentItem], - ); - return None; - }; + let module_id = base_node.expect("doc link without parent module"); let (mut res, fragment) = self.resolve_with_disambiguator_cached( ResolutionInfo { @@ -2140,17 +2121,6 @@ fn resolution_failure( expected_ns.descr() ) } - ResolutionFailure::NoParentItem => { - // FIXME(eddyb) this doesn't belong here, whatever made - // the `ResolutionFailure::NoParentItem` should emit an - // immediate or delayed `span_bug` about the issue. - tcx.sess.delay_span_bug( - sp.unwrap_or(DUMMY_SP), - "intra-doc link missing parent item", - ); - - "BUG: all intra-doc links should have a parent item".to_owned() - } ResolutionFailure::MalformedGenerics(variant) => match variant { MalformedGenerics::UnbalancedAngleBrackets => { String::from("unbalanced angle brackets") @@ -2331,17 +2301,6 @@ fn privacy_error(cx: &DocContext<'_>, diag_info: &DiagnosticInfo<'_>, path_str: }); } -/// Given an enum variant's res, return the res of its enum and the associated fragment. -fn handle_variant( - cx: &DocContext<'_>, - res: Res, -) -> Result<(Res, Option), ErrorKind<'static>> { - let parent = cx.tcx.parent(res.def_id(cx.tcx)); - let parent_def = Res::Def(DefKind::Enum, parent); - let variant = cx.tcx.expect_variant_res(res.as_hir_res().unwrap()); - Ok((parent_def, Some(ItemFragment(FragmentKind::Variant, variant.def_id)))) -} - /// Resolve a primitive type or value. fn resolve_primitive(path_str: &str, ns: Namespace) -> Option { if ns != TypeNS { From c979ef5d8c1347a26fd63035ef256931df9f8a91 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 22 Apr 2022 21:16:46 +0300 Subject: [PATCH 2/9] rustdoc: Remove `ResolutionFailure::MalformedGenerics` in favor of `PreprocessingError::MalformedGenerics` --- .../passes/collect_intra_doc_links.rs | 142 ++++++++---------- 1 file changed, 61 insertions(+), 81 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 1c5477b8d0d84..ec621a554935f 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -181,8 +181,6 @@ enum ResolutionFailure<'a> { /// In `[std::io::Error::x]`, `x` would be unresolved. unresolved: Cow<'a, str>, }, - /// This link has malformed generic parameters; e.g., the angle brackets are unbalanced. - MalformedGenerics(MalformedGenerics), /// Used to communicate that this should be ignored, but shouldn't be reported to the user. /// /// This happens when there is no disambiguator and one of the namespaces @@ -190,7 +188,7 @@ enum ResolutionFailure<'a> { Dummy, } -#[derive(Clone, Debug)] +#[derive(Clone, Copy, Debug)] enum MalformedGenerics { /// This link has unbalanced angle brackets. /// @@ -1088,12 +1086,20 @@ impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> { enum PreprocessingError { Anchor(AnchorFailure), Disambiguator(Range, String), - Resolution(ResolutionFailure<'static>, String, Option), + MalformedGenerics(MalformedGenerics, String), } -impl From for PreprocessingError { - fn from(err: AnchorFailure) -> Self { - Self::Anchor(err) +impl PreprocessingError { + fn report(&self, cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>) { + match self { + PreprocessingError::Anchor(err) => anchor_failure(cx, diag_info, *err), + PreprocessingError::Disambiguator(range, msg) => { + disambiguator_error(cx, diag_info, range.clone(), msg) + } + PreprocessingError::MalformedGenerics(err, path_str) => { + report_malformed_generics(cx, diag_info, *err, path_str) + } + } } } @@ -1138,7 +1144,7 @@ fn preprocess_link( let extra_fragment = parts.next(); if parts.next().is_some() { // A valid link can't have multiple #'s - return Some(Err(AnchorFailure::MultipleAnchors.into())); + return Some(Err(PreprocessingError::Anchor(AnchorFailure::MultipleAnchors))); } // Parse and strip the disambiguator from the link, if present. @@ -1166,13 +1172,9 @@ fn preprocess_link( let path_str = if path_str.contains(['<', '>'].as_slice()) { match strip_generics_from_path(path_str) { Ok(path) => path, - Err(err_kind) => { + Err(err) => { debug!("link has malformed generics: {}", path_str); - return Some(Err(PreprocessingError::Resolution( - err_kind, - path_str.to_owned(), - disambiguator, - ))); + return Some(Err(PreprocessingError::MalformedGenerics(err, path_str.to_owned()))); } } } else { @@ -1222,32 +1224,10 @@ impl LinkCollector<'_, '_> { link_range: ori_link.range.clone(), }; - let PreprocessingInfo { path_str, disambiguator, extra_fragment, link_text } = match pp_link - { - Ok(x) => x, - Err(err) => { - match err { - PreprocessingError::Anchor(err) => anchor_failure(self.cx, diag_info, *err), - PreprocessingError::Disambiguator(range, msg) => { - disambiguator_error(self.cx, diag_info, range.clone(), msg) - } - PreprocessingError::Resolution(err, path_str, disambiguator) => { - resolution_failure( - self, - diag_info, - path_str, - *disambiguator, - smallvec![err.clone()], - ); - } - } - return None; - } - }; + let PreprocessingInfo { path_str, disambiguator, extra_fragment, link_text } = + pp_link.as_ref().map_err(|err| err.report(self.cx, diag_info.clone())).ok()?; let disambiguator = *disambiguator; - let inner_docs = item.inner_docs(self.cx.tcx); - // In order to correctly resolve intra-doc links we need to // pick a base AST node to work from. If the documentation for // this module came from an inner comment (//!) then we anchor @@ -1259,6 +1239,7 @@ impl LinkCollector<'_, '_> { // we've already pushed this node onto the resolution stack but // for outer comments we explicitly try and resolve against the // parent_node first. + let inner_docs = item.inner_docs(self.cx.tcx); let base_node = if item.is_mod() && inner_docs { self.mod_ids.last().copied() } else { parent_node }; let module_id = base_node.expect("doc link without parent module"); @@ -2121,27 +2102,6 @@ fn resolution_failure( expected_ns.descr() ) } - ResolutionFailure::MalformedGenerics(variant) => match variant { - MalformedGenerics::UnbalancedAngleBrackets => { - String::from("unbalanced angle brackets") - } - MalformedGenerics::MissingType => { - String::from("missing type for generic parameters") - } - MalformedGenerics::HasFullyQualifiedSyntax => { - diag.note("see https://github.com/rust-lang/rust/issues/74563 for more information"); - String::from("fully-qualified syntax is unsupported") - } - MalformedGenerics::InvalidPathSeparator => { - String::from("has invalid path separator") - } - MalformedGenerics::TooManyAngleBrackets => { - String::from("too many angle brackets") - } - MalformedGenerics::EmptyAngleBrackets => { - String::from("empty angle brackets") - } - }, }; if let Some(span) = sp { diag.span_label(span, ¬e); @@ -2205,6 +2165,40 @@ fn disambiguator_error( }); } +fn report_malformed_generics( + cx: &DocContext<'_>, + diag_info: DiagnosticInfo<'_>, + err: MalformedGenerics, + path_str: &str, +) { + report_diagnostic( + cx.tcx, + BROKEN_INTRA_DOC_LINKS, + &format!("unresolved link to `{}`", path_str), + &diag_info, + |diag, sp| { + let note = match err { + MalformedGenerics::UnbalancedAngleBrackets => "unbalanced angle brackets", + MalformedGenerics::MissingType => "missing type for generic parameters", + MalformedGenerics::HasFullyQualifiedSyntax => { + diag.note( + "see https://github.com/rust-lang/rust/issues/74563 for more information", + ); + "fully-qualified syntax is unsupported" + } + MalformedGenerics::InvalidPathSeparator => "has invalid path separator", + MalformedGenerics::TooManyAngleBrackets => "too many angle brackets", + MalformedGenerics::EmptyAngleBrackets => "empty angle brackets", + }; + if let Some(span) = sp { + diag.span_label(span, note); + } else { + diag.note(note); + } + }, + ); +} + /// Report an ambiguity error, where there were multiple possible resolutions. fn ambiguity_error( cx: &DocContext<'_>, @@ -2340,7 +2334,7 @@ fn resolve_primitive(path_str: &str, ns: Namespace) -> Option { Some(Res::Primitive(prim)) } -fn strip_generics_from_path(path_str: &str) -> Result> { +fn strip_generics_from_path(path_str: &str) -> Result { let mut stripped_segments = vec![]; let mut path = path_str.chars().peekable(); let mut segment = Vec::new(); @@ -2355,9 +2349,7 @@ fn strip_generics_from_path(path_str: &str) -> Result { @@ -2365,14 +2357,10 @@ fn strip_generics_from_path(path_str: &str) -> Result { - return Err(ResolutionFailure::MalformedGenerics( - MalformedGenerics::TooManyAngleBrackets, - )); + return Err(MalformedGenerics::TooManyAngleBrackets); } Some('>') => { - return Err(ResolutionFailure::MalformedGenerics( - MalformedGenerics::EmptyAngleBrackets, - )); + return Err(MalformedGenerics::EmptyAngleBrackets); } Some(chr) => { segment.push(chr); @@ -2400,16 +2388,10 @@ fn strip_generics_from_path(path_str: &str) -> Result, -) -> Result> { +fn strip_generics_from_path_segment(segment: Vec) -> Result { let mut stripped_segment = String::new(); let mut param_depth = 0; @@ -2424,9 +2406,7 @@ fn strip_generics_from_path_segment( if latest_generics_chunk.contains(" as ") { // The segment tries to use fully-qualified syntax, which is currently unsupported. // Give a helpful error message instead of completely ignoring the angle brackets. - return Err(ResolutionFailure::MalformedGenerics( - MalformedGenerics::HasFullyQualifiedSyntax, - )); + return Err(MalformedGenerics::HasFullyQualifiedSyntax); } } else { if param_depth == 0 { @@ -2441,6 +2421,6 @@ fn strip_generics_from_path_segment( Ok(stripped_segment) } else { // The segment has unbalanced angle brackets, e.g. `Vec>` - Err(ResolutionFailure::MalformedGenerics(MalformedGenerics::UnbalancedAngleBrackets)) + Err(MalformedGenerics::UnbalancedAngleBrackets) } } From 95fb05d4d87e5cbf42d2208b218ba8c10d4c311a Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 22 Apr 2022 22:03:33 +0300 Subject: [PATCH 3/9] rustdoc: Remove `ResolutionFailure::Dummy` The variant resolution check didn't make sense, and derive trait collision could be processed in a different way --- .../passes/collect_intra_doc_links.rs | 57 +++++++------------ 1 file changed, 20 insertions(+), 37 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index ec621a554935f..2a999ed6567c6 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -181,11 +181,6 @@ enum ResolutionFailure<'a> { /// In `[std::io::Error::x]`, `x` would be unresolved. unresolved: Cow<'a, str>, }, - /// Used to communicate that this should be ignored, but shouldn't be reported to the user. - /// - /// This happens when there is no disambiguator and one of the namespaces - /// failed to resolve. - Dummy, } #[derive(Clone, Copy, Debug)] @@ -405,35 +400,22 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { let ty_res = self.resolve_path(&path, TypeNS, item_id, module_id).ok_or_else(no_res)?; match ty_res { - Res::Def(DefKind::Enum, did) => { - if tcx - .inherent_impls(did) - .iter() - .flat_map(|imp| tcx.associated_items(*imp).in_definition_order()) - .any(|item| item.name == variant_name) - { - // This is just to let `fold_item` know that this shouldn't be considered; - // it's a bug for the error to make it to the user - return Err(ResolutionFailure::Dummy.into()); - } - match tcx.type_of(did).kind() { - ty::Adt(def, _) if def.is_enum() => { - if let Some(field) = def.all_fields().find(|f| f.name == variant_field_name) - { - Ok((ty_res, Some(ItemFragment(FragmentKind::VariantField, field.did)))) - } else { - Err(ResolutionFailure::NotResolved { - item_id, - module_id, - partial_res: Some(Res::Def(DefKind::Enum, def.did())), - unresolved: variant_field_name.to_string().into(), - } - .into()) + Res::Def(DefKind::Enum, did) => match tcx.type_of(did).kind() { + ty::Adt(def, _) if def.is_enum() => { + if let Some(field) = def.all_fields().find(|f| f.name == variant_field_name) { + Ok((ty_res, Some(ItemFragment(FragmentKind::VariantField, field.did)))) + } else { + Err(ResolutionFailure::NotResolved { + item_id, + module_id, + partial_res: Some(Res::Def(DefKind::Enum, def.did())), + unresolved: variant_field_name.to_string().into(), } + .into()) } - _ => unreachable!(), } - } + _ => unreachable!(), + }, _ => Err(ResolutionFailure::NotResolved { item_id, module_id, @@ -1535,7 +1517,7 @@ impl LinkCollector<'_, '_> { } None => { // Try everything! - let mut candidates = PerNS { + let candidates = PerNS { macro_ns: self .resolve_macro(path_str, item_id, base_node) .map(|res| (res, extra_fragment.clone().map(UrlFragment::UserWritten))), @@ -1611,11 +1593,13 @@ impl LinkCollector<'_, '_> { } else if len == 2 && is_derive_trait_collision(&candidates) { Some(candidates.type_ns.unwrap()) } else { - if is_derive_trait_collision(&candidates) { - candidates.macro_ns = Err(ResolutionFailure::Dummy); - } + let ignore_macro = is_derive_trait_collision(&candidates); // If we're reporting an ambiguity, don't mention the namespaces that failed - let candidates = candidates.map(|candidate| candidate.ok().map(|(res, _)| res)); + let mut candidates = + candidates.map(|candidate| candidate.ok().map(|(res, _)| res)); + if ignore_macro { + candidates.macro_ns = None; + } ambiguity_error(self.cx, diag, path_str, candidates.present_items().collect()); None } @@ -2092,7 +2076,6 @@ fn resolution_failure( } let note = match failure { ResolutionFailure::NotResolved { .. } => unreachable!("handled above"), - ResolutionFailure::Dummy => continue, ResolutionFailure::WrongNamespace { res, expected_ns } => { suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp); From 7256c6f93ee0c0773d749d865e8b7041635dd6ed Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 22 Apr 2022 22:49:01 +0300 Subject: [PATCH 4/9] rustdoc: Remove `fn resolve_macro` and otherwise unify resolution in macro namespace and other namespaces --- .../passes/collect_intra_doc_links.rs | 162 +++++------------- 1 file changed, 43 insertions(+), 119 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 2a999ed6567c6..3b2b47b6c7953 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -54,12 +54,6 @@ enum ErrorKind<'a> { AnchorFailure(AnchorFailure), } -impl<'a> From> for ErrorKind<'a> { - fn from(err: ResolutionFailure<'a>) -> Self { - ErrorKind::Resolve(box err) - } -} - #[derive(Copy, Clone, Debug, Hash)] enum Res { Def(DefKind, DefId), @@ -371,7 +365,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'path str, item_id: ItemId, module_id: DefId, - ) -> Result<(Res, Option), ErrorKind<'path>> { + ) -> Result<(Res, Option), ResolutionFailure<'path>> { let tcx = self.cx.tcx; let no_res = || ResolutionFailure::NotResolved { item_id, @@ -445,25 +439,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }) } - /// Resolves a string as a macro. - /// - /// FIXME(jynelson): Can this be unified with `resolve()`? - fn resolve_macro( - &self, - path_str: &'a str, - item_id: ItemId, - module_id: DefId, - ) -> Result> { - self.resolve_path(path_str, MacroNS, item_id, module_id).ok_or_else(|| { - ResolutionFailure::NotResolved { - item_id, - module_id, - partial_res: None, - unresolved: path_str.into(), - } - }) - } - fn resolve_self_ty(&self, path_str: &str, ns: Namespace, item_id: ItemId) -> Option { if ns != TypeNS || path_str != "Self" { return None; @@ -556,12 +531,12 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id: DefId, user_fragment: &Option, ) -> Result<(Res, Option), ErrorKind<'path>> { - let (res, rustdoc_fragment) = self.resolve_inner(path_str, ns, item_id, module_id)?; + let (res, rustdoc_fragment) = self + .resolve_inner(path_str, ns, item_id, module_id) + .map_err(|err| ErrorKind::Resolve(box err))?; let chosen_fragment = match (user_fragment, rustdoc_fragment) { - (Some(_), Some(r_frag)) => { - let diag_res = match r_frag { - ItemFragment(_, did) => Res::Def(self.cx.tcx.def_kind(did), did), - }; + (Some(_), Some(ItemFragment(_, did))) => { + let diag_res = Res::Def(self.cx.tcx.def_kind(did), did); let failure = AnchorFailure::RustdocAnchorConflict(diag_res); return Err(ErrorKind::AnchorFailure(failure)); } @@ -578,7 +553,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ns: Namespace, item_id: ItemId, module_id: DefId, - ) -> Result<(Res, Option), ErrorKind<'path>> { + ) -> Result<(Res, Option), ResolutionFailure<'path>> { if let Some(res) = self.resolve_path(path_str, ns, item_id, module_id) { match res { // FIXME(#76467): make this fallthrough to lookup the associated @@ -595,6 +570,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // Not a trait item; just return what we found. _ => return Ok((res, None)), } + } else if ns == MacroNS { + return Err(ResolutionFailure::NotResolved { + item_id, + module_id, + partial_res: None, + unresolved: path_str.into(), + }); } // Try looking for methods and associated items. @@ -639,8 +621,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id, partial_res: None, unresolved: path_root.into(), - } - .into()) + }) } }) } @@ -862,18 +843,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id: DefId, extra_fragment: &Option, ) -> Option { - // resolve() can't be used for macro namespace - let result = match ns { - Namespace::MacroNS => self - .resolve_macro(path_str, item_id, module_id) - .map(|res| (res, None)) - .map_err(ErrorKind::from), - Namespace::TypeNS | Namespace::ValueNS => { - self.resolve(path_str, ns, item_id, module_id, extra_fragment) - } - }; - - let res = match result { + let res = match self.resolve(path_str, ns, item_id, module_id, extra_fragment) { Ok((res, frag)) => { if let Some(UrlFragment::Item(ItemFragment(_, id))) = frag { Some(Res::Def(self.cx.tcx.def_kind(id), id)) @@ -881,7 +851,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Some(res) } } - Err(ErrorKind::Resolve(box kind)) => kind.full_res(), + Err(ErrorKind::Resolve(kind)) => kind.full_res(), Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res))) => Some(res), Err(ErrorKind::AnchorFailure(AnchorFailure::MultipleAnchors)) => None, }; @@ -1481,80 +1451,57 @@ impl LinkCollector<'_, '_> { let extra_fragment = &key.extra_fragment; match disambiguator.map(Disambiguator::ns) { - Some(expected_ns @ (ValueNS | TypeNS)) => { + Some(expected_ns) => { match self.resolve(path_str, expected_ns, item_id, base_node, extra_fragment) { Ok(res) => Some(res), - Err(ErrorKind::Resolve(box mut kind)) => { + Err(ErrorKind::AnchorFailure(msg)) => { + anchor_failure(self.cx, diag, msg); + None + } + Err(ErrorKind::Resolve(mut err)) => { // We only looked in one namespace. Try to give a better error if possible. - if kind.full_res().is_none() { - let other_ns = if expected_ns == ValueNS { TypeNS } else { ValueNS }; - // FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator` - // See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach - for new_ns in [other_ns, MacroNS] { + // FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator`. + // See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach. + for other_ns in [TypeNS, ValueNS, MacroNS] { + if other_ns != expected_ns { if let Some(res) = self.check_full_res( - new_ns, + other_ns, path_str, item_id, base_node, extra_fragment, ) { - kind = ResolutionFailure::WrongNamespace { res, expected_ns }; + *err = ResolutionFailure::WrongNamespace { res, expected_ns }; break; } } } - resolution_failure(self, diag, path_str, disambiguator, smallvec![kind]); + resolution_failure(self, diag, path_str, disambiguator, smallvec![*err]); // This could just be a normal link or a broken link // we could potentially check if something is // "intra-doc-link-like" and warn in that case. None } - Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - None - } } } None => { // Try everything! - let candidates = PerNS { - macro_ns: self - .resolve_macro(path_str, item_id, base_node) - .map(|res| (res, extra_fragment.clone().map(UrlFragment::UserWritten))), - type_ns: match self.resolve( - path_str, - TypeNS, - item_id, - base_node, - extra_fragment, - ) { - Ok(res) => { - debug!("got res in TypeNS: {:?}", res); - Ok(res) - } + let mut candidate = + |ns| match self.resolve(path_str, ns, item_id, base_node, extra_fragment) { + Ok(res) => Some(Ok(res)), Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - return None; + anchor_failure(self.cx, diag.clone(), msg); + None } - Err(ErrorKind::Resolve(box kind)) => Err(kind), - }, - value_ns: match self.resolve( - path_str, - ValueNS, - item_id, - base_node, - extra_fragment, - ) { - Ok(res) => Ok(res), - Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - return None; - } - Err(ErrorKind::Resolve(box kind)) => Err(kind), - } - .and_then(|(res, fragment)| { - // Constructors are picked up in the type namespace. + Err(ErrorKind::Resolve(err)) => Some(Err(*err)), + }; + + let candidates = PerNS { + macro_ns: candidate(MacroNS)?, + type_ns: candidate(TypeNS)?, + value_ns: candidate(ValueNS)?.and_then(|(res, fragment)| { match res { + // Constructors are picked up in the type namespace. Res::Def(DefKind::Ctor(..), _) => { Err(ResolutionFailure::WrongNamespace { res, expected_ns: TypeNS }) } @@ -1604,29 +1551,6 @@ impl LinkCollector<'_, '_> { None } } - Some(MacroNS) => { - match self.resolve_macro(path_str, item_id, base_node) { - 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] { - if let Some(res) = self.check_full_res( - ns, - path_str, - item_id, - base_node, - extra_fragment, - ) { - kind = - ResolutionFailure::WrongNamespace { res, expected_ns: MacroNS }; - break; - } - } - resolution_failure(self, diag, path_str, disambiguator, smallvec![kind]); - None - } - } - } } } } From 7c2283d3ca0e8c8c6f872f4eb06b43e5527db8dc Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 23 Apr 2022 11:18:21 +0300 Subject: [PATCH 5/9] rustdoc: Do not create `UrlFragment`s until they are necessary This simplifies error types and allows to remove `fn resolve_inner` and `fn check_full_res` `visited_links` caching is not touched for now --- .../passes/collect_intra_doc_links.rs | 369 +++++++----------- 1 file changed, 133 insertions(+), 236 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 3b2b47b6c7953..25702c8ed0ae6 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -11,7 +11,7 @@ use rustc_hir::def::{DefKind, Namespace, PerNS}; use rustc_hir::def_id::{DefId, CRATE_DEF_ID}; use rustc_hir::Mutability; use rustc_middle::ty::{DefIdTree, Ty, TyCtxt}; -use rustc_middle::{bug, span_bug, ty}; +use rustc_middle::{bug, ty}; use rustc_resolve::ParentScope; use rustc_session::lint::Lint; use rustc_span::hygiene::MacroKind; @@ -48,12 +48,6 @@ fn collect_intra_doc_links(krate: Crate, cx: &mut DocContext<'_>) -> Crate { krate } -/// Top-level errors emitted by this pass. -enum ErrorKind<'a> { - Resolve(Box>), - AnchorFailure(AnchorFailure), -} - #[derive(Copy, Clone, Debug, Hash)] enum Res { Def(DefKind, DefId), @@ -91,6 +85,10 @@ impl Res { } } + fn from_def_id(tcx: TyCtxt<'_>, def_id: DefId) -> Res { + Res::Def(tcx.def_kind(def_id), def_id) + } + /// Used for error reporting. fn disambiguator_suggestion(self) -> Suggestion { let kind = match self { @@ -146,8 +144,25 @@ impl TryFrom for Res { } } -/// A link failed to resolve. -#[derive(Clone, Debug)] +/// The link failed to resolve. [`resolution_failure`] should look to see if there's +/// a more helpful error that can be given. +#[derive(Debug)] +struct UnresolvedPath<'a> { + /// Item on which the link is resolved, used for resolving `Self`. + item_id: ItemId, + /// The scope the link was resolved in. + module_id: DefId, + /// If part of the link resolved, this has the `Res`. + /// + /// In `[std::io::Error::x]`, `std::io::Error` would be a partial resolution. + partial_res: Option, + /// The remaining unresolved path segments. + /// + /// In `[std::io::Error::x]`, `x` would be unresolved. + unresolved: Cow<'a, str>, +} + +#[derive(Debug)] enum ResolutionFailure<'a> { /// This resolved, but with the wrong namespace. WrongNamespace { @@ -159,22 +174,7 @@ enum ResolutionFailure<'a> { /// even though `Result`'s actual namespace is [`Namespace::TypeNS`]. expected_ns: Namespace, }, - /// The link failed to resolve. [`resolution_failure`] should look to see if there's - /// a more helpful error that can be given. - NotResolved { - /// Item on which the link is resolved, used for resolving `Self`. - item_id: ItemId, - /// The scope the link was resolved in. - module_id: DefId, - /// If part of the link resolved, this has the `Res`. - /// - /// In `[std::io::Error::x]`, `std::io::Error` would be a partial resolution. - partial_res: Option, - /// The remaining unresolved path segments. - /// - /// In `[std::io::Error::x]`, `x` would be unresolved. - unresolved: Cow<'a, str>, - }, + NotResolved(UnresolvedPath<'a>), } #[derive(Clone, Copy, Debug)] @@ -218,35 +218,6 @@ enum MalformedGenerics { EmptyAngleBrackets, } -impl ResolutionFailure<'_> { - /// This resolved fully (not just partially) but is erroneous for some other reason - /// - /// Returns the full resolution of the link, if present. - fn full_res(&self) -> Option { - match self { - Self::WrongNamespace { res, expected_ns: _ } => Some(*res), - _ => None, - } - } -} - -#[derive(Clone, Copy)] -enum AnchorFailure { - /// User error: `[std#x#y]` is not valid - MultipleAnchors, - /// The anchor provided by the user conflicts with Rustdoc's generated anchor. - /// - /// This is an unfortunate state of affairs. Not every item that can be - /// linked to has its own page; sometimes it is a subheading within a page, - /// like for associated items. In those cases, rustdoc uses an anchor to - /// link to the subheading. Since you can't have two anchors for the same - /// link, Rustdoc disallows having a user-specified anchor. - /// - /// Most of the time this is fine, because you can just link to the page of - /// the item if you want to provide your own anchor. - RustdocAnchorConflict(Res), -} - #[derive(Clone, Debug, Hash, PartialEq, Eq)] crate enum UrlFragment { Item(ItemFragment), @@ -278,24 +249,32 @@ crate enum FragmentKind { VariantField, } -impl ItemFragment { - /// Create a fragment for an associated item. - #[instrument(level = "debug")] - fn from_assoc_item(item: &ty::AssocItem) -> Self { - let def_id = item.def_id; - match item.kind { - ty::AssocKind::Fn => { - if item.defaultness.has_value() { - ItemFragment(FragmentKind::Method, def_id) +impl FragmentKind { + fn from_def_id(tcx: TyCtxt<'_>, def_id: DefId) -> FragmentKind { + match tcx.def_kind(def_id) { + DefKind::AssocFn => { + if tcx.associated_item(def_id).defaultness.has_value() { + FragmentKind::Method } else { - ItemFragment(FragmentKind::TyMethod, def_id) + FragmentKind::TyMethod } } - ty::AssocKind::Const => ItemFragment(FragmentKind::AssociatedConstant, def_id), - ty::AssocKind::Type => ItemFragment(FragmentKind::AssociatedType, def_id), + DefKind::AssocConst => FragmentKind::AssociatedConstant, + DefKind::AssocTy => FragmentKind::AssociatedType, + DefKind::Variant => FragmentKind::Variant, + DefKind::Field => { + if tcx.def_kind(tcx.parent(def_id).unwrap()) == DefKind::Variant { + FragmentKind::VariantField + } else { + FragmentKind::StructField + } + } + kind => bug!("unexpected associated item kind: {:?}", kind), } } +} +impl ItemFragment { /// Render the fragment, including the leading `#`. crate fn render(&self, s: &mut String, tcx: TyCtxt<'_>) -> std::fmt::Result { write!(s, "#")?; @@ -365,9 +344,9 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { path_str: &'path str, item_id: ItemId, module_id: DefId, - ) -> Result<(Res, Option), ResolutionFailure<'path>> { + ) -> Result<(Res, DefId), UnresolvedPath<'path>> { let tcx = self.cx.tcx; - let no_res = || ResolutionFailure::NotResolved { + let no_res = || UnresolvedPath { item_id, module_id, partial_res: None, @@ -397,26 +376,24 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Res::Def(DefKind::Enum, did) => match tcx.type_of(did).kind() { ty::Adt(def, _) if def.is_enum() => { if let Some(field) = def.all_fields().find(|f| f.name == variant_field_name) { - Ok((ty_res, Some(ItemFragment(FragmentKind::VariantField, field.did)))) + Ok((ty_res, field.did)) } else { - Err(ResolutionFailure::NotResolved { + Err(UnresolvedPath { item_id, module_id, partial_res: Some(Res::Def(DefKind::Enum, def.did())), unresolved: variant_field_name.to_string().into(), - } - .into()) + }) } } _ => unreachable!(), }, - _ => Err(ResolutionFailure::NotResolved { + _ => Err(UnresolvedPath { item_id, module_id, partial_res: Some(ty_res), unresolved: variant_name.to_string().into(), - } - .into()), + }), } } @@ -426,16 +403,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { prim_ty: PrimitiveType, ns: Namespace, item_name: Symbol, - ) -> Option<(Res, ItemFragment)> { + ) -> Option<(Res, DefId)> { let tcx = self.cx.tcx; prim_ty.impls(tcx).find_map(|impl_| { tcx.associated_items(impl_) .find_by_name_and_namespace(tcx, Ident::with_dummy_span(item_name), ns, impl_) - .map(|item| { - let fragment = ItemFragment::from_assoc_item(item); - (Res::Primitive(prim_ty), fragment) - }) + .map(|item| (Res::Primitive(prim_ty), item.def_id)) }) } @@ -529,31 +503,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ns: Namespace, item_id: ItemId, module_id: DefId, - user_fragment: &Option, - ) -> Result<(Res, Option), ErrorKind<'path>> { - let (res, rustdoc_fragment) = self - .resolve_inner(path_str, ns, item_id, module_id) - .map_err(|err| ErrorKind::Resolve(box err))?; - let chosen_fragment = match (user_fragment, rustdoc_fragment) { - (Some(_), Some(ItemFragment(_, did))) => { - let diag_res = 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(UrlFragment::UserWritten(u_frag.clone())), - (None, Some(r_frag)) => Some(UrlFragment::Item(r_frag)), - (None, None) => None, - }; - Ok((res, chosen_fragment)) - } - - fn resolve_inner<'path>( - &mut self, - path_str: &'path str, - ns: Namespace, - item_id: ItemId, - module_id: DefId, - ) -> Result<(Res, Option), ResolutionFailure<'path>> { + ) -> Result<(Res, Option), UnresolvedPath<'path>> { if let Some(res) = self.resolve_path(path_str, ns, item_id, module_id) { match res { // FIXME(#76467): make this fallthrough to lookup the associated @@ -562,16 +512,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Res::Def(DefKind::AssocTy, _) => assert_eq!(ns, TypeNS), Res::Def(DefKind::Variant, def_id) => { let enum_def_id = self.cx.tcx.parent(def_id).expect("variant has no parent"); - return Ok(( - Res::Def(DefKind::Enum, enum_def_id), - Some(ItemFragment(FragmentKind::Variant, def_id)), - )); + return Ok((Res::Def(DefKind::Enum, enum_def_id), Some(def_id))); } // Not a trait item; just return what we found. _ => return Ok((res, None)), } } else if ns == MacroNS { - return Err(ResolutionFailure::NotResolved { + return Err(UnresolvedPath { item_id, module_id, partial_res: None, @@ -592,7 +539,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { // So we can be sure that `rustc_resolve` was accurate when it said it wasn't resolved. .ok_or_else(|| { debug!("found no `::`, assumming {} was correctly not in scope", item_name); - ResolutionFailure::NotResolved { + UnresolvedPath { item_id, module_id, partial_res: None, @@ -607,16 +554,13 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { resolve_primitive(&path_root, TypeNS) .or_else(|| self.resolve_path(&path_root, TypeNS, item_id, module_id)) .and_then(|ty_res| { - let (res, fragment) = - self.resolve_associated_item(ty_res, item_name, ns, module_id)?; - - Some(Ok((res, Some(fragment)))) + self.resolve_associated_item(ty_res, item_name, ns, module_id).map(Ok) }) .unwrap_or_else(|| { if ns == Namespace::ValueNS { self.variant_field(path_str, item_id, module_id) } else { - Err(ResolutionFailure::NotResolved { + Err(UnresolvedPath { item_id, module_id, partial_res: None, @@ -624,6 +568,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }) } }) + .map(|(res, def_id)| (res, Some(def_id))) } /// Convert a DefId to a Res, where possible. @@ -648,7 +593,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ty::FnPtr(_) => Res::Primitive(Fn), ty::Never => Res::Primitive(Never), ty::Adt(ty::AdtDef(Interned(&ty::AdtDefData { did, .. }, _)), _) | ty::Foreign(did) => { - Res::Def(self.cx.tcx.def_kind(did), did) + Res::from_def_id(self.cx.tcx, did) } ty::Projection(_) | ty::Closure(..) @@ -705,23 +650,18 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { item_name: Symbol, ns: Namespace, module_id: DefId, - ) -> Option<(Res, ItemFragment)> { + ) -> Option<(Res, DefId)> { let tcx = self.cx.tcx; match root_res { Res::Primitive(prim) => { self.resolve_primitive_associated_item(prim, ns, item_name).or_else(|| { - let assoc_item = self - .primitive_type_to_ty(prim) + self.primitive_type_to_ty(prim) .map(|ty| { resolve_associated_trait_item(ty, module_id, item_name, ns, self.cx) }) - .flatten(); - - assoc_item.map(|item| { - let fragment = ItemFragment::from_assoc_item(&item); - (root_res, fragment) - }) + .flatten() + .map(|item| (root_res, item.def_id)) }) } Res::Def(DefKind::TyAlias, did) => { @@ -742,10 +682,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ty::Adt(adt_def, _) => { for variant in adt_def.variants() { if variant.name == item_name { - return Some(( - root_res, - ItemFragment(FragmentKind::Variant, variant.def_id), - )); + return Some((root_res, variant.def_id)); } } } @@ -786,8 +723,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { debug!("got associated item {:?}", assoc_item); if let Some(item) = assoc_item { - let fragment = ItemFragment::from_assoc_item(&item); - return Some((root_res, fragment)); + return Some((root_res, item.def_id)); } if ns != Namespace::ValueNS { @@ -815,48 +751,22 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }; let field = def.non_enum_variant().fields.iter().find(|item| item.name == item_name)?; - Some((root_res, ItemFragment(FragmentKind::StructField, field.did))) + Some((root_res, 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 = ItemFragment::from_assoc_item(item); let res = Res::Def(item.kind.as_def_kind(), item.def_id); - (res, fragment) + (res, item.def_id) }), _ => None, } } +} - /// Used for reporting better errors. - /// - /// Returns whether the link resolved 'fully' in another namespace. - /// 'fully' here means that all parts of the link resolved, not just some path segments. - /// This returns the `Res` even if it was erroneous for some reason - /// (such as having invalid URL fragments or being in the wrong namespace). - fn check_full_res( - &mut self, - ns: Namespace, - path_str: &str, - item_id: ItemId, - module_id: DefId, - extra_fragment: &Option, - ) -> Option { - let res = match self.resolve(path_str, ns, item_id, module_id, extra_fragment) { - Ok((res, frag)) => { - if let Some(UrlFragment::Item(ItemFragment(_, id))) = frag { - Some(Res::Def(self.cx.tcx.def_kind(id), id)) - } else { - Some(res) - } - } - Err(ErrorKind::Resolve(kind)) => kind.full_res(), - Err(ErrorKind::AnchorFailure(AnchorFailure::RustdocAnchorConflict(res))) => Some(res), - Err(ErrorKind::AnchorFailure(AnchorFailure::MultipleAnchors)) => None, - }; - res - } +fn full_res(tcx: TyCtxt<'_>, (base, assoc_item): (Res, Option)) -> Res { + assoc_item.map_or(base, |def_id| Res::from_def_id(tcx, def_id)) } /// Look to see if a resolved item has an associated item named `item_name`. @@ -1036,7 +946,8 @@ impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> { } enum PreprocessingError { - Anchor(AnchorFailure), + /// User error: `[std#x#y]` is not valid + MultipleAnchors, Disambiguator(Range, String), MalformedGenerics(MalformedGenerics, String), } @@ -1044,7 +955,7 @@ enum PreprocessingError { impl PreprocessingError { fn report(&self, cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>) { match self { - PreprocessingError::Anchor(err) => anchor_failure(cx, diag_info, *err), + PreprocessingError::MultipleAnchors => report_multiple_anchors(cx, diag_info), PreprocessingError::Disambiguator(range, msg) => { disambiguator_error(cx, diag_info, range.clone(), msg) } @@ -1096,7 +1007,7 @@ fn preprocess_link( let extra_fragment = parts.next(); if parts.next().is_some() { // A valid link can't have multiple #'s - return Some(Err(PreprocessingError::Anchor(AnchorFailure::MultipleAnchors))); + return Some(Err(PreprocessingError::MultipleAnchors)); } // Parse and strip the disambiguator from the link, if present. @@ -1418,7 +1329,21 @@ impl LinkCollector<'_, '_> { } } - let res = self.resolve_with_disambiguator(&key, diag); + let res = self.resolve_with_disambiguator(&key, diag.clone()).and_then(|(res, def_id)| { + let fragment = match (&key.extra_fragment, def_id) { + (Some(_), Some(def_id)) => { + report_anchor_conflict(self.cx, diag, Res::from_def_id(self.cx.tcx, def_id)); + return None; + } + (Some(u_frag), None) => Some(UrlFragment::UserWritten(u_frag.clone())), + (None, Some(def_id)) => Some(UrlFragment::Item(ItemFragment( + FragmentKind::from_def_id(self.cx.tcx, def_id), + def_id, + ))), + (None, None) => None, + }; + Some((res, fragment)) + }); // Cache only if resolved successfully - don't silence duplicate errors if let Some(res) = res { @@ -1443,40 +1368,35 @@ impl LinkCollector<'_, '_> { &mut self, key: &ResolutionInfo, diag: DiagnosticInfo<'_>, - ) -> Option<(Res, Option)> { + ) -> Option<(Res, Option)> { let disambiguator = key.dis; let path_str = &key.path_str; let item_id = key.item_id; let base_node = key.module_id; - let extra_fragment = &key.extra_fragment; match disambiguator.map(Disambiguator::ns) { Some(expected_ns) => { - match self.resolve(path_str, expected_ns, item_id, base_node, extra_fragment) { + match self.resolve(path_str, expected_ns, item_id, base_node) { Ok(res) => Some(res), - Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag, msg); - None - } - Err(ErrorKind::Resolve(mut err)) => { + Err(err) => { // We only looked in one namespace. Try to give a better error if possible. // FIXME: really it should be `resolution_failure` that does this, not `resolve_with_disambiguator`. // See https://github.com/rust-lang/rust/pull/76955#discussion_r493953382 for a good approach. + let mut err = ResolutionFailure::NotResolved(err); for other_ns in [TypeNS, ValueNS, MacroNS] { if other_ns != expected_ns { - if let Some(res) = self.check_full_res( - other_ns, - path_str, - item_id, - base_node, - extra_fragment, - ) { - *err = ResolutionFailure::WrongNamespace { res, expected_ns }; + if let Ok(res) = + self.resolve(path_str, other_ns, item_id, base_node) + { + err = ResolutionFailure::WrongNamespace { + res: full_res(self.cx.tcx, res), + expected_ns, + }; break; } } } - resolution_failure(self, diag, path_str, disambiguator, smallvec![*err]); + resolution_failure(self, diag, path_str, disambiguator, smallvec![err]); // This could just be a normal link or a broken link // we could potentially check if something is // "intra-doc-link-like" and warn in that case. @@ -1486,37 +1406,21 @@ impl LinkCollector<'_, '_> { } None => { // Try everything! - let mut candidate = - |ns| match self.resolve(path_str, ns, item_id, base_node, extra_fragment) { - Ok(res) => Some(Ok(res)), - Err(ErrorKind::AnchorFailure(msg)) => { - anchor_failure(self.cx, diag.clone(), msg); - None - } - Err(ErrorKind::Resolve(err)) => Some(Err(*err)), - }; + let mut candidate = |ns| { + self.resolve(path_str, ns, item_id, base_node) + .map_err(ResolutionFailure::NotResolved) + }; let candidates = PerNS { - macro_ns: candidate(MacroNS)?, - type_ns: candidate(TypeNS)?, - value_ns: candidate(ValueNS)?.and_then(|(res, fragment)| { + macro_ns: candidate(MacroNS), + type_ns: candidate(TypeNS), + value_ns: candidate(ValueNS).and_then(|(res, def_id)| { match res { // Constructors are picked up in the type namespace. Res::Def(DefKind::Ctor(..), _) => { Err(ResolutionFailure::WrongNamespace { res, expected_ns: TypeNS }) } - _ => { - match (fragment, extra_fragment.clone()) { - (Some(fragment), Some(_)) => { - // Shouldn't happen but who knows? - Ok((res, Some(fragment))) - } - (fragment, None) => Ok((res, fragment)), - (None, fragment) => { - Ok((res, fragment.map(UrlFragment::UserWritten))) - } - } - } + _ => Ok((res, def_id)), } }), }; @@ -1865,12 +1769,12 @@ fn resolution_failure( } variants_seen.push(variant); - if let ResolutionFailure::NotResolved { + if let ResolutionFailure::NotResolved(UnresolvedPath { item_id, module_id, partial_res, unresolved, - } = &mut failure + }) = &mut failure { use DefKind::*; @@ -1896,11 +1800,9 @@ fn resolution_failure( }; name = start; for ns in [TypeNS, ValueNS, MacroNS] { - if let Some(res) = - collector.check_full_res(ns, start, item_id, module_id, &None) - { + if let Ok(res) = collector.resolve(start, ns, item_id, module_id) { debug!("found partial_res={:?}", res); - *partial_res = Some(res); + *partial_res = Some(full_res(collector.cx.tcx, res)); *unresolved = end.into(); break 'outer; } @@ -2020,22 +1922,24 @@ fn resolution_failure( ); } -/// Report an anchor failure. -fn anchor_failure(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>, failure: AnchorFailure) { - let (msg, anchor_idx) = match failure { - AnchorFailure::MultipleAnchors => { - (format!("`{}` contains multiple anchors", diag_info.ori_link), 1) - } - AnchorFailure::RustdocAnchorConflict(res) => ( - format!( - "`{}` contains an anchor, but links to {kind}s are already anchored", - diag_info.ori_link, - kind = res.descr(), - ), - 0, - ), - }; +fn report_multiple_anchors(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>) { + let msg = format!("`{}` contains multiple anchors", diag_info.ori_link); + anchor_failure(cx, diag_info, &msg, 1) +} +fn report_anchor_conflict(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>, res: Res) { + let (link, kind) = (diag_info.ori_link, res.descr()); + let msg = format!("`{link}` contains an anchor, but links to {kind}s are already anchored"); + anchor_failure(cx, diag_info, &msg, 0) +} + +/// Report an anchor failure. +fn anchor_failure( + cx: &DocContext<'_>, + diag_info: DiagnosticInfo<'_>, + msg: &str, + anchor_idx: usize, +) { report_diagnostic(cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, |diag, sp| { if let Some(mut sp) = sp { if let Some((fragment_offset, _)) = @@ -2045,13 +1949,6 @@ fn anchor_failure(cx: &DocContext<'_>, diag_info: DiagnosticInfo<'_>, failure: A } diag.span_label(sp, "invalid anchor"); } - if let AnchorFailure::RustdocAnchorConflict(Res::Primitive(_)) = failure { - if let Some(sp) = sp { - span_bug!(sp, "anchors should be allowed now"); - } else { - bug!("anchors should be allowed now"); - } - } }); } From 7689857884c8d74f25517373f2d3d4c6fe055614 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 23 Apr 2022 13:56:40 +0300 Subject: [PATCH 6/9] rustdoc: Do not resolve associated item paths unnecessarily --- .../passes/collect_intra_doc_links.rs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 25702c8ed0ae6..14929a3772a17 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -505,18 +505,16 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { module_id: DefId, ) -> Result<(Res, Option), UnresolvedPath<'path>> { if let Some(res) = self.resolve_path(path_str, ns, item_id, module_id) { - match res { - // FIXME(#76467): make this fallthrough to lookup the associated - // item a separate function. - Res::Def(DefKind::AssocFn | DefKind::AssocConst, _) => assert_eq!(ns, ValueNS), - Res::Def(DefKind::AssocTy, _) => assert_eq!(ns, TypeNS), - Res::Def(DefKind::Variant, def_id) => { - let enum_def_id = self.cx.tcx.parent(def_id).expect("variant has no parent"); - return Ok((Res::Def(DefKind::Enum, enum_def_id), Some(def_id))); + return Ok(match res { + Res::Def( + DefKind::AssocFn | DefKind::AssocConst | DefKind::AssocTy | DefKind::Variant, + def_id, + ) => { + let parent_def_id = self.cx.tcx.parent(def_id).unwrap(); + (Res::from_def_id(self.cx.tcx, parent_def_id), Some(def_id)) } - // Not a trait item; just return what we found. - _ => return Ok((res, None)), - } + _ => ((res, None)), + }); } else if ns == MacroNS { return Err(UnresolvedPath { item_id, From a9c14dc440aaf096cd4d69ede55799383b0f0a1e Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 23 Apr 2022 18:34:15 +0300 Subject: [PATCH 7/9] rustdoc: Use `Visibility::is_public` more --- .../passes/collect_intra_doc_links/early.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links/early.rs b/src/librustdoc/passes/collect_intra_doc_links/early.rs index 07d05cab1d1d7..09463ec85a23b 100644 --- a/src/librustdoc/passes/collect_intra_doc_links/early.rs +++ b/src/librustdoc/passes/collect_intra_doc_links/early.rs @@ -134,24 +134,21 @@ impl<'ra> EarlyDocLinkResolver<'_, 'ra> { // using privacy, private traits and impls from other crates are never documented in // the current crate, and links in their doc comments are not resolved. for &def_id in &all_traits { - if self.resolver.cstore().visibility_untracked(def_id) == Visibility::Public { + if self.resolver.cstore().visibility_untracked(def_id).is_public() { self.resolve_doc_links_extern_impl(def_id, false); } } for &(trait_def_id, impl_def_id, simplified_self_ty) in &all_trait_impls { - if self.resolver.cstore().visibility_untracked(trait_def_id) - == Visibility::Public + if self.resolver.cstore().visibility_untracked(trait_def_id).is_public() && simplified_self_ty.and_then(|ty| ty.def()).map_or(true, |ty_def_id| { - self.resolver.cstore().visibility_untracked(ty_def_id) - == Visibility::Public + self.resolver.cstore().visibility_untracked(ty_def_id).is_public() }) { self.resolve_doc_links_extern_impl(impl_def_id, false); } } for (ty_def_id, impl_def_id) in all_inherent_impls { - if self.resolver.cstore().visibility_untracked(ty_def_id) == Visibility::Public - { + if self.resolver.cstore().visibility_untracked(ty_def_id).is_public() { self.resolve_doc_links_extern_impl(impl_def_id, true); } } @@ -178,8 +175,7 @@ impl<'ra> EarlyDocLinkResolver<'_, 'ra> { self.resolver.cstore().associated_item_def_ids_untracked(def_id, self.sess), ); for assoc_def_id in assoc_item_def_ids { - if !is_inherent - || self.resolver.cstore().visibility_untracked(assoc_def_id) == Visibility::Public + if !is_inherent || self.resolver.cstore().visibility_untracked(assoc_def_id).is_public() { self.resolve_doc_links_extern_outer(assoc_def_id, def_id); } @@ -279,7 +275,7 @@ impl<'ra> EarlyDocLinkResolver<'_, 'ra> { for child in self.resolver.module_children_or_reexports(module_id) { // This condition should give a superset of `denied` from `fn clean_use_statement`. - if child.vis == Visibility::Public + if child.vis.is_public() || self.document_private_items && child.vis != Visibility::Restricted(module_id) && module_id.is_local() From 6c5c7f503eb476f7e2004abca9669c4715a9b275 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Mon, 2 May 2022 23:10:51 +0300 Subject: [PATCH 8/9] Fix rebase --- src/librustdoc/passes/collect_intra_doc_links.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 14929a3772a17..c25a0d3b14997 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -263,7 +263,7 @@ impl FragmentKind { DefKind::AssocTy => FragmentKind::AssociatedType, DefKind::Variant => FragmentKind::Variant, DefKind::Field => { - if tcx.def_kind(tcx.parent(def_id).unwrap()) == DefKind::Variant { + if tcx.def_kind(tcx.parent(def_id)) == DefKind::Variant { FragmentKind::VariantField } else { FragmentKind::StructField @@ -509,10 +509,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { Res::Def( DefKind::AssocFn | DefKind::AssocConst | DefKind::AssocTy | DefKind::Variant, def_id, - ) => { - let parent_def_id = self.cx.tcx.parent(def_id).unwrap(); - (Res::from_def_id(self.cx.tcx, parent_def_id), Some(def_id)) - } + ) => (Res::from_def_id(self.cx.tcx, self.cx.tcx.parent(def_id)), Some(def_id)), _ => ((res, None)), }); } else if ns == MacroNS { From 9ba5281c7606c232fb0a9519f2aeda2018fffad4 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 13 May 2022 21:36:36 +0300 Subject: [PATCH 9/9] resolve: Move collection of all `macro_rules` in the crate to rustdoc --- .../rustc_resolve/src/build_reduced_graph.rs | 1 - compiler/rustc_resolve/src/lib.rs | 18 +++++++----------- .../passes/collect_intra_doc_links/early.rs | 8 ++++++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_resolve/src/build_reduced_graph.rs b/compiler/rustc_resolve/src/build_reduced_graph.rs index e68d6fdeea55b..dffec44ddbcc3 100644 --- a/compiler/rustc_resolve/src/build_reduced_graph.rs +++ b/compiler/rustc_resolve/src/build_reduced_graph.rs @@ -1268,7 +1268,6 @@ impl<'a, 'b> BuildReducedGraphVisitor<'a, 'b> { }; let binding = (res, vis, span, expansion).to_name_binding(self.r.arenas); self.r.set_binding_parent_module(binding, parent_scope.module); - self.r.all_macro_rules.insert(ident.name, res); if is_macro_export { let module = self.r.graph_root; self.r.define(module, ident, MacroNS, (res, vis, span, expansion, IsMacroExport)); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 8d3c46c29a861..6c0148a17a1b8 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -59,7 +59,7 @@ use rustc_span::{Span, DUMMY_SP}; use smallvec::{smallvec, SmallVec}; use std::cell::{Cell, RefCell}; use std::collections::BTreeSet; -use std::{cmp, fmt, mem, ptr}; +use std::{cmp, fmt, ptr}; use tracing::debug; use diagnostics::{ImportSuggestion, LabelSuggestion, Suggestion}; @@ -966,8 +966,6 @@ pub struct Resolver<'a> { registered_attrs: FxHashSet, registered_tools: RegisteredTools, macro_use_prelude: FxHashMap>, - /// FIXME: The only user of this is a doc link resolution hack for rustdoc. - all_macro_rules: FxHashMap, macro_map: FxHashMap>, dummy_ext_bang: Lrc, dummy_ext_derive: Lrc, @@ -1360,7 +1358,6 @@ impl<'a> Resolver<'a> { registered_attrs, registered_tools, macro_use_prelude: FxHashMap::default(), - all_macro_rules: Default::default(), macro_map: FxHashMap::default(), dummy_ext_bang: Lrc::new(SyntaxExtension::dummy_bang(session.edition())), dummy_ext_derive: Lrc::new(SyntaxExtension::dummy_derive(session.edition())), @@ -1912,11 +1909,6 @@ impl<'a> Resolver<'a> { } } - // For rustdoc. - pub fn take_all_macro_rules(&mut self) -> FxHashMap { - mem::take(&mut self.all_macro_rules) - } - /// For rustdoc. /// For local modules returns only reexports, for external modules returns all children. pub fn module_children_or_reexports(&self, def_id: DefId) -> Vec { @@ -1928,8 +1920,12 @@ impl<'a> Resolver<'a> { } /// For rustdoc. - pub fn macro_rules_scope(&self, def_id: LocalDefId) -> MacroRulesScopeRef<'a> { - *self.macro_rules_scopes.get(&def_id).expect("not a `macro_rules` item") + pub fn macro_rules_scope(&self, def_id: LocalDefId) -> (MacroRulesScopeRef<'a>, Res) { + let scope = *self.macro_rules_scopes.get(&def_id).expect("not a `macro_rules` item"); + match scope.get() { + MacroRulesScope::Binding(mb) => (scope, mb.binding.res()), + _ => unreachable!(), + } } /// Retrieves the span of the given `DefId` if `DefId` is in the local crate. diff --git a/src/librustdoc/passes/collect_intra_doc_links/early.rs b/src/librustdoc/passes/collect_intra_doc_links/early.rs index 09463ec85a23b..0ac27087a9782 100644 --- a/src/librustdoc/passes/collect_intra_doc_links/early.rs +++ b/src/librustdoc/passes/collect_intra_doc_links/early.rs @@ -40,6 +40,7 @@ crate fn early_resolve_intra_doc_links( traits_in_scope: Default::default(), all_traits: Default::default(), all_trait_impls: Default::default(), + all_macro_rules: Default::default(), document_private_items, }; @@ -64,7 +65,7 @@ crate fn early_resolve_intra_doc_links( traits_in_scope: link_resolver.traits_in_scope, all_traits: Some(link_resolver.all_traits), all_trait_impls: Some(link_resolver.all_trait_impls), - all_macro_rules: link_resolver.resolver.take_all_macro_rules(), + all_macro_rules: link_resolver.all_macro_rules, } } @@ -82,6 +83,7 @@ struct EarlyDocLinkResolver<'r, 'ra> { traits_in_scope: DefIdMap>, all_traits: Vec, all_trait_impls: Vec, + all_macro_rules: FxHashMap>, document_private_items: bool, } @@ -339,8 +341,10 @@ impl Visitor<'_> for EarlyDocLinkResolver<'_, '_> { self.all_trait_impls.push(self.resolver.local_def_id(item.id).to_def_id()); } ItemKind::MacroDef(macro_def) if macro_def.macro_rules => { - self.parent_scope.macro_rules = + let (macro_rules_scope, res) = self.resolver.macro_rules_scope(self.resolver.local_def_id(item.id)); + self.parent_scope.macro_rules = macro_rules_scope; + self.all_macro_rules.insert(item.ident.name, res); } _ => {} }