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.

Sign up for GitHub

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

Jump to bottom

for purely return-type based searches, deprioritize clone-like functions #135302

Merged
merged 1 commit into from
Jan 15, 2025
Merged

for purely return-type based searches, deprioritize clone-like functions #135302

merged 1 commit into from
Jan 15, 2025

Conversation

lolbinarycat
Copy link
Contributor

closes #134935

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2025

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2025

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@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 Jan 9, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot 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 Jan 10, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat
Copy link
Contributor Author

@notriddle do you know how to make console.log print to node's stdout? the stack traces aren't very helpful due to minification messing up all the line numbers.

@lolbinarycat
Copy link
Contributor Author

update: looks like the issue was the minification was stripping the log statements, hacking my way around the minification fixed that

@lolbinarycat
Copy link
Contributor Author

second update: the workaround broke things catestrophically: #135345

@notriddle
Copy link
Contributor

I've usually worked around it by running the test case in a browser instead of the test harness. The docs for a tests/rustdoc-js/type-parameters.rs, for example, can be opened with

firefox build/x86_64-unknown-linux-gnu/test/rustdoc-js/type-parameters/foo/index.html

This way, search.js can be symlinked like normal, and you can try out queries against the test data without having to re-run the test harness.

@lolbinarycat
Copy link
Contributor Author

fair... but honestly i really just want to bring back --disable-minification (and have opened a PR doing just that)

i also really don't know what's going on here, to be honest. i feel like i'm doing everything right, but evidently i'm not.

@notriddle
Copy link
Contributor

Do you mean the change to tests/rustdoc-js/type-parameters.js? Because I think that's a real change. alef gets sorted first because it thinks bet and beta are Clone-like (bet has two different generics, but they both unify with generic:T because they're being checked separately).

@lolbinarycat
Copy link
Contributor Author

well, mainly that sometimes i get a test failure when running locally (the one you fixed with type.where_clause, maybe there was a cache invalidation issue).

but anyways, i don't really think this should count generics, is there some way to turn off generic unification? maybe in one of those callbacks i don't really understand?

@notriddle
Copy link
Contributor

notriddle commented Jan 11, 2025 •
edited
Loading

This code only fires if there's at least one item in the return spot. Check if any of them have a negative id number. If any of them do, that's a generic.

@lolbinarycat
Copy link
Contributor Author

but in the case of a search like -> SomeType, i don't want a function like T -> SomeTime to get deprioritized.

also, the code also triggers when sorting the "in return types" section.

@lolbinarycat
Copy link
Contributor Author

an example of a type that should not be considered clone-like is type_name_of_val

@lolbinarycat
Copy link
Contributor Author

ok it looks like that's not how generic unification works (right, they're universally quantified, not existentially), so the fix you recommended should work.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
notriddle
notriddle requested changes Jan 11, 2025
View reviewed changes
src/librustdoc/html/static/js/search.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/search.js Outdated Show resolved Hide resolved
@lolbinarycat
Copy link
Contributor Author

Thanks again... I'm too used to having a typechecker, so I didn't even consider this as a possibility (I mean, even python would give a TypeError there)

