Clean up search code and unify function returned values #91977

GuillaumeGomez · 2021-12-15T19:55:31Z

This PR is a cleanup: there is no changes in the search results or in the UI.

Depending if it was "literal search" or not, it was either returning booleans or integers. It's pretty bad so instead it all returns integers.

Another thing I did was to move the add and checks into a addIntoResults function to simplify things.

Last thing: I removed a loop in the sortResults function and moved its code directly into the first loop.

All these changes are done to make #90630 much smaller.

r? @jsha

rust-highfive · 2021-12-15T19:55:34Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

jsha · 2021-12-16T02:35:24Z

src/librustdoc/html/static/js/search.js

            for (var entry in results) {
                if (hasOwnPropertyRustdoc(results, entry)) {


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) { ... }).

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

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

We can use Object.entries.

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

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.

jsha · 2021-12-16T02:38:18Z

src/librustdoc/html/static/js/search.js

+                                    // Something wasn't found and this is a literal search so
+                                    // abort and return 1.
+                                    return 1;


Can you update the comment for this function (checkType) so it explains the type of its parameters and return value? And what the return value means? That is, what does it mean when this function returns 1 vs 0 vs something else?

Reading the code, it looks like this still treats 0 or 1 as special "truthy" or "falsy" values, but the function can also return a levenshtein distance, which could be 0, 1, or some other number. It seems like you've fixed the type problem (returning different types based on code path), but not the underlying conceptual problem.

Actually I should return MAX_LEV_DISTANCE + 1 just like everywhere else.

src/librustdoc/html/static/js/search.js

GuillaumeGomez · 2021-12-16T10:46:04Z

Updated!

jsha

It looks like you forgot to add a function comment on checkType?

jsha · 2021-12-16T18:28:16Z

src/librustdoc/html/static/js/search.js

@@ -173,19 +173,17 @@ window.initSearch = function(rawSearchIndex) {
        }

        function sortResults(results, isType) {
+            var result;


Move this down to line 180 and combine it with the assignment:

var result = results[entry];

jsha · 2021-12-16T18:30:58Z

src/librustdoc/html/static/js/search.js

+        // This function looks if the object (`obj`) has an argument with the given type (`val`).
+        //
+        // Returns the lev distance of the best match it found (if there are inputs), otherwise
+        // returns MAX_LEV_DISTANCE + 1`.


Suggested change

// This function looks if the object (`obj`) has an argument with the given type (`val`).

//

// Returns the lev distance of the best match it found (if there are inputs), otherwise

// returns MAX_LEV_DISTANCE + 1`.

// This function checks if the object (`obj`) has an argument with the given type (`val`).

//

// Returns a Levenshtein distance to the best match. If there is no match, returns MAX_LEV_DISTANCE + 1.

jsha · 2021-12-16T18:31:53Z

src/librustdoc/html/static/js/search.js

+        //
+        // `id` is the index in both `searchWords` and `searchIndex` arrays for this element.
+        //
+        // `index` is used as a value when sorting results.


"is used as a value when sorting results" doesn't tell me much. :-) Can you explain more? What type is it? How is it used in sorting?

jsha · 2021-12-16T18:35:02Z

src/librustdoc/html/static/js/search.js

+        // * 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`.
+        //
+        // `fullId` is the key used of the object we use for the `res` map.


What's the type of fullId? What are the expected contents of the res map?

GuillaumeGomez · 2021-12-16T19:08:44Z

Updated! :)

jsha · 2021-12-16T19:19:38Z

