Skip to content
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: show type signature on type-driven SERP #117112

Closed

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Oct 24, 2023

Preview

Screenshots of what these pages looked like before

image
image

Preview page:

Desktop screenshots:
image
image
image

Mobile screenshot:
image

Description

This displays the function signature, as rustdoc understands it, on the In Parameters, In Return Types, and In Function Signature pages, but not in the In Names page, since it's not used there. It also highlights the matching parts, to clarify why a function is considered a good match.

Rationale

Since the point of this feature is to answer the question "why this a match," it's important that it communicates a few things:

  • How do I use this? This UI shows types as Rustdoc sees them, using Rustdoc's search syntax, rather than the syntax used by normal Rust. It's supposed to act as a built-in, context-sensitive tutorial, since you can type a name in, switch to the In Parameters tab, and see what you could've typed to get a particular result.
  • Which type parameters got matched to other type parameters? Since names aren't relevant to matching, it shows the type parameters that the user wrote, not the ones that exist in the original source code (consider the above screenshot, that shows how T and U both got matched even though this particular result is a bit counterintuitive).
  • How does the search engine even work, anyway? It's important that this page accurately shows you what unboxing occurred and which mappings got followed. That's why parts of the highlighter are baked into the type unification function: it doesn't just show you where your atoms occur, but actually shows you which ones got matched.

This change also allows us to stop de-duplicating these results, so it's a follow-up for #109422 (comment). This PR doesn't remove the deduplication logic for the name-based search, because it seems like showing all of the Simd add functions in that context would be more annoying than useful, because it could push the result that you want off the screen, and there's no way to narrow down to a particular one without switching to an entirely different style of search.

Future possibilities

This needs merged with #116085. This branch is not currently based on that one.

Other future possibilities, copied directly from there:

@rustbot
Copy link
Collaborator

rustbot commented Oct 24, 2023

r? @GuillaumeGomez

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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 Oct 24, 2023
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from e09dd6e to 8e043ab Compare October 24, 2023 03:10
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 8e043ab to 22619c9 Compare October 24, 2023 03:55
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 22619c9 to 952e01e Compare October 24, 2023 04:27
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 952e01e to f5654ea Compare October 24, 2023 05:35
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 9a9f7a9 to c3a08be Compare October 24, 2023 17:07
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 9fda9c9 to a550f11 Compare October 24, 2023 17:50
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from a550f11 to 35bddbb Compare October 25, 2023 02:20
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/display-signature branch from 35bddbb to 8337170 Compare October 25, 2023 04:25
@GuillaumeGomez
Copy link
Member

This is really an awesome idea! Code looks good to me as well, nothing to say except: great job!

This reduces code size while still matching the common case
for plain, concrete types.
This displays the function signature, as rustdoc understands it,
on the In Parameters, In Return Types, and In Function Signature
pages, but not in the In Names page, since it's not used there.
It also highlights the matching parts, to clarify why a function
is considered a good match.
@notriddle notriddle force-pushed the notriddle/display-signature branch from 8337170 to 42616b3 Compare October 25, 2023 16:14
This cleans up the Simd page. It should be sufficient, even though
it can wind up showing more than necessary, but allows each item
to be considered in isolation.
@notriddle notriddle force-pushed the notriddle/display-signature branch from 42616b3 to a463f43 Compare October 25, 2023 16:18
@bors
Copy link
Contributor

bors commented Nov 16, 2023

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

@aDotInTheVoid
Copy link
Member

This is a really great UI improvement, and I'd quite like to see it landed.

I'm not qualified to review the implementation, but design wise, a couple of things:

image

  • Somehow the return type is listed here as [T], where it should be &[T]
  • This should show Vec<T, A>, not <T, U>, as that's clearer about the purpose of the second type parameter.

@notriddle
Copy link
Contributor Author

Somehow the return type is listed here as [T], where it should be &[T]

That's one major reason why this is still a draft. Everything it shows should be searchable, but the search index doesn't contain references and it can't parse them,

Once the search engine can parse and search for most type expressions, including & references, then I'll take this PR out of draft and look for more feedback.

This should show Vec<T, A>, not <T, U>, as that's clearer about the purpose of the second type parameter.

I need to communicate that the U type parameter in the search query got bound to the Vec's second type parameter (which is named A in its own code, but is U in the user's search query).

Instead of just using A, it might need to be something like A = U? I don't want to do that, though, because that's not real search syntax, and I'd like this display to act as a tutorial for it.

@notriddle
Copy link
Contributor Author

notriddle commented Jan 9, 2024

This PR is going to need to be mostly rewritten, because it mutates the function signature AST in-place, but #119756 causes chunks of that tree to be reused across functions. It should treat the AST as read-only and construct another data structure for highlighting.

Thanks for pointing out the A/U gemeric parameter confusion, @aDotInTheVoid. It's a good point; normal search engines show text excerpts from the document, and highlight matches with the query inside that, like this screenshot:

image

Notice how, in the above screenshot, "build" is highlighted even though the actual query says "compiled." Additional information about a result should probably go in a popover (the below screenshot is the right idea, but not nearly detailed enough for our use case):

image

Pulling this off is going to require stashing the definition-side names of generic type parameters somewhere (as well as implementing a popover with all the "why this result" information in it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants