rustdoc: Sort negative impls to the top

[camelid]

Fixes #51129.

[rust-highfive]

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

[camelid]

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

[jyn514]

$ 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;

[the8472]

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.

[GuillaumeGomez]

Then use the same logic used in compare_impl.

[jyn514]

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

[camelid]

Unfortunately it doesn't look this works for some reason...

[camelid]

The impls still aren't sorted :/

[GuillaumeGomez]

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.

[the8472]

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.

[GuillaumeGomez]

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).

[camelid]

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?

[jsha]

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.

[bors]

☔ The latest upstream changes (presumably #80041) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[camelid]

I haven't looked at this in a while. There are several threads of review comments and I feel a bit confused about what the overall approach should be. So, what is the "right" way to sort the impls?

[jsha]

I think what @jyn514 is saying is: If you merge the latest master, you'll need to update compare_impl_polarity to take advantage of the fact that polarity is now just a boolean, not a three-valued item. The natural, intuitive implementation there (sort true above false) will solve the transitivity of equality problem raised above.

I also commented at https://github.com/rust-lang/rust/pull/79453/files/1bfb64bb7cfb2b4b79c7f12417b353fe09119b28#r617221763 discussing why sorting on implementors wasn't taking effect (according to my reading of the code, at least; I haven't run it).

[jsha]

I think this is a candidate for closing. As discussed in #85398 and #51129, we need a more significant refactor in how impls are rendered before we can do this correctly.