Skip to content

Conversation

GuillaumeGomez
Copy link
Member

I uncovered this case when working on #90726 to debug #90385.

Explanations: if we have a full generic, we check if it has generics then we do the following:

  • If it has only one generic, we remove one nested level in order to not keep the "parent" generic (since it has empty name, it's useless after all).
  • Otherwise we add it alongside its generics.

However, I didn't handle the case where a generic had no generics. Meaning that we were adding items with empty names in the search index. So basically useless data in the search index.

r? @camelid

@GuillaumeGomez 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 labels Nov 9, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2021
@camelid camelid removed their assignment Nov 9, 2021
@camelid
Copy link
Member

camelid commented Nov 9, 2021

(I can't seem to assign @notriddle for some reason, even though they show up in the assignment selection box...)

@notriddle
Copy link
Contributor

notriddle commented Nov 9, 2021

The terminology here seems a bit confusing, though. Especially the term "full generic" seems to only appear in rustdoc:

notriddle:rust$ rg full_generic
src/librustdoc/clean/types.rs
1520:    crate fn is_full_generic(&self) -> bool {

src/librustdoc/html/render/cache.rs
259:        let is_full_generic = ty.is_full_generic();
261:        if is_full_generic && generics.len() == 1 {
305:        if is_full_generic {

It's getting increasingly hard to keep track of exactly when, if this code says the word "generic", is it referring to:

       type parameter         type parameter
       |                      |
       v                      v
fn foo<T: Display + Debug>(t: T) { ... }
       ---^^^^^^^^^^^^^^^
       |                |
       |                trait bounds (there is also such as thing as lifetime bounds, but search index excludes them)
       generic arguments (which can include type parameters and lifetime parameters)


                           type parameters
                           |
                      vvvvvv
fn bar<T>() -> Result<T, i32> { ... }

Just trying to translate this thing into terminology that is a bit more specific:

  • rustdoc creates a search index item called Type::Generic. This is actually a representation of an opaque type.
  • rustdoc types may have a "generics" field, which is overloaded. For a type parameter (Type::Generic), its "generics" are a list of traits. For a concrete type, such as Result above, its "generics" are actually a list of the type parameters that are supplied to it.
  • A type parameter with a single trait bound is compressed, so that it is represented as if the trait itself had been used. For example, fn baz<T: Foo>(t: T) and fn baz(t: Foo) are represented the same way.
  • This PR changes it so that a type parameter with no trait bounds at all (which you described above as a full generic with no generics) is simply excluded. For example, fn baz<T>(t: T) is represented the same way as fn baz().

@camelid
Copy link
Member

camelid commented Nov 9, 2021

rustdoc creates a search index item called Type::Generic. This is actually a representation of an opaque type.

By "opaque type", do you mean "type parameter"? An opaque type is impl Trait.

@camelid
Copy link
Member

camelid commented Nov 9, 2021

But I agree rustdoc's terminology is very confusing; I think we should really just use rustc's terminology. So "type parameter", not "full generic", and "(trait) bounds", not "generics on generics".

I think the "full generic" terminology arose because clean::Type::Generic(...) is not always a type parameter; sometimes it's a Self type. I have a WIP branch to fix that.

@notriddle
Copy link
Contributor

By "opaque type", do you mean "type parameter"? An opaque type is impl Trait.

You're right, yeah. It's called a TyKind::Param. I got them confused. I admit, this stuff is confusing!

@GuillaumeGomez
Copy link
Member Author

The terminology here seems a bit confusing, though. Especially the term "full generic" seems to only appear in rustdoc:

It's very likely that I messed it up...

* rustdoc types may have a "generics" field, which is overloaded. For a type parameter (`Type::Generic`), its "generics" are a list of traits. For a concrete type, such as `Result` above, its "generics" are actually a list of the type parameters that are supplied to it.

It's not always traits. For example: fn foo<P: AsRef<Path>>(p: P). Path in there is a type after all, so AsRef has a generic (and P is a "full generic"). But any idea to improve the naming is very welcome.

* A type parameter with a single trait bound is compressed, so that it is represented as if the trait itself had been used. For example, `fn baz<T: Foo>(t: T)` and `fn baz(t: Foo)` are represented the same way.

Exactly, it's the goal.

* This PR changes it so that a type parameter with no trait bounds at all (which you described above as **a full generic with no generics**) is simply excluded. For example, `fn baz<T>(t: T)` is represented the same way as `fn baz()`.

Again, it's the goal. :)

I'll update to try to clarify all this.

@GuillaumeGomez GuillaumeGomez force-pushed the remove-potential-useless-search-index-data branch from 6b8674a to 8d5ef32 Compare November 10, 2021 10:14
@GuillaumeGomez
Copy link
Member Author

I updated the comments. Please tell me if it still needs improvements. :)

@notriddle
Copy link
Contributor

Yeah, that is an improvement.

@GuillaumeGomez
Copy link
Member Author

But it feels like it could be better from your comment haha. What would you improve?

@notriddle
Copy link
Contributor

notriddle commented Nov 10, 2021

The code in this pull request itself is fine, but some of the code surrounding it (particularly the code handling Type::Generic below it in get_real_types) could use more comments.

But that should probably be done in separate PRs. 😄

@GuillaumeGomez
Copy link
Member Author

@notriddle I opened #90766 for adding more comments. Thanks to both of you for the suggestions!

@bors: r=notriddle,camelid rollup

@bors
Copy link
Collaborator

bors commented Nov 10, 2021

📌 Commit 8d5ef32 has been approved by notriddle,camelid

@bors 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 Nov 10, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#88447 (Use computed visibility in rustdoc)
 - rust-lang#88868 (Allow simd_bitmask to return byte arrays)
 - rust-lang#90727 (Remove potential useless data for search index)
 - rust-lang#90742 (Use AddAssign impl)
 - rust-lang#90758 (Fix collections entry API documentation.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b31f019 into rust-lang:master Nov 10, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 10, 2021
@GuillaumeGomez GuillaumeGomez deleted the remove-potential-useless-search-index-data branch November 10, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-search Area: Rustdoc's search feature S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants