bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #92635 - camelid:yet-more-cleanup, r=Manishearth

Browse source 
rustdoc: Yet more intra-doc links cleanup

r? `@Manishearth`
This commit is contained in:
Matthias Krüger 2022-01-16 16:58:14 +01:00 • committed by GitHub
commit e1b943991f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 182 additions and 163 deletions
Download patch file Download diff file Expand all files Collapse all files

310
src/librustdoc/passes/collect_intra_doc_links.rs
View file

@ -6,13 +6,12 @@ use rustc_ast as ast;
use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet}; use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet};
use rustc_errors::{Applicability, DiagnosticBuilder}; use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_expand::base::SyntaxExtensionKind; use rustc_expand::base::SyntaxExtensionKind;
use rustc_hir as hir;
use rustc_hir::def::{ use rustc_hir::def::{
DefKind, DefKind,
Namespace::{self, *}, Namespace::{self, *},
PerNS, 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::ty::{DefIdTree, Ty, TyCtxt};
use rustc_middle::{bug, span_bug, ty}; use rustc_middle::{bug, span_bug, ty};
use rustc_resolve::ParentScope; use rustc_resolve::ParentScope;
@ -109,6 +108,45 @@ impl Res {
Res::Primitive(_) => None, 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<ResolveRes> for Res { impl TryFrom<ResolveRes> for Res {
@ -346,7 +384,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
/// In particular, this will return an error whenever there aren't three /// In particular, this will return an error whenever there aren't three
/// full path segments left in the link. /// full path segments left in the link.
/// ///
/// [enum struct variant]: hir::VariantData::Struct /// [enum struct variant]: rustc_hir::VariantData::Struct
fn variant_field<'path>( fn variant_field<'path>(
&self, &self,
path_str: &'path str, path_str: &'path str,
@ -667,10 +705,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
})) }))
} }
/// Returns: /// Resolve an associated item, returning its containing page's `Res`
/// - None if no associated item was found /// and the fragment targeting the associated item on its page.
/// - Some((_, _, Some(_))) if an item was found and should go through a side channel
/// - Some((_, _, None)) otherwise
fn resolve_associated_item( fn resolve_associated_item(
&mut self, &mut self,
root_res: Res, root_res: Res,
@ -958,17 +994,7 @@ impl<'a, 'tcx> DocVisitor for LinkCollector<'a, 'tcx> {
{ {
self.cx.tcx.parent(did) self.cx.tcx.parent(did)
} }
Some(did) => match self.cx.tcx.parent(did) { Some(did) => Some(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),
},
}; };
// FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly // FIXME(jynelson): this shouldn't go through stringification, rustdoc should just use the DefId directly
@ -1277,34 +1303,88 @@ impl LinkCollector<'_, '_> {
} }
} }
let report_mismatch = |specified: Disambiguator, resolved: Disambiguator| { match res {
// The resolved item did not match the disambiguator; give a better error than 'not found' Res::Primitive(prim) => {
let msg = format!("incompatible link kind for `{}`", path_str); if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment {
let callback = |diag: &mut DiagnosticBuilder<'_>, sp: Option<rustc_span::Span>| { // We're actually resolving an associated item of a primitive, so we need to
let note = format!( // verify the disambiguator (if any) matches the type of the associated item.
"this link resolved to {} {}, which is not {} {}", // This case should really follow the same flow as the `Res::Def` branch below,
resolved.article(), // but attempting to add a call to `clean::register_res` causes an ICE. @jyn514
resolved.descr(), // thinks `register_res` is only needed for cross-crate re-exports, but Rust
specified.article(), // doesn't allow statements like `use str::trim;`, making this a (hopefully)
specified.descr() // valid omission. See https://github.com/rust-lang/rust/pull/80660#discussion_r551585677
); // for discussion on the matter.
if let Some(sp) = sp { let kind = self.cx.tcx.def_kind(id);
diag.span_label(sp, &note); self.verify_disambiguator(
} else { path_str,
diag.note(&note); &ori_link,
} kind,
suggest_disambiguator(resolved, diag, path_str, &ori_link.link, sp); id,
}; disambiguator,
report_diagnostic(self.cx.tcx, BROKEN_INTRA_DOC_LINKS, &msg, &diag_info, callback); item,
}; &diag_info,
)?;
let verify = |kind: DefKind, id: DefId| { // FIXME: it would be nice to check that the feature gate was enabled in the original crate, not just ignore it altogether.
let (kind, id) = if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment { // However I'm not sure how to check that across crates.
if prim == PrimitiveType::RawPointer
&& item.def_id.is_local()
&& !self.cx.tcx.features().intra_doc_pointers
{
self.report_rawptr_assoc_feature_gate(dox, &ori_link, item);
}
} else {
match disambiguator {
Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {}
Some(other) => {
self.report_disambiguator_mismatch(
path_str, &ori_link, other, res, &diag_info,
);
return None;
}
}
}
Some(ItemLink {
link: ori_link.link,
link_text,
did: res.def_id(self.cx.tcx),
fragment,
})
}
Res::Def(kind, id) => {
let (kind_for_dis, id_for_dis) =
if let Some(UrlFragment::Item(ItemFragment(_, id))) = fragment {
(self.cx.tcx.def_kind(id), id) (self.cx.tcx.def_kind(id), id)
} else { } else {
(kind, id) (kind, id)
}; };
debug!("intra-doc link to {} resolved to {:?} (id: {:?})", path_str, res, 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<Disambiguator>,
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@` // Disallow e.g. linking to enums with `struct@`
debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator); debug!("saw kind {:?} with disambiguator {:?}", kind, disambiguator);
@ -1321,7 +1401,7 @@ impl LinkCollector<'_, '_> {
=> {} => {}
(actual, Some(Disambiguator::Kind(expected))) if actual == expected => {} (actual, Some(Disambiguator::Kind(expected))) if actual == expected => {}
(_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => { (_, Some(specified @ Disambiguator::Kind(_) | specified @ Disambiguator::Primitive)) => {
report_mismatch(specified, Disambiguator::Kind(kind)); self.report_disambiguator_mismatch(path_str,ori_link,specified, Res::Def(kind, id),diag_info);
return None; return None;
} }
} }
@ -1338,59 +1418,39 @@ impl LinkCollector<'_, '_> {
if self.cx.tcx.privacy_access_levels(()).is_exported(src_id) if self.cx.tcx.privacy_access_levels(()).is_exported(src_id)
&& !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id) && !self.cx.tcx.privacy_access_levels(()).is_exported(dst_id)
{ {
privacy_error(self.cx, &diag_info, path_str); privacy_error(self.cx, diag_info, path_str);
} }
} }
Some(()) 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,
// 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)?;
// 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.
if prim == PrimitiveType::RawPointer
&& item.def_id.is_local()
&& !self.cx.tcx.features().intra_doc_pointers
{
self.report_rawptr_assoc_feature_gate(dox, &ori_link, item);
} }
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<rustc_span::Span>| {
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, &note);
} else { } else {
match disambiguator { diag.note(&note);
Some(Disambiguator::Primitive | Disambiguator::Namespace(_)) | None => {}
Some(other) => {
report_mismatch(other, Disambiguator::Primitive);
return None;
}
}
}
Some(ItemLink {
link: ori_link.link,
link_text,
did: res.def_id(self.cx.tcx),
fragment,
})
}
Res::Def(kind, id) => {
verify(kind, id)?;
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 })
}
} }
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) { fn report_rawptr_assoc_feature_gate(&self, dox: &str, ori_link: &MarkdownLink, item: &Item) {
@ -1413,7 +1473,6 @@ impl LinkCollector<'_, '_> {
diag: DiagnosticInfo<'_>, diag: DiagnosticInfo<'_>,
cache_resolution_failure: bool, cache_resolution_failure: bool,
) -> Option<(Res, Option<UrlFragment>)> { ) -> Option<(Res, Option<UrlFragment>)> {
// Try to look up both the result and the corresponding side channel value
if let Some(ref cached) = self.visited_links.get(&key) { if let Some(ref cached) = self.visited_links.get(&key) {
match cached { match cached {
Some(cached) => { Some(cached) => {
@ -1686,53 +1745,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 { fn ns(self) -> Namespace {
match self { match self {
Self::Namespace(n) => n, Self::Namespace(n) => n,
@ -1754,9 +1766,9 @@ impl Disambiguator {
fn descr(self) -> &'static str { fn descr(self) -> &'static str {
match self { match self {
Self::Namespace(n) => n.descr(), Self::Namespace(n) => n.descr(),
// HACK(jynelson): by looking at the source I saw the DefId we pass // HACK(jynelson): the source of `DefKind::descr` only uses the DefId for
// for `expected.descr()` doesn't matter, since it's not a crate // printing "module" vs "crate" so using the wrong ID is not a huge problem
Self::Kind(k) => k.descr(DefId::local(hir::def_id::DefIndex::from_usize(0))), Self::Kind(k) => k.descr(CRATE_DEF_ID.to_def_id()),
Self::Primitive => "builtin type", Self::Primitive => "builtin type",
} }
} }
@ -2080,16 +2092,7 @@ fn resolution_failure(
ResolutionFailure::NotResolved { .. } => unreachable!("handled above"), ResolutionFailure::NotResolved { .. } => unreachable!("handled above"),
ResolutionFailure::Dummy => continue, ResolutionFailure::Dummy => continue,
ResolutionFailure::WrongNamespace { res, expected_ns } => { ResolutionFailure::WrongNamespace { res, expected_ns } => {
if let Res::Def(kind, _) = res { suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp);
let disambiguator = Disambiguator::Kind(kind);
suggest_disambiguator(
disambiguator,
diag,
path_str,
diag_info.ori_link,
sp,
)
}
format!( format!(
"this link resolves to {}, which is not in the {} namespace", "this link resolves to {}, which is not in the {} namespace",
@ -2224,8 +2227,7 @@ fn ambiguity_error(
} }
for res in candidates { for res in candidates {
let disambiguator = Disambiguator::from_res(res); suggest_disambiguator(res, diag, path_str, diag_info.ori_link, sp);
suggest_disambiguator(disambiguator, diag, path_str, diag_info.ori_link, sp);
} }
}); });
} }
@ -2233,14 +2235,14 @@ fn ambiguity_error(
/// In case of an ambiguity or mismatched disambiguator, suggest the correct /// In case of an ambiguity or mismatched disambiguator, suggest the correct
/// disambiguator. /// disambiguator.
fn suggest_disambiguator( fn suggest_disambiguator(
disambiguator: Disambiguator, res: Res,
diag: &mut DiagnosticBuilder<'_>, diag: &mut DiagnosticBuilder<'_>,
path_str: &str, path_str: &str,
ori_link: &str, ori_link: &str,
sp: Option<rustc_span::Span>, sp: Option<rustc_span::Span>,
) { ) {
let suggestion = disambiguator.suggestion(); let suggestion = res.disambiguator_suggestion();
let help = format!("to link to the {}, {}", disambiguator.descr(), suggestion.descr()); let help = format!("to link to the {}, {}", res.descr(), suggestion.descr());
if let Some(sp) = sp { if let Some(sp) = sp {
let mut spans = suggestion.as_help_span(path_str, ori_link, sp); let mut spans = suggestion.as_help_span(path_str, ori_link, sp);

5
src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.rs
View file

@ -73,4 +73,9 @@ trait T {}
//~^ ERROR incompatible link kind for `f` //~^ ERROR incompatible link kind for `f`
//~| NOTE this link resolved //~| NOTE this link resolved
//~| HELP add parentheses //~| 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 crate, prefix with `mod@`
pub fn f() {} pub fn f() {}

13
src/test/rustdoc-ui/intra-doc/disambiguator-mismatch.stderr
View file

@ -138,5 +138,16 @@ LL - /// Link to [const@f]
LL + /// Link to [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 crate, prefix with `mod@`
|
LL | /// Link to [mod@std]
| ~~~~
error: aborting due to 13 previous errors

9
src/test/rustdoc/intra-link-prim-self.rs → src/test/rustdoc/intra-doc/prim-self.rs
View file

@ -1,13 +1,15 @@
#![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::broken_intra_doc_links)]
#![allow(incomplete_features)] // inherent_associated_types
#![feature(lang_items)] #![feature(lang_items)]
#![feature(no_core)] #![feature(no_core)]
#![feature(rustdoc_internals)] #![feature(rustdoc_internals)]
#![feature(inherent_associated_types)]
#![no_core] #![no_core]
#[lang = "usize"] #[lang = "usize"]
/// [Self::f] /// [Self::f]
/// [Self::MAX] /// [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#method.f"]' 'Self::f'
// @has - '//a[@href="primitive.usize.html#associatedconstant.MAX"]' 'Self::MAX' // @has - '//a[@href="primitive.usize.html#associatedconstant.MAX"]' 'Self::MAX'
impl usize { impl usize {
@ -17,10 +19,9 @@ impl usize {
/// 10 and 2^32 are basically the same. /// 10 and 2^32 are basically the same.
pub const MAX: usize = 10; pub const MAX: usize = 10;
// FIXME(#8995) uncomment this when associated types in inherent impls are supported // @has - '//a[@href="primitive.usize.html#associatedtype.ME"]' 'Self::ME'
// @ has - '//a[@href="{{channel}}/std/primitive.usize.html#associatedtype.ME"]' 'Self::ME'
/// [Self::ME] /// [Self::ME]
//pub type ME = usize; pub type ME = usize;
} }
#[doc(primitive = "usize")] #[doc(primitive = "usize")]

0
src/test/rustdoc/intra-link-self-cache.rs → src/test/rustdoc/intra-doc/self-cache.rs
View file