From b4a0ef066d645b430091017203882b6ea39b73a1 Mon Sep 17 00:00:00 2001 From: max-heller Date: Sun, 3 Jan 2021 15:34:06 -0500 Subject: [PATCH 1/6] fix issue 80559 --- src/librustdoc/passes/collect_intra_doc_links.rs | 15 ++++++++++----- src/test/rustdoc/issue-80559.rs | 4 ++++ 2 files changed, 14 insertions(+), 5 deletions(-) create mode 100644 src/test/rustdoc/issue-80559.rs diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 0cefbb34791b2..fd63d5eb5c934 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -394,10 +394,14 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { ns, impl_, ) - .map(|item| match item.kind { - ty::AssocKind::Fn => "method", - ty::AssocKind::Const => "associatedconstant", - ty::AssocKind::Type => "associatedtype", + .map(|item| { + let kind = item.kind; + self.kind_side_channel.set(Some((kind.as_def_kind(), item.def_id))); + match kind { + ty::AssocKind::Fn => "method", + ty::AssocKind::Const => "associatedconstant", + ty::AssocKind::Type => "associatedtype", + } }) .map(|out| { ( @@ -1143,7 +1147,7 @@ impl LinkCollector<'_, '_> { ); }; match res { - Res::Primitive(_) => match disambiguator { + Res::Primitive(_) if self.kind_side_channel.get().is_none() => match disambiguator { Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { Some(ItemLink { link: ori_link.link, link_text, did: None, fragment }) } @@ -1152,6 +1156,7 @@ impl LinkCollector<'_, '_> { None } }, + Res::Primitive(_) => Some(ItemLink { link: ori_link, link_text, did: None, fragment }), Res::Def(kind, id) => { debug!("intra-doc link to {} resolved to {:?}", path_str, res); diff --git a/src/test/rustdoc/issue-80559.rs b/src/test/rustdoc/issue-80559.rs new file mode 100644 index 0000000000000..34905c48653a1 --- /dev/null +++ b/src/test/rustdoc/issue-80559.rs @@ -0,0 +1,4 @@ +#![deny(broken_intra_doc_links)] +// Should link to https://doc.rust-lang.org/nightly/std/primitive.str.html#method.trim +// and not suggest prefixing with `prim@` +//! [str::trim()] From e33a205bdfb0fbbbad80f49e4d39180dc78087bf Mon Sep 17 00:00:00 2001 From: max-heller Date: Sun, 3 Jan 2021 20:34:07 -0500 Subject: [PATCH 2/6] primitive disambiguator tests --- .../rustdoc/intra-doc/incompatible-primitive-disambiguator.rs | 2 ++ src/test/rustdoc/intra-doc/primitive-disambiguators.rs | 4 ++++ src/test/rustdoc/issue-80559.rs | 4 ---- 3 files changed, 6 insertions(+), 4 deletions(-) create mode 100644 src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs create mode 100644 src/test/rustdoc/intra-doc/primitive-disambiguators.rs delete mode 100644 src/test/rustdoc/issue-80559.rs diff --git a/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs b/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs new file mode 100644 index 0000000000000..1ea6ad9c7e691 --- /dev/null +++ b/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs @@ -0,0 +1,2 @@ +#![deny(broken_intra_doc_links)] +//! [struct@str::trim] //~ ERROR incompatible link diff --git a/src/test/rustdoc/intra-doc/primitive-disambiguators.rs b/src/test/rustdoc/intra-doc/primitive-disambiguators.rs new file mode 100644 index 0000000000000..acdd07566c94d --- /dev/null +++ b/src/test/rustdoc/intra-doc/primitive-disambiguators.rs @@ -0,0 +1,4 @@ +#![deny(broken_intra_doc_links)] +// @has primitive_disambiguators/index.html +// @has - '//a/@href' 'https://doc.rust-lang.org/nightly/std/primitive.str.html#method.trim' +//! [str::trim()] diff --git a/src/test/rustdoc/issue-80559.rs b/src/test/rustdoc/issue-80559.rs deleted file mode 100644 index 34905c48653a1..0000000000000 --- a/src/test/rustdoc/issue-80559.rs +++ /dev/null @@ -1,4 +0,0 @@ -#![deny(broken_intra_doc_links)] -// Should link to https://doc.rust-lang.org/nightly/std/primitive.str.html#method.trim -// and not suggest prefixing with `prim@` -//! [str::trim()] From fc3a4058cefc09bebe98039c7fb8d74d1be9c6e6 Mon Sep 17 00:00:00 2001 From: max-heller Date: Sun, 3 Jan 2021 20:35:18 -0500 Subject: [PATCH 3/6] still verify disambiguators for primitives --- .../passes/collect_intra_doc_links.rs | 104 ++++++++++-------- 1 file changed, 57 insertions(+), 47 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index fd63d5eb5c934..090c45b30ff5f 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1146,60 +1146,70 @@ impl LinkCollector<'_, '_> { callback, ); }; - match res { - Res::Primitive(_) if self.kind_side_channel.get().is_none() => match disambiguator { - Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - Some(ItemLink { link: ori_link.link, link_text, did: None, fragment }) - } - Some(other) => { - report_mismatch(other, Disambiguator::Primitive); - None - } - }, - Res::Primitive(_) => Some(ItemLink { link: ori_link, link_text, did: None, fragment }), - Res::Def(kind, id) => { - debug!("intra-doc link to {} resolved to {:?}", path_str, res); - - // Disallow e.g. linking to enums with `struct@` - debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); - match (self.kind_side_channel.take().map(|(kind, _)| kind).unwrap_or(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, Disambiguator::Kind(kind)); - return None; + + let (kind, id) = match res { + Res::Primitive(_) => { + if let Some((kind, id)) = self.kind_side_channel.take() { + (kind, id) + } else { + match disambiguator { + Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { + return Some(ItemLink { + link: ori_link.link, + link_text, + did: None, + fragment, + }); + } + Some(other) => { + report_mismatch(other, Disambiguator::Primitive); + return None; + } } } + } + Res::Def(kind, id) => (kind, id), + }; - // item can be non-local e.g. when using #[doc(primitive = "pointer")] - if let Some((src_id, dst_id)) = id - .as_local() - .and_then(|dst_id| item.def_id.as_local().map(|src_id| (src_id, dst_id))) - { - use rustc_hir::def_id::LOCAL_CRATE; + debug!("intra-doc link to {} resolved to {:?}", path_str, res); + + // Disallow e.g. linking to enums with `struct@` + debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); + match (self.kind_side_channel.take().map(|(kind, _)| kind).unwrap_or(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, Disambiguator::Kind(kind)); + return None; + } + } - let hir_src = self.cx.tcx.hir().local_def_id_to_hir_id(src_id); - let hir_dst = self.cx.tcx.hir().local_def_id_to_hir_id(dst_id); + // item can be non-local e.g. when using #[doc(primitive = "pointer")] + if let Some((src_id, dst_id)) = + id.as_local().and_then(|dst_id| item.def_id.as_local().map(|src_id| (src_id, dst_id))) + { + use rustc_hir::def_id::LOCAL_CRATE; - if self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_src) - && !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_dst) - { - privacy_error(cx, &item, &path_str, dox, &ori_link); - } - } - let id = clean::register_res(cx, rustc_hir::def::Res::Def(kind, id)); - Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment }) + let hir_src = self.cx.tcx.hir().local_def_id_to_hir_id(src_id); + let hir_dst = self.cx.tcx.hir().local_def_id_to_hir_id(dst_id); + + if self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_src) + && !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_dst) + { + privacy_error(cx, &item, &path_str, dox, &ori_link); } } + let id = clean::register_res(cx, rustc_hir::def::Res::Def(kind, id)); + Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment }) } fn resolve_with_disambiguator_cached( From 06b0900a2dc53ed4b5fa98c859fec5c224929eb8 Mon Sep 17 00:00:00 2001 From: max-heller Date: Sun, 3 Jan 2021 21:55:53 -0500 Subject: [PATCH 4/6] half working --- .../passes/collect_intra_doc_links.rs | 101 +++++++++--------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 090c45b30ff5f..f8e37d7bb56cf 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1147,69 +1147,70 @@ impl LinkCollector<'_, '_> { ); }; - let (kind, id) = match res { + let verify = |kind: DefKind, id: DefId| { + debug!("intra-doc link to {} resolved to {:?}", path_str, res); + + // Disallow e.g. linking to enums with `struct@` + debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); + match (self.kind_side_channel.take().map(|(kind, _)| kind).unwrap_or(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, Disambiguator::Kind(kind)); + return None; + } + } + + // item can be non-local e.g. when using #[doc(primitive = "pointer")] + if let Some((src_id, dst_id)) = id + .as_local() + .and_then(|dst_id| item.def_id.as_local().map(|src_id| (src_id, dst_id))) + { + use rustc_hir::def_id::LOCAL_CRATE; + + let hir_src = self.cx.tcx.hir().local_def_id_to_hir_id(src_id); + let hir_dst = self.cx.tcx.hir().local_def_id_to_hir_id(dst_id); + + if self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_src) + && !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_dst) + { + privacy_error(cx, &item, &path_str, dox, &ori_link); + } + } + + Some((kind, id)) + }; + + match res { Res::Primitive(_) => { if let Some((kind, id)) = self.kind_side_channel.take() { - (kind, id) + verify(kind, id)?; } else { match disambiguator { - Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => { - return Some(ItemLink { - link: ori_link.link, - link_text, - did: None, - fragment, - }); - } + Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {} Some(other) => { report_mismatch(other, Disambiguator::Primitive); return None; } } } + Some(ItemLink { link: ori_link.link, link_text, did: None, fragment }) } - Res::Def(kind, id) => (kind, id), - }; - - debug!("intra-doc link to {} resolved to {:?}", path_str, res); - - // Disallow e.g. linking to enums with `struct@` - debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); - match (self.kind_side_channel.take().map(|(kind, _)| kind).unwrap_or(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, Disambiguator::Kind(kind)); - return None; - } - } - - // item can be non-local e.g. when using #[doc(primitive = "pointer")] - if let Some((src_id, dst_id)) = - id.as_local().and_then(|dst_id| item.def_id.as_local().map(|src_id| (src_id, dst_id))) - { - use rustc_hir::def_id::LOCAL_CRATE; - - let hir_src = self.cx.tcx.hir().local_def_id_to_hir_id(src_id); - let hir_dst = self.cx.tcx.hir().local_def_id_to_hir_id(dst_id); - - if self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_src) - && !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_dst) - { - privacy_error(cx, &item, &path_str, dox, &ori_link); + Res::Def(kind, id) => { + let (kind, id) = verify(kind, id)?; + let id = clean::register_res(cx, rustc_hir::def::Res::Def(kind, id)); + Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment }) } } - let id = clean::register_res(cx, rustc_hir::def::Res::Def(kind, id)); - Some(ItemLink { link: ori_link.link, link_text, did: Some(id), fragment }) } fn resolve_with_disambiguator_cached( From 6f0413316f786dc27de5af8d15a070f368f8746d Mon Sep 17 00:00:00 2001 From: max-heller Date: Mon, 4 Jan 2021 15:38:30 -0500 Subject: [PATCH 5/6] fix incompatible disambiguator test --- .../incompatible-primitive-disambiguator.rs | 3 +++ .../incompatible-primitive-disambiguator.stderr | 15 +++++++++++++++ .../incompatible-primitive-disambiguator.rs | 2 -- 3 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.rs create mode 100644 src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.stderr delete mode 100644 src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs diff --git a/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.rs b/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.rs new file mode 100644 index 0000000000000..0d1d5d1134b7b --- /dev/null +++ b/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.rs @@ -0,0 +1,3 @@ +#![deny(broken_intra_doc_links)] +//! [static@u8::MIN] +//~^ ERROR incompatible link kind diff --git a/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.stderr b/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.stderr new file mode 100644 index 0000000000000..ed1c10f9e0cb8 --- /dev/null +++ b/src/test/rustdoc-ui/intra-doc/incompatible-primitive-disambiguator.stderr @@ -0,0 +1,15 @@ +error: incompatible link kind for `u8::MIN` + --> $DIR/incompatible-primitive-disambiguator.rs:2:6 + | +LL | //! [static@u8::MIN] + | ^^^^^^^^^^^^^^ help: to link to the associated constant, prefix with `const@`: `const@u8::MIN` + | +note: the lint level is defined here + --> $DIR/incompatible-primitive-disambiguator.rs:1:9 + | +LL | #![deny(broken_intra_doc_links)] + | ^^^^^^^^^^^^^^^^^^^^^^ + = note: this link resolved to an associated constant, which is not a static + +error: aborting due to previous error + diff --git a/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs b/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs deleted file mode 100644 index 1ea6ad9c7e691..0000000000000 --- a/src/test/rustdoc/intra-doc/incompatible-primitive-disambiguator.rs +++ /dev/null @@ -1,2 +0,0 @@ -#![deny(broken_intra_doc_links)] -//! [struct@str::trim] //~ ERROR incompatible link From 2bdbb0d1b4a8be7e8fef6d1e35856190f5c91e0f Mon Sep 17 00:00:00 2001 From: max-heller Date: Tue, 5 Jan 2021 15:15:25 -0500 Subject: [PATCH 6/6] Document hackiness around primitive associated item disambiguators --- src/librustdoc/passes/collect_intra_doc_links.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index f8e37d7bb56cf..11ee59b2401c8 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -1193,6 +1193,14 @@ impl LinkCollector<'_, '_> { match res { Res::Primitive(_) => { if let Some((kind, id)) = self.kind_side_channel.take() { + // 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, + // but attempting to add a call to `clean::register_res` causes an ICE. @jyn514 + // thinks `register_res` is only needed for cross-crate re-exports, but Rust + // 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)?; } else { match disambiguator {