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

Clean up search code and unify function returned values #91977

Merged
merged 2 commits into from
Dec 18, 2021
Merged

Clean up search code and unify function returned values #91977

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba824ec
Clean up search code and unify function returned values
GuillaumeGomez Dec 15, 2021
080b926
Make loop more clear by renaming variables
GuillaumeGomez Dec 16, 2021
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
204 changes: 96 additions & 108 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 @@ -176,16 +176,13 @@ window.initSearch = function(rawSearchIndex) {
var ar = [];
for (var entry in results) {
if (hasOwnPropertyRustdoc(results, entry)) {
Comment on lines 177 to 178
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have to be part of this PR, but: we are allowed to use ECMASCript 5 features, right? Seems like this should be results.forEach(function(entry) { ... }).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Just like you suggested, I'll do it in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we can't use forEach on dict/Object... T_T

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use Object.entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but I read that the performance were less good than a simple for loop. To be investigated...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there was a post in 2018 saying it was slower: https://medium.com/hackernoon/5-techniques-to-iterate-over-javascript-object-entries-and-their-performance-6602dcb708a8

But it had a followup, also in 2018, saying Object.keys() is now faster: https://gists.cwidanage.com/2018/06/how-to-iterate-over-object-entries-in.html

I'm kinda surprised that there would be any significant difference given how similar the operations are and how optimized JS runtimes are.

ar.push(results[entry]);
var result = results[entry];
result.word = searchWords[result.id];
result.item = searchIndex[result.id] || {};
ar.push(result);
}
}
results = ar;
var i, len, result;
for (i = 0, len = results.length; i < len; ++i) {
result = results[i];
result.word = searchWords[result.id];
result.item = searchIndex[result.id] || {};
}
// if there are no results then return to default and fail
if (results.length === 0) {
return [];
Expand Down Expand Up @@ -258,7 +255,7 @@ window.initSearch = function(rawSearchIndex) {
return 0;
});

for (i = 0, len = results.length; i < len; ++i) {
for (var i = 0, len = results.length; i < len; ++i) {
result = results[i];

// this validation does not make sense when searching by types
Expand Down Expand Up @@ -344,7 +341,17 @@ window.initSearch = function(rawSearchIndex) {
return MAX_LEV_DISTANCE + 1;
}

// Check for type name and type generics (if any).
/**
* This function checks if the object (`obj`) matches the given type (`val`) and its
* generics (if any).
*
* @param {Object} obj
* @param {string} val
* @param {boolean} literalSearch
*
* @return {integer} - Returns a Levenshtein distance to the best match. If there is
* no match, returns `MAX_LEV_DISTANCE + 1`.
*/
function checkType(obj, val, literalSearch) {
var lev_distance = MAX_LEV_DISTANCE + 1;
var tmp_lev = MAX_LEV_DISTANCE + 1;
Expand All @@ -363,24 +370,23 @@ window.initSearch = function(rawSearchIndex) {
elems[obj[GENERICS_DATA][x][NAME]] += 1;
}

var allFound = true;
len = val.generics.length;
for (x = 0; x < len; ++x) {
firstGeneric = val.generics[x];
if (elems[firstGeneric]) {
elems[firstGeneric] -= 1;
} else {
allFound = false;
break;
// Something wasn't found and this is a literal search so
// abort and return a "failing" distance.
return MAX_LEV_DISTANCE + 1;
}
}
if (allFound) {
return true;
}
// Everything was found, success!
return 0;
}
return false;
return MAX_LEV_DISTANCE + 1;
}
return true;
return 0;
} else {
// If the type has generics but don't match, then it won't return at this point.
// Otherwise, `checkGenerics` will return 0 and it'll return.
Expand All @@ -392,14 +398,15 @@ window.initSearch = function(rawSearchIndex) {
}
}
} else if (literalSearch) {
var found = false;
if ((!val.generics || val.generics.length === 0) &&
obj.length > GENERICS_DATA && obj[GENERICS_DATA].length > 0) {
return obj[GENERICS_DATA].some(
found = obj[GENERICS_DATA].some(
function(gen) {
return gen[NAME] === val.name;
});
}
return false;
return found ? 0 : MAX_LEV_DISTANCE + 1;
}
lev_distance = Math.min(levenshtein(obj[NAME], val.name), lev_distance);
if (lev_distance <= MAX_LEV_DISTANCE) {
Expand Down Expand Up @@ -430,6 +437,17 @@ window.initSearch = function(rawSearchIndex) {
return Math.min(lev_distance, tmp_lev) + 1;
}

/**
* This function checks if the object (`obj`) has an argument with the given type (`val`).
*
* @param {Object} obj
* @param {string} val
* @param {boolean} literalSearch
* @param {integer} typeFilter
*
* @return {integer} - Returns a Levenshtein distance to the best match. If there is no
* match, returns `MAX_LEV_DISTANCE + 1`.
*/
function findArg(obj, val, literalSearch, typeFilter) {
var lev_distance = MAX_LEV_DISTANCE + 1;

Expand All @@ -441,19 +459,15 @@ window.initSearch = function(rawSearchIndex) {
continue;
}
tmp = checkType(tmp, val, literalSearch);
if (literalSearch) {
if (tmp) {
return true;
}
if (tmp === 0) {
return 0;
jsha marked this conversation as resolved.
Show resolved Hide resolved
} else if (literalSearch) {
continue;
}
lev_distance = Math.min(tmp, lev_distance);
if (lev_distance === 0) {
return 0;
}
}
}
return literalSearch ? false : lev_distance;
return literalSearch ? MAX_LEV_DISTANCE + 1 : lev_distance;
}

function checkReturned(obj, val, literalSearch, typeFilter) {
Expand All @@ -470,19 +484,15 @@ window.initSearch = function(rawSearchIndex) {
continue;
}
tmp = checkType(tmp, val, literalSearch);
if (literalSearch) {
if (tmp) {
return true;
}
if (tmp === 0) {
return 0;
} else if (literalSearch) {
continue;
}
lev_distance = Math.min(tmp, lev_distance);
if (lev_distance === 0) {
return 0;
}
}
}
return literalSearch ? false : lev_distance;
return literalSearch ? MAX_LEV_DISTANCE + 1 : lev_distance;
}

function checkPath(contains, lastElem, ty) {
Expand Down Expand Up @@ -612,6 +622,44 @@ window.initSearch = function(rawSearchIndex) {
onEach(crateAliases, pushFunc);
}

/**
* This function adds the given result into the provided `res` map if it matches the
* following condition:
*
* * If it is a "literal search" (`isExact`), then `lev` must be 0.
* * If it is not a "literal search", `lev` must be <= `MAX_LEV_DISTANCE`.
*
* The `res` map contains information which will be used to sort the search results:
*
* * `fullId` is a `string`` used as the key of the object we use for the `res` map.
* * `id` is the index in both `searchWords` and `searchIndex` arrays for this element.
* * `index` is an `integer`` used to sort by the position of the word in the item's name.
* * `lev` is the main metric used to sort the search results.
*
* @param {boolean} isExact
* @param {Object} res
* @param {string} fullId
* @param {integer} id
* @param {integer} index
* @param {integer} lev
*/
function addIntoResults(isExact, res, fullId, id, index, lev) {
jsha marked this conversation as resolved.
Show resolved Hide resolved
if (lev === 0 || (!isExact && lev <= MAX_LEV_DISTANCE)) {
if (res[fullId] !== undefined) {
var result = res[fullId];
if (result.dontValidate || result.lev <= lev) {
return;
}
}
res[fullId] = {
id: id,
index: index,
dontValidate: isExact,
lev: lev,
};
}
}

// quoted values mean literal search
var nSearchWords = searchWords.length;
var i, it;
Expand All @@ -634,28 +682,11 @@ window.initSearch = function(rawSearchIndex) {
fullId = ty.id;

if (searchWords[i] === val.name
&& typePassesFilter(typeFilter, searchIndex[i].ty)
&& results[fullId] === undefined) {
results[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
}
if (in_args && results_in_args[fullId] === undefined) {
results_in_args[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
}
if (returned && results_returned[fullId] === undefined) {
results_returned[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
&& typePassesFilter(typeFilter, searchIndex[i].ty)) {
addIntoResults(true, results, fullId, i, -1, 0);
}
addIntoResults(true, results_in_args, fullId, i, -1, in_args);
addIntoResults(true, results_returned, fullId, i, -1, returned);
}
query.inputs = [val];
query.output = val;
Expand Down Expand Up @@ -684,39 +715,23 @@ window.initSearch = function(rawSearchIndex) {
fullId = ty.id;

returned = checkReturned(ty, output, true, NO_TYPE_FILTER);
if (output.name === "*" || returned) {
if (output.name === "*" || returned === 0) {
in_args = false;
var is_module = false;

if (input === "*") {
is_module = true;
} else {
var allFound = true;
for (it = 0, len = inputs.length; allFound && it < len; it++) {
var allFound = 0;
for (it = 0, len = inputs.length; allFound === 0 && it < len; it++) {
allFound = checkType(type, inputs[it], true);
}
in_args = allFound;
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop is pretty confusing to me. I tried to rewrite it with different variable names and a structure that makes its intended meaning clearer (at least I think so!).

Suggested change
var allFound = 0;
for (it = 0, len = inputs.length; allFound === 0 && it < len; it++) {
allFound = checkType(type, inputs[it], true);
}
in_args = allFound;
var firstNonZeroDistance = 0;
for (var it = 0, len = inputs.length; it < len; it++) {
var distance = checkType(type, inputs[it], true);
if (distance > 0) {
firstNonZeroDistance = distance;
break;
}
}
in_args = firstNonZeroDistance;

}
if (in_args) {
results_in_args[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
}
if (returned) {
results_returned[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
}
addIntoResults(true, results_in_args, fullId, i, -1, in_args);
addIntoResults(true, results_returned, fullId, i, -1, returned);
if (is_module) {
results[fullId] = {
id: i,
index: -1,
dontValidate: true,
};
addIntoResults(true, results, fullId, i, -1, 0);
}
}
}
Expand Down Expand Up @@ -788,41 +803,14 @@ window.initSearch = function(rawSearchIndex) {
lev = 0;
}
}
if (in_args <= MAX_LEV_DISTANCE) {
if (results_in_args[fullId] === undefined) {
results_in_args[fullId] = {
id: j,
index: index,
lev: in_args,
};
}
results_in_args[fullId].lev =
Math.min(results_in_args[fullId].lev, in_args);
}
if (returned <= MAX_LEV_DISTANCE) {
if (results_returned[fullId] === undefined) {
results_returned[fullId] = {
id: j,
index: index,
lev: returned,
};
}
results_returned[fullId].lev =
Math.min(results_returned[fullId].lev, returned);
}
addIntoResults(false, results_in_args, fullId, j, index, in_args);
addIntoResults(false, results_returned, fullId, j, index, returned);
if (typePassesFilter(typeFilter, ty.ty) &&
(index !== -1 || lev <= MAX_LEV_DISTANCE)) {
if (index !== -1 && paths.length < 2) {
lev = 0;
}
if (results[fullId] === undefined) {
results[fullId] = {
id: j,
index: index,
lev: lev,
};
}
results[fullId].lev = Math.min(results[fullId].lev, lev);
addIntoResults(false, results, fullId, j, index, lev);
}
}
}
Expand Down