Put negative implementors first and apply same ordering logic to foreign implementors #142380
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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, | ||||||
| } | ||||||
lolbinarycat marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| 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>")?; | ||||||
|
There was a problem hiding this comment.
Suggested change
Using a There was a problem hiding this comment. Based on the docs/spec I read, it's exactly what
As for linebreaks, I couldn't find information about how There was a problem hiding this comment.
span is essentially the equivalent of |
||||||
| 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(); | ||||||
|
|
@@ -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>")?; | ||||||
|
|
@@ -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, | ||||||
| "{}", | ||||||
|
|
@@ -2302,11 +2327,18 @@ where | |||||
| } | ||||||
|
|
||||||
| #[derive(PartialEq, Eq)] | ||||||
| struct ImplString { | ||||||
| rendered: String, | ||||||
| is_negative: bool, | ||||||
| } | ||||||
|
|
||||||
| impl ImplString { | ||||||
| fn new(i: &Impl, cx: &Context<'_>) -> ImplString { | ||||||
| let impl_ = i.inner_impl(); | ||||||
| ImplString { | ||||||
| is_negative: impl_.is_negative_trait_impl(), | ||||||
| rendered: format!("{}", impl_.print(false, cx)), | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -2318,7 +2350,12 @@ impl PartialOrd for ImplString { | |||||
|
|
||||||
| impl Ord for ImplString { | ||||||
| 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, | ||||||
lolbinarycat marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| _ => compare_names(&self.rendered, &other.rendered), | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 _}; | ||
|
|
@@ -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}; | ||
|
|
@@ -703,7 +705,7 @@ impl TraitAliasPart { | |
| fn blank() -> SortedTemplate<<Self as CciPart>::FileFormat> { | ||
| SortedTemplate::from_before_after( | ||
| r"(function() { | ||
| const implementors = Object.fromEntries([", | ||
| r"]); | ||
| if (window.register_implementors) { | ||
| window.register_implementors(implementors); | ||
|
|
@@ -741,7 +743,7 @@ impl TraitAliasPart { | |
| }, | ||
| }; | ||
|
|
||
| let mut implementors = imps | ||
| .iter() | ||
| .filter_map(|imp| { | ||
| // If the trait and implementation are in the same crate, then | ||
|
|
@@ -756,10 +758,12 @@ impl TraitAliasPart { | |
| { | ||
| None | ||
| } else { | ||
| let impl_ = imp.inner_impl(); | ||
| Some(Implementor { | ||
| 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(), | ||
| }) | ||
| } | ||
| }) | ||
|
|
@@ -778,7 +782,9 @@ impl TraitAliasPart { | |
| } | ||
| path.push(format!("{remote_item_type}.{}.js", remote_path[remote_path.len() - 1])); | ||
|
|
||
| implementors.sort_unstable(); | ||
|
|
||
| let part = OrderedJson::array_unsorted( | ||
|
There was a problem hiding this comment. This is definitely an improvement, as it eliminates an allocation, but we're still allocating an intermediate string for each element.
I can open an issue for improving the There was a problem hiding this comment. An issue would be welcome indeed. |
||
| implementors | ||
| .iter() | ||
| .map(OrderedJson::serialize) | ||
|
|
@@ -791,10 +797,12 @@ impl TraitAliasPart { | |
| } | ||
| } | ||
|
|
||
| #[derive(PartialEq, Eq)] | ||
|
There was a problem hiding this comment. logic error: this can easily be fixed by writing a impl PartialEq for Implementor {
fn eq(&self, other: &Self) -> bool {
self.cmp(other) == Ordering::Equal
}
}There was a problem hiding this comment. Interesting approach, I like it. Adding it. There was a problem hiding this comment. Wait, in which would it be an issue to have this difference? Ordering and equality don't have to be equivalent. There was a problem hiding this comment. it's a correctness requirement on PartialEq. it doesn't cause any issue with the current code, bur it could cause issues with generic datastructures like BTreeMap, so I think it would be best to try to avoid something that can cause this type of sneaky logic error. I think this is technically the same class of error as if you had |
||
| struct Implementor { | ||
| text: String, | ||
| synthetic: bool, | ||
| types: Vec<String>, | ||
| is_negative: bool, | ||
| } | ||
|
|
||
| impl Serialize for Implementor { | ||
|
|
@@ -804,6 +812,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)?; | ||
|
|
@@ -812,6 +821,23 @@ impl Serialize for Implementor { | |
| } | ||
| } | ||
|
|
||
| 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> | ||
| /// | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.