rustdoc: Fix handling of `Self` type in search index and refactor its representation #128471

camelid · 2024-08-01T00:40:03Z

Summary

Add enum variant clean::Type::SelfTy and use it instead of clean::Type::Generic(kw::SelfUpper).
Stop treating Self as a generic in the search index.
Remove struct formerly known as clean::SelfTy (constructed as representation of function receiver type). We're better off without it.

Before

After

camelid · 2024-08-01T00:41:11Z

src/librustdoc/json/conversions.rs

+            // FIXME: add dedicated variant to json Type?
+            SelfTy => Type::Generic("Self".to_owned()),


cc @aDotInTheVoid -- what would you like to do here?

This is fine for now as it preserves the existing behavior, but yeah this should almost certainly be changed in a future PR

after some discussion with @compiler-errors, i’m not sure the current json behaviour is wrong. but either way, this is fine for this PR

Yes, technically speaking Self is either an alias (in the case of concrete impls) or a type parameter (in the case of traits). So what we're doing here in Rustdoc is more of a convenience for how users perceive Self, rather than how the compiler does.

camelid · 2024-08-01T00:42:19Z

I highly recommend reviewing commit-by-commit. The diff for the whole PR looks larger than it really is since in one commit I replaced a long sequence of if lets with a match.

camelid · 2024-08-01T00:43:29Z

src/librustdoc/clean/inline.rs

                    trait_: Some(trait_),
                    ..
                }),
            bounds,
            ..
-        } => !(bounds.is_empty() || *s == kw::SelfUpper && trait_.def_id() == trait_did),
+        } => !bounds.is_empty() && trait_.def_id() != trait_did,


Just an application of De Morgan rules combined with knowing that clean::Generic(kw::SelfUpper) is gone now.

notriddle · 2024-08-01T01:09:04Z

Please add a test case in tests/rustdoc-js?

camelid · 2024-08-01T03:14:34Z

Done, let me know if it looks correct.

tests/rustdoc-js/self-is-not-generic.js

camelid · 2024-08-01T22:01:37Z

@rfcbot poll rustdoc-frontend

rfcbot · 2024-08-01T22:01:42Z

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

compiler-errors · 2024-08-01T23:42:56Z

src/librustdoc/clean/utils.rs

@@ -468,7 +468,7 @@ pub(crate) fn resolve_type(cx: &mut DocContext<'_>, path: Path) -> Type {
    match path.res {
        Res::PrimTy(p) => Primitive(PrimitiveType::from(p)),
        Res::SelfTyParam { .. } | Res::SelfTyAlias { .. } if path.segments.len() == 1 => {


I don't believe that the handling of SelfTyAlias is correct here 🤔 Or at least I do want to point out that SelfTyParam and SelfTyAlias act differently and should probably not continue to be conflated.

SelfTyAlias is used for the self type of an impl; this is not some abstract Self (which is literally just parameter -- unrelated, but I'm not sure why this PR moves away from representing self as a parameter when it is one in trait, but I digress) -- SelfTyAlias instead refers to a specific, sometimes concrete type. For example, in:

impl MyCustomType<i32> { /* Self is a SelfTypeAlias here */ }

Yes, thanks for raising this. I believe that some other part of rustdoc is "unwrapping" the Self alias; see for example Vec::new()'s return type. It may make sense to centralize that behavior in one place—namely here—though.

Regarding your comment about the change in representation: I understand that Self in trait is logically a parameter (à la Haskell type classes) in the compiler's view. However, from the perspective of users and many of Rustdoc's transformations, we treat it specially and don't think of it as such. For example, we don't normally render it as a parameter on a trait's page (though we do currently have some bugs, like the : PartialEq<Self> here but not here). So I feel like from Rustdoc's perspective, it makes sense to treat it specially unless and until we replace clean::Type with rustc_middle::ty::Ty. Thoughts?

I do intend to add information about the meaning of the type (alias, with value, or parameter, with DefId) to Type::SelfTy, but rustdoc currently doesn't use that information.

Yes, thanks for raising this. I believe that some other part of rustdoc is "unwrapping" the Self alias; see for example Vec::new()'s return type.

No, that's not the reason. You've linked to std's Vec::new which is an inlined cross-crate re-export. If you take a look at the local docs in alloc: https://doc.rust-lang.org/nightly/alloc/vec/struct.Vec.html#method.new, you can see that rustdoc retains the literal Self.

This is due to the fact that in the local case rustdoc cleans the HIR which contains the still-unresolved Self path. In the IXCRE case, rustdoc cleans rustc_middle::ty types were the SelfTyAlias has been resolved to a 'concrete' type (here: Vec<T>). In rustc_middle::ty only SelfTyParam is retained (as tcx.types.self_param / "TyKind::Param(0, kw::SelfUpper)").

This bug (or discrepancy at least) is tracked in #44306. See my latest comment from almost a year ago: #44306 (comment) describing how we might go about fixing it and why I haven't taken it on (yet). Note that that comment assumes we want to render Self over the resolved aliased type which might lead to a better UX (unknown).

GuillaumeGomez · 2024-08-02T22:16:50Z

Nice improvement, thanks!

@rfcbot reviewed

camelid · 2024-08-03T00:50:03Z

Ok, according to our new procedure for rustdoc-frontend changes, this change is now approved ✔️ . (Though the PR itself is not done with review quite yet.)

…riddle rustdoc: Cleanup `CacheBuilder` code for building search index This code was very convoluted and hard to reason about. It is now (I hope) much clearer and more suitable for both future enhancements and future cleanups. I'm doing this as a precursor, with no UI changes, to changing rustdoc to [ignore blanket impls][1] in type-based search. [1]: rust-lang#128471 (comment) r? `@notriddle`

…riddle rustdoc: Cleanup `CacheBuilder` code for building search index This code was very convoluted and hard to reason about. It is now (I hope) much clearer and more suitable for both future enhancements and future cleanups. I'm doing this as a precursor, with no UI changes, to changing rustdoc to [ignore blanket impls][1] in type-based search. [1]: rust-lang#128471 (comment) r? ``@notriddle``

Rollup merge of rust-lang#128578 - camelid:cache-index-cleanup, r=notriddle rustdoc: Cleanup `CacheBuilder` code for building search index This code was very convoluted and hard to reason about. It is now (I hope) much clearer and more suitable for both future enhancements and future cleanups. I'm doing this as a precursor, with no UI changes, to changing rustdoc to [ignore blanket impls][1] in type-based search. [1]: rust-lang#128471 (comment) r? ``@notriddle``

`SelfTy` makes it sound like it is literally the `Self` type, whereas in fact it may be `&Self` or other types. Plus, I want to use the name `SelfTy` for a new variant of `clean::Type`. Having both causes resolution conflicts or at least confusion.

Rustdoc often has to special-case `Self` because it is, well, a special type of generic parameter (although it also behaves as an alias in concrete impls). Instead of spreading this special-casing throughout the code base, create a new variant of the `clean::Type` enum that is for `Self` types. This is a refactoring that has almost no impact on rustdoc's behavior, except that `&Self`, `(Self,)`, `&[Self]`, and other similar occurrences of `Self` no longer link to the wrapping type (reference primitive, tuple primitive, etc.) as regular generics do. I felt this made more sense since users would expect `Self` to link to the containing trait or aliased type (though those are usually expanded), not the primitive that is wrapping it. For an example of the change, see the docs for `std::alloc::Allocator::by_ref`.

We already have special-cased code to handle inlining `Self` as the type or trait it refers to, and this was just causing glitches like the search `A -> B` yielding blanket `Into` impls.

This is much more readable and idiomatic, and also may help performance since `match`es usually use switches while `if`s may not. I also fixed an incorrect comment.

It was barely used, and the places that used it are actually clearer without it since they were often undoing some of its work. This also avoids an unnecessary clone of the receiver type and removes a layer of logical indirection in the code.

notriddle · 2024-08-04T23:46:27Z

@bors r+ rollup

bors · 2024-08-04T23:46:29Z

📌 Commit dac7f20 has been approved by notriddle

It is now in the queue for this repository.

rustdoc: Fix handling of `Self` type in search index and refactor its representation ### Summary - Add enum variant `clean::Type::SelfTy` and use it instead of `clean::Type::Generic(kw::SelfUpper)`. - Stop treating `Self` as a generic in the search index. - Remove struct formerly known as `clean::SelfTy` (constructed as representation of function receiver type). We're better off without it. ### Before ![image](https://github.com/user-attachments/assets/d257bdd8-3a62-4c71-84a5-9c950f2e4f00) ### After ![image](https://github.com/user-attachments/assets/8f6d3f22-92c1-41e3-9ab8-a881b66816c0) r? `@notriddle` cc rust-lang#127589 (comment)

…enton Rollup of 11 pull requests Successful merges: - rust-lang#128026 (std::thread: available_parallelism implementation for vxWorks proposal.) - rust-lang#128471 (rustdoc: Fix handling of `Self` type in search index and refactor its representation) - rust-lang#128607 (Use `object` in `run-make/symbols-visibility`) - rust-lang#128609 (Remove unnecessary constants from flt2dec dragon) - rust-lang#128611 (run-make: Remove cygpath) - rust-lang#128630 (docs(resolve): more explain about `target`) - rust-lang#128638 (run-make: enable msvc for `link-dedup`) - rust-lang#128647 (Enable msvc for link-args-order) - rust-lang#128649 (run-make: Enable msvc for `no-duplicate-libs` and `zero-extend-abi-param-passing`) - rust-lang#128656 (Enable msvc for run-make/rust-lld) - rust-lang#128660 (tests: more crashes) r? `@ghost` `@rustbot` modify labels: rollup

rustdoc: Fix handling of `Self` type in search index and refactor its representation ### Summary - Add enum variant `clean::Type::SelfTy` and use it instead of `clean::Type::Generic(kw::SelfUpper)`. - Stop treating `Self` as a generic in the search index. - Remove struct formerly known as `clean::SelfTy` (constructed as representation of function receiver type). We're better off without it. ### Before ![image](https://github.com/user-attachments/assets/d257bdd8-3a62-4c71-84a5-9c950f2e4f00) ### After ![image](https://github.com/user-attachments/assets/8f6d3f22-92c1-41e3-9ab8-a881b66816c0) r? ``@notriddle`` cc rust-lang#127589 (comment)

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#128026 (std::thread: available_parallelism implementation for vxWorks proposal.) - rust-lang#128471 (rustdoc: Fix handling of `Self` type in search index and refactor its representation) - rust-lang#128607 (Use `object` in `run-make/symbols-visibility`) - rust-lang#128609 (Remove unnecessary constants from flt2dec dragon) - rust-lang#128611 (run-make: Remove cygpath) - rust-lang#128619 (Correct the const stabilization of `<[T]>::last_chunk`) - rust-lang#128630 (docs(resolve): more explain about `target`) - rust-lang#128660 (tests: more crashes) r? `@ghost` `@rustbot` modify labels: rollup

rustdoc: Cleanup `CacheBuilder` code for building search index This code was very convoluted and hard to reason about. It is now (I hope) much clearer and more suitable for both future enhancements and future cleanups. I'm doing this as a precursor, with no UI changes, to changing rustdoc to [ignore blanket impls][1] in type-based search. [1]: rust-lang/rust#128471 (comment) r? ``@notriddle``

Rollup merge of rust-lang#128471 - camelid:rustdoc-self, r=notriddle rustdoc: Fix handling of `Self` type in search index and refactor its representation ### Summary - Add enum variant `clean::Type::SelfTy` and use it instead of `clean::Type::Generic(kw::SelfUpper)`. - Stop treating `Self` as a generic in the search index. - Remove struct formerly known as `clean::SelfTy` (constructed as representation of function receiver type). We're better off without it. ### Before ![image](https://github.com/user-attachments/assets/d257bdd8-3a62-4c71-84a5-9c950f2e4f00) ### After ![image](https://github.com/user-attachments/assets/8f6d3f22-92c1-41e3-9ab8-a881b66816c0) r? ```@notriddle``` cc rust-lang#127589 (comment)

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? `@fmease`

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ``@fmease``

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ```@fmease```

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ````@fmease````

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? `````@fmease`````

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ``````@fmease``````

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ```````@fmease```````

…=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ````````@fmease````````

Rollup merge of rust-lang#129123 - aDotInTheVoid:rustdoc-json-self, r=fmease rustdoc-json: Add test for `Self` type Inspired by rust-lang#128471, the rustdoc-json suite had no tests in place for the `Self` type. This PR adds one. I've also manually checked locally that this test passes on 29e9248, confirming that adding `clean::Type::SelfTy` didn't change the JSON output. (potentially adding a self type to json (insead of (ab)using generic) is tracked in rust-lang#128522) Updates rust-lang#81359 r? ````````@fmease````````

rustbot assigned notriddle Aug 1, 2024

rustbot added A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 1, 2024

camelid commented Aug 1, 2024

View reviewed changes

camelid added A-type-based-search Area: Searching rustdoc pages using type signatures A-rustdoc-search Area: Rustdoc's search feature labels Aug 1, 2024

camelid commented Aug 1, 2024

View reviewed changes

notriddle reviewed Aug 1, 2024

View reviewed changes

tests/rustdoc-js/self-is-not-generic.js Show resolved Hide resolved

aDotInTheVoid mentioned this pull request Aug 1, 2024

rustdoc-json: Change Self type representation #128522

Open

compiler-errors reviewed Aug 1, 2024

View reviewed changes

GuillaumeGomez approved these changes Aug 2, 2024

View reviewed changes

camelid mentioned this pull request Aug 3, 2024

rustdoc: Cleanup CacheBuilder code for building search index #128578

Merged

This comment was marked as resolved.

Sign in to view

camelid added 6 commits August 4, 2024 12:48

rustdoc: Rename SelfTy to ReceiverTy

249d686

`SelfTy` makes it sound like it is literally the `Self` type, whereas in fact it may be `&Self` or other types. Plus, I want to use the name `SelfTy` for a new variant of `clean::Type`. Having both causes resolution conflicts or at least confusion.

rustdoc: Stop treating Self as a generic in search index

4e348fa

We already have special-cased code to handle inlining `Self` as the type or trait it refers to, and this was just causing glitches like the search `A -> B` yielding blanket `Into` impls.

Use match instead of sequence of if lets

e452e3d

This is much more readable and idiomatic, and also may help performance since `match`es usually use switches while `if`s may not. I also fixed an incorrect comment.

Add test for Self not being a generic in search index

dac7f20

camelid force-pushed the rustdoc-self branch from 1543c38 to dac7f20 Compare August 4, 2024 19:49

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 Aug 4, 2024

ChrisDenton mentioned this pull request Aug 5, 2024

Rollup of 11 pull requests #128668

Closed

This was referenced Aug 5, 2024

Rollup of 8 pull requests #128671

Closed

Rollup of 8 pull requests #128673

Merged

bors merged commit 4adefa4 into rust-lang:master Aug 5, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 5, 2024

camelid deleted the rustdoc-self branch August 5, 2024 09:41

aDotInTheVoid mentioned this pull request Aug 15, 2024

rustdoc-json: Add test for Self type #129123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Fix handling of `Self` type in search index and refactor its representation #128471

rustdoc: Fix handling of `Self` type in search index and refactor its representation #128471

camelid commented Aug 1, 2024

camelid Aug 1, 2024

aDotInTheVoid Aug 1, 2024

aDotInTheVoid Aug 1, 2024

camelid Aug 1, 2024

camelid commented Aug 1, 2024

camelid Aug 1, 2024

notriddle commented Aug 1, 2024

camelid commented Aug 1, 2024

camelid commented Aug 1, 2024

rfcbot commented Aug 1, 2024 •

edited by camelid

Loading

compiler-errors Aug 1, 2024 •

edited

Loading

camelid Aug 1, 2024

fmease Aug 15, 2024 •

edited

Loading

GuillaumeGomez commented Aug 2, 2024

camelid commented Aug 3, 2024

This comment was marked as resolved.

notriddle commented Aug 4, 2024

bors commented Aug 4, 2024

		// FIXME: add dedicated variant to json Type?
		SelfTy => Type::Generic("Self".to_owned()),

rustdoc: Fix handling of Self type in search index and refactor its representation #128471

rustdoc: Fix handling of Self type in search index and refactor its representation #128471

Conversation

camelid commented Aug 1, 2024

Summary

Before

After

camelid Aug 1, 2024

Choose a reason for hiding this comment

aDotInTheVoid Aug 1, 2024

Choose a reason for hiding this comment

aDotInTheVoid Aug 1, 2024

Choose a reason for hiding this comment

camelid Aug 1, 2024

Choose a reason for hiding this comment

camelid commented Aug 1, 2024

camelid Aug 1, 2024

Choose a reason for hiding this comment

notriddle commented Aug 1, 2024

camelid commented Aug 1, 2024

camelid commented Aug 1, 2024

rfcbot commented Aug 1, 2024 • edited by camelid Loading

compiler-errors Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

camelid Aug 1, 2024

Choose a reason for hiding this comment

fmease Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 2, 2024

camelid commented Aug 3, 2024

This comment was marked as resolved.

notriddle commented Aug 4, 2024

bors commented Aug 4, 2024

rustdoc: Fix handling of `Self` type in search index and refactor its representation #128471

rustdoc: Fix handling of `Self` type in search index and refactor its representation #128471

rfcbot commented Aug 1, 2024 •

edited by camelid

Loading

compiler-errors Aug 1, 2024 •

edited

Loading

fmease Aug 15, 2024 •

edited

Loading