-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rustdoc-search: add impl disambiguator to duplicate assoc items #109422
rustdoc-search: add impl disambiguator to duplicate assoc items #109422
Conversation
r? @jsha (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @Folyd, @jsha |
7fcf823
to
5420ac6
Compare
This comment has been minimized.
This comment has been minimized.
cdb9cbc
to
a73aa9b
Compare
This comment has been minimized.
This comment has been minimized.
5a07dea
to
e799570
Compare
This comment has been minimized.
This comment has been minimized.
e799570
to
da5d112
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Just checked and it looks pretty nice. One thing though (a GUI test to be added for this too): when clicking on the |
@GuillaumeGomez A fix has been committed and an update pushed to the preview. |
It's a really nice improvement! It's a shame that we need to add JS to handle this but considering that the alternative would be to increase significantly the HTML size, I think it's an acceptable tradeoff. Let's see what others think about it. @rfcbot fcp merge |
Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Restating the problem for my own understanding. Maybe you want to incorporate some of this in the PR desc as an intro? When a struct/enum/union has multiple impls with different type parameters, it can have multiple methods that have the same name, but which are on different impls. Besides Simd, Any also demonstrates this pattern. It has three methods named When that happens, it presents a challenge in linking to the method. Normally we link like It also presents a challenge for our search code. Currently we only store one of the various To fix this, we need three things:
I thought we already had an old PR floating around for (1)? Also FYI for any other reviewers, it can be hard to see the new fragment format in action because on page load the fragment immediately gets rewritten to the old-style format. Anyhow in general I like the idea. I'd kind of like the fully-specified fragment format to be the default, because it's more stable. But it would also be nice for single-impl structs to be able to use the simpler |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@notriddle Feel free to r+ if it's all good (I saw you pushed a few commits recently so just in case...). |
I was just rebasing with the other changes to search. @bors r+ rollup |
@bors r- @bors r=@GuillaumeGomez rollup |
…ate-search, r=GuillaumeGomez rustdoc-search: add impl disambiguator to duplicate assoc items Preview (to see the difference, click the link and pay attention to the specific function that comes up): | Before | After | |--|--| | [`simd<i64>, simd<i64> -> simd<i64>`](https://doc.rust-lang.org/nightly/std/?search=simd%3Ci64%3E%2C%20simd%3Ci64%3E%20-%3E%20simd%3Ci64%3E) | [`simd<i64>, simd<i64> -> simd<i64>`](https://notriddle.com/rustdoc-demo-html-3/impl-disambiguate-search/std/index.html?search=simd%3Ci64%3E%2C%20simd%3Ci64%3E%20-%3E%20simd%3Ci64%3E) | | [`cow, vec -> bool`](https://doc.rust-lang.org/nightly/std/?search=cow%2C%20vec%20-%3E%20bool) | [`cow, vec -> bool`](https://notriddle.com/rustdoc-demo-html-3/impl-disambiguate-search/std/index.html?search=cow%2C%20vec%20-%3E%20bool) Helps with rust-lang#90929 This changes the search results, specifically, when there's more than one impl with an associated item with the same name. For example, the search queries `simd<i8> -> simd<i8>` and `simd<i64> -> simd<i64>` don't link to the same function, but most of the functions have the same names. This change should probably be FCP-ed, especially since it adds a new anchor link format for `main.js` to handle, so that URLs like `struct.Vec.html#impl-AsMut<[T]>-for-Vec<T,+A>/method.as_mut` redirect to `struct.Vec.html#method.as_mut-2`. It's a strange design, but there are a few reasons for it: * I'd like to avoid making the HTML bigger. Obviously, fixing this bug is going to add at least a little more data to the search index, but adding more HTML penalises viewers for the benefit of searchers. * Breaking `struct.Vec.html#method.len` would also be a disappointment. On the other hand: * The path-style anchors might be less prone to link rot than the numbered anchors. It's definitely less likely to have URLs that appear to "work", but silently point at the wrong thing. * This commit arranges the path-style anchor to redirect to the numbered anchor. Nothing stops rustdoc from doing the opposite, making path-style anchors the default and redirecting the "legacy" numbered ones. ### The bug On the "Before" links, this example search calls for `i64`: ![image](https://github.com/rust-lang/rust/assets/1593513/9431d89d-41dc-4f68-bbb1-3e2704a973d2) But if I click any of the results, I get `f64` instead. ![image](https://github.com/rust-lang/rust/assets/1593513/6d89c692-1847-421a-84d9-22e359d9cf82) The PR fixes this problem by adding enough information to the search result `href` to disambiguate methods with different types but the same name. More detailed description of the problem at: rust-lang#109422 (comment) > When a struct/enum/union has multiple impls with different type parameters, it can have multiple methods that have the same name, but which are on different impls. Besides Simd, [Any](https://doc.rust-lang.org/nightly/std/any/trait.Any.html?search=any%3A%3Adowncast) also demonstrates this pattern. It has three methods named `downcast`, on three different impls. > > When that happens, it presents a challenge in linking to the method. Normally we link like `#method.foo`. When there are multiple `foo`, we number them like `#method.foo`, `#method.foo-1`, `#method.foo-2`, etc. > > It also presents a challenge for our search code. Currently we store all the variants in the index, but don’t have any way to generate unambiguous URLs in the results page, or to distinguish them in the SERP. > > To fix this, we need three things: > > 1. A fragment format that fully specifies the impl type parameters when needed to disambiguate (`#impl-SimdOrd-for-Simd<i64,+LANES>/method.simd_max`) > 2. A search index that stores methods with enough information to disambiguate the impl they were on. > 3. A search results interface that can display multiple methods on the same type with the same name, when appropriate OR a disambiguation landing section on item pages? > > For reviewers: it can be hard to see the new fragment format in action since it immediately gets rewritten to the numbered form.
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#109422 (rustdoc-search: add impl disambiguator to duplicate assoc items) - rust-lang#115948 (rustdoc: show crate name beside smaller logo) - rust-lang#116444 (add test for const-eval error in dead code during monomorphization) - rust-lang#116503 (Update docs for mips target tier demotion.) - rust-lang#116559 (Mark `new_in` as `const` for BTree collections) - rust-lang#116560 (In smir use `FxIndexMap` to store indexed ids) - rust-lang#116574 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#109422 (rustdoc-search: add impl disambiguator to duplicate assoc items) - rust-lang#116250 (On type error of closure call argument, point at earlier calls that affected inference) - rust-lang#116444 (add test for const-eval error in dead code during monomorphization) - rust-lang#116503 (Update docs for mips target tier demotion.) - rust-lang#116559 (Mark `new_in` as `const` for BTree collections) - rust-lang#116560 (In smir use `FxIndexMap` to store indexed ids) - rust-lang#116574 (Update books) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#109422 - notriddle:notriddle/impl-disambiguate-search, r=GuillaumeGomez rustdoc-search: add impl disambiguator to duplicate assoc items Preview (to see the difference, click the link and pay attention to the specific function that comes up): | Before | After | |--|--| | [`simd<i64>, simd<i64> -> simd<i64>`](https://doc.rust-lang.org/nightly/std/?search=simd%3Ci64%3E%2C%20simd%3Ci64%3E%20-%3E%20simd%3Ci64%3E) | [`simd<i64>, simd<i64> -> simd<i64>`](https://notriddle.com/rustdoc-demo-html-3/impl-disambiguate-search/std/index.html?search=simd%3Ci64%3E%2C%20simd%3Ci64%3E%20-%3E%20simd%3Ci64%3E) | | [`cow, vec -> bool`](https://doc.rust-lang.org/nightly/std/?search=cow%2C%20vec%20-%3E%20bool) | [`cow, vec -> bool`](https://notriddle.com/rustdoc-demo-html-3/impl-disambiguate-search/std/index.html?search=cow%2C%20vec%20-%3E%20bool) Helps with rust-lang#90929 This changes the search results, specifically, when there's more than one impl with an associated item with the same name. For example, the search queries `simd<i8> -> simd<i8>` and `simd<i64> -> simd<i64>` don't link to the same function, but most of the functions have the same names. This change should probably be FCP-ed, especially since it adds a new anchor link format for `main.js` to handle, so that URLs like `struct.Vec.html#impl-AsMut<[T]>-for-Vec<T,+A>/method.as_mut` redirect to `struct.Vec.html#method.as_mut-2`. It's a strange design, but there are a few reasons for it: * I'd like to avoid making the HTML bigger. Obviously, fixing this bug is going to add at least a little more data to the search index, but adding more HTML penalises viewers for the benefit of searchers. * Breaking `struct.Vec.html#method.len` would also be a disappointment. On the other hand: * The path-style anchors might be less prone to link rot than the numbered anchors. It's definitely less likely to have URLs that appear to "work", but silently point at the wrong thing. * This commit arranges the path-style anchor to redirect to the numbered anchor. Nothing stops rustdoc from doing the opposite, making path-style anchors the default and redirecting the "legacy" numbered ones. ### The bug On the "Before" links, this example search calls for `i64`: ![image](https://github.com/rust-lang/rust/assets/1593513/9431d89d-41dc-4f68-bbb1-3e2704a973d2) But if I click any of the results, I get `f64` instead. ![image](https://github.com/rust-lang/rust/assets/1593513/6d89c692-1847-421a-84d9-22e359d9cf82) The PR fixes this problem by adding enough information to the search result `href` to disambiguate methods with different types but the same name. More detailed description of the problem at: rust-lang#109422 (comment) > When a struct/enum/union has multiple impls with different type parameters, it can have multiple methods that have the same name, but which are on different impls. Besides Simd, [Any](https://doc.rust-lang.org/nightly/std/any/trait.Any.html?search=any%3A%3Adowncast) also demonstrates this pattern. It has three methods named `downcast`, on three different impls. > > When that happens, it presents a challenge in linking to the method. Normally we link like `#method.foo`. When there are multiple `foo`, we number them like `#method.foo`, `#method.foo-1`, `#method.foo-2`, etc. > > It also presents a challenge for our search code. Currently we store all the variants in the index, but don’t have any way to generate unambiguous URLs in the results page, or to distinguish them in the SERP. > > To fix this, we need three things: > > 1. A fragment format that fully specifies the impl type parameters when needed to disambiguate (`#impl-SimdOrd-for-Simd<i64,+LANES>/method.simd_max`) > 2. A search index that stores methods with enough information to disambiguate the impl they were on. > 3. A search results interface that can display multiple methods on the same type with the same name, when appropriate OR a disambiguation landing section on item pages? > > For reviewers: it can be hard to see the new fragment format in action since it immediately gets rewritten to the numbered form.
48: Pull upstream master 2023 10 12 r=tshepang a=Dajamante * rust-lang/rust#113487 * rust-lang/rust#116506 * rust-lang/rust#116448 * rust-lang/rust#116640 * rust-lang/rust#116627 * rust-lang/rust#116597 * rust-lang/rust#116436 * rust-lang/rust#116315 * rust-lang/rust#116219 * rust-lang/rust#113218 * rust-lang/rust#115937 * rust-lang/rust#116014 * rust-lang/rust#116623 * rust-lang/rust#112818 * rust-lang/rust#115948 * rust-lang/rust#116622 * rust-lang/rust#116621 * rust-lang/rust#116612 * rust-lang/rust#116611 * rust-lang/rust#116530 * rust-lang/rust#95967 * rust-lang/rust#116578 * rust-lang/rust#113915 * rust-lang/rust#116605 * rust-lang/rust#116574 * rust-lang/rust#116560 * rust-lang/rust#116559 * rust-lang/rust#116503 * rust-lang/rust#116444 * rust-lang/rust#116250 * rust-lang/rust#109422 * rust-lang/rust#116598 * rust-lang/rust#116596 * rust-lang/rust#116595 * rust-lang/rust#116589 * rust-lang/rust#116586 * rust-lang/rust#116551 * rust-lang/rust#116409 * rust-lang/rust#116548 * rust-lang/rust#116366 * rust-lang/rust#109882 * rust-lang/rust#116497 * rust-lang/rust#116532 * rust-lang/rust#116569 * rust-lang/rust#116561 * rust-lang/rust#116556 * rust-lang/rust#116549 * rust-lang/rust#116543 * rust-lang/rust#116537 * rust-lang/rust#115882 * rust-lang/rust#116142 * rust-lang/rust#115238 * rust-lang/rust#116533 * rust-lang/rust#116096 * rust-lang/rust#116468 * rust-lang/rust#116515 * rust-lang/rust#116454 * rust-lang/rust#116183 * rust-lang/rust#116514 * rust-lang/rust#116509 * rust-lang/rust#116487 * rust-lang/rust#116486 * rust-lang/rust#116450 * rust-lang/rust#114623 * rust-lang/rust#116416 * rust-lang/rust#116437 * rust-lang/rust#100806 * rust-lang/rust#116330 * rust-lang/rust#116310 * rust-lang/rust#115583 * rust-lang/rust#116457 * rust-lang/rust#116508 * rust-lang/rust#109214 * rust-lang/rust#116318 * rust-lang/rust#116501 * rust-lang/rust#116500 * rust-lang/rust#116458 * rust-lang/rust#116400 * rust-lang/rust#116277 * rust-lang/rust#114709 * rust-lang/rust#116492 * rust-lang/rust#116484 * rust-lang/rust#116481 * rust-lang/rust#116474 * rust-lang/rust#116466 * rust-lang/rust#116423 * rust-lang/rust#116297 * rust-lang/rust#114564 * rust-lang/rust#114811 * rust-lang/rust#116489 * rust-lang/rust#115304 Co-authored-by: Peter Hall <peter.hall@hyperexponential.com> Co-authored-by: Emanuele Vannacci <emanuele.vannacci@gmail.com> Co-authored-by: Neven Villani <vanille@crans.org> Co-authored-by: Alex Macleod <alex@macleod.io> Co-authored-by: Tamir Duberstein <tamird@gmail.com> Co-authored-by: Eduardo Sánchez Muñoz <eduardosm-dev@e64.io> Co-authored-by: koka <koka.code@gmail.com> Co-authored-by: bors <bors@rust-lang.org> Co-authored-by: Philipp Krones <hello@philkrones.com> Co-authored-by: Camille GILLOT <gillot.camille@gmail.com> Co-authored-by: Esteban Küber <esteban@kuber.com.ar> Co-authored-by: Ralf Jung <post@ralfj.de>
48: Pull upstream master 2023 10 12 r=tshepang a=Dajamante * rust-lang/rust#113487 * rust-lang/rust#116506 * rust-lang/rust#116448 * rust-lang/rust#116640 * rust-lang/rust#116627 * rust-lang/rust#116597 * rust-lang/rust#116436 * rust-lang/rust#116315 * rust-lang/rust#116219 * rust-lang/rust#113218 * rust-lang/rust#115937 * rust-lang/rust#116014 * rust-lang/rust#116623 * rust-lang/rust#112818 * rust-lang/rust#115948 * rust-lang/rust#116622 * rust-lang/rust#116621 * rust-lang/rust#116612 * rust-lang/rust#116611 * rust-lang/rust#116530 * rust-lang/rust#95967 * rust-lang/rust#116578 * rust-lang/rust#113915 * rust-lang/rust#116605 * rust-lang/rust#116574 * rust-lang/rust#116560 * rust-lang/rust#116559 * rust-lang/rust#116503 * rust-lang/rust#116444 * rust-lang/rust#116250 * rust-lang/rust#109422 * rust-lang/rust#116598 * rust-lang/rust#116596 * rust-lang/rust#116595 * rust-lang/rust#116589 * rust-lang/rust#116586 * rust-lang/rust#116551 * rust-lang/rust#116409 * rust-lang/rust#116548 * rust-lang/rust#116366 * rust-lang/rust#109882 * rust-lang/rust#116497 * rust-lang/rust#116532 * rust-lang/rust#116569 * rust-lang/rust#116561 * rust-lang/rust#116556 * rust-lang/rust#116549 * rust-lang/rust#116543 * rust-lang/rust#116537 * rust-lang/rust#115882 * rust-lang/rust#116142 * rust-lang/rust#115238 * rust-lang/rust#116533 * rust-lang/rust#116096 * rust-lang/rust#116468 * rust-lang/rust#116515 * rust-lang/rust#116454 * rust-lang/rust#116183 * rust-lang/rust#116514 * rust-lang/rust#116509 * rust-lang/rust#116487 * rust-lang/rust#116486 * rust-lang/rust#116450 * rust-lang/rust#114623 * rust-lang/rust#116416 * rust-lang/rust#116437 * rust-lang/rust#100806 * rust-lang/rust#116330 * rust-lang/rust#116310 * rust-lang/rust#115583 * rust-lang/rust#116457 * rust-lang/rust#116508 * rust-lang/rust#109214 * rust-lang/rust#116318 * rust-lang/rust#116501 * rust-lang/rust#116500 * rust-lang/rust#116458 * rust-lang/rust#116400 * rust-lang/rust#116277 * rust-lang/rust#114709 * rust-lang/rust#116492 * rust-lang/rust#116484 * rust-lang/rust#116481 * rust-lang/rust#116474 * rust-lang/rust#116466 * rust-lang/rust#116423 * rust-lang/rust#116297 * rust-lang/rust#114564 * rust-lang/rust#114811 * rust-lang/rust#116489 * rust-lang/rust#115304 Co-authored-by: Emanuele Vannacci <emanuele.vannacci@gmail.com> Co-authored-by: Neven Villani <vanille@crans.org> Co-authored-by: Alex Macleod <alex@macleod.io> Co-authored-by: Tamir Duberstein <tamird@gmail.com> Co-authored-by: Eduardo Sánchez Muñoz <eduardosm-dev@e64.io> Co-authored-by: koka <koka.code@gmail.com> Co-authored-by: bors <bors@rust-lang.org> Co-authored-by: Philipp Krones <hello@philkrones.com> Co-authored-by: Camille GILLOT <gillot.camille@gmail.com> Co-authored-by: Esteban Küber <esteban@kuber.com.ar> Co-authored-by: Ralf Jung <post@ralfj.de> Co-authored-by: ShE3py <52315535+she3py@users.noreply.github.com>
Preview (to see the difference, click the link and pay attention to the specific function that comes up):
simd<i64>, simd<i64> -> simd<i64>
simd<i64>, simd<i64> -> simd<i64>
cow, vec -> bool
cow, vec -> bool
Helps with #90929
This changes the search results, specifically, when there's more than one impl with an associated item with the same name. For example, the search queries
simd<i8> -> simd<i8>
andsimd<i64> -> simd<i64>
don't link to the same function, but most of the functions have the same names.This change should probably be FCP-ed, especially since it adds a new anchor link format for
main.js
to handle, so that URLs likestruct.Vec.html#impl-AsMut<[T]>-for-Vec<T,+A>/method.as_mut
redirect tostruct.Vec.html#method.as_mut-2
. It's a strange design, but there are a few reasons for it:I'd like to avoid making the HTML bigger. Obviously, fixing this bug is going to add at least a little more data to the search index, but adding more HTML penalises viewers for the benefit of searchers.
Breaking
struct.Vec.html#method.len
would also be a disappointment.On the other hand:
The path-style anchors might be less prone to link rot than the numbered anchors. It's definitely less likely to have URLs that appear to "work", but silently point at the wrong thing.
This commit arranges the path-style anchor to redirect to the numbered anchor. Nothing stops rustdoc from doing the opposite, making path-style anchors the default and redirecting the "legacy" numbered ones.
The bug
On the "Before" links, this example search calls for
i64
:But if I click any of the results, I get
f64
instead.The PR fixes this problem by adding enough information to the search result
href
to disambiguate methods with different types but the same name.More detailed description of the problem at:
#109422 (comment)