src/librustdoc/html/static/js/search.js

            for (var entry in results) {
                if (hasOwnPropertyRustdoc(results, entry)) {


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.

jsha · 2021-12-16T19:23:23Z

src/librustdoc/html/static/js/search.js

+        // This function checks if the object (`obj`) matches the given type (`val`) and its
+        // generics (if any).
+        //
+        // Returns a Levenshtein distance to the best match. If there is no match, returns
+        // `MAX_LEV_DISTANCE + 1`.


Can you define the types of obj, val, and literalSearch? In other places in this file, such parameters are documented using jsdoc syntax, like so:

/** * Validate performs the following boolean logic. For example: * "File::open" will give IF A PARENT EXISTS => ("file" && "open") * exists in (name || path || parent) OR => ("file" && "open") exists in * (name || path ) * * This could be written functionally, but I wanted to minimise * functions on stack. * * @param {[string]} name [The name of the result] * @param {[string]} path [The path of the result] * @param {[string]} keys [The keys to be used (["file", "open"])] * @param {[object]} parent [The parent of the result] * @return {boolean} [Whether the result is valid or not] */

We don't use jsdoc AFAIK, but it's a good syntax for formalizing information about parameters and return types. Also worth noting: I'm not sure why the types are wrapped in brackets. But I think it's okay to follow local style, and we can do a pass later to bring all jsdoc comments in line with the spec.

I'm definitely not a big fan of the syntax but why not. :)

Fair enough, but I think a syntax that exists and has tooling built for it is much better than no syntax. :-)

No sorry, not possible. It's just impossible to read. I'll add the types but the comment is just not human readable...

Do you feel that way even for the properly formatted jsdoc style (from https://jsdoc.app/about-getting-started.html#adding-documentation-comments-to-your-code)?

/** * Represents a book. * @constructor * @param {string} title - The title of the book. * @param {string} author - The author of the book. */ function Book(title, author) { }

Oh, that's better! That I can do.

GuillaumeGomez · 2021-12-16T19:50:45Z

Updated with jsdoc comments.

jsha · 2021-12-16T21:58:45Z

src/librustdoc/html/static/js/search.js

+                        var allFound = 0;
+                        for (it = 0, len = inputs.length; allFound === 0 && it < len; it++) {
                            allFound = checkType(type, inputs[it], true);
                        }
                        in_args = allFound;


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;

GuillaumeGomez · 2021-12-16T22:17:22Z

@jsha Good idea, done as well.

jsha · 2021-12-17T02:31:41Z

@bors r+ rollup

bors · 2021-12-17T02:31:43Z

📌 Commit 080b926 has been approved by jsha

camelid · 2021-12-17T23:19:46Z

Doesn't have to be part of this PR, but: we are allowed to use ECMASCript 5 features, right?

I think at this point we can even use ES6, but I'm not certain.

GuillaumeGomez · 2021-12-18T14:41:16Z

I think at this point we can even use ES6, but I'm not certain.

We mentioned it yes. Let's wait for the year to be over before making the change to ensure that we're not missing something.

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#91858 (pass -Wl,-z,origin to set DF_ORIGIN when using rpath) - rust-lang#91923 (Remove `in_band_lifetimes` from `rustc_query_impl`) - rust-lang#91925 (Remove `in_band_lifetimes` from `rustc_privacy`) - rust-lang#91977 (Clean up search code and unify function returned values) - rust-lang#92018 (Fix typo in "new region bound" suggestion) - rust-lang#92022 (Eliminate duplicate codes of expected_found_bool) - rust-lang#92032 (hir: Do not introduce dummy type names for `extern` blocks in def paths) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-search Area: Rustdoc's search feature A-rustdoc-js Area: Rustdoc's JS front-end labels Dec 15, 2021

rust-highfive assigned jsha Dec 15, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 15, 2021

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the unify-search-code branch from 067430c to b03618c Compare December 15, 2021 20:32

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the unify-search-code branch from b03618c to 474b7c7 Compare December 15, 2021 21:21

jsha requested changes Dec 16, 2021

View reviewed changes

GuillaumeGomez mentioned this pull request Dec 16, 2021

search.js: split out nested functions #91992

Closed

GuillaumeGomez force-pushed the unify-search-code branch from 474b7c7 to f183511 Compare December 16, 2021 10:45

GuillaumeGomez requested a review from jsha December 16, 2021 10:45

jsha requested changes Dec 16, 2021

View reviewed changes

GuillaumeGomez force-pushed the unify-search-code branch 2 times, most recently from 303b924 to bdd6886 Compare December 16, 2021 19:08

jsha requested changes Dec 16, 2021

View reviewed changes

Clean up search code and unify function returned values

ba824ec

GuillaumeGomez force-pushed the unify-search-code branch from bdd6886 to ba824ec Compare December 16, 2021 19:49

jsha requested changes Dec 16, 2021

View reviewed changes

Make loop more clear by renaming variables

080b926

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2021

matthiaskrgr mentioned this pull request Dec 18, 2021

Rollup of 7 pull requests #92064

Merged

bors merged commit 8bb238b into rust-lang:master Dec 18, 2021

rustbot added this to the 1.59.0 milestone Dec 18, 2021

GuillaumeGomez deleted the unify-search-code branch December 18, 2021 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up search code and unify function returned values #91977

Clean up search code and unify function returned values #91977

GuillaumeGomez commented Dec 15, 2021 •

edited

Loading

rust-highfive commented Dec 15, 2021

This comment has been minimized.

This comment has been minimized.

jsha Dec 16, 2021

GuillaumeGomez Dec 16, 2021

GuillaumeGomez Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez Dec 16, 2021

jsha Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez Dec 16, 2021

GuillaumeGomez commented Dec 16, 2021

jsha left a comment

jsha Dec 16, 2021

jsha Dec 16, 2021

jsha Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez commented Dec 16, 2021

jsha Dec 16, 2021

jsha Dec 16, 2021 •

edited

Loading

GuillaumeGomez Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez Dec 16, 2021

GuillaumeGomez commented Dec 16, 2021

jsha Dec 16, 2021

GuillaumeGomez commented Dec 16, 2021

jsha commented Dec 17, 2021

bors commented Dec 17, 2021

camelid commented Dec 17, 2021

GuillaumeGomez commented Dec 18, 2021

		for (var entry in results) {
		if (hasOwnPropertyRustdoc(results, entry)) {

-                        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;

Clean up search code and unify function returned values #91977

Clean up search code and unify function returned values #91977

Conversation

GuillaumeGomez commented Dec 15, 2021 • edited Loading

rust-highfive commented Dec 15, 2021

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 16, 2021

jsha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 16, 2021

Choose a reason for hiding this comment

jsha Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 16, 2021

Choose a reason for hiding this comment

GuillaumeGomez commented Dec 16, 2021

jsha commented Dec 17, 2021

bors commented Dec 17, 2021

camelid commented Dec 17, 2021

GuillaumeGomez commented Dec 18, 2021

GuillaumeGomez commented Dec 15, 2021 •

edited

Loading

jsha Dec 16, 2021 •

edited

Loading