Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upRFC: Add `is_sorted` to the standard library #2351
Conversation
LukasKalbertodt
added some commits
Feb 24, 2018
Centril
added
the
T-libs
label
Feb 25, 2018
Centril
reviewed
Feb 25, 2018
| overhead while writing *and* reading the code. | ||
|
|
||
| In [the corresponding issue on the main repository](https://github.com/rust-lang/rust/issues/44370) | ||
| (from which I will reference a few comments) everyone seems to agree on the |
This comment has been minimized.
This comment has been minimized.
Centril
Feb 25, 2018
Contributor
s/from which I will reference a few comments/from which a few comments are referenced/
| [unresolved]: #unresolved-questions | ||
|
|
||
|
|
||
| ### Is `Iterator::is_sorted_by_key` useless? |
This comment has been minimized.
This comment has been minimized.
Centril
Feb 25, 2018
Contributor
Yes, I think so, precisely for the reason you mention.
We already have quite many methods in Iterator and I think that additions should only be made if they increase readability and helps you express things which are quite tedious and unidiomatic to express otherwise. In this case I think iter.map(extract).is_sorted() is better than iter.is_sorted_by_key(extract).
If there is popular demand for this later we can always add it then, but for now I think it is premature.
| C::Item: Ord, | ||
| ``` | ||
| This can be seen as a better design as it avoids the question about which data |
This comment has been minimized.
This comment has been minimized.
Centril
Feb 25, 2018
Contributor
Perhaps if Rust has a built in operator for function composition and made point-free notation easy, but that is not the case, so let's keep the RFC as is.
| fn is_sorted_by<F>(mut self, compare: F) -> bool | ||
| where | ||
| F: FnMut(&Self::Item, &Self::Item) -> Ordering, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Centril
Feb 25, 2018
Contributor
Yes, I am aware. But just because you can do something doesn't mean you ought to do something. So I'm wondering if there are any good reasons why you should be able to pass in a stateful closure.
This comment has been minimized.
This comment has been minimized.
jonas-schievink
Feb 25, 2018
Member
Ah, okay. But this would apply to almost all libstd functions that take closures, then. I don't think any method uses an Fn bound when FnMut suffices. This also applies to all sort methods we already have, so consistency is another argument for FnMut.
This comment has been minimized.
This comment has been minimized.
LukasKalbertodt
Feb 25, 2018
•
Author
Contributor
Yep, that's exactly the reason I chose FnMut: it's everywhere already. In particular: [T]::sort_by_key.
Do you think I should mention this in the RFC? Maybe that would be a good idea to prevent future readers from asking the same question about FnMut...
Oh, and just for protocol: I wondered about the usage of FnMuts, too. Exactly for the "should be stateless" reason you, @Centril, mentioned.
This comment has been minimized.
This comment has been minimized.
Centril
Feb 25, 2018
Contributor
So I knew about the consistency reason beforehand, but I think that we should decide on this with a rationale more than "this is what we've done in other places", unless there's some other place where rationale has been given(?).
This comment has been minimized.
This comment has been minimized.
ExpHP
Feb 25, 2018
•
@Centril why should a function deliberately choose a more restrictive Fn than it needs, unless it wants to open itself to implementation changes that may require those restrictions?
The rules for picking Fn traits are simple:
- If you only need to call it once, ask for
FnOnce. - If you call it multiple times but do not require reentrancy, ask for
FnMut. - If you need to call it concurrently, ask for
Fn.
...and I rather doubt the stdlib needs to keep itself open to the possibility of changing this function to run in multiple threads.
By requiring Fn you make it difficult for the user to do things like caching and memoization, forcing people to use RefCell and thereby deferring compile-time borrow checks to runtime. There is no upside.
This comment has been minimized.
This comment has been minimized.
ExpHP
reviewed
Feb 25, 2018
| > ```rust | ||
| > fn is_sorted(self) -> bool | ||
| > where | ||
| > Self::Item: Ord, |
This comment has been minimized.
This comment has been minimized.
ExpHP
Feb 25, 2018
•
Why Ord instead of PartialOrd? Testing if a <= b <= c <= ... <= z is a perfectly well-defined operation on a partially-ordered set, and to give floating point numbers the shaft yet again even for something as innocent as this just seems spiteful.
(note: I'm anthropomorphizing the std lib here; not calling you spiteful)
This comment has been minimized.
This comment has been minimized.
LukasKalbertodt
Feb 25, 2018
Author
Contributor
Good point, sorry for not properly thinking about that beforehand. So the comparator for the _by versions may also return an Option<Ordering>, right? And how do we want to handle a None then? I might be too sleepy think... but it's not immediately obvious whether [1.0, NaN, 2.0] should be considered ordered or not, right?
This comment has been minimized.
This comment has been minimized.
ExpHP
Feb 26, 2018
Hm. This is a predicament.
If this existed in a vacuum, then I would say the comparator should produce bool, because an <= operation really is most natural definition for a partial ordering. Option<Ordering> seems to me to be a sort of an allowance to make deriving Ord from PartialOrd possible.
But obviously, this does not exist in a vacuum, and so a bool comparitor could create unnecessary confusion for users. So yes, I suppose we should try Option<Ordering>.
Re: [1.0, NaN, 2.0], I am about to post a much larger comment on this, but in summary, I am of the stance that there is only one sensible implementation of is_sorted on a partially ordered set, and this implementation returns false for a sequence like [1.0, NaN, 2.0].
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Feb 26, 2018
•
|
In response to my request for Needless to say, there is more than one way that somebody could define The possible implementations of
|
This comment has been minimized.
This comment has been minimized.
|
@ExpHP So to summarize, you propose to use the stricter version I'm not quite sure about the "what should the closure return?" issue. I'd love to read more comments on these two questions. I'll change the RFC later :) |
This comment has been minimized.
This comment has been minimized.
|
Great comment, @ExpHP! I'd add that I'd expect the following, which also suggests definition 1: is_sorted(a) → ∀i,j (i ≤ j → ai ≤ aj ∧ ai < aj → i < j) (You do obliquely mention this, but I think it's more interesting as a property-but-not-implementation, since transitivity means a linear implementation provides a quadratic guarantee.) |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Feb 26, 2018
Yes, this is my recommendation. |
This comment has been minimized.
This comment has been minimized.
|
@ExpHP that pairwise function is essentially itertools' |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Mar 1, 2018
|
@clarcharr Maybe not |
This comment has been minimized.
This comment has been minimized.
|
@ExpHP What about |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Mar 2, 2018
•
|
@eddyb I'm not sure, what makes Comparing the two, I feel fn is_sorted<I>(iter: I) -> bool
where I: IntoIterator, I::Item: PartialOrd + Clone,
{
use itertools::Itertools;
iter.into_iter().tuple_windows().all(|(a, b)| a < b)
}N.B. my original intent was not to propose |
This comment has been minimized.
This comment has been minimized.
|
@ExpHP Do you think |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Mar 2, 2018
|
I am currently leaning towards considering them as an alternative. (i.e. for the Alternatives section). These are my thoughts:
It is perhaps less discoverable than |
This comment has been minimized.
This comment has been minimized.
leonardo-m
commented
Mar 3, 2018
|
is_sorted() is sufficiently common and sufficiently clear to deserve a function. If a programmer writes ".all(|(a, b)| a < b)" instead of ".all(|(a, b)| a <= b)" this could lead to troubles. |
This comment has been minimized.
This comment has been minimized.
|
I agree with @leonardo-m: writing Apart from that, I proposed to add |
This comment has been minimized.
This comment has been minimized.
|
Fully generic |
This comment has been minimized.
This comment has been minimized.
|
I don't think that |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Mar 11, 2018
•
Here are all of the "by key" methods in Iterator and Itertools: (i.e. any method that takes a
None of them could be accomplished with a |
This comment has been minimized.
This comment has been minimized.
|
Hmm, I suppose something like fn all_tuples<F>(&mut self, mut f: F) -> bool
where Self: Sized, F: FnMut<Self::Item, Output=bool>
{
self.all(|x| f.call_mut(x))
} |
This comment has been minimized.
This comment has been minimized.
ExpHP
commented
Mar 11, 2018
•
|
(and while I will admit that I have needed With the specific way you have written it, the function takes multiple args instead of a tuple, which is neat, but also relies on unstable details about the |
This comment has been minimized.
This comment has been minimized.
|
@scottmcm Shouldn't it be possible to get the behaviors you mentioned by using
|
LukasKalbertodt
added some commits
Apr 12, 2018
This comment has been minimized.
This comment has been minimized.
|
I just read an article about implementing I added a paragraph about this in the RFC. And since the discussion died down in the last month, a quick summary (although there aren't too many comments):
So to me it seems like all issues (that were mentioned) were addressed and there are not really major concerns left. So... maybe FCP? ^_^ |
This comment has been minimized.
This comment has been minimized.
|
So I just read the RFC. I wrote a crate that implements
For consistency with The only other comment I have is that the motivation of this RFC is pretty weak:
The most important case is probably implementing a sorting algorithm, since the first thing you want to do there is check whether the elements are sorted, and if the answer is yes, you are done. The check is The vectorized implementations in my library require a long list of unstable features: |
This comment has been minimized.
This comment has been minimized.
Seems a bit important to me to be left implicit Well if we assume that the This notational axiom also implies irreflexitivity for So maybe it is more fitting to think about
Additionally, two relations are derived:
Why?
I still think it should be fairly obvious what |
This comment has been minimized.
This comment has been minimized.
|
I've filled rust-lang/rust#50230 to ask for at least clarification. I agree with you that, at least the way things are currently documented, the requirements (and guarantees) of Please fill free to chime in there with other things that might need to be clarified, like the notational axiom: |
This comment has been minimized.
This comment has been minimized.
|
I'd propose to not let the trait bound decision block merging this RFC.
I also think that we should initially implement |
This comment has been minimized.
This comment has been minimized.
|
@LukasKalbertodt I think we can leave that as an unresolved question to resolve before stabilization, but whether it requires The most conservative thing to do is to require The ideal thing would be if we could just require |
kennytm
reviewed
Jul 10, 2018
| The lack of an `is_sorted()` function in Rust's standard library has led to | ||
| [countless programmers implementing their own](https://github.com/search?l=Rust&q=%22fn+is_sorted%22&type=Code&utf8=%E2%9C%93). | ||
| While it is possible to write a one-liner using iterators (e.g. | ||
| `(0..arr.len() - 1).all(|i| arr[i] < arr[i + 1])`), it is still unnecessary |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
LukasKalbertodt
Jul 10, 2018
Author
Contributor
That was my secret plan: hide a subtle bug in the "motivation" section that no one notices for months. Now that it has been discovered, it's the perfect motivation that is_sorted is certainly needed! Muhaha!
In all seriousness, it happens all the time.
Will fix this bug in the RFC later. It's blocked right now anyway.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ExpHP
Aug 8, 2018
Heh, I'd almost love to see a section in the motivation showcasing just how easy it is to make this mistake!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The libs team discussed this today and we’d like to accept the RFC as-is: three methods for each of slices and iterators, with @rfcbot fcp merge This algorithm is just tricky enough to implement that having it in the standard library seems helpful enough to be worth the added API surface. Some comments have said that adding |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Aug 8, 2018
•
|
Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
disposition-merge
labels
Aug 8, 2018
This comment has been minimized.
This comment has been minimized.
|
@LukasKalbertodt If we merge LukasKalbertodt/partial-cmp-docs#1 with the proposed tweaks pretty much as is, the blocker here is resolved anyways. So with those semantics merged, what would be the appropriate bound for IIRC:
EDIT: Floats Under a strict partial ordering relation, all of the following would be sorted (that is,
Even in the absence of
because I don't know, my intuition tells me that if we use In
Since we can't implement both ordering using Another example that's similar to floats is a It cannot implement That is, for all permutations of |
This comment has been minimized.
This comment has been minimized.
I don't believe that |
This comment has been minimized.
This comment has been minimized.
I mixed |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Aug 9, 2018
|
|
rfcbot
added
final-comment-period
and removed
proposed-final-comment-period
labels
Aug 9, 2018
This comment has been minimized.
This comment has been minimized.
|
@gnzlbg Unfortunately, I currently don't have the time to work on the I think then it would be the best to reboot the |
This comment has been minimized.
This comment has been minimized.
DDOtten
commented
Aug 18, 2018
•
|
I would be very surprised when |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
Aug 19, 2018
|
The final comment period, with a disposition to merge, as per the review above, is now complete. |
rfcbot
added
finished-final-comment-period
and removed
final-comment-period
labels
Aug 19, 2018
Centril
referenced this pull request
Aug 19, 2018
Open
Tracking issue for RFC 2351, "Add `is_sorted` to the standard library" #53485
Centril
merged commit d424128
into
rust-lang:master
Aug 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Huzzah! This RFC has been merged! Tracking issue: rust-lang/rust#53485 |
LukasKalbertodt commentedFeb 25, 2018
•
edited by Centril
Add the methods
is_sorted,is_sorted_byandis_sorted_by_keyto[T]andIterator.Rendered
Tracking issue
CC rust-lang/rust#44370
EDIT: I posted a comment summarizing the discussion so far.