rustdoc: Fix generics generation in search index #88268

GuillaumeGomez · 2021-08-23T21:36:24Z

The generics were not added to the search index as they should, instead they were added as arguments. I used this opportunity to allow generics to have generics themselves (will come in very handy for my current rewrite of the search engine!).

r? @jyn514

rust-highfive · 2021-08-23T21:36:26Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

jyn514

I used this opportunity to allow generics to have generics themselves

What does this mean? What code does it allow you to represent? I'm not really clear what any of this code is doing ...

src/librustdoc/formats/item_type.rs

jyn514 · 2021-08-24T11:53:55Z

src/librustdoc/html/render/cache.rs

    RenderType {
        name: get_index_type_name(clean_type, true).map(|s| s.as_str().to_ascii_lowercase()),
-        generics: get_generics(clean_type),
+        generics: if generics.is_empty() { None } else { Some(generics) },


Why store an option for this? The caller can check if the Vec is empty, this means it has to check for both None and empty.

Because if it's not an option, it'll generate [], which is unneeded. Before to not generate anything then.

I'll add a comment to make it clear.

I'm confused though - can't you just do this same check wherever it would generate an empty Vec? Why does it have to be specifically here?

I'm not sure about the other places... In this case, it's contained inside a vec, so if the vec length is 1 or 2, it's not really a problem. For the other places, it might be different.

jyn514 · 2021-08-24T11:55:53Z

src/librustdoc/html/render/cache.rs

-            1
-        } else {
-            0
+            res.push(TypeWithKind::from((index_ty, ItemType::Primitive)));


Why did you change this bit? How do you know it's a primitive? Can you add an assertion?

There is a ty.is_primitive() check just like before. Did I miss something?

Oh, I misread the code. Why did you remove the else branch? What did 0 represent?

Before, it returned the number of items inserted to know if something needed to be added into the HashSet. Now we add all arguments (as long as they have a name).

So basically, the else branch did nothing and returned value confirming it did nothing.

src/librustdoc/html/render/cache.rs

GuillaumeGomez · 2021-08-24T12:13:43Z

I used this opportunity to allow generics to have generics themselves

What does this mean? What code does it allow you to represent? I'm not really clear what any of this code is doing ...

Let's take an example then!

struct Foo<A, B>(A, B);

fn foo<A: Display, B: Foo<u32, u64>, C: Foo<A, B>>(arg: A) {}

Like this, arg will be a generic A which itself contains the generics B and C which themselves have generics. Before that, it was all inlined as arguments of the functions, which is at best incorrect.

jyn514

Is there a reference for how the search index works somewhere? Your examples helped but I have no idea how they'll actually affect search, I don't really feel comfortable signing off on this. Maybe @notriddle or @ollie27 are familiar with search?

jyn514 · 2021-08-24T14:32:14Z

src/librustdoc/html/render/cache.rs

-            1
-        } else {
-            0
+            res.push(TypeWithKind::from((index_ty, ItemType::Primitive)));


Oh, I misread the code. Why did you remove the else branch? What did 0 represent?

src/librustdoc/html/render/cache.rs

jyn514 · 2021-08-24T14:33:57Z

src/librustdoc/html/render/cache.rs

+            // }]
+            // ```
+            //
+            // To be noted that it can work if there is ONLY ONE generic, otherwise we still


Why does it only work if there's a single generic argument? It seems weird to have an outer object that just has name: ""

It's what I tried to explain but I failed I guess haha.

So basically, arguments can only have a type and generics. What happens in case the argument is a full generic which is Display + Debug + Write? Then we can't say it's one type (from a search perspective) but a generic type (with empty name) having three generics.

I just discussed this with Guillaume, and I think I can explain it now.

For purposes of search-indexing, fn foo<T: Display>(x: T) is treated as the type fn(x: Display). This makes a search like trait:Display -> show foo. (It turns out you can filter by item type in rustdoc searches, which is part of what ItemType is for.)

However, fn foo<T: Display + Clone>(x: T)'s argument cannot be reduced into an ItemType::Trait because there are two traits involved, the Display trait and the Clone trait. So instead it's represented as a ItemType::Generic, which means there was more than one bound involved.

Exactly. :)

src/librustdoc/json/conversions.rs

jyn514 · 2021-08-24T14:35:47Z

