Skip to content

rustdoc search: rank aliases lower than exact matches #142653

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −11
Open

rustdoc search: rank aliases lower than exact matches #142653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/librustdoc/html/static/js/rustdoc.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ declare namespace rustdoc {

/**
* A single parsed "atom" in a search query. For example,
*
*
* std::fmt::Formatter, Write -> Result<()>
* ┏━━━━━━━━━━━━━━━━━━ ┌──── ┏━━━━━┅┅┅┅┄┄┄┄┄┄┄┄┄┄┄┄┄┄┐
* ┃ │ ┗ QueryElement { ┊
Expand Down Expand Up @@ -264,6 +264,7 @@ declare namespace rustdoc {
displayTypeSignature: Promise<rustdoc.DisplayTypeSignature> | null,
item: Row,
dontValidate?: boolean,
alias?: string,
}

/**
Expand Down
19 changes: 16 additions & 3 deletions src/librustdoc/html/static/js/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4273,7 +4273,11 @@ class DocSearch {
};
}

// @ts-expect-error
/**
* Handle searching by doc aliases
*
* @type {function(rustdoc.ResultsTable, string, Object=, Object=): Promise<void>}
*/
const handleAliases = async(ret, query, filterCrates, currentCrate) => {
const lowerQuery = query.toLowerCase();
// We separate aliases and crate aliases because we want to have current crate
Expand Down Expand Up @@ -4328,6 +4332,16 @@ class DocSearch {
Promise.all(aliases.map(fetchDesc)),
]);

// if there are any existing results that match exactly, those go before aliases.
let exactMatches = 0;
while (
ret.others[exactMatches] !== undefined &&
ret.others[exactMatches].dist === 0 &&
ret.others[exactMatches].path_dist === 0
) {
exactMatches += 1;
}
Comment on lines +4336 to +4343
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's the right approach: considering we plan to treat aliases the same way as all other items in the search, why not instead increase the computed distance a little (like by 0.1) to ensure it will always go after items of the same distance?

With this current code (iiuc), for each matching alias, we need to go through all results, which doesn't seem great performance wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we only have to go through all the exact matches, which there typically shouldn't only be a handful of, then the loop stops.

i initially was going to try to add alias handling before the sorting, but it turns out the sorting function is also what cleans the results, and i wasn't confident enough in our typechecking yet to translate the alias results into the correct form. I was also worried about aliases missing a bunch of fields that are used in sorting.

I think it makes sense to fix this annoying issue sooner even if it has to be redone in the future, but if you prefer I refactor aliases immediately, I can do that.

Also, there's no need to meddle with the distance, we can just sort on the presence of the alias or is_alias field.

Copy link
Member

Choose a reason for hiding this comment

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

Changing the distance is simple and doesn't require any other change in the sorting, hence why I think it's the way to go.

And I'd prefer for things to be done all at once directly. The changes should be rather small so let's do it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DocSearch.ALIASES does seem to store aliases in the standard rustdoc.Row format, so that's not an issue, but solving this properly is semi-blocked on #142270, as that PR gives addIntoResults an actually correct type signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose since that PR has merge conflict I could just close it and integrate its changes into this PR.


// @ts-expect-error
const pushFunc = alias => {
alias.alias = query;
Expand All @@ -4336,12 +4350,11 @@ class DocSearch {
alias.fullPath = alias.displayPath + alias.name;
alias.href = res[1];

ret.others.unshift(alias);
ret.others.splice(exactMatches, 0, alias);
if (ret.others.length > MAX_RESULTS) {
ret.others.pop();
}
};

aliases.forEach((alias, i) => {
// @ts-expect-error
alias.desc = descs[i];
Expand Down
10 changes: 10 additions & 0 deletions tests/rustdoc-js/alias-sort.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// rank doc aliases lower than exact matches
// regression test for <https://github.com/rust-lang/rust/issues/140968>

const EXPECTED = {
'query': 'foobazbar',
'others': [
{ 'name': 'foobazbar' },
{ 'name': 'foo' },
],
};
5 changes: 5 additions & 0 deletions tests/rustdoc-js/alias-sort.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/// asdf
pub fn foobazbar() {}

#[doc(alias("foobazbar"))]
pub fn foo() {}
14 changes: 7 additions & 7 deletions tests/rustdoc-js/doc-alias.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,13 @@ const EXPECTED = [
{
'query': 'UnionItem',
'others': [
// normalization actually removes the underscores, so this counts as an exact match.
{
'path': 'doc_alias::Union',
'name': 'union_item',
'desc': 'Doc for <code>Union::union_item</code>',
'href': '../doc_alias/union.Union.html#structfield.union_item'
},
{
'path': 'doc_alias',
'name': 'Union',
Expand All @@ -239,13 +246,6 @@ const EXPECTED = [
'href': '../doc_alias/union.Union.html',
'is_alias': true
},
// Not an alias!
{
'path': 'doc_alias::Union',
'name': 'union_item',
'desc': 'Doc for <code>Union::union_item</code>',
'href': '../doc_alias/union.Union.html#structfield.union_item'
},
],
},
{
Expand Down
2 changes: 2 additions & 0 deletions tests/rustdoc-js/non-english-identifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// ignore-order

const PARSED = [
{
query: '中文',
Expand Down
Loading