Tracking issue for RangeBounds

[durka]

This issue tracks the collections_range feature which applies to std::collections::range::RangeArgument. It used to be lumped in with drain (#27711) but that was stabilized while RangeArgument may continue to be affected by experiments with inclusive ranges, so here we are.

[durka]

I'm currently working on inclusive ranges, which is why I noticed this. A PR to change the issue link is incoming.

[withoutboats]

This trait is not actually being exported by std right now, meaning this feature can't be used without importing libcollections. I would submit a PR, but I'm not sure if a new range module in std::collections is the right place to mount it, or if it belongs under a different path.

[sfackler]

The libs team discussed this, and are placing RangeArgument into a final comment period to stabilize. We will need to tweak the API to support inclusive and exclusive bounds on each side to look something like:

pub enum Bound<T> {
    Unbound,
    Inclusive(T),
    Exclusive(T),
}

pub trait RangeArgument<T> {
    fn lower(&self) -> Bound<&T>;

    fn upper(&self) -> Bound<&T>;
}

[Stebalien]

@sfackler shouldn't the new API go though another stabilization period? Also, a way to extract the bounds by-value would be nice (but probably not necessary).

[sfackler]

Probably at this point yeah, particularly since we still haven't actually made the change.

[alexcrichton]

The libs team discussed this at triage the other day and the conclusion was to punt on stabilization/deprecation because we didn't have time to implement the changes here

[clarfon]

Question: is there a reason why this trait is in collections but not core?

[sfackler]

I don't think there's a good reason for it to not be in core off the top of my head - it should probably move!

[J-F-Liu]

Should also impl RangeArgument for usize.

impl RangeArgument<usize> for usize {
    fn start(&self) -> Bound<usize> { Included(self) }
    fn end(&self) -> Bound<usize> { Included(self) }
}

[SimonSapin]

@J-F-Liu There’s also a std::slice::SliceIndex trait that is implemented for both usize and various range types.

[J-F-Liu]

@SimonSapin thanks for let me know this, it indicates should impl RangeArgument for usize as well.
SliceIndex is much more complex than RangeArgument, and its name is not obvious for range purpose.

[clarfon]

Would people be willing to accept a PR for moving this to core? Everything would have the same features, and collections would re-export them for compatibility.

Only problem I see is that Bound is already stabilised but I'm not too worried about that.

[sfackler]

Makes sense to move to me!

…

On Sat, Apr 8, 2017 at 9:35 PM Clar ***@***.***> wrote: Would people be willing to accept a PR for moving this to core? Everything would have the same features, and collections would re-export them for compatibility. Only problem I see is that Bound is already stabilised but I'm not too worried about that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30877 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY2UaqUry1YVcrwUqcWz7caLespreVdks5ruGAdgaJpZM4HEQZG> .

[Gankro]

Can I nominate this for FCP? Seems like it does what it needs to do; fully general.

[glaebhoerl]

I've had various ideas w.r.t. the design which I posted and each time the discussion got moved somewhere else, I can't remember what they were and I'm too lazy to look them up, but at least let's call it something like IsRange instead of RangeArgument because there's nothing limiting it to being used in argument position.

[sfackler]

Seems reasonable to FCP for stabilization. I'd be fine renaming as part of that!

@rfcbot fcp merge

[ltratt]

On stable Rust today, you can also write your own RangeArgument trait and impls (or even copy the one in libstd into your own code) and use it in your API.

Yes, good point -- I hadn't considered that. I'm going to do it even though it feels pretty icky. Despite my griping, thanks for the pointer!

[Gankro]

In case anyone else has a collection they want RangeArgument for: https://github.com/Gankro/thin-vec/blob/master/src/range.rs

This is an especially clean case of the unstable trait in public API pattern, as there's very little incentive to implement these traits outside of the like 4-5 std types which do.

[SimonSapin]

PR #49163: Rename RangeArgument to RangeBounds, move it and Bound to libcore

[SimonSapin]

Assuming this PR is accepted, the only issue in this thread that I feel might not be resolved is the possibility of replacing this trait with a type to be used with Into<_>.

@sfackler wrote “the Into<Foo> approach will have troubles with non-copy bounds” but I don’t quite understand what this troubles are. Into::into takes/moves its self argument. Either way, I think that a dedicated trait is fine.

@rust-lang/libs what do you think? If we decide to keep the dedicated trait, should we do another round of FCP or are we good to stabilize? (Again assuming the PR linked above.)

[SimonSapin]

The libs team discussed this and consensus was to stabilize after renaming the methods to start_bound and end_bound to avoid name collision with new inherent start and end methods of RangeInclusive.

@rfcbot fcp merge

Affected APIs are:

pub trait RangeBounds<T: ?Sized> {
    fn start_bound(&self) -> Bound<&T>;
    fn end_bound(&self) -> Bound<&T>;
}

impl<T: ?Sized> RangeBounds<T> for RangeFull {…}
impl<T> RangeBounds<T> for RangeFrom<T> {…}
impl<T> RangeBounds<T> for RangeTo<T> {…}
impl<T> RangeBounds<T> for Range<T> {…}
impl<T> RangeBounds<T> for RangeInclusive<T> {…}
impl<T> RangeBounds<T> for RangeToInclusive<T> {…}
impl<T> RangeBounds<T> for (Bound<T>, Bound<T>) {…}
impl<'a, T: ?Sized + 'a> RangeBounds<T> for (Bound<&'a T>, Bound<&'a T>) {…}
impl<'a, T> RangeBounds<T> for RangeFrom<&'a T> {…}
impl<'a, T> RangeBounds<T> for RangeTo<&'a T> {…}
impl<'a, T> RangeBounds<T> for Range<&'a T> {…}
impl<'a, T> RangeBounds<T> for RangeInclusive<&'a T> {…}
impl<'a, T> RangeBounds<T> for RangeToInclusive<&'a T> {…}

[rfcbot]

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Kimundi
• @SimonSapin
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler
• @withoutboats

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.

[scottmcm]

Since the bikeshed was opened: why not lower_bound and upper_bound if they're going to include the word "bound"?

[SimonSapin]

This change is motivated by avoiding collision with the exact start and end method names of RangeInclusive #49022, but they’re still strongly related to those as well as the start and end fields of other range types. I think the names should reflect that relation in some way.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[DDOtten]

I think a method like fn into_bounds(self) -> (Bound<T>, Bound<T>) would be very helpful. Currently RangeInclusive implements into_inner but other range types do not. (It also is the only range type to implement new, start and end and the only one to where the inner types are not public)

This would give a way to get the inner values without having to use clone. I am currently implementing a trie/prefix-tree and want to add a range function similar to a BTreeMap. This function takes something like a RangeBounds<I> where I: IntoIterator<T>. I would like to avoid an extra Clone bound for I and cloning these types is less efficient.