Treat other items as functions for the purpose of type-based search

[lolbinarycat]

specifically, constants and statics are nullary functions, and struct fields are unary functions.

fixes #130204

r? @notriddle

[notriddle]

The test failure in the gui test doesn't seem like a bug. It really is finding an item that it didn't use to find. You can just bump the number.

[rustbot]

Some changes occurred in GUI tests.

cc @GuillaumeGomez

[notriddle]

Looks nice

@rfcbot poll is this a good idea?

[rfcbot]

Team member @notriddle has asked teams: T-rustdoc-frontend, for consensus on:

is this a good idea?

• @GuillaumeGomez
• @camelid
• @jsha
• @notriddle

[lolbinarycat]

i just realized something: since this is implemented in rust code, unlike my other PRs, we don't have to make this insta-stable, it would actually be pretty easy to gate this behind a nightly feature (similarly to what is done with rustdoc-json)

i would honestly prefer running this as a t-rustdoc experiment first, since it still has a few unresolved questions (eg. how it will interact with generics)

personally, i would like to ship a MVP as a nightly-only feature, allowing me to start getting user feedback while i continue to iterate on it.

[notriddle]

We could, technically, put this behind a -Z flag. But it wouldn't make much sense for docs.rs, since it's the reader who would want to choose, but it's the crate author who has the ability to set build parameters.

Making it a javascript Settings option would get it in front of more people.

[lolbinarycat]

hmm.. i guess that would involve explicitly filtering only functions when doing type-based search unless that setting was enabled?

[notriddle]

Exactly, yes.

[GuillaumeGomez]

That seems extremely strange to have constants and statics when we filter on functions.

[lolbinarycat]

It's how hoogle works. treating constants as pure nullary functions is not that unusual, especially if we're treating field accessors as unary functions.

when someone searches -> SomeType, what they're really asking is "how do i get a value of this type", to which a new() constructor is equally as valid of an answer as a constant. it's not like they're actually writing a rust function signature, they don't have "fn" anywhere in their search.

the alternative is having different syntaxes for every way to get a value... which is more work both in implementation complexity, and also for users, who now have to perform 3 different queries to see every way they can construct a certain type.

[GuillaumeGomez]

I see, thanks for the explanation. With this point of view, it indeed does make sense. The only requirement for this change for me will then to have documentation explaining what you just wrote. Sounds like a nice improvement.

[lolbinarycat]

It seems like we're all in agreement that this should be done, but the next question is how?

insta-stable, settings panel, or nightly feature?

how i write the documentation will depend on how it is implemented.

[GuillaumeGomez]

As far as I know, any change in search is insta-stable.

As for how to implement it, what are the different ways you see?

[lolbinarycat]

As far as I know, any change in search is insta-stable.

well, since this is actually implemented in rust, and not js, we do actually have easy access to feature gates, although it might be a bit confusing.

As for how to implement it, what are the different ways you see?

I just mean should it be gated behind anything, because if it is, i'll add an extra section for it, but if not, i'll replace/modify the existing type-based search section.

[notriddle]

Changes to search are usually insta-stable. It's not an API, so if something turns out to be a bad idea, we can undo it.

[lolbinarycat]

somehow #132123 didn't cause a merge conflict??

[camelid]

I can see this making sense for consts and statics, but it feels like a stretch to consider struct fields as unary functions -- it also doesn't seem useful.

[lolbinarycat]

I can see this making sense for consts and statics, but it feels like a stretch to consider struct fields as unary functions -- it also doesn't seem useful.

the point is to improve devx in the case where you are looking for a getter method, but there isn't one, since you are expected to just access the field directly.

this is especially common with things like rustdoc::Context, where direct field access and getter methods are used in equal measure.

as i explained to GG:

1. this improves parity with hoogle, which treats struct fields as unary functions. people who are already familiar with hoogle are those most likely to use the advanced features of rustdoc search, so we should try to avoid friction between the behavior of the two tools.
2. when people search Foo -> Bar, what they're really asking is "how do I get Bar out of Foo?". usually this will be a getter method, but sometimes it will be a field. (sometimes it will require a chain of multiple operations, but at least in that case, -> Bar will now show any fields that might be a relevant pathway)

[GuillaumeGomez]

I agreed for consts and statics, not for fields. ^^' I agree with @camelid: it's too much of a fetch here.

I'm not sure trying to reach parity with hoogle should be a goal for rustdoc either.

[notriddle]

I agreed for consts and statics, not for fields.

But why consts and statics, then? What makes them different from fields?

[camelid]

Probably the biggest concern I have with fields is it feels like noise. For most structs, you already know what the fields are just from the sidebar of the struct's page. When I search for e.g. rustdoc::Context -> I want to see what functions do stuff with it, not all its fields -- since those are easy to find.

