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.

lolbinarycat · 2025-07-02T21:53:13Z

Well, my point was mostly: we have UI saying to the user "generating more content". But in some cases, there might no be. Which then seems like there is a bug (at least it would to me) since something was being loaded and nothing changed.

But if we are hiding all the impls, then there would almost always be something that shows up no?

GuillaumeGomez · 2025-07-03T13:10:36Z

The section are always present, whether they're empty or not. Currently, content appear (almost) instantly. If we add a visual marker that content is loading and once done, it just disappears because there was nothing to add (ie, no foreign implementors), then it would seem like something failed.

lolbinarycat · 2025-07-03T16:37:38Z

The section are always present, whether they're empty or not. Currently, content appear (almost) instantly. If we add a visual marker that content is loading and once done, it just disappears because there was nothing to add (ie, no foreign implementors), then it would seem like something failed.

It seems like you still don't understand what I'm saying.

As stated before, one way to prevent layout shifting is to hide all implementations until the foreign impls are loaded. IF we do that, we should also add a loading icon, otherwise every page would initially look like they have no impls.

If a loading indicator into nothing is too confusing, we could add a simple bit of text saying "no implementors" when none are found. (nojs users could even be shown "no local implementors" if we want to be technically correct).

GuillaumeGomez · 2025-07-03T16:42:17Z

I got what you said. The current implementation of this PR hides all implementations until foreign ones are loaded. You suggest to add a visual indicator that it is loading and I answered to that.

I don't think adding text to indicate the section is empty is very useful either.

lolbinarycat · 2025-07-03T16:45:23Z

If we add a visual marker that content is loading and once done, it just disappears because there was nothing to add (ie, no foreign implementors), then it would seem like something failed.

You're saying that the indicator would disappear with no changes if there are no foreign implementors, but if we are hiding all implementors, that is incorrect. Such a situation would only happen if there are no implementors at all.

GuillaumeGomez · 2025-07-03T16:46:10Z

Yep that's my point.

lolbinarycat · 2025-07-03T16:50:04Z

I think that "not done loading" and "no results" being indistinguishable is bad ui design, and is likely to cause frustration and/or confusion if the loading time is ever significant.

bors · 2025-09-27T19:26:42Z

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

GuillaumeGomez · 2025-10-24T09:22:39Z

So back to this: what do you think about going forward with the current implementation as it improves the current situation and discuss further improvements later on?

bors · 2025-11-09T05:52:17Z

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

yotamofek · 2025-12-09T19:59:04Z

src/librustdoc/html/render/print_item.rs


+/// Struct used to handle insertion of "negative impl" marker in the generated DOM.
+///
+/// This marker appears once in all trait impl lists to divide negative impls from positive impls.


nit:

Suggested change

/// This marker appears once in all trait impl lists to divide negative impls from positive impls.

/// This marker may appear at most once in every trait impl lists to divide negative impls from positive impls.

It might not appear, if item has only negative impls, I think?
And "in all trailt impl lists" makes it sound like it's only gonna appear once on a single HTML page.

Nah, we put it all all the time so JS knows where the separation is.

yotamofek · 2025-12-09T19:59:50Z

src/librustdoc/html/render/print_item.rs

+///
+/// This marker appears once in all trait impl lists to divide negative impls from positive impls.
+struct NegativeMarker {
+    inserted_negative_marker: bool,


nit:

Suggested change

inserted_negative_marker: bool,

inserted: bool,

no need to repeat what's being inserted :)

yotamofek · 2025-12-09T20:00:53Z

src/librustdoc/html/render/print_item.rs

+
+    fn insert_if_needed(&mut self, w: &mut fmt::Formatter<'_>, implementor: &Impl) -> fmt::Result {
+        if !self.inserted_negative_marker && !implementor.is_negative_trait_impl() {
+            write!(w, "<div class=\"negative-marker\"></div>")?;


Suggested change

write!(w, "<div class=\"negative-marker\"></div>")?;

w.write_str("<div class=\"negative-marker\"></div>")?;

yotamofek · 2025-12-09T20:23:33Z

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

+#synthetic-implementors-list, #implementors-list {
+	/* To prevent layout shift when loading the page with extra implementors being loaded
+	   from JS, we hide the list until it's complete. */
+	display: none;
+}


I think it would be nicer to not use el.style.display = 'block' to override this, but rather use classes somehow.

Maybe:

Suggested change

#synthetic-implementors-list, #implementors-list {

/* To prevent layout shift when loading the page with extra implementors being loaded

from JS, we hide the list until it's complete. */

display: none;

}

#synthetic-implementors-list:not(.loaded), #implementors-list:not(.loaded) {

/* To prevent layout shift when loading the page with extra implementors being loaded

from JS, we hide the list until it's complete. */

display: none;

}

