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.

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

rustdoc: Unify external_paths and paths cache map #86909

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::clean::{
self, utils, Attributes, AttributesExt, GetDefId, ItemId, NestedAttributesExt, Type,
};
use crate::core::DocContext;
use crate::formats::item_type::ItemType;
use crate::formats::{cache::CachedPath, item_type::ItemType};

use super::{Clean, Visibility};

Expand Down Expand Up @@ -189,7 +189,7 @@ crate fn record_extern_fqn(cx: &mut DocContext<'_>, did: DefId, kind: ItemType)
if did.is_local() {
cx.cache.exact_paths.insert(did, fqn);
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge the exact_paths too?

} else {
cx.cache.external_paths.insert(did, (fqn, kind));
cx.cache.paths.insert(did, CachedPath::Extern(fqn, kind));
}
}

Expand Down
51 changes: 33 additions & 18 deletions src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@ use crate::html::markdown::short_markdown_summary;
use crate::html::render::cache::{get_index_search_type, ExternalLocation};
use crate::html::render::IndexItem;

/// A path that is either local, or external. It consists of a fully qualified name,
/// and a type description.
#[derive(Debug, Clone)]
crate enum CachedPath {
Local(Vec<String>, ItemType),
Extern(Vec<String>, ItemType),
}

impl CachedPath {
crate fn is_local(&self) -> bool {
matches!(self, CachedPath::Local(..))
}

crate fn is_extern(&self) -> bool {
matches!(self, CachedPath::Extern(..))
}
}

/// This cache is used to store information about the [`clean::Crate`] being
/// rendered in order to provide more useful documentation. This contains
/// information like all implementors of a trait, all traits a type implements,
Expand All @@ -35,16 +53,12 @@ crate struct Cache {
/// found on that implementation.
crate impls: FxHashMap<DefId, Vec<Impl>>,

/// Maintains a mapping of local crate `DefId`s to the fully qualified name
/// and "short type description" of that node. This is used when generating
/// URLs when a type is being linked to. External paths are not located in
/// this map because the `External` type itself has all the information
/// necessary.
crate paths: FxHashMap<DefId, (Vec<String>, ItemType)>,

/// Similar to `paths`, but only holds external paths. This is only used for
/// generating explicit hyperlinks to other crates.
crate external_paths: FxHashMap<DefId, (Vec<String>, ItemType)>,
/// Maintain a mapping of `DefId`s to the fully qualified name and "shorty type description" of
/// that node. This map contains local and external cached paths that are differentiated using
/// the [`CachedPath`] enum.
///
/// This was previously known as `extern_paths` and `paths`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this comment, people can find it in the git history if they really care.

Suggested change
/// This was previously known as `extern_paths` and `paths`.

crate paths: FxHashMap<DefId, CachedPath>,

/// Maps local `DefId`s of exported types to fully qualified paths.
/// Unlike 'paths', this mapping ignores any renames that occur
Expand Down Expand Up @@ -156,7 +170,7 @@ impl Cache {
let extern_url = extern_html_root_urls.get(&*name.as_str()).map(|u| &**u);
let did = DefId { krate: n, index: CRATE_DEF_INDEX };
self.extern_locations.insert(n, e.location(extern_url, &dst, tcx));
self.external_paths.insert(did, (vec![name.to_string()], ItemType::Module));
self.paths.insert(did, CachedPath::Extern(vec![name.to_string()], ItemType::Module));
}

// Cache where all known primitives have their documentation located.
Expand Down Expand Up @@ -267,15 +281,15 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
// for where the type was defined. On the other
// hand, `paths` always has the right
// information if present.
Some(&(
Some(CachedPath::Local(
ref fqp,
ItemType::Trait
| ItemType::Struct
| ItemType::Union
| ItemType::Enum,
)) => Some(&fqp[..fqp.len() - 1]),
Some(..) => Some(&*self.cache.stack),
None => None,
Some(CachedPath::Local(..)) => Some(&*self.cache.stack),
_ => None,
};
((Some(*last), path), true)
}
Expand Down Expand Up @@ -353,15 +367,16 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
{
self.cache.paths.insert(
item.def_id.expect_def_id(),
(self.cache.stack.clone(), item.type_()),
CachedPath::Local(self.cache.stack.clone(), item.type_()),
);
}
}
}
clean::PrimitiveItem(..) => {
self.cache
.paths
.insert(item.def_id.expect_def_id(), (self.cache.stack.clone(), item.type_()));
self.cache.paths.insert(
item.def_id.expect_def_id(),
CachedPath::Local(self.cache.stack.clone(), item.type_()),
);
}

