Put negative implementors first and apply same ordering logic to foreign implementors #142380

GuillaumeGomez · 2025-06-11T20:26:33Z

This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls.

I also used this occasion to make the foreign implementors sort the same as the local ones by using compare_names.

You can test it here.

r? @notriddle

rustbot · 2025-06-11T20:26:40Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

GuillaumeGomez · 2025-06-12T14:37:40Z

Took me a while to figure out where the typescript definition was stored to fix the error. ^^'

src/librustdoc/html/render/print_item.rs

lolbinarycat · 2025-06-16T19:09:28Z

src/librustdoc/html/render/write_shared.rs

-            let part = OrderedJson::array_sorted(
+            implementors.sort_unstable();
+
+            let part = OrderedJson::array_unsorted(


This is definitely an improvement, as it eliminates an allocation, but we're still allocating an intermediate string for each element.

OrderedJson could be much more efficient if it had a method that appended using serde_json::to_writer.

I can open an issue for improving the OrderedJson interface, if you want.

An issue would be welcome indeed.

src/librustdoc/html/render/write_shared.rs

src/librustdoc/html/static/css/rustdoc.css

src/librustdoc/html/static/js/rustdoc.d.ts

tests/rustdoc-gui/implementors.goml

src/librustdoc/html/render/print_item.rs

alternate interface for OrderedJson to reduce allocations inspired by #142380 (comment) r? `@GuillaumeGomez`

bors · 2025-06-22T03:11:47Z

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

GuillaumeGomez · 2025-06-23T14:14:10Z

Fixed merge conflicts.

notriddle · 2025-06-23T16:45:01Z

Either JavaScript-loaded content needs to go on the bottom, or it needs to be loaded in a blocking manner. Otherwise, you get layout shift and potential misclicks.

output.mp4

notriddle

Need a solution to the layout shift problem.

GuillaumeGomez · 2025-06-24T13:31:53Z

Fair enough. I can make the implementors content to be hidden by default (when JS is enabled) and display it when the updated is done.

GuillaumeGomez · 2025-06-24T15:11:03Z

So now, the implementors lists are only displayed once they have been filled by the JS. I still display the headings though. I also opened the hosted example.

Does that sound good to you @notriddle or do you have something else in mind?

notriddle · 2025-06-24T16:14:19Z

How does this work on a page like

rust/tests/rustdoc-gui/trait-with-bounds.goml

Lines 3 to 4 in e76a5eb

    
           // It tries to load a JS for each trait but there are none since they're not implemented. 
        
           fail-on-request-error: false

that has no foreign impls at all?

notriddle · 2025-06-24T16:29:25Z

Does that sound good to you @notriddle or do you have something else in mind?

I wanted to put downstream foreign trait impls on the bottom, so we end up equivalent to impls.sort_by_key(|i| (!i.is_foreign, i.is_negative)). That way main.js and trait.Sync.js don't block us from showing content that's already in the HTML anyway.

But, in order of preference, if I have to pick between longer load time and more layout shift, I'll usually pick longer load time. But I'd rather go with an option that avoids both.

rustbot · 2025-12-10T16:34:27Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

GuillaumeGomez · 2025-12-10T16:35:15Z

Implemented suggestions.

… to show/hide implementors

yotamofek · 2025-12-10T17:08:08Z

Looks really good!

All that's left is my question about the noscript thingy (can maybe be resolved by just explaining to me why I'm wrong 😅),
and maybe some squashing? The last commit can probably just be part of the previous commits. Not a big deal though, it's fine like this too.

GuillaumeGomez · 2025-12-10T19:34:00Z

I'll leave it as is if you don't mind. No energy to squash any additional commit today. ^^'

yotamofek · 2025-12-10T19:52:30Z

@bors r+
Let's not roll this up for the off chance it affects perf

bors · 2025-12-10T19:52:33Z

📌 Commit fb9d3a5 has been approved by yotamofek

It is now in the queue for this repository.

…=yotamofek Put negative implementors first and apply same ordering logic to foreign implementors Fixes rust-lang#51129. This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls. I also used this occasion to make the foreign implementors sort the same as the local ones by using `compare_names`. You can test it [here](https://rustdoc.crud.net/imperio/neg-implementors/core/marker/trait.Sync.html#implementors). r? `@notriddle`

Rollup of 4 pull requests Successful merges: - #142380 (Put negative implementors first and apply same ordering logic to foreign implementors) - #149807 (Use ubuntu:24.04 for the `x86_64-gnu-miri` job) - #149850 (Remove "tidy" tool for `tests/rustdoc` testsuite) - #149867 (only resolve main in bin crates) r? `@ghost` `@rustbot` modify labels: rollup

…=yotamofek Put negative implementors first and apply same ordering logic to foreign implementors Fixes rust-lang#51129. This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls. I also used this occasion to make the foreign implementors sort the same as the local ones by using `compare_names`. You can test it [here](https://rustdoc.crud.net/imperio/neg-implementors/core/marker/trait.Sync.html#implementors). r? ``@notriddle``

…uwer Rollup of 10 pull requests Successful merges: - #142380 (Put negative implementors first and apply same ordering logic to foreign implementors) - #146584 (remove duplicated columns from `rustc_error_code::error_codes!`) - #148717 (Point at span within local macros even when error happens in nested external macro) - #148825 (Add SystemTime::{MIN, MAX}) - #149565 (rustdoc: Add unstable `--merge-doctests=yes/no/auto` flag) - #149770 (Rename some issue-* tests) - #149807 (Use ubuntu:24.04 for the `x86_64-gnu-miri` job) - #149850 (Remove "tidy" tool for `tests/rustdoc` testsuite) - #149863 (Do not suggest moving expression out of for loop when hitting `break` from desugaring) - #149867 (only resolve main in bin crates) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 9 pull requests Successful merges: - #142380 (Put negative implementors first and apply same ordering logic to foreign implementors) - #146584 (remove duplicated columns from `rustc_error_code::error_codes!`) - #148717 (Point at span within local macros even when error happens in nested external macro) - #149565 (rustdoc: Add unstable `--merge-doctests=yes/no/auto` flag) - #149770 (Rename some issue-* tests) - #149807 (Use ubuntu:24.04 for the `x86_64-gnu-miri` job) - #149850 (Remove "tidy" tool for `tests/rustdoc` testsuite) - #149863 (Do not suggest moving expression out of for loop when hitting `break` from desugaring) - #149867 (only resolve main in bin crates) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #142380 - GuillaumeGomez:neg-implementors, r=yotamofek Put negative implementors first and apply same ordering logic to foreign implementors Fixes #51129. This PR changeda surprisingly small amount of things to put negative trait impls before the others: basically just adding a new information in the generated JS file for foreign implementors and a "negative marker" DOM element to know where to insert negative impls. I also used this occasion to make the foreign implementors sort the same as the local ones by using `compare_names`. You can test it [here](https://rustdoc.crud.net/imperio/neg-implementors/core/marker/trait.Sync.html#implementors). r? ```@notriddle```

Rollup of 9 pull requests Successful merges: - rust-lang/rust#142380 (Put negative implementors first and apply same ordering logic to foreign implementors) - rust-lang/rust#146584 (remove duplicated columns from `rustc_error_code::error_codes!`) - rust-lang/rust#148717 (Point at span within local macros even when error happens in nested external macro) - rust-lang/rust#149565 (rustdoc: Add unstable `--merge-doctests=yes/no/auto` flag) - rust-lang/rust#149770 (Rename some issue-* tests) - rust-lang/rust#149807 (Use ubuntu:24.04 for the `x86_64-gnu-miri` job) - rust-lang/rust#149850 (Remove "tidy" tool for `tests/rustdoc` testsuite) - rust-lang/rust#149863 (Do not suggest moving expression out of for loop when hitting `break` from desugaring) - rust-lang/rust#149867 (only resolve main in bin crates) r? `@ghost` `@rustbot` modify labels: rollup

Kobzol · 2025-12-16T08:20:29Z

@GuillaumeGomez Any idea about how to claw back the doc perf? The regression is pretty big.

GuillaumeGomez · 2025-12-16T08:59:27Z

Gonna take a look, I'm surprised the impact is this big.

rustbot assigned notriddle Jun 11, 2025

GuillaumeGomez mentioned this pull request Jun 11, 2025

[rustdoc] Implementors section of Sync (and other similar traits) should separate implementors and !implementors #51129

Closed

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the neg-implementors branch from 27e69ca to 2b0bda6 Compare June 11, 2025 20:37

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the neg-implementors branch from 2b0bda6 to 2dd2db9 Compare June 11, 2025 21:26

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the neg-implementors branch from 2dd2db9 to cc15898 Compare June 12, 2025 12:17

lolbinarycat requested changes Jun 16, 2025

View reviewed changes

GuillaumeGomez force-pushed the neg-implementors branch from cc15898 to 80a06df Compare June 17, 2025 14:27

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the neg-implementors branch from 80a06df to 3028afb Compare June 17, 2025 14:55

lolbinarycat mentioned this pull request Jun 17, 2025

alternate interface for OrderedJson to reduce allocations #142634

Closed

GuillaumeGomez force-pushed the neg-implementors branch from 3028afb to bc014e0 Compare June 23, 2025 13:25

notriddle requested changes Jun 23, 2025

View reviewed changes

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 Jun 23, 2025

GuillaumeGomez added 2 commits December 10, 2025 16:18

Add GUI test to ensure that negative impls are correctly sorted

985178d

Make implementors list visible only when filled

13091dd

GuillaumeGomez force-pushed the neg-implementors branch from 42134ec to 01f8a22 Compare December 10, 2025 16:34

This comment has been minimized.

Sign in to view

Simplify the sorting of impls and use CSS classes instead of plain JS…

fb9d3a5

… to show/hide implementors

GuillaumeGomez force-pushed the neg-implementors branch from 01f8a22 to fb9d3a5 Compare December 10, 2025 16:42

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 10, 2025

jhpratt mentioned this pull request Dec 11, 2025

Rollup of 4 pull requests #149872

Closed

JonathanBrouwer mentioned this pull request Dec 11, 2025

Rollup of 10 pull requests #149890

Closed

matthiaskrgr mentioned this pull request Dec 11, 2025

Rollup of 9 pull requests #149891

Merged

bors merged commit 52cadc0 into rust-lang:main Dec 12, 2025
11 checks passed

rustbot added this to the 1.94.0 milestone Dec 12, 2025

GuillaumeGomez deleted the neg-implementors branch December 12, 2025 09:51

GuillaumeGomez mentioned this pull request Dec 12, 2025

Improve UI/UX for trait implementors async loading #149914

Open

Uh oh!

Put negative implementors first and apply same ordering logic to foreign implementors #142380

Put negative implementors first and apply same ordering logic to foreign implementors #142380

Uh oh!

Conversation

GuillaumeGomez commented Jun 11, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 11, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

lolbinarycat Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Jun 22, 2025

Uh oh!

GuillaumeGomez commented Jun 23, 2025

Uh oh!

notriddle commented Jun 23, 2025

Uh oh!

notriddle left a comment

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jun 24, 2025

Uh oh!

GuillaumeGomez commented Jun 24, 2025

Uh oh!

notriddle commented Jun 24, 2025

Uh oh!

notriddle commented Jun 24, 2025

Uh oh!

rustbot commented Dec 10, 2025

Uh oh!

GuillaumeGomez commented Dec 10, 2025

Uh oh!

This comment has been minimized.

yotamofek commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Dec 10, 2025

Uh oh!

yotamofek commented Dec 10, 2025

Uh oh!

bors commented Dec 10, 2025

Uh oh!

Uh oh!

Kobzol commented Dec 16, 2025

Uh oh!

GuillaumeGomez commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

GuillaumeGomez commented Jun 11, 2025 •

edited by rustbot

Loading

yotamofek commented Dec 10, 2025 •

edited

Loading