-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[rustdoc] Make aliases search support partial matching #143988
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
base: master
Are you sure you want to change the base?
Conversation
r? @notriddle rustbot has assigned @notriddle. Use |
Some changes occurred in HTML/CSS/JS. |
@@ -116,7 +116,7 @@ pub(crate) fn build_index( | |||
// Set up alias indexes. | |||
for (i, item) in cache.search_index.iter().enumerate() { | |||
for alias in &item.aliases[..] { | |||
aliases.entry(alias.as_str().to_lowercase()).or_default().push(i); | |||
aliases.entry(alias.to_string()).or_default().push(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no longer any transformation taking place, and since aliases
doesn't seem to escape the function (based on the fact CrateData
is a struct defined within this function and it borrows the aliases variable), couldn't aliases have its type changed to BTreeMap<&str, Vec<usize>>
to save a bunch of allocations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is with serde
and borrow checker here. If I use &str
, we can't do:
let mut search_index = std::mem::take(&mut cache.search_index);
just below. Other solution would be to keep Symbol
instead, but Serialize
is not implemented on it, so not possible either. Fixing this would require a lot of code, so better keep it for later optimization.
@@ -2030,6 +2031,8 @@ class DocSearch { | |||
// normalized names, type signature objects and fingerprints, and aliases. | |||
id = 0; | |||
|
|||
/** @type {Array<Array<any>>} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** @type {Array<Array<any>>} */ | |
/** @type {Array<[string, { [key: string]: Array<number> }, number]>} */ |
Something like this to avoid using any
? The { [key: string]: Array<number> }
could alternatively be replaced with just Object
for simplicity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, updating.
const normalizedName = word.indexOf("_") === -1 ? word : word.replace(/_/g, ""); | ||
for (const alias of alias_refs) { | ||
const originalIndex = alias + index; | ||
const original = searchIndex[originalIndex]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const original = searchIndex[originalIndex]; | |
// @ts-expect-error | |
/** @type{rustdoc.Row} */ | |
const original = searchIndex[originalIndex]; |
If we could use nonnull
in search.js we would want to use that here, but we can do this at the very least, which would remove the need for the @ts-ignore
on the fields below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they're not defined in Row
either. ^^'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined more fields in Row
.
// @ts-ignore | ||
is_alias: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add is_alias?: bool
to the definition of rustdoc.Row
, to reflect the fact that this field sometimes exists, and when it does its a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few other fields as well.
7c21fbf
to
c079c96
Compare
Fixes #140782.
To make this work, I moved aliases into the
searchIndex
like any other item. It links to the "original" item with a neworiginal
field. No so great part is that we need to have some fields likebitIndex
to be set on the alias to make the description load to work but I consider it minor enough to be ok.This PR voluntarily doesn't handle de-prioritization of aliases as @lolbinarycat wished to work on this so I'll leave them this part. 😉
cc @lolbinarycat