clean::ExternCrateItem { .. }
Expand Down
9 changes: 4 additions & 5 deletions src/librustdoc/html/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use rustc_target::spec::abi::Abi;
use crate::clean::{
self, utils::find_nearest_parent_module, ExternalCrate, GetDefId, ItemId, PrimitiveType,
};
use crate::formats::item_type::ItemType;
use crate::formats::{cache::CachedPath, item_type::ItemType};
use crate::html::escape::Escape;
use crate::html::render::cache::ExternalLocation;
use crate::html::render::Context;
Expand Down Expand Up @@ -483,13 +483,12 @@ crate fn href(did: DefId, cx: &Context<'_>) -> Option<(String, ItemType, Vec<Str
return None;
}

let (fqp, shortty, mut url_parts) = match cache.paths.get(&did) {
Some(&(ref fqp, shortty)) => (fqp, shortty, {
let (fqp, shortty, mut url_parts) = match *cache.paths.get(&did)? {
CachedPath::Local(ref fqp, shortty) => (fqp, shortty, {
let module_fqp = to_module_fqp(shortty, fqp);
href_relative_parts(module_fqp, relative_to)
}),
None => {
let &(ref fqp, shortty) = cache.external_paths.get(&did)?;
CachedPath::Extern(ref fqp, shortty) => {
let module_fqp = to_module_fqp(shortty, fqp);
(
fqp,
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/html/render/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::clean;
use crate::clean::types::{
FnDecl, FnRetTy, GenericBound, Generics, GetDefId, Type, WherePredicate,
};
use crate::formats::cache::Cache;
use crate::formats::cache::{Cache, CachedPath};
use crate::formats::item_type::ItemType;
use crate::html::markdown::short_markdown_summary;
use crate::html::render::{IndexItem, IndexItemFunctionType, RenderType, TypeWithKind};
Expand All @@ -33,7 +33,7 @@ crate fn build_index<'tcx>(krate: &clean::Crate, cache: &mut Cache, tcx: TyCtxt<
// Attach all orphan items to the type's definition if the type
// has since been learned.
for &(did, ref item) in &cache.orphan_impl_items {
if let Some(&(ref fqp, _)) = cache.paths.get(&did) {
if let Some(CachedPath::Local(fqp, _)) = cache.paths.get(&did) {
let desc = item
.doc_value()
.map_or_else(String::new, |s| short_markdown_summary(&s, &item.link_names(&cache)));
Expand Down Expand Up @@ -90,7 +90,7 @@ crate fn build_index<'tcx>(krate: &clean::Crate, cache: &mut Cache, tcx: TyCtxt<
defid_to_pathid.insert(defid, pathid);
lastpathid += 1;

if let Some(&(ref fqp, short)) = paths.get(&defid) {
if let Some(&CachedPath::Local(ref fqp, short)) = paths.get(&defid) {
crate_paths.push((short, fqp.last().unwrap().clone()));
Some(pathid)
} else {
Expand Down
6 changes: 4 additions & 2 deletions src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::clean::ExternalCrate;
use crate::config::RenderOptions;
use crate::docfs::{DocFS, PathError};
use crate::error::Error;
use crate::formats::cache::Cache;
use crate::formats::cache::{Cache, CachedPath};
use crate::formats::item_type::ItemType;
use crate::formats::FormatRenderer;
use crate::html::escape::Escape;
Expand Down Expand Up @@ -230,7 +230,9 @@ impl<'tcx> Context<'tcx> {
&self.shared.style_files,
)
} else {
if let Some(&(ref names, ty)) = self.cache.paths.get(&it.def_id.expect_def_id()) {
if let Some(&CachedPath::Local(ref names, ty)) =
self.cache.paths.get(&it.def_id.expect_def_id())
{
let mut path = String::new();
for name in &names[..names.len() - 1] {
path.push_str(name);
Expand Down
9 changes: 7 additions & 2 deletions src/librustdoc/html/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ use serde::{Serialize, Serializer};
use crate::clean::{self, GetDefId, ItemId, RenderedLink, SelfTy};
use crate::docfs::PathError;
use crate::error::Error;
use crate::formats::cache::Cache;
use crate::formats::cache::{Cache, CachedPath};
use crate::formats::item_type::ItemType;
use crate::formats::{AssocItemRender, Impl, RenderMode};
use crate::html::escape::Escape;
Expand Down Expand Up @@ -2339,7 +2339,12 @@ fn collect_paths_for_type(first_ty: clean::Type, cache: &Cache) -> Vec<String> {

match ty {
clean::Type::ResolvedPath { did, .. } => {
let get_extern = || cache.external_paths.get(&did).map(|s| s.0.clone());
let get_extern = || {
cache.paths.get(&did).and_then(|p| match p {
CachedPath::Extern(a, _) => Some(a.clone()),
CachedPath::Local(..) => None,
})
};
let fqp = cache.exact_paths.get(&did).cloned().or_else(get_extern);

if let Some(path) = fqp {
Expand Down
12 changes: 5 additions & 7 deletions src/librustdoc/html/render/print_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use super::{
Context,
};
use crate::clean::{self, GetDefId};
use crate::formats::item_type::ItemType;
use crate::formats::{cache::CachedPath, item_type::ItemType};
use crate::formats::{AssocItemRender, Impl, RenderMode};
use crate::html::escape::Escape;
use crate::html::format::{
Expand Down Expand Up @@ -696,7 +696,7 @@ fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Tra
i.inner_impl()
.for_
.def_id_full(cx.cache())
.map_or(true, |d| cx.cache.paths.contains_key(&d))
.map_or(true, |d| cx.cache.paths.get(&d).map_or(false, CachedPath::is_local))
});

let (mut synthetic, mut concrete): (Vec<&&Impl>, Vec<&&Impl>) =
Expand Down Expand Up @@ -784,11 +784,9 @@ fn item_trait(w: &mut Buffer, cx: &Context<'_>, it: &clean::Item, t: &clean::Tra
src=\"{root_path}/implementors/{path}/{ty}.{name}.js\" async>\
</script>",
root_path = vec![".."; cx.current.len()].join("/"),
path = if it.def_id.is_local() {
cx.current.join("/")
} else {
let (ref path, _) = cx.cache.external_paths[&it.def_id.expect_def_id()];
path[..path.len() - 1].join("/")
path = match cx.cache.paths[&it.def_id.expect_def_id()] {
CachedPath::Extern(_, _) => cx.current.join("/"),
CachedPath::Local(ref path, _) => path[..path.len() - 1].join("/"),
Comment on lines -787 to +789
Copy link
Member

Choose a reason for hiding this comment

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

This logic doesn't seem the same at all. What does Extern have to do with item.def_id.is_local()? Why do you use the code for Local that used to be used for external_paths?

},
ty = it.type_(),
name = *it.name.as_ref().unwrap()
Expand Down
13 changes: 6 additions & 7 deletions src/librustdoc/html/render/write_shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use crate::clean::Crate;
use crate::config::{EmitType, RenderOptions};
use crate::docfs::PathError;
use crate::error::Error;
use crate::formats::cache::CachedPath;
use crate::html::{layout, static_files};

static FILES_UNVERSIONED: Lazy<FxHashMap<&str, &[u8]>> = Lazy::new(|| {
Expand Down Expand Up @@ -488,12 +489,9 @@ pub(super) fn write_shared(
//
// FIXME: this is a vague explanation for why this can't be a `get`, in
// theory it should be...
let &(ref remote_path, remote_item_type) = match cx.cache.paths.get(&did) {
Some(p) => p,
None => match cx.cache.external_paths.get(&did) {
Some(p) => p,
None => continue,
},
let (remote_path, remote_item_type) = match cx.cache.paths.get(&did) {
Some(&CachedPath::Local(ref a, b) | &CachedPath::Extern(ref a, b)) => (a, b),
None => continue,
};

#[derive(Serialize)]
Expand Down Expand Up @@ -528,7 +526,8 @@ pub(super) fn write_shared(
// Only create a js file if we have impls to add to it. If the trait is
// documented locally though we always create the file to avoid dead
// links.
if implementors.is_empty() && !cx.cache.paths.contains_key(&did) {
if implementors.is_empty() && cx.cache.paths.get(&did).map_or(false, CachedPath::is_extern)
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same logic.

Suggested change
if implementors.is_empty() && cx.cache.paths.get(&did).map_or(false, CachedPath::is_extern)
if implementors.is_empty() && cx.cache.paths.get(&did).map_or(true, CachedPath::is_extern)

{
continue;
}

Expand Down
14 changes: 5 additions & 9 deletions src/librustdoc/json/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use crate::clean;
use crate::clean::ExternalCrate;
use crate::config::RenderOptions;
use crate::error::Error;
use crate::formats::cache::Cache;
use crate::formats::cache::{Cache, CachedPath};
use crate::formats::FormatRenderer;
use crate::html::render::cache::ExternalLocation;
use crate::json::conversions::{from_item_id, IntoWithTcx};
Expand Down Expand Up @@ -99,13 +99,10 @@ impl JsonRenderer<'tcx> {
.cache
.paths
.get(&id)
.unwrap_or_else(|| {
self.cache
.external_paths
.get(&id)
.expect("Trait should either be in local or external paths")
.map(|p| match p {
CachedPath::Local(a, _) | CachedPath::Extern(a, _) => a,
})
.0
.unwrap()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.unwrap()
.expect("Trait should either be in local or external paths")

.last()
.map(Clone::clone),
visibility: types::Visibility::Public,
Expand Down Expand Up @@ -204,8 +201,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
.paths
.clone()
.into_iter()
.chain(self.cache.external_paths.clone().into_iter())
.map(|(k, (path, kind))| {
.map(|(k, CachedPath::Local(path, kind) | CachedPath::Extern(path, kind))| {
(
from_item_id(k.into()),
types::ItemSummary {
Expand Down