Add customized compare for Link in rustdoc

[chenyukang]

Maybe some other types in sidebar need to be sorted in this way, maybe add this crate natord is ok?

r? clubby789

Fixes #137098

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[chenyukang]

how to assert u8 and u32 is in a sorted way ...

[GuillaumeGomez]

We can check if it's the nth item of this type, but not sure we can outdide gui tests...

[chenyukang]

I just added new ui testcase:

tests/rustdoc/sidebar-link-sort.rs and tests/rustdoc/sidebar-link-sort.sidebar.html

[fmease]

r? rustdoc

[GuillaumeGomez]

Do you have a before/after screenshot?

[chenyukang]

Before:

After:

[GuillaumeGomez]

That sadly doesn't enforce really well the sorting order here since a --bless will just update the html file.

[chenyukang]

I didn't get you point, we treat the HTML files as compared files just like stderr file in other unit testing, we can see the names and links in HTML file (tests/rustdoc/sidebar-link-sort.sidebar.html) are sorted as assumed, if anything broke the sorting order, the HTML files will be changed.

[GuillaumeGomez]

I mentioned that it should likely be a gui test (tests/rustdoc-gui) here.

[chenyukang]

added test case in tests/rustdoc-gui.

[GuillaumeGomez]

@notriddle Didn't we do something like this already? I remember something you did on "naturally" sorting items some time ago.

[notriddle]

@GuillaumeGomez Yes, we do. It's called rustdoc::html::render::print_item::compare_names.

@chenyukang Please use that instead of pulling in a second implementation.

[rustbot]

Some changes occurred in GUI tests.

cc @GuillaumeGomez

[chenyukang]

When I run all the tests in tests/rustdoc-gui with command on my dev machine:

./x.py test tests/rustdoc-gui --stage 1

There are some failed with errors, I verified it's not related to my new added test case:

➜  rust git:(yukang-fix-sidebar-sort) ./x.py test tests/rustdoc-gui --stage 1
Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.11s
Building stage0 library artifacts (aarch64-apple-darwin)
    Finished `release` profile [optimized + debuginfo] target(s) in 0.04s
Building compiler artifacts (stage0 -> stage1, aarch64-apple-darwin)
    Finished `release` profile [optimized + debuginfo] target(s) in 0.44s
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
Building stage1 library artifacts (aarch64-apple-darwin)
    Finished `release` profile [optimized + debuginfo] target(s) in 0.05s
Building stage0 tool rustdoc-gui-test (aarch64-apple-darwin)
    Finished `release` profile [optimized + debuginfo] target(s) in 0.17s
Building tool rustdoc (stage0 -> stage1, aarch64-apple-darwin)
   Compiling rustdoc v0.0.0 (/Users/yukang/rust/src/librustdoc)
   Compiling rustdoc-tool v0.0.0 (/Users/yukang/rust/src/tools/rustdoc)
    Finished `release` profile [optimized + debuginfo] target(s) in 16.41s
Testing rustdoc-gui (stage1 -> stage2, aarch64-apple-darwin)
 Documenting settings v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/settings)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.88s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/settings/index.html
    Checking http v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/lib2/http)
 Documenting http v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/lib2/http)
    Checking implementors v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/lib2/implementors)
 Documenting implementors v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/lib2/implementors)
 Documenting lib2 v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/lib2)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.39s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/lib2/index.html
 Documenting link_to_definition v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/link_to_definition)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.44s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/link_to_definition/index.html
 Documenting huge_logo v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/huge_logo)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.43s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/huge_logo/index.html
    Checking scrape_examples v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/scrape_examples)
    Scraping scrape_examples v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/scrape_examples)
 Documenting scrape_examples v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/scrape_examples)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.33s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/scrape_examples/index.html
 Documenting proc_macro_test v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/proc_macro_test)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.62s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/proc_macro_test/index.html
 Documenting staged_api v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/staged_api)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.44s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/staged_api/index.html
 Documenting extend_css v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/extend_css)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.53s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/extend_css/index.html
 Documenting theme_css v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/theme_css)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.42s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/theme_css/index.html
   Compiling test_docs v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/test_docs)
 Documenting test_docs v0.1.0 (/Users/yukang/rust/tests/rustdoc-gui/src/test_docs)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.92s
   Generated /Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/test_docs/index.html
Running 135 rustdoc-gui (10 concurrently) ...
.......... (10/135)
.......... (20/135)
.......... (30/135)
.......... (40/135)
.......... (50/135)
.......... (60/135)
.......... (70/135)
.......... (80/135)
.......... (90/135)
.......... (100/135)
.......... (110/135)
.......... (120/135)
.......... (130/135)
FFFFF      (135/135)


/Users/yukang/rust/tests/rustdoc-gui/item-info.goml item-info... FAILED
[ERROR] `tests/rustdoc-gui/item-info.goml` line 23: Condition `|first_line_y| != |second_line_y| && |first_line_y| == 718 && |second_line_y| == 741` (`718 != 742 && 718 == 718 && 742 == 741`) was evaluated as false: for command `assert: |first_line_y| != |second_line_y| && |first_line_y| == 718 && |second_line_y| == 741`