I don't feel super strongly about this, but comparing it to Hoogle is a little bit of a false comparison because in Haskell fields are actually functions (and same with constants). In Rust, it's more of an analogy -- we don't have .field(my_struct) syntax, for example.

[lolbinarycat]

When I search for e.g. rustdoc::Context -> I want to see what functions do stuff with it, not all its fields -- since those are easy to find.

Hm, I hadn't considered this form of search. I would say we can de-prioritize fields when no return type is given, but I'd like to wait and do that in a follow-up PR.

[notriddle]

It would certainly be possible to exclude fields from the single-element type-based queries:

diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js
index eed64d024c0..f41975bf759 100644
--- a/src/librustdoc/html/static/js/search.js
+++ b/src/librustdoc/html/static/js/search.js
@@ -52,6 +52,7 @@ const itemTypes = [
 // used for special search precedence
 const TY_GENERIC = itemTypes.indexOf("generic");
 const TY_IMPORT = itemTypes.indexOf("import");
+const TY_STRUCTFIELD = itemTypes.indexOf("structfield");
 const ROOT_PATH = typeof window !== "undefined" ? window.rootPath : "../";
 
 // Hard limit on how deep to recurse into generics when doing type-driven search.
@@ -2984,7 +2985,7 @@ class DocSearch {
                 fullId,
                 parsedQuery.typeFingerprint,
             );
-            if (tfpDist !== null) {
+            if (tfpDist !== null && row.ty !== TY_STRUCTFIELD) {
                 const in_args = row.type && row.type.inputs
                     && checkIfInList(row.type.inputs, elem, row.type.where_clause, null, 0);
                 const returned = row.type && row.type.output
@@ -3285,6 +3286,9 @@ class DocSearch {
                 parsedQuery.elems.sort(sortQ);
                 parsedQuery.returned.sort(sortQ);
                 for (let i = 0, nSearchIndex = this.searchIndex.length; i < nSearchIndex; ++i) {
+                    if (this.searchIndex[i].ty === TY_STRUCTFIELD && parsedQuery.foundElems === 1) {
+                        continue;
+                    }
                     handleArgs(this.searchIndex[i], i, results_others);
                 }
             }

The other option is to make the ranking algorithm sort them lower.

diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js
index eed64d024c0..349db638986 100644
--- a/src/librustdoc/html/static/js/search.js
+++ b/src/librustdoc/html/static/js/search.js
@@ -52,6 +52,7 @@ const itemTypes = [
 // used for special search precedence
 const TY_GENERIC = itemTypes.indexOf("generic");
 const TY_IMPORT = itemTypes.indexOf("import");
+const TY_STRUCTFIELD = itemTypes.indexOf("structfield");
 const ROOT_PATH = typeof window !== "undefined" ? window.rootPath : "../";
 
 // Hard limit on how deep to recurse into generics when doing type-driven search.
@@ -2144,6 +2144,13 @@ class DocSearch {
                     return a - b;
                 }
 
+                // sort fields lower in type-based search
+                a = aaa.item.ty === TY_STRUCTFIELD;
+                b = bbb.item.ty === TY_STRUCTFIELD;
+                if (a !== b && isType) {
+                    return a - b;
+                }
+
                 // Sort by distance in the name part, the last part of the path
                 // (less changes required to match means higher rankings)
                 a = (aaa.dist);

Probably the biggest concern I have with fields is it feels like noise. For most structs, you already know what the fields are just from the sidebar of the struct's page. When I search for e.g. rustdoc::Context -> I want to see what functions do stuff with it, not all its fields -- since those are easy to find.

All else being equal, I prefer ranking.

Also, by that reasoning, shouldn't we also exclude methods?

[GuillaumeGomez]

@bors r=notriddle rollup

[bors]

📌 Commit fe6deb0 has been approved by notriddle

It is now in the queue for this repository.

[GuillaumeGomez]

I think it's the one which failed in #135533.

@bors r-

[lolbinarycat]

Is this a bug in the test suite? The failure is from the test introduced in #135302, and seems to want AllocError::clone to appear before itself.

[lolbinarycat]

i'm pretty sure this is a bug in tester.js, specifically this line:

                error_text.push(queryName + "==> '" + JSON.stringify(elem) + "' was supposed " +
                                "to be before '" + JSON.stringify(results[key][entry_pos]) + "'");

the second part should be using prev_pos instead of entry_pos. currently it prints:

1. the current entry, as it appears in EXPECTED.
2. the current entry, as it appears in the search results.

this conflicts with what the error message suggests is printed.

[notriddle]

@bors r+

[bors]

📌 Commit 9397d13 has been approved by notriddle

It is now in the queue for this repository.