Vec::dedup_by docs explicit function argument order

[m4dh0rs3]

Clarify and expand Vec::dedup_by and slice::partition_dedup_by docs.

Fixes #157959

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 11 candidates
• Random selection from 6 candidates

[Mark-Simulacrum]

Mostly extra extensions - generally looks OK. Thanks!

View changes since this review

[Mark-Simulacrum]

Suggested change

	/// The element `prev` occurs *before* `current` in the vector (`[.., prev, current, ..]`),
	/// The element `prev` occurs *before* `current` in the vector (`[.., prev, .., current, ..]`),

It's not obvious to me whether / why we make the ordering guarantee, but I think it's probably clearer if we don't imply prev is directly before current? It's definitely not the case if you view this as the 'original' slice -- we batch move elements back into place, so there's dropped elements between prev and current.

[m4dh0rs3]

Yeah, I misread the source-code. The first part checks whether a duplicate element exists, and elements are in consecutive order there. But that's not true for the actual de-duplication part later on.

For me its not about the ordering guarantee, but mentioning that the order (of the current implementation) is unexpected. And since that ordering does exist, I think we now should also guarantee it. Any future re-implementation of dedup_by changing the order would break a lot of code.

[Mark-Simulacrum]

I think the ordering is guaranteed already:

The elements are passed in opposite order from their order in the slice

even if it's a bit more vaguely worded, I think this is the same guarantee you are making, right?

(happy to take a wording improvement but I'm not seeing a new guarantee here)

[m4dh0rs3]

Yes, its only a wording improvement.

[Mark-Simulacrum]

Are there simple examples for both the 'complicated predicates' and the 'merging' discussed here that we could add? It seems plausibly useful to show how that might look. Definitely the current example given seems like it would be better to stay as a, b arguments -- current, prev is just adding noise there.

[m4dh0rs3]

The reason I changed a, b in general was that (for me) they imply the order [.., a, b, ..], which is not the case. If you mean just change the naming in the example, then at least use maybe x, p or something.

[m4dh0rs3]

Maybe I can think of a simple example of complicated predicates and merging yet. The example I came across in my personal code was sorting Vec<Range<usize>> by .start and then merging intervals that were subsets of another.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[m4dh0rs3]

I added a non-trivial example which depends on the sorting order and demonstrates how merging could look like. I also shortened the variable names.

@rustbot ready

[m4dh0rs3]

Because they are essentially the same docs, I wanted to link from slice::partition_dedup_by to Vec::dedup_by. Looking at core/src/time.rs, the doc syntax /// [Instant]: ../../std/time/struct.Instant.html to link to an item in std should work, but did not for me. The linkchecker complains, but the link works for me in my browser for local docs. So now I just left the docs somewhat duplicated instead.

@rustbot ready