and then the "reset" to display: block in noscript.css can be removed (IIUC), and the JS will be something like:

if (implementors[0]) { implementors[0].classList.add('loaded'); } if (syntheticImplementors[0]) { syntheticImplementors[0].classList.add('loaded'); }

Your call, but I think it's slightly nicer this way. Also means we don't have to get the default value for display right :)

Maybe we can even add a loaded class on some ancestor element, and then do something like:

.some-parent:not(.loaded) #synthetic-implementors-list, .some-parent:not(.loaded) #implementors-list { ... }

it might come in handy for other "loaded" states we might add later on

I'd rather keep the "loaded" class close to the content which is actually loaded because I imagine we could have other async data load and I'd rather not have to handle that in the JS (checking if all data is loaded).

Just to precise: I think the loaded class is a good idea. :)

Also: we need to keep the noscript part because it will not have the .loaded class (thank you past-me for adding a test for this case T_T).

Also: we need to keep the noscript part because it will not have the .loaded class (thank you past-me for adding a test for this case T_T).

That's confusing.
Without the .loaded class, the display should default to block.
With noscript, there would be no JS to add that .loaded class in the first place, so I don't understand why we would need to override the display: none with display: block.
I'm probably missing something.

i.e., this part of the noscript stylesheet is an "oxymoron":
https://github.com/rust-lang/rust/pull/142380/files#diff-6298483d735c3524d041be8e87a755ab53e75cf10dee016939929d3a2e620af5R32-R34

I don't see how it will ever be applicable, noscript and .loaded are two things that should never co-exist.

NVM
understood it now
was completely confused there for a second
haha
sorry!

mind blown 😆

Took me a few seconds as well to understand what was going on when test failed because it seemed logical that noscript wasn't needed... until I realized. I feel less alone in this "brain misery" now haha

yotamofek · 2025-12-09T20:46:55Z

src/librustdoc/html/render/write_shared.rs

    }
 }

+impl PartialEq for Implementor {


I think it's not great, at all, that the Eq and PartialEq impls don't behave the same.

I understand that all these impls are just so you're able to sort the implementors, but maybe it would be better to just use sort_unstable_by. Will also be less boilerplate-y, but most importantly more correct. IMHO, at least.

What do you think?

Yeah you're right. It was bugging me even when I wrote it so I'm glad it's a shared feeling. Using sort_unstable_by instead.

(it also allowed to remove a lot of code, so big 👍 for the idea)

…ign implementors

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`

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

Open

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 force-pushed the neg-implementors branch from e76a5eb to 42134ec Compare October 24, 2025 09:21

This comment has been minimized.

Sign in to view

GuillaumeGomez removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 8, 2025

yotamofek reviewed Dec 9, 2025

View reviewed changes

GuillaumeGomez added 3 commits December 10, 2025 16:18

Put negative implementors first and apply same ordering logic to fore…

4509163

…ign implementors

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

Open

	/// This marker appears once in all trait impl lists to divide negative impls from positive impls.
	/// This marker may appear at most once in every trait impl lists to divide negative impls from positive impls.

	write!(w, "<div class=\"negative-marker\"></div>")?;
	w.write_str("<div class=\"negative-marker\"></div>")?;

Uh oh!

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

Are you sure you want to change the base?

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

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!

Choose a reason for hiding this comment

Uh oh!

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!

lolbinarycat commented Jul 2, 2025

Uh oh!

GuillaumeGomez commented Jul 3, 2025

Uh oh!

lolbinarycat commented Jul 3, 2025

Uh oh!

GuillaumeGomez commented Jul 3, 2025

Uh oh!

lolbinarycat commented Jul 3, 2025

Uh oh!

GuillaumeGomez commented Jul 3, 2025

Uh oh!

lolbinarycat commented Jul 3, 2025

Uh oh!

bors commented Sep 27, 2025

Uh oh!

This comment has been minimized.

GuillaumeGomez commented Oct 24, 2025

Uh oh!

bors commented Nov 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jun 11, 2025 •

edited by rustbot

Loading

yotamofek commented Dec 10, 2025 •

edited

Loading