Skip to content

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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/librustdoc/formats/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,8 @@ impl Impl {
};
true
}

pub(crate) fn is_negative_trait_impl(&self) -> bool {
self.inner_impl().is_negative_trait_impl()
}
}
43 changes: 40 additions & 3 deletions src/librustdoc/html/render/print_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,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_negative_marker: bool,
}

impl NegativeMarker {
fn new() -> Self {
Self { inserted_negative_marker: false }
}

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>")?;
self.inserted_negative_marker = true;
}
Ok(())
}
}

fn item_trait(cx: &Context<'_>, it: &clean::Item, t: &clean::Trait) -> impl fmt::Display {
fmt::from_fn(|w| {
let tcx = cx.tcx();
Expand Down Expand Up @@ -1069,7 +1090,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>")?;
Expand All @@ -1085,7 +1108,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,
"{}",
Expand Down Expand Up @@ -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!("{}", i.inner_impl().print(false, cx)))
let impl_ = i.inner_impl();
ImplString {
is_negative: impl_.is_negative_trait_impl(),
rendered: format!("{}", impl_.print(false, cx)),
}
}
}

Expand All @@ -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),
}
}
}

Expand Down
45 changes: 41 additions & 4 deletions src/librustdoc/html/render/write_shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
//! or contains "invocation-specific".

use std::cell::RefCell;
use std::cmp::Ordering;
use std::ffi::OsString;
use std::fs::File;
use std::io::{self, Write as _};
Expand Down Expand Up @@ -46,6 +47,7 @@ use crate::formats::Impl;
use crate::formats::item_type::ItemType;
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};
Expand Down Expand Up @@ -695,7 +697,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);
Expand Down Expand Up @@ -748,10 +750,12 @@ impl TraitAliasPart {
{
None
} else {
let impl_ = imp.inner_impl();
Some(Implementor {
text: imp.inner_impl().print(false, cx).to_string(),
text: impl_.print(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(),
})
}
})
Expand All @@ -770,19 +774,28 @@ 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();

let part = OrderedJson::array_unsorted(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

An issue would be welcome indeed.

implementors
.iter()
.map(OrderedJson::serialize)
.collect::<Result<Vec<_>, _>>()
.unwrap(),
);
path_parts.push(path, OrderedJson::array_unsorted([crate_name_json, &part]));
}
Ok(path_parts)
}
}

#[derive(Eq)]
struct Implementor {
text: String,
synthetic: bool,
types: Vec<String>,
is_negative: bool,
}

impl Serialize for Implementor {
Expand All @@ -792,6 +805,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)?;
Expand All @@ -800,6 +814,29 @@ impl Serialize for Implementor {
}
}

impl PartialEq for Implementor {
fn eq(&self, other: &Self) -> bool {
self.cmp(other) == Ordering::Equal
}
}

impl PartialOrd for Implementor {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(Ord::cmp(self, other))
}
}

impl Ord for Implementor {
fn cmp(&self, other: &Self) -> Ordering {
// We sort negative impls first.
match (self.is_negative, other.is_negative) {
(false, true) => Ordering::Greater,
(true, false) => Ordering::Less,
_ => compare_names(&self.text, &other.text),
}
}
}

/// Collect the list of aliased types and their aliases.
/// <https://github.com/search?q=repo%3Arust-lang%2Frust+[RUSTDOCIMPL]+type.impl&type=code>
///
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/html/render/write_shared/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,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 {
Expand All @@ -113,7 +113,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 {
Expand All @@ -125,7 +125,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 {
Expand Down
4 changes: 4 additions & 0 deletions src/librustdoc/html/static/css/noscript.css
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ nav.sub {
display: none;
}

#synthetic-implementors-list, #implementors-list {
display: block;
}

/* Begin: styles for themes
Keep the default light and dark themes synchronized with the ones
in rustdoc.css */
Expand Down
9 changes: 9 additions & 0 deletions src/librustdoc/html/static/css/rustdoc.css
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,12 @@ div.where {
margin-left: calc(var(--docblock-indent) + var(--impl-items-indent));
}

#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;
}

.item-info code {
font-size: 0.875rem;
}
Expand Down Expand Up @@ -2776,6 +2782,9 @@ in src-script.js and main.js
{
margin-bottom: 0.75em;
}
.negative-marker {
display: none;
}

.variants > .docblock,
.implementors-toggle > .docblock,
Expand Down
47 changes: 37 additions & 10 deletions src/librustdoc/html/static/js/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,21 +674,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;
Expand All @@ -701,7 +714,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 + "-";
Expand All @@ -723,7 +736,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.
Expand Down Expand Up @@ -758,11 +771,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].style.display = "block";
}
if (syntheticImplementors[0]) {
syntheticImplementors[0].style.display = "block";
}
};
if (window.pending_implementors) {
window.register_implementors(window.pending_implementors);
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/static/js/rustdoc.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ declare namespace rustdoc {
* Provied 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 = {
Expand Down
Loading
Loading