Move Range*::contains to a single default impl on RangeBounds #49130

smmalis37 · 2018-03-18T08:44:46Z

Per the ongoing discussion in #32311.

This is my first PR to Rust (woo!), so I don't know if this requires an amendment to the original range_contains RFC, or not, or if we can just do a psuedo-RFC here. While this may no longer follow the explicit decision made in that RFC, I believe this better follows its spirit by adding the new contains method to all Ranges. It also allows users to be generic over all ranges and use this method without writing it themselves (my personal desired use case).

This also somewhat answers the unanswered question about Wrapping ranges in the above issue by instead just punting it to the question of what those types should return for start() & end(), or if they should implement RangeArgument at all. Those types could also implement their own contains method without implementing this trait, in which case the question remains the same.

This does add a new contains method to types that already implemented RangeArgument but not contains. These types are RangeFull, (Bound, Bound), (Bound<&'a T>, Bound<&'a T>). No tests have been added for these types yet. No inherent method has been added either.

r? @alexcrichton

rust-highfive · 2018-03-18T08:44:59Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

varkor · 2018-03-18T11:02:17Z

src/liballoc/range.rs

+    /// assert!(!(..=5).contains(6));
+    /// ```
+    #[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
+    fn contains(&self, item: T) -> bool


Let's make the method generic (e.g. fn contains(&self, item: &U) -> bool) so that we can check for containment of types which are not directly the same time as the range (i.e. heterogeneous comparisons).

Oh that's what was meant, makes sense.

varkor · 2018-03-18T11:06:29Z

I think if this change is being made, then it's the right time to make the other two changes discussed in the issue at the same time (changing method signatures later can be a pain; it's much better to get it right first time): i.e. taking a reference and being generic over the item type.

I think the Wrapping issue is less important, because technically such a case is not permitted according to the documentation.

smmalis37 · 2018-03-18T19:29:21Z

Makes sense to me. I've made those changes locally and am running the tests now, will push once they finish.

scottmcm

Great to see this making progress!

scottmcm · 2018-03-19T07:12:23Z

src/liballoc/range.rs

+    /// assert!( (..=5).contains(&-1_000_000_000));
+    /// assert!( (..=5).contains(&5));
+    /// assert!(!(..=5).contains(&6));
+    /// ```


