Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rustdoc: Cache resolved links #77700

Merged
merged 3 commits into from
Dec 15, 2020
Merged

Rustdoc: Cache resolved links #77700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
850437b
Cache link resolution results in current module
bugadani Oct 9, 2020
cc31b99
Review suggestions
bugadani Nov 30, 2020
fa64c27
Add test case for Self:: links
bugadani Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 120 additions & 47 deletions src/librustdoc/passes/collect_intra_doc_links.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! [RFC 1946]: https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md

use rustc_ast as ast;
use rustc_data_structures::stable_set::FxHashSet;
use rustc_data_structures::{fx::FxHashMap, stable_set::FxHashSet};
use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_expand::base::SyntaxExtensionKind;
use rustc_hir as hir;
Expand Down Expand Up @@ -168,6 +168,27 @@ enum AnchorFailure {
RustdocAnchorConflict(Res),
}

#[derive(Clone, Debug, Hash, PartialEq, Eq)]
struct ResolutionInfo {
module_id: DefId,
dis: Option<Disambiguator>,
path_str: String,
extra_fragment: Option<String>,
}

struct DiagnosticInfo<'a> {
item: &'a Item,
dox: &'a str,
ori_link: &'a str,
link_range: Option<Range<usize>>,
}

#[derive(Clone, Debug, Hash)]
struct CachedLink {
pub res: (Res, Option<String>),
pub side_channel: Option<(DefKind, DefId)>,
}

