Sort negative impls before positive ones.

[jsha]

Fixes #51129

[rust-highfive]

r? @jyn514

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

[jsha]

@bors r? @camelid

[jsha]

Hm, actually, this doesn't quite work but I'm not sure why. See demo at https://hoffman-andrews.com/rust/sort-neg/std/marker/trait.Sync.html#implementors.

This moves many !Sync impls towards the top, like *const T. But there are still some out of place like:

impl<T: ?Sized> !Sync for Rc<T>
impl<T: ?Sized> !Sync for Weak<T>
impl !Sync for TokenStream
impl !Sync for LexError
impl !Sync for Span

[jyn514]

@jsha you're only sorting foreign impls, not local ones

[jsha]

Good point. I've fixed that, but the results are the same (I've updated the demo).

Also my understanding is that "foreign" means "in a different crate from the one we're looking at." But looking at the Sync page, there are only 4 "implementations on foreign types:" Alignment, Argument, Count, FormatSpec. But there are a number of other implementations on other crates. For instance, TokenStream and friends are from the proc_macro crate. And there are two !Sync for Rc<T> lines. One, for std::rc::Rc, is sorted correctly. The other, for alloc::rc::Rc, is sorted lower than it should be.

[jyn514]

Hmm, that sounds related to re-exports, both of those are the same item. I don't have time to look into it right now.

[jsha]

I think you're right. That seems related to #85418 - the re-exported implementors seem to be rendered via a completely different code path. Given that, I think it's reasonable to land this PR without waiting for a fix on #85418, since the two probably won't collide, and this gets us a step closer to solving #51129.

[jsha]

An update about the things that aren't affected by this sorting, from #85418: Implementors from outside the crate are put into a .js file and rendered into the document at load time by JS. That's why they're rendered differently (the code paths have diverged). It also means it will be hard to make them part of the same sort order as these, unless we move sorting into JS or do something else kinda complicated. I still think it makes sense to track those fixes at #85418 and land this as an incremental improvement.

[camelid]

Could you change this to use a pattern match instead? I think it would be easier to understand the logic and reduce bugs. Something like this:

match (lhs.inner_impl().negative_polarity, rhs.inner_impl().negative_polarity) {
    (true, false) => return Ordering::Less,
    // ...
}

[camelid]

The case where the polarities were the same would fall through via => {}.

[camelid]

Also, I see a bug in the demo output: there are a bunch of negative impls, then some positive ones (as expected), but then some more negative impls at the end. Is that bug present in the latest version of the code, or is the demo just not up-to-date?

EDIT: Oh, is that what #85398 (comment) is referring to? It does seem a little confusing that the new order is inconsistent (negative impls first, but only if they're local, and then you get surprise negative impls at the end).

[camelid]

This doesn't seem to be have been in the original issue, but I feel like it might make sense to sort trait implementations (i.e., the trait impls shown on a type's page) as well so as to be more consistent. E.g., https://hoffman-andrews.com/rust/sort-neg/std/ptr/struct.NonNull.html#trait-implementations has its negative impls of Send and Sync after all the other impls.

[jsha]

Also, I see a bug in the demo output: there are a bunch of negative impls, then some positive ones (as expected), but then some more negative impls at the end. Is that bug present in the latest version of the code, or is the demo just not up-to-date?
EDIT: Oh, is that what #85398 (comment) is referring to? It does seem a little confusing that the new order is inconsistent (negative impls first, but only if they're local, and then you get surprise negative impls at the end).

Yep, exactly. I talk about some possible fixes over in #85418, mainly moving all the implementors into JS-loaded data, and sorting in JS (not stoked about this solution), or having a separate heading for out-of-crate implementors, so its clear they aren't sorted along with in-crate implementors (also not stoked about this).

Note that we already have this inconsistency with regards to the current sorting: implementors are sorted alphabetically with regards to in-crate implementors, but then all out-of-crate implementors come after them.

[camelid]

Note that we already have this inconsistency with regards to the current sorting: implementors are sorted alphabetically with regards to in-crate implementors, but then all out-of-crate implementors come after them.

That's true, but the one thing I'm thinking is that alphabetical ordering has no inherent meaning, whereas sorting by polarity does. E.g., someone might want to find all the negative impls, but they likely don't want to find all the impls starting with the letter 'A'. So, if only local negative impls are first, then it might confuse people that there are actually more negative impls, just out-of-crate ones, lower down.

I'm wondering what others think—is it confusing to have only local impls sorted by polarity and to have only local negative impls at the top? cc @rust-lang/rustdoc

[jyn514]

I agree, I don't think "defined in crate" vs. "defined out of crate" is very useful for users.

[jsha]

I agree, I don't think "defined in crate" vs. "defined out of crate" is very useful for users.

Fair. So am I understanding you to say: unless we can move the sorting by polarity into JavaScript (and thus sort both local and out-of-crate impls together), we shouldn't do this?

And to be clear, I assume you're saying you don't like this proposal:

having a separate heading for out-of-crate implementors, so its clear they aren't sorted along with in-crate implementors (also not stoked about this).

[jyn514]

unless we can move the sorting by polarity into JavaScript (and thus sort both local and out-of-crate impls together), we shouldn't do this?

Unless we can unify the sorting somehow; you said earlier that this isn't possible to do wholly in Rust but tbh I didn't read it very carefully 😅

I am not a fan of the second proposal. I don't think we should do it solely because we want search by polarity; if people think it's a good idea independent of this PR that's a different story.

[Manishearth]

I'm wondering what others think—is it confusing to have only local impls sorted by polarity and to have only local negative impls at the top?

I think it's fine but I would prefer the bigger fix.

I think alphabetical is not very useful; sorting by where they were defined can be more useful (same module, different module, different crate) in general. So I don't think it's that bad if things are sorted by locality and then polarity.

[GuillaumeGomez]

There are much less negative impls so I think showing them first could be an improvement.

[jsha]

Unless we can unify the sorting somehow; you said earlier that this isn't possible to do wholly in Rust but tbh I didn't read it very carefully

The short version is: the in-crate implementations are generated by one run of rustdoc. The out-of-crate implementations are generated by subsequent runs. Those subsequent runs write implementation data into adjacent crates in a JS (JSON-ish) file because that's structured and easy to modify safely post-hoc.

If we wanted to do this entirely in Rust, we would have to know about all other crates that are going to be documented into a given directory, which breaks a supported feature of rustdoc (multiple runs generating into the same directory).

[camelid]

If we wanted to do this entirely in Rust, we would have to know about all other crates that are going to be documented into a given directory, which breaks a supported feature of rustdoc (multiple runs generating into the same directory).

What if each subsequent run of rustdoc deserialized and then sorted that file?

[jsha]

What if each subsequent run of rustdoc deserialized and then sorted that file?

That would sort the JS content, but not the generated HTML content. We would still need to sort the contents of that file in with the contents of the .html file at load time.

That's the crux of the issue - we have a section that is currently generated half as HTML and half as JSON that gets rendered into HTML at load time. We will presumably continue to get inconsistent results unless we switch the entire section over to JSON.

[jsha]

I'm closing this out for now. Fixing this properly will require a refactor (described above), which I won't have time for for a little while.