Skip to content

Doc alias improvements #71724

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

Merged
merged 9 commits into from
May 16, 2020
9 changes: 9 additions & 0 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,15 @@ impl Attributes {
})
.collect()
}

pub fn get_doc_aliases(&self) -> FxHashSet<String> {
self.other_attrs
.lists(sym::doc)
.filter(|a| a.check_name(sym::alias))
.filter_map(|a| a.value_str().map(|s| s.to_string().replace("\"", "")))
.filter(|v| !v.is_empty())
.collect::<FxHashSet<_>>()
}
}

impl PartialEq for Attributes {
Expand Down
1 change: 0 additions & 1 deletion src/librustdoc/html/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ pub fn render<T: Print, S: Print>(
window.rootPath = \"{root_path}\";\
window.currentCrate = \"{krate}\";\
</script>\
<script src=\"{root_path}aliases{suffix}.js\"></script>\
<script src=\"{static_root_path}main{suffix}.js\"></script>\
{static_extra_scripts}\
{extra_scripts}\
Expand Down
43 changes: 6 additions & 37 deletions src/librustdoc/html/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,12 @@ impl Serialize for IndexItem {
where
S: Serializer,
{
assert_eq!(self.parent.is_some(), self.parent_idx.is_some());
assert_eq!(
self.parent.is_some(),
self.parent_idx.is_some(),
"`{}` is missing idx",
self.name
);

(self.ty, &self.name, &self.path, &self.desc, self.parent_idx, &self.search_type)
.serialize(serializer)
Expand Down Expand Up @@ -820,42 +825,6 @@ themePicker.onblur = handleThemeButtonsBlur;
Ok((ret, krates))
}

fn show_item(item: &IndexItem, krate: &str) -> String {
format!(
"{{'crate':'{}','ty':{},'name':'{}','desc':'{}','p':'{}'{}}}",
krate,
item.ty as usize,
item.name,
item.desc.replace("'", "\\'"),
item.path,
if let Some(p) = item.parent_idx { format!(",'parent':{}", p) } else { String::new() }
)
}

let dst = cx.dst.join(&format!("aliases{}.js", cx.shared.resource_suffix));
{
let (mut all_aliases, _) = try_err!(collect(&dst, &krate.name, "ALIASES"), &dst);
let mut output = String::with_capacity(100);
for (alias, items) in &cx.cache.aliases {
if items.is_empty() {
continue;
}
output.push_str(&format!(
"\"{}\":[{}],",
alias,
items.iter().map(|v| show_item(v, &krate.name)).collect::<Vec<_>>().join(",")
));
}
all_aliases.push(format!("ALIASES[\"{}\"] = {{{}}};", krate.name, output));
all_aliases.sort();
let mut v = Buffer::html();
writeln!(&mut v, "var ALIASES = {{}};");
for aliases in &all_aliases {
writeln!(&mut v, "{}", aliases);
}
cx.shared.fs.write(&dst, v.into_inner().into_bytes())?;
}

use std::ffi::OsString;

#[derive(Debug)]
Expand Down
64 changes: 24 additions & 40 deletions src/librustdoc/html/render/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ crate struct Cache {

/// Aliases added through `#[doc(alias = "...")]`. Since a few items can have the same alias,
/// we need the alias element to have an array of items.
pub(super) aliases: FxHashMap<String, Vec<IndexItem>>,
pub(super) aliases: BTreeMap<String, Vec<usize>>,
}

impl Cache {
Expand Down Expand Up @@ -311,7 +311,7 @@ impl DocFolder for Cache {
};

match parent {
(parent, Some(path)) if is_inherent_impl_item || (!self.stripped_mod) => {
(parent, Some(path)) if is_inherent_impl_item || !self.stripped_mod => {
debug_assert!(!item.is_stripped());

// A crate has a module at its root, containing all items,
Expand All @@ -327,6 +327,13 @@ impl DocFolder for Cache {
parent_idx: None,
search_type: get_index_search_type(&item),
});

for alias in item.attrs.get_doc_aliases() {
self.aliases
.entry(alias.to_lowercase())
Copy link
Member

Choose a reason for hiding this comment

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

I think that the case insensitive searching should be handled by the JavaScript so that the alias displayed in the results will have the same case as it was specified with.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because otherwise, we might have conflicts to handle on the JS side if we have Hello and hello. Also, I think it's better to lighten JS computations. In any case, we'll have to turn keys into lowercase, so I think it's better to do it directly on the Rust side once and for all.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's not a big issue for now. It does seem weird that aliases are treated differently to the other search results though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The alias match is "exact", this is the base difference. But we can debate it in an issue if you want?

.or_insert(Vec::new())
.push(self.search_index.len() - 1);
}
}
}
(Some(parent), None) if is_inherent_impl_item => {
Expand Down Expand Up @@ -376,11 +383,8 @@ impl DocFolder for Cache {
{
self.paths.insert(item.def_id, (self.stack.clone(), item.type_()));
}
self.add_aliases(&item);
}

clean::PrimitiveItem(..) => {
self.add_aliases(&item);
self.paths.insert(item.def_id, (self.stack.clone(), item.type_()));
}

Expand Down Expand Up @@ -488,40 +492,6 @@ impl DocFolder for Cache {
}
}

impl Cache {
fn add_aliases(&mut self, item: &clean::Item) {
if item.def_id.index == CRATE_DEF_INDEX {
return;
}
if let Some(ref item_name) = item.name {
let path = self
.paths
.get(&item.def_id)
.map(|p| p.0[..p.0.len() - 1].join("::"))
.unwrap_or("std".to_owned());
for alias in item
.attrs
.lists(sym::doc)
.filter(|a| a.check_name(sym::alias))
.filter_map(|a| a.value_str().map(|s| s.to_string().replace("\"", "")))
.filter(|v| !v.is_empty())
.collect::<FxHashSet<_>>()
.into_iter()
{
self.aliases.entry(alias).or_insert(Vec::with_capacity(1)).push(IndexItem {
ty: item.type_(),
name: item_name.to_string(),
path: path.clone(),
desc: shorten(plain_summary_line(item.doc_value())),
parent: None,
parent_idx: None,
search_type: get_index_search_type(&item),
});
}
}
}
}

/// Attempts to find where an external crate is located, given that we're
/// rendering in to the specified source destination.
fn extern_location(
Expand Down Expand Up @@ -567,7 +537,8 @@ fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String {
let mut crate_items = Vec::with_capacity(cache.search_index.len());
let mut crate_paths = vec![];

let Cache { ref mut search_index, ref orphan_impl_items, ref paths, .. } = *cache;
let Cache { ref mut search_index, ref orphan_impl_items, ref paths, ref mut aliases, .. } =
*cache;

// Attach all orphan items to the type's definition if the type
// has since been learned.
Expand All @@ -582,6 +553,12 @@ fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String {
parent_idx: None,
search_type: get_index_search_type(&item),
});
for alias in item.attrs.get_doc_aliases() {
aliases
.entry(alias.to_lowercase())
.or_insert(Vec::new())
.push(search_index.len() - 1);
}
}
}

Expand Down Expand Up @@ -630,6 +607,12 @@ fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String {
items: Vec<&'a IndexItem>,
#[serde(rename = "p")]
paths: Vec<(ItemType, String)>,
// The String is alias name and the vec is the list of the elements with this alias.
//
// To be noted: the `usize` elements are indexes to `items`.
#[serde(rename = "a")]
#[serde(skip_serializing_if = "BTreeMap::is_empty")]
aliases: &'a BTreeMap<String, Vec<usize>>,
}

// Collect the index into a string
Expand All @@ -640,6 +623,7 @@ fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String {
doc: crate_doc,
items: crate_items,
paths: crate_paths,
aliases,
})
.expect("failed serde conversion")
// All these `replace` calls are because we have to go through JS string for JSON content.
Expand Down
Loading