ordered ranges 2.0

[Gankro]

Replace range(Bound::Excluded(&min), Bound::Included(&max)) with range().ge(&min).lt(&max).

Add BTreeMap::drain as appropriate.

Flesh out the general "ordered query" story.

Rendered

[alexcrichton]

Would it be possible to remove the L type parameter on the range structures? It looks like it's primarily a kind of pseudo-type-state preventing you from calling lt and gt in the wrong order or too often, but perhaps it's ok to allow that? This would remove a type parameter and avoid having to expose Unbounded, Inclusive, and Exclusive.

[Gankro]

I'll admit L and R are sort-of artifacts of an earlier design -- the get alternative. I believe it would be possible to just store enums, yes. Although it's kind've nice to prevent calling lt too many times.

Note that today's Range allows L and R to have different types. In principle this enables searching with different borrows for different end-points.

[alexcrichton]

Yeah I agree that removing the type parameter does indeed reduce some functionality (e.g. not statically rule out erroneous situations), but making the signatures a little more readable and removing items from the public API are always nice!

I also think it's a good point that the left/right bounds could be different types today, but I'm also not 100% convinced that this is necessary.

[apasel422]

Assuming {L, R} are kept, I think it should be possible to have

pub struct Unbounded;

instead of

pub struct Unbounded<'a, Q: ?Sized + 'a>(PhantomData<&'a Q>);

[Gankro]

@apasel422 the associated types for the IntoBound trait don't work out, I think.

[apasel422]

@Gankro Yeah, IntoBound would have to use a type parameter instead of an associated type, although I wonder if we could just use the preexisting Into trait instead of a new one.

[Gankro]

I think a type parameters runs afoul of coherence/orphan checking in the IntoIter impl (which is why I didn't use Into).

[apasel422]

Hmm. I'm inclined to agree that we should just store the enums directly rather than use these marker types. We could still parameterize the range over multiple endpoint types though:

use std::collections::Bound;

pub struct Range<'a, K: 'a, V: 'a, Min: ?Sized + 'a, Max: ?Sized + 'a> {
    map: &'a Map<K, V>,
    min: Bound<&'a Min>,
    max: Bound<&'a Max>,
}

[apasel422]

Also, what are you planning to name these types if btree_map::{Range, RangeMut} already exist? Or will these live somewhere else?

[Gankro]

Uuugh naming.These can be called RangeBuilder or whatever I guess.

[apasel422]

Will the builder be hardcoded to store &BTreeMap<K, V>? If so, I guess that means that there has to be a RangeMutBuilder, and also a by-ref/by-mut builder for each of the collection types.

Could the builder itself also be parameterized over the collection? Then we could get away with one declaration like

pub struct RangeBuilder<'a, C, Min: ?Sized + 'a, Max: ?Sized + 'a = Min> {
    collection: C,
    min: Bound<&'a Min>,
    max: Bound<&'a Max>,
}

impl<'a, 'b, K, V, Min: ?Sized, Max: ?Sized> IntoIterator for RangeBuilder<'a, &'b BTreeMap<K, V>, Min, Max> where
    K: Borrow<Min> + Borrow<Max> + Ord,
    Min: Ord,
    Max: Ord,
{
    type IntoIter = btree_map::Range<'b, K, V>;
    ...
}

impl<'a, 'b, K, V, Min: ?Sized, Max: ?Sized> IntoIterator for RangeBuilder<'a, &'b mut BTreeMap<K, V>, Min, Max> where
    K: Borrow<Min> + Borrow<Max> + Ord,
    Min: Ord,
    Max: Ord,
{
    type IntoIter = btree_map::RangeMut<'b, K, V>;
    ...
}

This would also enable more code reuse for external crates that want to provide the ordered query API.

[bluss]

Let's question something we never do: Should the range really be double ended? I'm reasonably sure it has an irremovable overhead to support, and rust often claims to go for the least costly abstractions. Double ended iterators are nice where they are cheap, but here it has a cost (or is this wrong?).

On the whole I think the range builder thing is fine, it's something users will recognize, even if it's not from other collections.

[Gankro]