struct LinkCollector<'a, 'tcx> {
cx: &'a DocContext<'tcx>,
/// A stack of modules used to decide what scope to resolve in.
Expand All @@ -179,11 +200,18 @@ struct LinkCollector<'a, 'tcx> {
/// because `clean` and the disambiguator code expect them to be different.
/// See the code for associated items on inherent impls for details.
kind_side_channel: Cell<Option<(DefKind, DefId)>>,
/// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link
visited_links: FxHashMap<ResolutionInfo, CachedLink>,
}

impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
fn new(cx: &'a DocContext<'tcx>) -> Self {
LinkCollector { cx, mod_ids: Vec::new(), kind_side_channel: Cell::new(None) }
LinkCollector {
cx,
mod_ids: Vec::new(),
kind_side_channel: Cell::new(None),
visited_links: FxHashMap::default(),
}
}

/// Given a full link, parse it as an [enum struct variant].
Expand Down Expand Up @@ -937,7 +965,7 @@ impl LinkCollector<'_, '_> {
///
/// FIXME(jynelson): this is way too many arguments
fn resolve_link(
&self,
&mut self,
item: &Item,
dox: &str,
self_name: &Option<String>,
Expand All @@ -962,6 +990,7 @@ impl LinkCollector<'_, '_> {
let link = ori_link.replace("`", "");
let parts = link.split('#').collect::<Vec<_>>();
let (link, extra_fragment) = if parts.len() > 2 {
// A valid link can't have multiple #'s
anchor_failure(cx, &item, &link, dox, link_range, AnchorFailure::MultipleAnchors);
return None;
} else if parts.len() == 2 {
Expand Down Expand Up @@ -1075,16 +1104,15 @@ impl LinkCollector<'_, '_> {
return None;
}

let (mut res, mut fragment) = self.resolve_with_disambiguator(
disambiguator,
item,
dox,
path_str,
let key = ResolutionInfo {
module_id,
dis: disambiguator,
path_str: path_str.to_owned(),
extra_fragment,
&ori_link,
link_range.clone(),
)?;
};
let diag =
DiagnosticInfo { item, dox, ori_link: &ori_link, link_range: link_range.clone() };
let (mut res, mut fragment) = self.resolve_with_disambiguator_cached(key, diag)?;

// Check for a primitive which might conflict with a module
// Report the ambiguity and require that the user specify which one they meant.
Expand Down Expand Up @@ -1192,22 +1220,49 @@ impl LinkCollector<'_, '_> {
}
}

fn resolve_with_disambiguator_cached(
&mut self,
key: ResolutionInfo,
diag: DiagnosticInfo<'_>,
) -> Option<(Res, Option<String>)> {
// Try to look up both the result and the corresponding side channel value
if let Some(ref cached) = self.visited_links.get(&key) {
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
self.kind_side_channel.set(cached.side_channel.clone());
return Some(cached.res.clone());
}

let res = self.resolve_with_disambiguator(&key, diag);

// Cache only if resolved successfully - don't silence duplicate errors
if let Some(res) = &res {
// Store result for the actual namespace
self.visited_links.insert(
key,
CachedLink {
res: res.clone(),
side_channel: self.kind_side_channel.clone().into_inner(),
},
);
}

res
}

/// After parsing the disambiguator, resolve the main part of the link.
// FIXME(jynelson): wow this is just so much
fn resolve_with_disambiguator(
bugadani marked this conversation as resolved.
Show resolved Hide resolved
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
&self,
disambiguator: Option<Disambiguator>,
item: &Item,
dox: &str,
path_str: &str,
base_node: DefId,
extra_fragment: Option<String>,
ori_link: &str,
link_range: Option<Range<usize>>,
key: &ResolutionInfo,
diag: DiagnosticInfo<'_>,
) -> Option<(Res, Option<String>)> {
let disambiguator = key.dis;
let path_str = &key.path_str;
let base_node = key.module_id;
let extra_fragment = &key.extra_fragment;

match disambiguator.map(Disambiguator::ns) {
Some(ns @ (ValueNS | TypeNS)) => {
match self.resolve(path_str, ns, base_node, &extra_fragment) {
match self.resolve(path_str, ns, base_node, extra_fragment) {
Ok(res) => Some(res),
Err(ErrorKind::Resolve(box mut kind)) => {
// We only looked in one namespace. Try to give a better error if possible.
Expand All @@ -1216,24 +1271,21 @@ impl LinkCollector<'_, '_> {
// 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] {
if let Some(res) = self.check_full_res(
new_ns,
path_str,
base_node,
&extra_fragment,
) {
if let Some(res) =
self.check_full_res(new_ns, path_str, base_node, extra_fragment)
{
kind = ResolutionFailure::WrongNamespace(res, ns);
break;
}
}
}
resolution_failure(
self,
&item,
diag.item,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just change these 4 arguments to take diag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either it was forgotten, or resolution_failure is called in a lot of places where creating DiagInfo would have been a chore. I don't recall exactly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a few other places but they could be made to use the struct.

path_str,
disambiguator,
dox,
link_range,
diag.dox,
diag.link_range,
smallvec![kind],
);
// This could just be a normal link or a broken link
Expand All @@ -1242,7 +1294,14 @@ impl LinkCollector<'_, '_> {
return None;
}
Err(ErrorKind::AnchorFailure(msg)) => {
anchor_failure(self.cx, &item, &ori_link, dox, link_range, msg);
anchor_failure(
self.cx,
diag.item,
diag.ori_link,
diag.dox,
diag.link_range,
msg,
);
return None;
}
}
Expand All @@ -1253,21 +1312,35 @@ impl LinkCollector<'_, '_> {
macro_ns: self
.resolve_macro(path_str, base_node)
.map(|res| (res, extra_fragment.clone())),
type_ns: match self.resolve(path_str, TypeNS, base_node, &extra_fragment) {
type_ns: match self.resolve(path_str, TypeNS, base_node, extra_fragment) {
Ok(res) => {
debug!("got res in TypeNS: {:?}", res);
Ok(res)
}
Err(ErrorKind::AnchorFailure(msg)) => {
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
anchor_failure(
self.cx,
diag.item,
diag.ori_link,
diag.dox,
diag.link_range,
msg,
);
return None;
}
Err(ErrorKind::Resolve(box kind)) => Err(kind),
},
value_ns: match self.resolve(path_str, ValueNS, base_node, &extra_fragment) {
value_ns: match self.resolve(path_str, ValueNS, base_node, extra_fragment) {
Ok(res) => Ok(res),
Err(ErrorKind::AnchorFailure(msg)) => {
anchor_failure(self.cx, &item, ori_link, dox, link_range, msg);
anchor_failure(
self.cx,
diag.item,
diag.ori_link,
diag.dox,
diag.link_range,
msg,
);
return None;
}
Err(ErrorKind::Resolve(box kind)) => Err(kind),
Expand All @@ -1278,7 +1351,7 @@ impl LinkCollector<'_, '_> {
Res::Def(DefKind::Ctor(..), _) | Res::SelfCtor(..) => {
Err(ResolutionFailure::WrongNamespace(res, TypeNS))
}
_ => match (fragment, extra_fragment) {
_ => match (fragment, extra_fragment.clone()) {
(Some(fragment), Some(_)) => {
// Shouldn't happen but who knows?
Ok((res, Some(fragment)))
Expand All @@ -1294,11 +1367,11 @@ impl LinkCollector<'_, '_> {
if len == 0 {
resolution_failure(
self,
&item,
diag.item,
path_str,
disambiguator,
dox,
link_range,
diag.dox,
diag.link_range,
candidates.into_iter().filter_map(|res| res.err()).collect(),
);
// this could just be a normal link
Expand All @@ -1317,35 +1390,35 @@ impl LinkCollector<'_, '_> {
let candidates = candidates.map(|candidate| candidate.ok().map(|(res, _)| res));
ambiguity_error(
self.cx,
&item,
diag.item,
path_str,
dox,
link_range,
diag.dox,
diag.link_range,
candidates.present_items().collect(),
);
return None;
}
}
Some(MacroNS) => {
match self.resolve_macro(path_str, base_node) {
Ok(res) => Some((res, extra_fragment)),
Ok(res) => Some((res, extra_fragment.clone())),
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, base_node, &extra_fragment)
self.check_full_res(ns, path_str, base_node, extra_fragment)
{
kind = ResolutionFailure::WrongNamespace(res, MacroNS);
break;
}
}
resolution_failure(
self,
&item,
diag.item,
path_str,
disambiguator,
dox,
link_range,
diag.dox,
diag.link_range,
smallvec![kind],
);
return None;
Expand All @@ -1356,7 +1429,7 @@ impl LinkCollector<'_, '_> {
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
/// Disambiguators for a link.
enum Disambiguator {
/// `prim@`
Expand Down
14 changes: 14 additions & 0 deletions src/test/rustdoc/intra-link-self-cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![crate_name = "foo"]
// @has foo/enum.E1.html '//a/@href' '../foo/enum.E1.html#variant.A'

/// [Self::A::b]
pub enum E1 {
A { b: usize }
}

// @has foo/enum.E2.html '//a/@href' '../foo/enum.E2.html#variant.A'

/// [Self::A::b]
pub enum E2 {
A { b: usize }
}