Skip to content
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.

Sign up for GitHub

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

rustdoc: Sort negative impls to the top #79453

Closed
wants to merge 1 commit into from
Closed

rustdoc: Sort negative impls to the top #79453

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 17 additions & 1 deletion src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ use rustc_span::symbol::{sym, Symbol};
use serde::ser::SerializeSeq;
use serde::{Serialize, Serializer};

use crate::clean::{self, AttributesExt, Deprecation, GetDefId, RenderedLink, SelfTy, TypeKind};
use crate::clean::{
self, AttributesExt, Deprecation, GetDefId, ImplPolarity, RenderedLink, SelfTy, TypeKind,
};
use crate::config::{RenderInfo, RenderOptions};
use crate::docfs::{DocFS, PathError};
use crate::doctree;
Expand Down Expand Up @@ -2532,6 +2534,16 @@ fn compare_impl<'a, 'b>(lhs: &'a &&Impl, rhs: &'b &&Impl) -> Ordering {
compare_names(&lhs, &rhs)
}

fn compare_impl_polarity(a: &Impl, b: &Impl) -> Ordering {
Copy link
Member

Choose a reason for hiding this comment

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

You only take into account the polarity and not the name for the sorting, which isn't great... In case both impls have the same polarity, please return the comparison of the traits' name.

Copy link
Member

@the8472 the8472 Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that stable sorts are composable. If you first sort by property A and then by property B then the ordering for A will effectively serve as tie-breakers where B-comparisons return Ordering::Equal.

match (a.inner_impl().polarity.as_ref(), b.inner_impl().polarity.as_ref()) {
(Some(ImplPolarity::Positive), Some(ImplPolarity::Negative)) => Ordering::Greater,
(Some(ImplPolarity::Negative), Some(ImplPolarity::Positive)) => Ordering::Less,
(Some(ImplPolarity::Positive), Some(ImplPolarity::Positive))
| (Some(ImplPolarity::Negative), Some(ImplPolarity::Negative)) => Ordering::Equal,
(None, _) | (_, None) => Ordering::Equal,
Copy link
Member Author

Choose a reason for hiding this comment

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

What does it mean if the polarity is None? Is this the correct way to handle that case?

Copy link
Member

Choose a reason for hiding this comment

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

$ rg polarity.*None src/librustdoc/
src/librustdoc/clean/blanket_impl.rs
136:                        polarity: None,

src/librustdoc/clean/auto_trait.rs
90:                        polarity = None;

I'm not sure what it means, but apparently it only happens for blanket impls and auto traits:

rust/src/librustdoc/clean/blanket_impl.rs

Line 136 in cb56a44

polarity: None,

rust/src/librustdoc/clean/auto_trait.rs

Lines 89 to 90 in cb56a44

AutoTraitResult::PositiveImpl(new_generics) => {
polarity = None;

Copy link
Member

@the8472 the8472 Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this violates the transitivity requirement of sort_by.

Given a = Positive; b = None; c = Negative then

  • a == b
  • b == c
  • a != c

You might want to use sort_by_key and assign an artificial ranking instead.

Copy link
Member

Choose a reason for hiding this comment

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

Then use the same logic used in compare_impl.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be fixed on its own after #80825 - try rebasing.

}
}

fn item_trait(w: &mut Buffer, cx: &Context, it: &clean::Item, t: &clean::Trait, cache: &Cache) {
let bounds = bounds(&t.bounds, false);
let types = t.items.iter().filter(|m| m.is_associated_type()).collect::<Vec<_>>();
Expand Down Expand Up @@ -2718,6 +2730,10 @@ fn item_trait(w: &mut Buffer, cx: &Context, it: &clean::Item, t: &clean::Trait,
}
}

let mut implementors = implementors.clone();
Copy link
Member

Choose a reason for hiding this comment

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

It's kinda useless to sort implementors since it's not used for rendering as is. Look at synthetic and concrete (and update the compare_impl function too like I said above).

Copy link
Member Author

@camelid camelid Nov 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But I wanted to sort foreign types too. Why is it bad to sort implementors? Aren’t synthetic and concrete created it out of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that synthetic and concrete are later re-sorted by compare_impl (lines 2744 and 2745 in this diff, or

rust/src/librustdoc/html/render/print_item.rs

Lines 656 to 667 in 6df26f8

let (local, foreign) = implementors.iter().partition::<Vec<_>, _>(|i| {
i.inner_impl()
.for_
.def_id_full(cx.cache())
.map_or(true, |d| cx.cache.paths.contains_key(&d))
});
let (mut synthetic, mut concrete): (Vec<&&Impl>, Vec<&&Impl>) =
local.iter().partition(|i| i.inner_impl().synthetic);
synthetic.sort_by(|a, b| compare_impl(a, b, cx));
concrete.sort_by(|a, b| compare_impl(a, b, cx));
).

You need a sort function that compares first by polarity, and then by compare_impl. You should sort implementors by that, and then remove the subsequent .sort_by calls on synthetic and concrete so you don't undo the sorting.


implementors.sort_by(compare_impl_polarity);

let (local, foreign) = implementors.iter().partition::<Vec<_>, _>(|i| {
i.inner_impl().for_.def_id().map_or(true, |d| cache.paths.contains_key(&d))
});
Expand Down