src/librustdoc/html/render/cache.rs

    RenderType {
        name: get_index_type_name(clean_type, true).map(|s| s.as_str().to_ascii_lowercase()),
-        generics: get_generics(clean_type),
+        generics: if generics.is_empty() { None } else { Some(generics) },


I'm confused though - can't you just do this same check wherever it would generate an empty Vec? Why does it have to be specifically here?

src/test/rustdoc-js/generics.js

GuillaumeGomez · 2021-08-29T18:54:01Z

ping @jyn514

GuillaumeGomez · 2021-08-31T17:14:05Z

I think @jyn514 is busy and I need this PR to be merged to end the rewrite of the rustdoc search engine so I'll set @camelid as reviewer.

r? @camelid

camelid · 2021-09-02T17:10:33Z

I used this opportunity to allow generics to have generics themselves

What does this mean? What code does it allow you to represent? I'm not really clear what any of this code is doing ...

Let's take an example then!
struct Foo<A, B>(A, B);

fn foo<A: Display, B: Foo<u32, u64>, C: Foo<A, B>>(arg: A) {}
Like this, arg will be a generic A which itself contains the generics B and C which themselves have generics. Before that, it was all inlined as arguments of the functions, which is at best incorrect.

I don't really understand this example. Why will A contain B and C? Is it some kind of linked-list/tree structure? And if so, why?

Also, Foo is a struct, so it can't be a trait bound. I assume you meant something like trait Foo<A, B> {}?

How did the search index look previously? Based on your description, it sounds like the type of this function used to be treated as something like fn(Display, Foo<u32, u64>, Foo<A, B>, A)? Is that correct? And what will it look like after this change?

Sorry for all the questions; I'm not very familiar with the search-index code ;)

GuillaumeGomez · 2021-09-02T17:23:35Z

Woups, I meant that C contains A and B. It should have been:

struct Foo<A, B>(A, B);

fn foo<A: Display, B: Foo<u32, u64>, C: Foo<A, B>>(arg: C) {}

And indeed, it looks like a tree:

arg: [
    {
        type: "Foo",
        generics: [
            {
                type: "Display",
                generics: []
            },
            {
                type: "Foo"
                generics: [
                    {
                        type: "u32",
                        generics: []
                    },
                    {
                        type: "u64",
                        generics: []
                    }
                ]
            }
        ]
    }
]

camelid · 2021-09-02T17:34:11Z

src/librustdoc/formats/item_type.rs

@@ -48,6 +48,7 @@ crate enum ItemType {
    ProcAttribute = 23,
    ProcDerive = 24,
    TraitAlias = 25,
+    Generic = 26,


Why are generic parameters treated as items?

The pure generic ones, like fn foo<T: Display + Debug>(a: T), in here, a cannot be both "Display" and "Debug", it needs to go through a level:

arg: [ { name: "", generics: [ { name: "Display" generics: [] }, { name: "Debug" generics: [] } ] } ]

So in here, the argument with name "" is a "pure" generic, hence creating a new ItemType for it.

But value parameters like a: T don't have item types (at least from looking at the ItemType enum definition), so how are they represented?

Just with an empty name. If they are still present, might be interesting to remove them since they're useless for the search index.

jyn514 · 2021-09-06T21:50:45Z

Oh, sorry, I just saw this ping. Do you still need me to review?

GuillaumeGomez · 2021-09-07T12:06:53Z

If you have time, it'd very appreciated, otherwise I'll try to help @camelid finish it.

jyn514 · 2021-09-07T14:21:20Z

It sounds like Camelid understands what's going on better than me, I'll leave them assigned.

bors · 2021-10-25T21:45:40Z

☔ The latest upstream changes (presumably #89430) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-10-27T09:32:23Z

☔ The latest upstream changes (presumably #90337) made this pull request unmergeable. Please resolve the merge conflicts.

GuillaumeGomez · 2021-10-29T10:53:17Z

@notriddle Could you take another look on the rust changes please? :)

notriddle

Other than the naming thing, I really have only one thing to ask. The code changes, in substance, look reasonable to me.

Can you write some test cases for the JavaScript changes?

notriddle · 2021-10-29T15:59:55Z

src/librustdoc/html/render/cache.rs

@@ -301,6 +326,7 @@ crate fn get_real_types<'tcx>(
            }
            _ => false,
        }) {
+            let mut gens = Vec::new();


This isn't a very useful name. Maybe it can instead be something like ty_generics?

notriddle · 2021-10-29T16:17:58Z

In particular, it doesn't look like there's any rustdoc test cases for fn gamma<T: Two + Traits>(t: T). There should probably be one.

GuillaumeGomez · 2021-10-29T18:38:09Z

I added the test you suggested @notriddle (in src/test/rustdoc-js/generics.*) and improved the naming as suggested. :)

GuillaumeGomez · 2021-10-29T20:25:43Z