GuillaumeGomez
GuillaumeGomez reviewed Jan 13, 2025
View reviewed changes
src/librustdoc/html/static/js/search.js
* @param {[FunctionType]} where_clause - Trait bounds for generic items.
*/
function containsTypeFromQuery(list, where_clause) {
if (!list) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add parens (here an below) for coherency with the rest of the code.

@GuillaumeGomez
Copy link
Member

Once ready, please squash your commits as well.

And final point: should we start documenting how search items are ranked and (de)prioritized in the rustdoc book?

@lolbinarycat
Copy link
Contributor Author

i think adding a section in the rustdoc book about how results are sorted would be a good idea, yes

@lolbinarycat @notriddle
for purely return-type based searches, deprioritize clone-like functions
ebd5ce1 
A clone-like function in a function that takes as an argument the type
that it returns.

However, functions that return a type variable are not counted as
clone-line. Because we're not unifying the whole function at once,
a function like `U -> T` would otherwise be counted as "clone-like"
because the generics will just unify with anything when done seperatly.

Co-authored-by: Michael Howell <[email protected]>
@lolbinarycat lolbinarycat force-pushed the rustdoc-search-return-sort-134935 branch from a234b59 to ebd5ce1 Compare January 13, 2025 18:25
@lolbinarycat lolbinarycat mentioned this pull request Jan 13, 2025
Open
@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 14, 2025

📌 Commit ebd5ce1 has been approved by notriddle

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 14, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2025
@matthiaskrgr
Rollup merge of rust-lang#135302 - lolbinarycat:rustdoc-search-return…
487983f 
…-sort-134935, r=notriddle

for purely return-type based searches, deprioritize clone-like functions

closes rust-lang#134935
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 14, 2025
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2025
@matthiaskrgr
Rollup merge of rust-lang#135302 - lolbinarycat:rustdoc-search-return…
e351eae 
…-sort-134935, r=notriddle

for purely return-type based searches, deprioritize clone-like functions

closes rust-lang#134935
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 14, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2025
@bors
Auto merge of rust-lang#135509 - matthiaskrgr:rollup-4ua3iix, r=matth…
d5e900b 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#134940 (Make sure to scrape region constraints from deeply normalizing type outlives assumptions in borrowck)
 - rust-lang#135047 (Add gpu-kernel calling convention)
 - rust-lang#135228 (Improve `DispatchFromDyn` and `CoerceUnsized` impl validation)
 - rust-lang#135264 (Consider more erroneous layouts as `LayoutError::ReferencesError` to suppress spurious errors)
 - rust-lang#135302 (for purely return-type based searches, deprioritize clone-like functions)
 - rust-lang#135380 (Make sure we can produce `ConstArgHasWrongType` errors for valtree consts)
 - rust-lang#135425 (Do not consider traits that have unsatisfied const conditions to be conditionally const)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee workingjubilee mentioned this pull request Jan 15, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2025
@bors
Auto merge of rust-lang#135519 - workingjubilee:rollup-xq9yryh, r=wor…
00ded39 
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#134913 (bootstrap: do not rely on LIBRARY_PATH env variable)
 - rust-lang#134940 (Make sure to scrape region constraints from deeply normalizing type outlives assumptions in borrowck)
 - rust-lang#135228 (Improve `DispatchFromDyn` and `CoerceUnsized` impl validation)
 - rust-lang#135264 (Consider more erroneous layouts as `LayoutError::ReferencesError` to suppress spurious errors)
 - rust-lang#135302 (for purely return-type based searches, deprioritize clone-like functions)
 - rust-lang#135353 (re-add --disable-minification to rustdoc)
 - rust-lang#135380 (Make sure we can produce `ConstArgHasWrongType` errors for valtree consts)
 - rust-lang#135423 (Enforce syntactical stability of const traits in HIR)
 - rust-lang#135425 (Do not consider traits that have unsatisfied const conditions to be conditionally const)
 - rust-lang#135499 (fix underlining of hovered intra-doc links.)
 - rust-lang#135505 (Fix clippy lints in rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0a5a8c4 into rust-lang:master Jan 15, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 15, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2025
@rust-timer
Unrolled build for rust-lang#135302
4c0a0c2 
Rollup merge of rust-lang#135302 - lolbinarycat:rustdoc-search-return-sort-134935, r=notriddle

for purely return-type based searches, deprioritize clone-like functions

closes rust-lang#134935
@lolbinarycat lolbinarycat mentioned this pull request Jan 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

@tgross35 tgross35 tgross35 left review comments

@notriddle notriddle notriddle requested changes

Assignees

@notriddle notriddle

Labels
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
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.

rustdoc search: for return-type based searches, deprioritize functions that also have that type in an argument
7 participants
@lolbinarycat @rustbot @rust-log-analyzer @notriddle @GuillaumeGomez @bors @tgross35