Because this is bound only to PartialOrd, I suggest you include some NAN examples in here too. For example, I would expect !(0.0..1.0).contains(&f32::NAN), !(0.0..f32::NAN).contains(&0.5), and !(f32::NAN..1.0).contains(&0.5). (I think that's what the previous code did.)

I think that will mean changing the implementation, since a range like NAN..NAN will never get to the return false lines. Maybe keep the positive phrasing of the originals, something like

(match self.start() { Included(ref start) => item >= *start, Excluded(ref start) => item > *start, Unbounded => true, }) && (match self.end() { Included(ref end) => item <= *end, Excluded(ref end) => item < *end, Unbounded => true, })

scottmcm · 2018-03-19T07:16:09Z

src/liballoc/range.rs

+    #[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
+    fn contains<U>(&self, item: &U) -> bool
+    where
+        U: ?Sized + PartialOrd<T>,


Something I don't understand coherence well enough to answer: how does U: PartialOrd<T> as a bound compare to T: PartialOrd? Would we prefer to make it easier for T or U to be a custom type, and which one makes that easier? Should this just bound both of them for now?

alexcrichton · 2018-03-20T14:50:48Z

These changes all seem great to me, thanks @smmalis37! I wonder though if it may also be best to leave the inherent methods on the various common range types? That way you can still use the method without having to import the trait (but the implementation would simply delegate to the trait implementation).

I think I also agree with @scottmcm as well in that the direction of the type parameters may matter here and I'd probably expect T: PartialOrd rather than the other way around.

varkor · 2018-03-20T14:56:25Z

Though PartialOrd is required informally to be symmetric, it turns out people don't read the documentation and this is currently not enforced by the compiler. I would be in favour of enforcing T: PartialOrd and U: PartialOrd<T>, just to avoid a #46934 situation in the future.

smmalis37 · 2018-03-28T18:49:54Z

@alexcrichton Finally finding time to work on this again. I like your suggestion, however the trait is currently defined in liballoc while the Range* types are defined in libcore. So I don't think it's possible for the types to delegate to the trait without moving it into libcore, which would also require moving Bound into libcore. Is my understanding correct? Should I go make that move?

smmalis37 · 2018-03-28T19:33:33Z

I just noticed #49163, guess I'm waiting on that to be merged.

smmalis37 · 2018-03-28T20:11:18Z

Now that RangeBounds is in the same module as the Range* types, do we still want to add the inherent method as well?

bors · 2018-03-29T14:27:25Z

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

shepmaster · 2018-04-07T19:33:31Z

Ping from triage, @smmalis37 ! Will you have time to pick this back up?

I'm waiting on that to be merged.

This is now merged.

smmalis37 · 2018-04-07T22:32:47Z

Yep, I'm working on it now.

smmalis37 · 2018-04-08T07:28:49Z

Pardon the force push and removing the past commits, but it needed to be rebased anyways. The PR description has also been updated.

scottmcm · 2018-04-08T08:30:35Z

src/libcore/ops/range.rs

+    #[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
+    fn contains<U>(&self, item: &U) -> bool
+    where
+        T: PartialOrd<U>,


Re-mentioning the T: PartialOrd vs U: PartialOrd<T> vs both discussion, since I'm not sure it was fully resolved.

I think we agreed on not U: PartialOrd<T>, but the other two are still options.

I'm still in favour of where T: PartialOrd, U: PartialOrd<T> for compatibility. For anyone implementing these traits correctly, there'll be no inconvenience, but it makes it easier in the future to modify in a way that's backwards-compatible.

There's definitely the advantage there of making sure that everything is implemented correctly, however it does raise the question of consistency. Do we think this is a small enough issue that it's more worthwhile to be consistent with the rest of std? Are there plans to address this issue in a more broad way that would apply everywhere?

The rest of std isn't consistent. For example, Vec::contains isn't even generic over the search type. The issue is that, in the past, the methods weren't defined generally enough, and when people come back to try to fix that, there are type inference issues.

The general practice now is "make new methods behave correctly" and we'll try to fix the old ones when it's possible (e.g. with #27336 and #46946). These aren't going to be done very soon, though, so as someone who has been bitten by bad signatures in the past, a little more inconsistency is far preferable.

Sounds good to me, I'll make the change once I'm home from work.

Kixunil · 2018-04-08T11:04:46Z

Any good reason to not define Contains<T> trait and make RangeBounds<T>: Contains<T>?

smmalis37 · 2018-04-08T18:36:45Z

Such a proposal hasn't gone through the RFC process yet to my knowledge. This is just a tweak/update to one that already has.

smmalis37 · 2018-04-09T00:35:21Z

This is no longer waiting-on-author, but waiting-on-review. Not sure how to change the label.

pietroalbini · 2018-04-16T10:56:53Z

Ping from triage! Can @alexcrichton (or someone else from @rust-lang/libs) review this?

alexcrichton · 2018-04-16T13:55:22Z

@smmalis37 ah my apologies, sorry about that!

@bors: r+

bors · 2018-04-16T13:55:25Z

📌 Commit 51f24ec has been approved by alexcrichton

bors · 2018-04-16T16:07:17Z

⌛ Testing commit 51f24ec with merge 49317cd...

@alexcrichton

Move Range*::contains to a single default impl on RangeBounds Per the ongoing discussion in #32311. This is my first PR to Rust (woo!), so I don't know if this requires an amendment to the original range_contains RFC, or not, or if we can just do a psuedo-RFC here. While this may no longer follow the explicit decision made in that RFC, I believe this better follows its spirit by adding the new contains method to all Ranges. It also allows users to be generic over all ranges and use this method without writing it themselves (my personal desired use case). This also somewhat answers the unanswered question about Wrapping ranges in the above issue by instead just punting it to the question of what those types should return for start() & end(), or if they should implement RangeArgument at all. Those types could also implement their own contains method without implementing this trait, in which case the question remains the same. This does add a new contains method to types that already implemented RangeArgument but not contains. These types are RangeFull, (Bound<T>, Bound<T>), (Bound<&'a T>, Bound<&'a T>). No tests have been added for these types yet. No inherent method has been added either. r? @alexcrichton

bors · 2018-04-16T18:51:19Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 49317cd to master...

kennytm-githubbot · 2018-04-16T18:52:45Z

📣 Toolstate changed by #49130!

Tested on commit 49317cd.
Direct link to PR: #49130

💔 rls on windows: test-pass → build-fail (cc @nrc).
💔 rls on linux: test-pass → build-fail (cc @nrc).
💔 rustfmt on windows: test-pass → build-fail (cc @nrc).
💔 rustfmt on linux: test-pass → build-fail (cc @nrc).

@nrc

Tested on commit rust-lang/rust@49317cd. Direct link to PR: <rust-lang/rust#49130> 💔 rls on windows: test-pass → build-fail (cc @nrc). 💔 rls on linux: test-pass → build-fail (cc @nrc). 💔 rustfmt on windows: test-pass → build-fail (cc @nrc). 💔 rustfmt on linux: test-pass → build-fail (cc @nrc).

rust-highfive assigned alexcrichton Mar 18, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2018

varkor reviewed Mar 18, 2018

View reviewed changes

scottmcm reviewed Mar 19, 2018

View reviewed changes

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2018

smmalis37 added 3 commits April 7, 2018 15:47

Update based on RangeBounds trait being moved to libcore.

f5a367c

fix tests

16f30c2

Add float NaN tests.

249dc9e

smmalis37 changed the title ~~Move Range*::contains to a single default impl on RangeArgument.~~ Move Range*::contains to a single default impl on RangeBounds Apr 8, 2018

scottmcm reviewed Apr 8, 2018

View reviewed changes

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2018

Add symmetric requirement of PartialOrd.

51f24ec

pietroalbini added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 16, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2018

bors merged commit 51f24ec into rust-lang:master Apr 16, 2018

smmalis37 deleted the range branch April 16, 2018 19:00

scottmcm mentioned this pull request Jun 18, 2018

Specialize StepBy<Range(Inclusive)> #51601

Merged

pnkfelix mentioned this pull request Dec 3, 2018

NLL error on closure, but not on equivalent function #55526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Range*::contains to a single default impl on RangeBounds #49130

Move Range*::contains to a single default impl on RangeBounds #49130

smmalis37 commented Mar 18, 2018 •

edited

Loading

rust-highfive commented Mar 18, 2018

varkor Mar 18, 2018

smmalis37 Mar 18, 2018

varkor commented Mar 18, 2018 •

edited

Loading

smmalis37 commented Mar 18, 2018

scottmcm left a comment

scottmcm Mar 19, 2018 •

edited

Loading

scottmcm Mar 19, 2018

alexcrichton commented Mar 20, 2018

varkor commented Mar 20, 2018

smmalis37 commented Mar 28, 2018

smmalis37 commented Mar 28, 2018

smmalis37 commented Mar 28, 2018

bors commented Mar 29, 2018

shepmaster commented Apr 7, 2018

smmalis37 commented Apr 7, 2018

smmalis37 commented Apr 8, 2018

scottmcm Apr 8, 2018

smmalis37 Apr 8, 2018

varkor Apr 8, 2018

smmalis37 Apr 10, 2018

varkor Apr 10, 2018

smmalis37 Apr 11, 2018 •

edited

Loading

Kixunil commented Apr 8, 2018

smmalis37 commented Apr 8, 2018

smmalis37 commented Apr 9, 2018

pietroalbini commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

kennytm-githubbot commented Apr 16, 2018

Move Range*::contains to a single default impl on RangeBounds #49130

Move Range*::contains to a single default impl on RangeBounds #49130

Conversation

smmalis37 commented Mar 18, 2018 • edited Loading

rust-highfive commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor commented Mar 18, 2018 • edited Loading

smmalis37 commented Mar 18, 2018

scottmcm left a comment

Choose a reason for hiding this comment

scottmcm Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 20, 2018

varkor commented Mar 20, 2018

smmalis37 commented Mar 28, 2018

smmalis37 commented Mar 28, 2018

smmalis37 commented Mar 28, 2018

bors commented Mar 29, 2018

shepmaster commented Apr 7, 2018

smmalis37 commented Apr 7, 2018

smmalis37 commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smmalis37 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Kixunil commented Apr 8, 2018

smmalis37 commented Apr 8, 2018

smmalis37 commented Apr 9, 2018

pietroalbini commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

bors commented Apr 16, 2018

kennytm-githubbot commented Apr 16, 2018

smmalis37 commented Mar 18, 2018 •

edited

Loading

varkor commented Mar 18, 2018 •

edited

Loading

scottmcm Mar 19, 2018 •

edited

Loading

smmalis37 Apr 11, 2018 •

edited

Loading