From 49553bbc98add28ed6f126225ffe6854a7ad7f29 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 13:44:46 -0800 Subject: [PATCH 1/9] Remove hack that is no longer necessary This hack was added in 6ab1f05697c3f2df4e439a05ebcee479a9a16d80. I don't know what change allowed removing the hack, but that commit added a test (which I presume covered the hack's behavior), and all tests are passing with this change. So, I think it should be good. --- src/librustdoc/passes/collect_intra_doc_links.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 9d1a8b3f80fec..d02ef9dafe7ff 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -958,17 +958,7 @@ impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> { { self.cx.tcx.parent(did) } - Some(did) => match self.cx.tcx.parent(did) { - // HACK(jynelson): `clean` marks associated types as `TypedefItem`, not as `AssocTypeItem`. - // Fixing this breaks `fn render_deref_methods`. - // As a workaround, see if the parent of the item is an `impl`; if so this must be an associated item, - // regardless of what rustdoc wants to call it. - Some(parent) => { - let parent_kind = self.cx.tcx.def_kind(parent); - Some(if parent_kind == DefKind::Impl { parent } else { did }) - } - None => Some(did), - }, + Some(did) => Some(did), }; // FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly From e18b23b7f494d57090be94351e92c1d69251f1a9 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 13:59:01 -0800 Subject: [PATCH 2/9] Move two intra-doc-link tests into the `intra-doc` folder --- .../rustdoc/{intra-link-prim-self.rs => intra-doc/prim-self.rs} | 2 +- .../{intra-link-self-cache.rs => intra-doc/self-cache.rs} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/test/rustdoc/{intra-link-prim-self.rs => intra-doc/prim-self.rs} (94%) rename src/test/rustdoc/{intra-link-self-cache.rs => intra-doc/self-cache.rs} (100%) diff --git a/src/test/rustdoc/intra-link-prim-self.rs b/src/test/rustdoc/intra-doc/prim-self.rs similarity index 94% rename from src/test/rustdoc/intra-link-prim-self.rs rename to src/test/rustdoc/intra-doc/prim-self.rs index 8a564acf2ca4b..dbd0a7cf0eb45 100644 --- a/src/test/rustdoc/intra-link-prim-self.rs +++ b/src/test/rustdoc/intra-doc/prim-self.rs @@ -7,7 +7,7 @@ #[lang = "usize"] /// [Self::f] /// [Self::MAX] -// @has intra_link_prim_self/primitive.usize.html +// @has prim_self/primitive.usize.html // @has - '//a[@href="primitive.usize.html#method.f"]' 'Self::f' // @has - '//a[@href="primitive.usize.html#associatedconstant.MAX"]' 'Self::MAX' impl usize { diff --git a/src/test/rustdoc/intra-link-self-cache.rs b/src/test/rustdoc/intra-doc/self-cache.rs similarity index 100% rename from src/test/rustdoc/intra-link-self-cache.rs rename to src/test/rustdoc/intra-doc/self-cache.rs From ca20d64fb77dc0aa5448e7bf9bcaa19164f1f521 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 14:12:48 -0800 Subject: [PATCH 3/9] Enable ignored part of test Inherent associated types *are* supported, just unstable. --- src/test/rustdoc/intra-doc/prim-self.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/rustdoc/intra-doc/prim-self.rs b/src/test/rustdoc/intra-doc/prim-self.rs index dbd0a7cf0eb45..7a65723d77b21 100644 --- a/src/test/rustdoc/intra-doc/prim-self.rs +++ b/src/test/rustdoc/intra-doc/prim-self.rs @@ -1,7 +1,9 @@ #![deny(rustdoc::broken_intra_doc_links)] +#![allow(incomplete_features)] // inherent_associated_types #![feature(lang_items)] #![feature(no_core)] #![feature(rustdoc_internals)] +#![feature(inherent_associated_types)] #![no_core] #[lang = "usize"] @@ -17,10 +19,9 @@ impl usize { /// 10 and 2^32 are basically the same. pub const MAX: usize = 10; - // FIXME(#8995) uncomment this when associated types in inherent impls are supported - // @ has - '//a[@href="{{channel}}/std/primitive.usize.html#associatedtype.ME"]' 'Self::ME' - // / [Self::ME] - //pub type ME = usize; + // @has - '//a[@href="primitive.usize.html#associatedtype.ME"]' 'Self::ME' + /// [Self::ME] + pub type ME = usize; } #[doc(primitive = "usize")] From 977a7ca2e4b9c02d7f43a999bbeee651a31241d8 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 14:49:28 -0800 Subject: [PATCH 4/9] Add test for disambiguator mismatch with crate This currently calls `std` a "crate" in one part of the message and a "module" in another part. The next commits fix this so it says "crate" in both places. --- .../rustdoc-ui/intra-doc/disambiguator-mismatch.rs | 5 +++++ .../intra-doc/disambiguator-mismatch.stderr | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs index 142008cf76508..ae48db48c1833 100644 --- a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs +++ b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs @@ -73,4 +73,9 @@ trait T {} //~^ ERROR incompatible link kind for `f` //~| NOTE this link resolved //~| HELP add parentheses + +/// Link to [fn@std] +//~^ ERROR unresolved link to `std` +//~| NOTE this link resolves to the crate `std` +//~| HELP to link to the module, prefix with `mod@` pub fn f() {} diff --git a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr index 12122f5fa8674..1d48b5f7471b6 100644 --- a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr +++ b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr @@ -138,5 +138,16 @@ LL - /// Link to [const@f] LL + /// Link to [f()] | -error: aborting due to 12 previous errors +error: unresolved link to `std` + --> $DIR/disambiguator-mismatch.rs:77:14 + | +LL | /// Link to [fn@std] + | ^^^^^^ this link resolves to the crate `std`, which is not in the value namespace + | +help: to link to the module, prefix with `mod@` + | +LL | /// Link to [mod@std] + | ~~~~ + +error: aborting due to 13 previous errors From 9acd8133806504f54e023151ec789c134656a1cc Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 14:43:27 -0800 Subject: [PATCH 5/9] Use Res instead of Disambiguator for `resolved` in `report_mismatch` This allows simplifying a lot of code. It also fixes a subtle bug, exemplified by the test output changes. --- .../passes/collect_intra_doc_links.rs | 115 ++++++++---------- .../intra-doc/disambiguator-mismatch.rs | 2 +- .../intra-doc/disambiguator-mismatch.stderr | 2 +- 3 files changed, 52 insertions(+), 67 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index d02ef9dafe7ff..20e248a445cb7 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -109,6 +109,45 @@ impl Res { Res::Primitive(_) => None, } } + + /// Used for error reporting. + fn disambiguator_suggestion(self) -> Suggestion { + let kind = match self { + Res::Primitive(_) => return Suggestion::Prefix("prim"), + Res::Def(kind, _) => kind, + }; + if kind == DefKind::Macro(MacroKind::Bang) { + return Suggestion::Macro; + } else if kind == DefKind::Fn || kind == DefKind::AssocFn { + return Suggestion::Function; + } else if kind == DefKind::Field { + return Suggestion::RemoveDisambiguator; + } + + let prefix = match kind { + DefKind::Struct => "struct", + DefKind::Enum => "enum", + DefKind::Trait => "trait", + DefKind::Union => "union", + DefKind::Mod => "mod", + DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst => { + "const" + } + DefKind::Static => "static", + DefKind::Macro(MacroKind::Derive) => "derive", + // Now handle things that don't have a specific disambiguator + _ => match kind + .ns() + .expect("tried to calculate a disambiguator for a def without a namespace?") + { + Namespace::TypeNS => "type", + Namespace::ValueNS => "value", + Namespace::MacroNS => "macro", + }, + }; + + Suggestion::Prefix(prefix) + } } impl TryFrom for Res { @@ -1267,7 +1306,7 @@ impl LinkCollector<'_, '_> { } } - let report_mismatch = |specified: Disambiguator, resolved: Disambiguator| { + let report_mismatch = |specified: Disambiguator, resolved: Res| { // The resolved item did not match the disambiguator; give a better error than 'not found' let msg = format!("incompatible link kind for `{}`", path_str); let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option| { @@ -1276,7 +1315,7 @@ impl LinkCollector<'_, '_> { resolved.article(), resolved.descr(), specified.article(), - specified.descr() + specified.descr(), ); if let Some(sp) = sp { diag.span_label(sp, ¬e); @@ -1311,7 +1350,7 @@ impl LinkCollector<'_, '_> { => {} (actual, Some(Disambiguator::Kind(expected))) if actual == expected => {} (_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => { - report_mismatch(specified, Disambiguator::Kind(kind)); + report_mismatch(specified, Res::Def(kind, id)); return None; } } @@ -1362,7 +1401,7 @@ impl LinkCollector<'_, '_> { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {} Some(other) => { - report_mismatch(other, Disambiguator::Primitive); + report_mismatch(other, res); return None; } } @@ -1676,53 +1715,6 @@ impl Disambiguator { } } - fn from_res(res: Res) -> Self { - match res { - Res::Def(kind, _) => Disambiguator::Kind(kind), - Res::Primitive(_) => Disambiguator::Primitive, - } - } - - /// Used for error reporting. - fn suggestion(self) -> Suggestion { - let kind = match self { - Disambiguator::Primitive => return Suggestion::Prefix("prim"), - Disambiguator::Kind(kind) => kind, - Disambiguator::Namespace(_) => panic!("display_for cannot be used on namespaces"), - }; - if kind == DefKind::Macro(MacroKind::Bang) { - return Suggestion::Macro; - } else if kind == DefKind::Fn || kind == DefKind::AssocFn { - return Suggestion::Function; - } else if kind == DefKind::Field { - return Suggestion::RemoveDisambiguator; - } - - let prefix = match kind { - DefKind::Struct => "struct", - DefKind::Enum => "enum", - DefKind::Trait => "trait", - DefKind::Union => "union", - DefKind::Mod => "mod", - DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst => { - "const" - } - DefKind::Static => "static", - DefKind::Macro(MacroKind::Derive) => "derive", - // Now handle things that don't have a specific disambiguator - _ => match kind - .ns() - .expect("tried to calculate a disambiguator for a def without a namespace?") - { - Namespace::TypeNS => "type", - Namespace::ValueNS => "value", - Namespace::MacroNS => "macro", - }, - }; - - Suggestion::Prefix(prefix) - } - fn ns(self) -> Namespace { match self { Self::Namespace(n) => n, @@ -2070,15 +2062,9 @@ fn resolution_failure( ResolutionFailure::NotResolved { .. } => unreachable!("handled above"), ResolutionFailure::Dummy => continue, ResolutionFailure::WrongNamespace { res, expected_ns } => { - if let Res::Def(kind, _) = res { - let disambiguator = Disambiguator::Kind(kind); - suggest_disambiguator( - disambiguator, - diag, - path_str, - diag_info.ori_link, - sp, - ) + // FIXME: does this need to be behind an `if`? + if matches!(res, Res::Def(..)) { + suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp); } format!( @@ -2214,8 +2200,7 @@ fn ambiguity_error( } for res in candidates { - let disambiguator = Disambiguator::from_res(res); - suggest_disambiguator(disambiguator, diag, path_str, diag_info.ori_link, sp); + suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp); } }); } @@ -2223,14 +2208,14 @@ fn ambiguity_error( /// In case of an ambiguity or mismatched disambiguator, suggest the correct /// disambiguator. fn suggest_disambiguator( - disambiguator: Disambiguator, + res: Res, diag: &mut DiagnosticBuilder<'_>, path_str: &str, ori_link: &str, sp: Option, ) { - let suggestion = disambiguator.suggestion(); - let help = format!("to link to the {}, {}", disambiguator.descr(), suggestion.descr()); + let suggestion = res.disambiguator_suggestion(); + let help = format!("to link to the {}, {}", res.descr(), suggestion.descr()); if let Some(sp) = sp { let mut spans = suggestion.as_help_span(path_str, ori_link, sp); diff --git a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs index ae48db48c1833..2d66566119bc3 100644 --- a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs +++ b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs @@ -77,5 +77,5 @@ trait T {} /// Link to [fn@std] //~^ ERROR unresolved link to `std` //~| NOTE this link resolves to the crate `std` -//~| HELP to link to the module, prefix with `mod@` +//~| HELP to link to the crate, prefix with `mod@` pub fn f() {} diff --git a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr index 1d48b5f7471b6..ad9102c506f7f 100644 --- a/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr +++ b/src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr @@ -144,7 +144,7 @@ error: unresolved link to `std` LL | /// Link to [fn@std] | ^^^^^^ this link resolves to the crate `std`, which is not in the value namespace | -help: to link to the module, prefix with `mod@` +help: to link to the crate, prefix with `mod@` | LL | /// Link to [mod@std] | ~~~~ From 591ec49df312ec4cbcdec0f082f123f473c182a9 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 14:45:23 -0800 Subject: [PATCH 6/9] Remove unnecessary conditional for suggesting disambiguator Now that `res` is used directly, it seems the conditional is unnecessary. --- src/librustdoc/passes/collect_intra_doc_links.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 20e248a445cb7..dd2fa9d627360 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -2062,10 +2062,7 @@ fn resolution_failure( ResolutionFailure::NotResolved { .. } => unreachable!("handled above"), ResolutionFailure::Dummy => continue, ResolutionFailure::WrongNamespace { res, expected_ns } => { - // FIXME: does this need to be behind an `if`? - if matches!(res, Res::Def(..)) { - suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp); - } + suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp); format!( "this link resolves to {}, which is not in the {} namespace", From a5f09f74d6dc0f52fd2c73fca0a9e5bb99eb756c Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 15:06:35 -0800 Subject: [PATCH 7/9] Update comment and make code clearer I'm still not sure why this hack works so seemingly well. --- src/librustdoc/passes/collect_intra_doc_links.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index dd2fa9d627360..13e1992a31caa 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -6,13 +6,12 @@ use rustc_ast as ast; use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet}; use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_expand::base::SyntaxExtensionKind; -use rustc_hir as hir; use rustc_hir::def::{ DefKind, Namespace::{self, *}, PerNS, }; -use rustc_hir::def_id::{CrateNum, DefId}; +use rustc_hir::def_id::{CrateNum, DefId, CRATE_DEF_ID}; use rustc_middle::ty::{DefIdTree, Ty, TyCtxt}; use rustc_middle::{bug, span_bug, ty}; use rustc_resolve::ParentScope; @@ -1736,9 +1735,9 @@ impl Disambiguator { fn descr(self) -> &'static str { match self { Self::Namespace(n) => n.descr(), - // HACK(jynelson): by looking at the source I saw the DefId we pass - // for `expected.descr()` doesn't matter, since it's not a crate - Self::Kind(k) => k.descr(DefId::local(hir::def_id::DefIndex::from_usize(0))), + // HACK(jynelson): the source of `DefKind::descr` only uses the DefId for + // printing "module" vs "crate" so using the wrong ID is not a huge problem + Self::Kind(k) => k.descr(CRATE_DEF_ID.to_def_id()), Self::Primitive => "builtin type", } } From 895fa9cd5c79b5c30614852c4c74a963b3ec458a Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 15:22:22 -0800 Subject: [PATCH 8/9] Extract functions for two closures These closures were quite complex and part of a quite complex function. The fact that they are closures makes mistakes likely when refactoring. For example, earlier, I meant to use `resolved`, an argument of the closure, but I instead typed `res`, which captured a local variable and caused a subtle bug that led to a confusing test failure. Extracting them as functions makes the code easier to understand and refactor. --- .../passes/collect_intra_doc_links.rs | 180 +++++++++++------- 1 file changed, 107 insertions(+), 73 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 13e1992a31caa..13b1d5b65a58a 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1305,79 +1305,9 @@ impl LinkCollector<'_, '_> { } } - let report_mismatch = |specified: Disambiguator, resolved: Res| { - // The resolved item did not match the disambiguator; give a better error than 'not found' - let msg = format!("incompatible link kind for `{}`", path_str); - let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option| { - let note = format!( - "this link resolved to {} {}, which is not {} {}", - resolved.article(), - resolved.descr(), - specified.article(), - specified.descr(), - ); - if let Some(sp) = sp { - diag.span_label(sp, ¬e); - } else { - diag.note(¬e); - } - suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp); - }; - report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback); - }; - - let verify = |kind: DefKind, id: DefId| { - let (kind, id) = if let Some(UrlFragment::Item(ItemFragment(_, 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@` - debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); - match (kind, disambiguator) { - | (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const))) - // NOTE: this allows 'method' to mean both normal functions and associated functions - // This can't cause ambiguity because both are in the same namespace. - | (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn))) - // These are namespaces; allow anything in the namespace to match - | (_, Some(Disambiguator::Namespace(_))) - // If no disambiguator given, allow anything - | (_, None) - // All of these are valid, so do nothing - => {} - (actual, Some(Disambiguator::Kind(expected))) if actual == expected => {} - (_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => { - report_mismatch(specified, Res::Def(kind, id)); - return None; - } - } - - // item can be non-local e.g. when using #[doc(primitive = "pointer")] - if let Some((src_id, dst_id)) = id - .as_local() - // The `expect_def_id()` should be okay because `local_def_id_to_hir_id` - // would presumably panic if a fake `DefIndex` were passed. - .and_then(|dst_id| { - item.def_id.expect_def_id().as_local().map(|src_id| (src_id, dst_id)) - }) - { - if self.cx.tcx.privacy_access_levels(()).is_exported(src_id) - && !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id) - { - privacy_error(self.cx, &diag_info, path_str); - } - } - - Some(()) - }; - match res { Res::Primitive(prim) => { 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 // 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, @@ -1386,7 +1316,16 @@ impl LinkCollector<'_, '_> { // doesn't allow statements like `use str::trim;`, making this a (hopefully) // valid omission. See https://github.com/rust-lang/rust/pull/80660#discussion_r551585677 // for discussion on the matter. - verify(kind, id)?; + let kind = self.cx.tcx.def_kind(id); + self.verify_disambiguator( + path_str, + &ori_link, + kind, + id, + disambiguator, + item, + &diag_info, + )?; // FIXME: it would be nice to check that the feature gate was enabled in the original crate, not just ignore it altogether. // However I'm not sure how to check that across crates. @@ -1400,7 +1339,9 @@ impl LinkCollector<'_, '_> { match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {} Some(other) => { - report_mismatch(other, res); + self.report_disambiguator_mismatch( + path_str, &ori_link, other, res, &diag_info, + ); return None; } } @@ -1414,13 +1355,106 @@ impl LinkCollector<'_, '_> { }) } Res::Def(kind, id) => { - verify(kind, id)?; + let (kind_for_dis, id_for_dis) = + if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment { + (self.cx.tcx.def_kind(id), id) + } else { + (kind, id) + }; + self.verify_disambiguator( + path_str, + &ori_link, + kind_for_dis, + id_for_dis, + disambiguator, + item, + &diag_info, + )?; let id = clean::register_res(self.cx, rustc_hir::def::Res::Def(kind, id)); Some(ItemLink { link: ori_link.link, link_text, did: id, fragment }) } } } + fn verify_disambiguator( + &self, + path_str: &str, + ori_link: &MarkdownLink, + kind: DefKind, + id: DefId, + disambiguator: Option, + item: &Item, + diag_info: &DiagnosticInfo<'_>, + ) -> Option<()> { + debug!("intra-doc link to {} resolved to {:?}", path_str, (kind, id)); + + // Disallow e.g. linking to enums with `struct@` + debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); + match (kind, disambiguator) { + | (DefKind::Const | DefKind::ConstParam | DefKind::AssocConst | DefKind::AnonConst, Some(Disambiguator::Kind(DefKind::Const))) + // NOTE: this allows 'method' to mean both normal functions and associated functions + // This can't cause ambiguity because both are in the same namespace. + | (DefKind::Fn | DefKind::AssocFn, Some(Disambiguator::Kind(DefKind::Fn))) + // These are namespaces; allow anything in the namespace to match + | (_, Some(Disambiguator::Namespace(_))) + // If no disambiguator given, allow anything + | (_, None) + // All of these are valid, so do nothing + => {} + (actual, Some(Disambiguator::Kind(expected))) if actual == expected => {} + (_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => { + self.report_disambiguator_mismatch(path_str,ori_link,specified, Res::Def(kind, id),diag_info); + return None; + } + } + + // item can be non-local e.g. when using #[doc(primitive = "pointer")] + if let Some((src_id, dst_id)) = id + .as_local() + // The `expect_def_id()` should be okay because `local_def_id_to_hir_id` + // would presumably panic if a fake `DefIndex` were passed. + .and_then(|dst_id| { + item.def_id.expect_def_id().as_local().map(|src_id| (src_id, dst_id)) + }) + { + if self.cx.tcx.privacy_access_levels(()).is_exported(src_id) + && !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id) + { + privacy_error(self.cx, diag_info, path_str); + } + } + + Some(()) + } + + fn report_disambiguator_mismatch( + &self, + path_str: &str, + ori_link: &MarkdownLink, + specified: Disambiguator, + resolved: Res, + diag_info: &DiagnosticInfo<'_>, + ) { + // The resolved item did not match the disambiguator; give a better error than 'not found' + let msg = format!("incompatible link kind for `{}`", path_str); + let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option| { + let note = format!( + "this link resolved to {} {}, which is not {} {}", + resolved.article(), + resolved.descr(), + specified.article(), + specified.descr(), + ); + if let Some(sp) = sp { + diag.span_label(sp, ¬e); + } else { + diag.note(¬e); + } + suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp); + }; + report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback); + } + 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) From 28d2353f3b1150313921916ae37a8525e9c2838d Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Thu, 6 Jan 2022 15:57:51 -0800 Subject: [PATCH 9/9] Update some comments post the side channel removal --- 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 13b1d5b65a58a..adeafc0d7589c 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -705,10 +705,8 @@ 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 + /// Resolve an associated item, returning its containing page's `Res` + /// and the fragment targeting the associated item on its page. fn resolve_associated_item( &mut self, root_res: Res, @@ -1475,7 +1473,6 @@ impl LinkCollector<'_, '_> { diag: DiagnosticInfo<'_>, cache_resolution_failure: bool, ) -> Option<(Res, Option)> { - // Try to look up both the result and the corresponding side channel value if let Some(ref cached) = self.visited_links.get(&key) { match cached { Some(cached) => {