@bors: r=notriddle,camelid,jyn514

bors · 2021-10-29T20:25:44Z

📌 Commit 355e6ed has been approved by notriddle,camelid,jyn514

bors · 2021-10-30T04:12:44Z

⌛ Testing commit 355e6ed with merge 22f1ad7...

bors · 2021-10-30T07:21:14Z

☀️ Test successful - checks-actions
Approved by: notriddle,camelid,jyn514
Pushing 22f1ad7 to master...

rust-timer · 2021-10-30T08:53:32Z

Finished benchmarking commit (22f1ad7): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

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

GuillaumeGomez requested a review from jyn514 August 23, 2021 21:36

rust-highfive assigned jyn514 Aug 23, 2021

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

jyn514 changed the title ~~Fix generics generation in search index~~ rustdoc: Fix generics generation in search index Aug 24, 2021

jyn514 reviewed Aug 24, 2021

View reviewed changes

GuillaumeGomez force-pushed the generics-search-index branch 3 times, most recently from 843afc5 to 6758aa8 Compare August 24, 2021 13:31

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the generics-search-index branch from 6758aa8 to 518e8aa Compare August 24, 2021 14:29

jyn514 reviewed Aug 24, 2021

View reviewed changes

notriddle reviewed Aug 24, 2021

View reviewed changes

src/test/rustdoc-js/generics.js Show resolved Hide resolved

GuillaumeGomez force-pushed the generics-search-index branch from 518e8aa to c6b1f3e Compare August 24, 2021 15:50

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the generics-search-index branch from c6b1f3e to ee93609 Compare August 24, 2021 16:08

rust-highfive assigned camelid and unassigned jyn514 Aug 31, 2021

camelid reviewed Sep 2, 2021

View reviewed changes

GuillaumeGomez mentioned this pull request Sep 5, 2021

Rustdoc search feature request: support a "field" item kind filter #88660

Open

GuillaumeGomez force-pushed the generics-search-index branch from adae003 to 81b333b Compare October 26, 2021 09:33

GuillaumeGomez force-pushed the generics-search-index branch from 81b333b to a924745 Compare October 27, 2021 09:53

notriddle requested changes Oct 29, 2021

View reviewed changes

camelid assigned notriddle and unassigned camelid Oct 29, 2021

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2021

GuillaumeGomez force-pushed the generics-search-index branch from a924745 to 87f4894 Compare October 29, 2021 18:36

This comment has been minimized.

Sign in to view

Fix invalid handling of generics

355e6ed

GuillaumeGomez force-pushed the generics-search-index branch from 87f4894 to 355e6ed Compare October 29, 2021 18:54

notriddle approved these changes Oct 29, 2021

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 29, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 30, 2021

bors merged commit 22f1ad7 into rust-lang:master Oct 30, 2021

rustbot added this to the 1.58.0 milestone Oct 30, 2021

bors mentioned this pull request Oct 30, 2021

rustdoc: use Type::def_id() instead of Type::def_id_no_primitives() #90385

Merged

GuillaumeGomez deleted the generics-search-index branch October 30, 2021 11:00

Dylan-DPC mentioned this pull request Jun 21, 2022

Rustdoc search should support space-separated queries (in additon to ::-separated) #89384

Closed

rustdoc: Fix generics generation in search index #88268

rustdoc: Fix generics generation in search index #88268

Conversation

GuillaumeGomez commented Aug 23, 2021

rust-highfive commented Aug 23, 2021

jyn514 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

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 Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 24, 2021

This comment has been minimized.

jyn514 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

GuillaumeGomez commented Aug 29, 2021

GuillaumeGomez commented Aug 31, 2021 • edited Loading

camelid commented Sep 2, 2021

GuillaumeGomez commented Sep 2, 2021

Choose a reason for hiding this comment

GuillaumeGomez Sep 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Sep 6, 2021

GuillaumeGomez commented Sep 7, 2021

jyn514 commented Sep 7, 2021

bors commented Oct 25, 2021

bors commented Oct 27, 2021

GuillaumeGomez commented Oct 29, 2021

notriddle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notriddle commented Oct 29, 2021

GuillaumeGomez commented Oct 29, 2021

This comment has been minimized.

GuillaumeGomez commented Oct 29, 2021

bors commented Oct 29, 2021

bors commented Oct 30, 2021

bors commented Oct 30, 2021

rust-timer commented Oct 30, 2021

GuillaumeGomez Aug 24, 2021 •

edited

Loading

GuillaumeGomez commented Aug 31, 2021 •

edited

Loading

GuillaumeGomez Sep 2, 2021 •

edited

Loading