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```

Author: matthiaskrgr

src/librustdoc/formats/mod.rs

@@ -76,4 +76,8 @@ impl Impl {
         };
         true
     }
+
+    pub(crate) fn is_negative_trait_impl(&self) -> bool {
+        self.inner_impl().is_negative_trait_impl()
+    }
 }

src/librustdoc/html/render/print_item.rs

@@ -646,6 +646,27 @@ fn item_function(cx: &Context<'_>, it: &clean::Item, f: &clean::Function) -> imp
     })
 }
 
+/// 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.
+struct NegativeMarker {
+    inserted: bool,
+}
+
+impl NegativeMarker {
+    fn new() -> Self {
+        Self { inserted: false }
+    }
+
+    fn insert_if_needed(&mut self, w: &mut fmt::Formatter<'_>, implementor: &Impl) -> fmt::Result {
+        if !self.inserted && !implementor.is_negative_trait_impl() {
+            w.write_str("<div class=\"negative-marker\"></div>")?;
+            self.inserted = true;
+        }
+        Ok(())
+    }
+}
+
 fn item_trait(cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) -> impl fmt::Display {
     fmt::from_fn(|w| {
         let tcx = cx.tcx();
@@ -1072,7 +1093,9 @@ fn item_trait(cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) -> impl fmt:
                     "<div id=\"implementors-list\">",
                 )
             )?;
+            let mut negative_marker = NegativeMarker::new();
             for implementor in concrete {
+                negative_marker.insert_if_needed(w, implementor)?;
                 write!(w, "{}", render_implementor(cx, implementor, it, &implementor_dups, &[]))?;
             }
             w.write_str("</div>")?;
@@ -1088,7 +1111,9 @@ fn item_trait(cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) -> impl fmt:
                         "<div id=\"synthetic-implementors-list\">",
                     )
                 )?;
+                let mut negative_marker = NegativeMarker::new();
                 for implementor in synthetic {
+                    negative_marker.insert_if_needed(w, implementor)?;
                     write!(
                         w,
                         "{}",
@@ -2302,11 +2327,18 @@ where
 }
 
 #[derive(PartialEq, Eq)]
-struct ImplString(String);
+struct ImplString {
+    rendered: String,
+    is_negative: bool,
+}
 
 impl ImplString {
     fn new(i: &Impl, cx: &Context<'_>) -> ImplString {
-        ImplString(format!("{}", print_impl(i.inner_impl(), false, cx)))
+        let impl_ = i.inner_impl();
+        ImplString {
+            is_negative: impl_.is_negative_trait_impl(),
+            rendered: format!("{}", print_impl(impl_, false, cx)),
+        }
     }
 }
 
@@ -2318,7 +2350,12 @@ impl PartialOrd for ImplString {
 
 impl Ord for ImplString {
     fn cmp(&self, other: &Self) -> Ordering {
-        compare_names(&self.0, &other.0)
+        // We sort negative impls first.
+        match (self.is_negative, other.is_negative) {
+            (false, true) => Ordering::Greater,
+            (true, false) => Ordering::Less,
+            _ => compare_names(&self.rendered, &other.rendered),
+        }
     }
 }
 

src/librustdoc/html/render/write_shared.rs

@@ -14,6 +14,7 @@
 //!    or contains "invocation-specific".
 
 use std::cell::RefCell;
+use std::cmp::Ordering;
 use std::ffi::{OsStr, OsString};
 use std::fs::File;
 use std::io::{self, Write as _};
@@ -47,6 +48,7 @@ use crate::formats::item_type::ItemType;
 use crate::html::format::{print_impl, print_path};
 use crate::html::layout;
 use crate::html::render::ordered_json::{EscapedJson, OrderedJson};
+use crate::html::render::print_item::compare_names;
 use crate::html::render::search_index::{SerializedSearchIndex, build_index};
 use crate::html::render::sorted_template::{self, FileFormat, SortedTemplate};
 use crate::html::render::{AssocItemLink, ImplRenderingParameters, StylePath};
@@ -667,7 +669,7 @@ impl TraitAliasPart {
     fn blank() -> SortedTemplate<<Self as CciPart>::FileFormat> {
         SortedTemplate::from_before_after(
             r"(function() {
-    var implementors = Object.fromEntries([",
+    const implementors = Object.fromEntries([",
             r"]);
     if (window.register_implementors) {
         window.register_implementors(implementors);
@@ -720,10 +722,12 @@ impl TraitAliasPart {
                     {
                         None
                     } else {
+                        let impl_ = imp.inner_impl();
                         Some(Implementor {
-                            text: print_impl(imp.inner_impl(), false, cx).to_string(),
+                            text: print_impl(impl_, false, cx).to_string(),
                             synthetic: imp.inner_impl().kind.is_auto(),
                             types: collect_paths_for_type(&imp.inner_impl().for_, cache),
+                            is_negative: impl_.is_negative_trait_impl(),
                         })
                     }
                 })
@@ -742,8 +746,22 @@ impl TraitAliasPart {
             }
             path.push(format!("{remote_item_type}.{}.js", remote_path[remote_path.len() - 1]));
 
-            let part = OrderedJson::array_sorted(
-                implementors.map(|implementor| OrderedJson::serialize(implementor).unwrap()),
+            let mut implementors = implementors.collect::<Vec<_>>();
+            implementors.sort_unstable_by(|a, b| {
+                // We sort negative impls first.
+                match (a.is_negative, b.is_negative) {
+                    (false, true) => Ordering::Greater,
+                    (true, false) => Ordering::Less,
+                    _ => compare_names(&a.text, &b.text),
+                }
+            });
+
+            let part = OrderedJson::array_unsorted(
+                implementors
+                    .iter()
+                    .map(OrderedJson::serialize)
+                    .collect::<Result<Vec<_>, _>>()
+                    .unwrap(),
             );
             path_parts.push(path, OrderedJson::array_unsorted([crate_name_json, &part]));
         }
@@ -755,6 +773,7 @@ struct Implementor {
     text: String,
     synthetic: bool,
     types: Vec<String>,
+    is_negative: bool,
 }
 
 impl Serialize for Implementor {
@@ -764,6 +783,7 @@ impl Serialize for Implementor {
     {
         let mut seq = serializer.serialize_seq(None)?;
         seq.serialize_element(&self.text)?;
+        seq.serialize_element(if self.is_negative { &1 } else { &0 })?;
         if self.synthetic {
             seq.serialize_element(&1)?;
             seq.serialize_element(&self.types)?;

src/librustdoc/html/render/write_shared/tests.rs

@@ -68,7 +68,7 @@ fn trait_alias_template() {
     assert_eq!(
         but_last_line(&template.to_string()),
         r#"(function() {
-    var implementors = Object.fromEntries([]);
+    const implementors = Object.fromEntries([]);
     if (window.register_implementors) {
         window.register_implementors(implementors);
     } else {
@@ -80,7 +80,7 @@ fn trait_alias_template() {
     assert_eq!(
         but_last_line(&template.to_string()),
         r#"(function() {
-    var implementors = Object.fromEntries([["a"]]);
+    const implementors = Object.fromEntries([["a"]]);
     if (window.register_implementors) {
         window.register_implementors(implementors);
     } else {
@@ -92,7 +92,7 @@ fn trait_alias_template() {
     assert_eq!(
         but_last_line(&template.to_string()),
         r#"(function() {
-    var implementors = Object.fromEntries([["a"],["b"]]);
+    const implementors = Object.fromEntries([["a"],["b"]]);
     if (window.register_implementors) {
         window.register_implementors(implementors);
     } else {

src/librustdoc/html/static/css/noscript.css

@@ -29,6 +29,10 @@ nav.sub {
 	display: none;
 }
 
+#synthetic-implementors-list:not(.loaded), #implementors-list:not(.loaded) {
+	display: block;
+}
+
 /* Begin: styles for themes
 	Keep the default light and dark themes synchronized with the ones
 	in rustdoc.css */

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

@@ -1158,6 +1158,12 @@ div.where {
 	margin-left: calc(var(--docblock-indent) + var(--impl-items-indent));
 }
 
+#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;
+}
+
 .item-info code {
 	font-size: 0.875rem;
 }
@@ -2976,6 +2982,9 @@ in src-script.js and main.js
 {
 	margin-bottom: 0.75em;
 }
+.negative-marker {
+	display: none;
+}
 
 .variants > .docblock,
 .implementors-toggle > .docblock,

src/librustdoc/html/static/js/main.js

@@ -800,21 +800,34 @@ function preLoadCss(cssUrl) {
 
     // <https://github.com/search?q=repo%3Arust-lang%2Frust+[RUSTDOCIMPL]+trait.impl&type=code>
     window.register_implementors = imp => {
-        const implementors = document.getElementById("implementors-list");
-        const synthetic_implementors = document.getElementById("synthetic-implementors-list");
+        /** Takes an ID as input and returns a list of two elements. The first element is the DOM
+         * element with the given ID and the second is the "negative marker", meaning the location
+         * between the negative and non-negative impls.
+         *
+         * @param {string} id: ID of the DOM element.
+         *
+         * @return {[HTMLElement|null, HTMLElement|null]}
+         */
+        function implementorsElems(id) {
+            const elem = document.getElementById(id);
+            return [elem, elem ? elem.querySelector(".negative-marker") : null];
+        }
+        const implementors = implementorsElems("implementors-list");
+        const syntheticImplementors = implementorsElems("synthetic-implementors-list");
         const inlined_types = new Set();
 
         const TEXT_IDX = 0;
-        const SYNTHETIC_IDX = 1;
-        const TYPES_IDX = 2;
+        const IS_NEG_IDX = 1;
+        const SYNTHETIC_IDX = 2;
+        const TYPES_IDX = 3;
 
-        if (synthetic_implementors) {
+        if (syntheticImplementors[0]) {
             // This `inlined_types` variable is used to avoid having the same implementation
             // showing up twice. For example "String" in the "Sync" doc page.
             //
             // By the way, this is only used by and useful for traits implemented automatically
             // (like "Send" and "Sync").
-            onEachLazy(synthetic_implementors.getElementsByClassName("impl"), el => {
+            onEachLazy(syntheticImplementors[0].getElementsByClassName("impl"), el => {
                 const aliases = el.getAttribute("data-aliases");
                 if (!aliases) {
                     return;
@@ -827,7 +840,7 @@ function preLoadCss(cssUrl) {
         }
 
         // @ts-expect-error
-        let currentNbImpls = implementors.getElementsByClassName("impl").length;
+        let currentNbImpls = implementors[0].getElementsByClassName("impl").length;
         // @ts-expect-error
         const traitName = document.querySelector(".main-heading h1 > .trait").textContent;
         const baseIdName = "impl-" + traitName + "-";
@@ -849,7 +862,7 @@ function preLoadCss(cssUrl) {
 
             struct_loop:
             for (const struct of structs) {
-                const list = struct[SYNTHETIC_IDX] ? synthetic_implementors : implementors;
+                const list = struct[SYNTHETIC_IDX] ? syntheticImplementors : implementors;
 
                 // The types list is only used for synthetic impls.
                 // If this changes, `main.js` and `write_shared.rs` both need changed.
@@ -884,11 +897,25 @@ function preLoadCss(cssUrl) {
                 addClass(display, "impl");
                 display.appendChild(anchor);
                 display.appendChild(code);
-                // @ts-expect-error
-                list.appendChild(display);
+
+                // If this is a negative implementor, we put it into the right location (just
+                // before the negative impl marker).
+                if (struct[IS_NEG_IDX]) {
+                    // @ts-expect-error
+                    list[1].before(display);
+                } else {
+                    // @ts-expect-error
+                    list[0].appendChild(display);
+                }
                 currentNbImpls += 1;
             }
         }
+        if (implementors[0]) {
+            implementors[0].classList.add("loaded");
+        }
+        if (syntheticImplementors[0]) {
+            syntheticImplementors[0].classList.add("loaded");
+        }
     };
     if (window.pending_implementors) {
         window.register_implementors(window.pending_implementors);

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

@@ -520,7 +520,7 @@ declare namespace rustdoc {
      * Provided by generated `trait.impl` files.
      */
     type Implementors = {
-        [key: string]: Array<[string, number, Array<string>]>
+        [key: string]: Array<[string, 0|1, number, Array<string>]>
     }
 
     type TypeImpls = {

tests/rustdoc-gui/implementors.goml

@@ -1,19 +1,46 @@
 // The goal of this test is to check that the external trait implementors, generated with JS,
 // have the same display than the "local" ones.
 go-to: "file://" + |DOC_PATH| + "/implementors/trait.Whatever.html"
-assert: "#implementors-list"
-// There are supposed to be two implementors listed.
-assert-count: ("#implementors-list .impl", 2)
+wait-for-css: ("#implementors-list", {"display": "block"})
+// There are supposed to be four implementors listed.
+assert-count: ("#implementors-list .impl", 4)
+// There are supposed to be two non-negative implementors.
+assert-count: ("#implementors-list .negative-marker ~ *", 2)
 // Now we check that both implementors have an anchor, an ID and a similar DOM.
-assert: ("#implementors-list .impl:nth-child(1) > a.anchor")
-assert-attribute: ("#implementors-list .impl:nth-child(1)", {"id": "impl-Whatever-for-Struct"})
-assert-attribute: ("#implementors-list .impl:nth-child(1) > a.anchor", {"href": "#impl-Whatever-for-Struct"})
-assert: "#implementors-list .impl:nth-child(1) > .code-header"
+define-function: (
+    "check-dom",
+    [id],
+    block {
+        assert-attribute: (|id| + " > a.anchor", {"href": |id|})
+        assert: |id| + " > .code-header"
+    },
+)
 
-assert: ("#implementors-list .impl:nth-child(2) > a.anchor")
-assert-attribute: ("#implementors-list .impl:nth-child(2)", {"id": "impl-Whatever-1"})
-assert-attribute: ("#implementors-list .impl:nth-child(2) > a.anchor", {"href": "#impl-Whatever-1"})
-assert: "#implementors-list .impl:nth-child(2) > .code-header"
+call-function: ("check-dom", {"id": "#impl-Whatever-for-Struct2"})
+call-function: ("check-dom", {"id": "#impl-Whatever-2"})
+call-function: ("check-dom", {"id": "#impl-Whatever-for-Struct"})
+call-function: ("check-dom", {"id": "#impl-Whatever-3"})
+
+// Ensure that negative impl are sorted first.
+assert-property: (
+    "#implementors-list > *:nth-child(1) > h3",
+    {"textContent": "impl !Whatever for Struct2"},
+)
+assert-property: (
+    "#implementors-list > *:nth-child(2) > h3",
+    {"textContent": "impl !Whatever for StructToImplOnReexport"},
+)
+// Third one is the negative marker.
+assert-attribute: ("#implementors-list > *:nth-child(3)", {"class": "negative-marker"})
+// This one is a `<detail>` so the selector is a bit different.
+assert-property: (
+    "#implementors-list > *:nth-child(4) section > h3",
+    {"textContent": "impl Whatever for Struct"},
+)
+assert-property: (
+    "#implementors-list > *:nth-child(5) > h3",
+    {"textContent": "impl Whatever for Foo"},
+)
 
 go-to: "file://" + |DOC_PATH| + "/test_docs/struct.HasEmptyTraits.html"
 compare-elements-position-near-false: (
@@ -39,3 +66,8 @@ assert-count: ("#implementors-list .impl", 1)
 go-to: "file://" + |DOC_PATH| + "/http/trait.HttpTrait.html"
 assert-count: ("#implementors-list .impl", 1)
 assert-attribute: ("#implementors-list .impl a.trait", {"href": "../http/trait.HttpTrait.html"})
+
+// Now we check that if JS is disabled, the implementors list will be visible.
+javascript: false
+reload:
+assert-css: ("#implementors-list", {"display": "block"})