@bluss That's another benefit of making range a builder: we can do that as different finalizers if it proves useful. By default I'd like to assume they'll be DoubleEnded, and investigate during/after implementation of the functionality that most would expect.

[Gankro]

@apasel422 What do you suggest that Drain's RangeBuilder contains? A newtype Drain(&mut BTreeMap)?

[apasel422]

@Gankro Missed this earlier. I'm not sure I totally understand your question.

[bluss]

@Gankro That sounds great

[Gankro]

@apasel422 If there is only one DrainBuilder, and DrainBuilder<& BTree> and DrainBuilder<&mut BTree> are taken for Iter and IterMut, what does Drain use?

[alexcrichton]

🔔 This RFC is now entering its week-long final comment period 🔔

[alexcrichton]

My personal preference would be to remove the two impl blocks on Range in favor of just one (e.g. don't specialize where one end is Unbounded. I think that the cases it's statically ruling out aren't too particularly useful and otherwise calling gt or such in a generic context is just more of a pain if Unbounded is required.

[Gankro]

@alexcrichton what do you expect the behaviour of .gt(a).gt(b) to be?

[alexcrichton]

I'd want the upper bound of b to override the upper bound of a. I'd view ranges as having basically an Option<T> at both ends, and each time you call g* or l* you're just replacing that Option<T>

[RalfJung]

@alexcrichton I would expect the "other" sensible behavior. Listing a bunch of conditions looks very much like a conjunction to me, so .gt(a).gt(b) means "greater than a, and greater than b". I see no reason, based on the syntax, to expect the second gt to entirely overwrite the first.

[aturon]

I'm late to the party here, but I have some concerns about this RFC.

First, I want to say that in isolation, I think a builder design like this is a pretty slick way of solving the problem. Kudos!

However, I'm pretty concerned about the global design ramifications here. In particular, we've gradually been growing our range notation -- which is meant to be a generic way to express ranges, usable for a variety of purposes. We recently merged an RFC to add inclusive ranges. And we use this syntax heavily for indexed collections.

It seems a real shame to have a completely different way of expressing ranges in the two cases, just because the syntax we happened to settle on doesn't support exclusivity on the left. I feel like this is a sign that we dropped the ball on the design of the range syntax, not accounting for the full, ahem, range of uses.

In particular, if we had a design that allowed you to choose between exclusivity/inclusivity on both the left and the right -- basically, the equivalent to notation like (n, m] and [n, m) etc in mathematics -- we wouldn't need special builder APIs here, and both indexed and ordered collections would provide a uniform API with nice syntactic support.

Put differently, I fear it's going to be extremely annoying to get used to doing things like vec[n..m] and then not being able to do that for ordered collections. It's possible we could provide both range notation and a builder-style API, but that also seems unfortunate -- again, it feels like an admission that range notation failed to be adequately general.

Given that inclusive ranges have not yet landed, I wonder whether we should take a step back and reconsider an overall range notation design that can express the full suite of ranges.

[glaebhoerl]

Strawman proposal: the base syntax for ranges is .., which can be modified on both the left and the right with either < to specify exlusivity or = for inclusivity. So:

• [a, b] -> a=..=b
• [a, b) -> a=..<b
• (a, b] -> a<..=b
• (a, b) -> a<..<b

The modifier can be omitted on either or both sides, on the left it defaults to =, on the right to <. So in practice the ones we'd likely see would be a..b, a..=b, a<..b, and a<..=b.

I actually like the currently accepted ... syntax for inclusive-on-the-right ranges, but it hardly seems extensible in a consistent and regular fashion to exclusive-on-the-left as well. Whereas this is a bit too cryptic for my tastes, but at least it's straightforward and predictable. I'm not sure if I like it.

(This syntax seems so logical that I'm almost sure somebody has proposed it before: if so, my apologies!)

[aturon]

@glaebhoerl Something along those lines seems pretty reasonable to me. I think in the previous round ... was somewhat taken as a hard constraint due to its existing use in patterns, but I think we should give serious thought to deprecating that syntax (which is fairly rare) in favor of something more uniform as you're suggesting, which would also alleviate the various concerns raised about the .. and ... pairs.

(Not sure whether we want to re-start the whole bikeshed within this RFC. But I'm going to raise my cross-cutting concerns here with the core team, since it feels like the incremental design work on the syntax side has not fully taken library concerns into account.)

[BurntSushi]

@aturon I feel somewhat similar. While reading the RFC, I thought "it's really unfortunate we can't reuse range notation for this." I will say though, that I do actually really like the API proposed. The bummer, as you say, is having two different ways to express ranges. Spending some extra time to see if there exists one way to express ranges might be worth it.

[alexcrichton]

The libs team discussed this RFC today and the decision was to move it out of FCP for now, but perhaps leave it open for a little more discussion. In light of @aturon's recent comment there's been some more thinking about this, and one idea brought up by @wycats was to perhaps have method syntax plus a builder to "emulate" what sugar one day might buy us. For example:

fn range<T>() -> RangeBuilder<T> { /* ... */ }

impl<T> RangeBuilder<T> {
    fn inclusive_left(&mut self, t: T) -> &mut Self { /* ... */ }
    fn exclusive_left(&mut self, t: T) -> &mut Self { /* ... */ }
    fn inclusive_right(&mut self, t: T) -> &mut Self { /* ... */ }
    fn exclusive_right(&mut self, t: T) -> &mut Self { /* ... */ }
}

impl<T> RangeTrait<T> for RangeBuilder<T> {
    /* ... */
}

With this we'd have something like:

impl<K, V> BTreeMap<K, V> {
    fn range<T: RangeTrait<U>, U>(&self, t: T) -> Range<U> where K: Borrow<U> {
        /* ... */
    }
}

The current range syntax we have (e.g. .. and ... would implement RangeTrait and so would this builder. This would allow the full flexibility of the methods proposed in this RFC, as well as allowing usage of sugar if it later exists. For example @glaebhoerl's <..= syntax would be range().exclusive_left(a).inclusive_right(b) (albeit much less ergonomically).

The general idea would be to introduce a more verbose library-based implementation of "all kinds of ranges" which can be supplanted with special sugar syntax later on if it becomes available. All along the way the RangeTrait trait would allow consumers to be generic over all of them.

Thoughts @Gankro?

[Gankro]

Seem basically fine (the methods should be self -> self imo)

I'm a bit ambivalent about the temporary assymetry here. a..b, a...b, a.gt(a).le(b) ..

[RalfJung]

But wouldn't that mean we would end up, one day, with two kinds of syntax (not counting library methods here) to say whether the right-hand end of a range is inclusive? We'd have, e.g., a...b and a..=b. That'd be rather unfortunately, I think.

[bluss]

@glaebhoerl I think that's a rather refreshing proposal, to not introduce inclusive range syntax. We could consider unshackling a..b from meaning half-open range, into simply being "a range", with convention dependent on API. I don't think Exclude(a)..Include(b) is that bad, except maybe from overly long identifiers..

[RalfJung]

I don't think Exclude(a)..Include(b) is that bad, except maybe from overly long identifiers..

Gt(a)..Le(b) would be a shorter proposal.

[alexcrichton]

@Gankro yeah the details of the actual API itself will likely be tweaked a bit, and I'd be fine with consumption so long as it turns out nicely.

@RalfJung yeah I agree that it would lead us to a situation of "two ways to do the same thing", but I don't really see it as all that bad as it would in theory be a very well known and mechanical conversion. Additionally we'd perhaps deprecate the "builder API" if sugar was available instead.

@bluss that's not a bad idea as well! I think the major question there is in terms of the ergonomics and what the downstream traits would look like for consuming a number of ranges, but otherwise it may obviate the need for ... even

[Gankro]

Personally, I think any decent API for this problem shouldn't require importing anything.

[arthurprs]

Are there any note worthy implementations of this feature in other languages?

[Gankro]

This RFC is dead, pending the lang team investigating more complete range syntax.

[robsmith11]

This RFC is dead, pending the lang team investigating more complete range syntax.

@Gankro Has this been resolved?
If so, has work on BTreeMap::drain been picked up elsewhere?