/Users/yukang/rust/tests/rustdoc-gui/search-filter.goml search-filter... FAILED
[ERROR] `tests/rustdoc-gui/search-filter.goml` line 24: The following errors happened: [document property `URL` (`file:///Users/yukang/rust/build/aarch64-apple-darwin/test/rustdoc-gui/doc/test_docs/index.html?search=test`) doesn't contain `&filter-crate=` (for CONTAINS check)]: for command `assert-document-property: ({"URL": "&filter-crate="}, CONTAINS)`
[ERROR] `tests/rustdoc-gui/search-filter.goml` line 26: assert didn't fail: for command `assert-false: "#results .externcrate"`
[ERROR] `tests/rustdoc-gui/search-filter.goml` line 28: The following errors happened (for selector `#crate-search`): [expected `lib2` for property `"value"`, found `all crates`]: for command `assert-property: ("#crate-search", {"value": "lib2"})`
[ERROR] `tests/rustdoc-gui/search-filter.goml` line 37: assert didn't fail: for command `assert-false: "#results .externcrate"`
[ERROR] `tests/rustdoc-gui/search-filter.goml` line 38: The following errors happened (for selector `#crate-search`): [expected `lib2` for property `"value"`, found `all crates`]: for command `assert-property: ("#crate-search", {"value": "lib2"})`

/Users/yukang/rust/tests/rustdoc-gui/search-result-display.goml search-result-display... FAILED
[WARNING] `tests/rustdoc-gui/search-result-display.goml` line 37: Delta is 0 for "x", maybe try to use `compare-elements-position` instead?

[ERROR] `tests/rustdoc-gui/search-result-display.goml` line 53: The following errors happened (for selector `#crate-search`): [expected `159px` for key `width`, found `158.5px`]: for command `assert-css: ("#crate-search", {"width": "159px"})`

/Users/yukang/rust/tests/rustdoc-gui/sidebar-mobile.goml sidebar-mobile... FAILED
[ERROR] `tests/rustdoc-gui/sidebar-mobile.goml` line 52: The following errors happened: [different Y values: 45 (or 45) != 46]: for command `assert-position: ("#method\.must_use", {"y": 46})`

/Users/yukang/rust/tests/rustdoc-gui/sidebar-source-code-display.goml sidebar-source-code-display... FAILED
[ERROR] `tests/rustdoc-gui/sidebar-source-code-display.goml` line 145: The following errors happened: [expected `2578` for window property `pageYOffset`, found `2578.5`]: for command `assert-window-property: {"pageYOffset": |y_offset|}`
[ERROR] `tests/rustdoc-gui/sidebar-source-code-display.goml` line 152: The following errors happened: [expected `2578` for window property `pageYOffset`, found `2578.5`]: for command `assert-window-property: {"pageYOffset": |y_offset|}`

Error: ()
Build completed unsuccessfully in 0:01:26

Is it because of my own browser settings, I don't know.

[notriddle]

I still don't like this particular implementation. There's two problems with it:

1. There's no actual rule against having two sidebar items with the same name. I'd like to make sure they get sorted stably.
2. As you mentioned, this only applies to foreign impls. It would be better to sort everything (except Sections, but those are already done correctly).

Instead, we can actually change the PartialOrd implementation for Link, and, that way, continue sorting everything the same way except for names:

diff --git a/src/librustdoc/html/render/sidebar.rs b/src/librustdoc/html/render/sidebar.rs
index 2675eb3025a..7181751ae26 100644
--- a/src/librustdoc/html/render/sidebar.rs
+++ b/src/librustdoc/html/render/sidebar.rs
@@ -1,4 +1,5 @@
 use std::borrow::Cow;
+use std::cmp::{Ordering, PartialOrd};
 
 use rinja::Template;
 use rustc_data_structures::fx::FxHashSet;
@@ -78,7 +79,7 @@ pub fn should_render(&self) -> bool {
 }
 
 /// A link to an item. Content should not be escaped.
-#[derive(PartialOrd, Ord, PartialEq, Eq, Hash, Clone)]
+#[derive(Ord, PartialEq, Eq, Hash, Clone)]
 pub(crate) struct Link<'a> {
     /// The content for the anchor tag and title attr
     name: Cow<'a, str>,
@@ -90,6 +91,20 @@ pub(crate) struct Link<'a> {
     children: Vec<Link<'a>>,
 }
 
+impl PartialOrd for Link<'_> {
+    fn partial_cmp(&self, other: &Link<'_>) -> Option<Ordering> {
+        match compare_names(&self.name, &other.name) {
+            Ordering::Equal => (),
+            result => return Some(result),
+        }
+        (&self.name_html, &self.href, &self.children).partial_cmp(&(
+            &other.name_html,
+            &other.href,
+            &other.children,
+        ))
+    }
+}
+
 impl<'a> Link<'a> {
     pub fn new(href: impl Into<Cow<'a, str>>, name: impl Into<Cow<'a, str>>) -> Self {
         Self { href: href.into(), name: name.into(), children: vec![], name_html: None }
@@ -298,7 +313,7 @@ fn filter_items<'a>(
                 .filter_map(|i| super::extract_for_impl_name(&i.impl_item, cx))
                 .map(|(name, id)| Link::new(id, name)),
         );
-        foreign_impls.sort_by(|a, b| compare_names(&a.name, &b.name));
+        foreign_impls.sort();
     }
 
     blocks.extend(

[notriddle]

Is it because of my own browser settings, I don't know.

Yes, it's true. I've tried to run the rustdoc-gui tests on Mac and get similar errors. The gui test here passes when I run it on Linux.

[notriddle]

@bors r+ rollup

[bors]

📌 Commit a467eca has been approved by notriddle

It is now in the queue for this repository.