Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Apr 16, 2018
Merged

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

merged 4 commits into from
Apr 16, 2018

Conversation

smmalis37
Copy link
Contributor

@smmalis37 smmalis37 commented Mar 18, 2018

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
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2018
/// assert!(!(..=5).contains(6));
/// ```
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
fn contains(&self, item: T) -> bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the method generic (e.g. fn contains<U>(&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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's what was meant, makes sense.

@varkor
Copy link
Member

varkor commented Mar 18, 2018

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
Copy link
Contributor Author

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

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this making progress!

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

@scottmcm scottmcm Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
        })

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I don't understand coherence well enough to answer: how does U: PartialOrd<T> as a bound compare to T: PartialOrd<U>? 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
Copy link
Member

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<U> rather than the other way around.

@varkor
Copy link
Member

varkor commented Mar 20, 2018

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<U> and U: PartialOrd<T>, just to avoid a #46934 situation in the future.

@pietroalbini 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@smmalis37
Copy link
Contributor Author

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

@bors
Copy link
Contributor

bors commented Mar 29, 2018

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

@shepmaster
Copy link
Member

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
Copy link
Contributor Author

Yep, I'm working on it now.

@smmalis37
Copy link
Contributor Author

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

@smmalis37 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
#[unstable(feature = "range_contains", reason = "recently added as per RFC", issue = "32311")]
fn contains<U>(&self, item: &U) -> bool
where
T: PartialOrd<U>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still in favour of where T: PartialOrd<U>, 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@smmalis37 smmalis37 Apr 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Kixunil
Copy link
Contributor

Kixunil commented Apr 8, 2018

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

@smmalis37
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Mark-Simulacrum 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
@pietroalbini 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
@pietroalbini
Copy link
Member

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

@alexcrichton
Copy link
Member

@smmalis37 ah my apologies, sorry about that!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 16, 2018

📌 Commit 51f24ec has been approved by alexcrichton

@bors 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
Copy link
Contributor

bors commented Apr 16, 2018

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

bors added a commit that referenced this pull request Apr 16, 2018
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
Copy link
Contributor

bors commented Apr 16, 2018

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

@bors bors merged commit 51f24ec into rust-lang:master Apr 16, 2018
@kennytm-githubbot
Copy link

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

kennytm-githubbot added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Apr 16, 2018
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).
@smmalis37 smmalis37 deleted the range branch April 16, 2018 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.