RFC: .drain(range) and .drain()

[bluss]

RFC: .drain(range) and .drain()

Implement .drain(range) and .drain() respectively as appropriate on collections.

Rendered

[bluss]

Previous Vec::drain RFC: /pull/574

[aturon]

@bluss Sorry for the lack of commentary!

I was wondering if you can elaborate on the motivation for IntoCheckedRange? The RFC says:

IntoCheckedRange is designed to allow bounds checking half-open and closed ranges. Bounds checking before conversion allows handling otherwise tricky extreme values correctly.

but it's not entirely clear to me what you mean.

Compared to the RangeArgument trait, IntoCheckedRange feels much more "specialized" (i.e., to enums and to this particular use of ranges), and is a new unsafe trait. I'd like to see a very clear motivation before going in that direction.

OTOH, RangeArgument could be expanded with an inclusive flag, and thereby cover all of our range notation.

[bluss]

It doesn't have to be a trait, it could be a function (parameterized over a different trait), to implement the same kind of bounds checking. That's mostly just to not have to repeat it in Vec::drain, String::drain, VecDeque::drain etc.

Bounds checking before conversion allows handling otherwise tricky extreme values correctly.

This refers mostly to other trait ideas that would convert all ranges to either a half open or closed range. These fail on tricky extreme values, since the maximum inclusive range does not fit inside an half open range without overflow, and 0..0 is not representable as an inclusive range. (At least not without the exhausted / empty flag, which wasn't on my mind or on my radar; I haven't been involved in the inclusive range design).

So yes, I think an accessor for fn includes_end(&self) -> bool could work in the trait RangeArgument. Since the method will effectively be compile time constant (from the type information), it should be just as efficient, just more verbose (and possibly error prone) where it is used.

IntoCheckedRange is indeed written just to be what Drain needs, but that was also true for the RangeArgument that it would replace.

[aturon]

@bluss

IntoCheckedRange is indeed written just to be what Drain needs, but that was also true for the RangeArgument that it would replace.

I disagree with that: RangeArgument is usable for any API that can work with the full set of our range notations. It's not tied to a particular type (usize) nor to bounds checking or any other particular operation.

I'm concerned mostly about the public-facing API here. I'd much rather stabilize a general-purpose trait like RangeArgument that can become "the standard way" to take the full set of range type, and use that trait in APIs like drain. If the proposal here is mainly about factoring out code, and can be made a pure implementation detail rather than complicating the user-visible API, that seems much better to me.

[bluss]

Do you mean you disagree with replacing it?

You can't disagree that it was added to support .drain(). That's how it was added.

That's ok, I didn't really consider writing the general purpose range trait for this RFC, inclusive ranges were not even ready. Should it be part of this RFC?

[aturon]

You can't disagree that it was added to support .drain(). That's how it was added.

I disagree that its design is specific to drain. It doesn't really matter if drain happens to be the first API that wanted it :)

[bluss]

I propose the range trait gets its own RFC and is removed from this one.

[bluss]

cc @Stebalien We're writing a general range trait

[alexcrichton]

I agree with @aturon that it'd be nice to have the trait here be a more general than just for these collections its targeting. Avoiding usize has the concrete win of making the trait usable for BTreeMap's drain method (to be eventually implemented I assume).

I'd also personally prefer the trait to be considered as part of this RFC as it's difficult to stabilize the methods on collections without stabilizing the traits that they're parameterizing over. It's not unheard of, but it'd make me more comfortable to stabilize everything at once.

[bluss]

@alexcrichton BTreeMap goes its own road (see the BTreeMap range RFC), and as noted here in this RFC, its drain should foremost be part of or consistent with its range iterator API. It doesn't look like they will use something similar to the index ranges that Vec::drain uses.

[bluss]

BTreeMap's own road for drain is over at /pull/1254

[bluss]

I think we are getting derailed.

Vec::drain and String::drain have already been through an RFC.

My aim here is to get put down on paper a mandate to implement drain for all collections, not just Vec and String.

My main questions that we need to get out of the way:

• Is the design with inherent methods called drain ok, or should there be a Drain trait with a drain method?
• Is it ok that some collections implement drain(range) and others only drain()?

In my opinion, these are the questions that are blocking stabilization of any of the drain methods, just to agree on the basic consistency across all collections.

The range trait questions can be settled later.

[alexcrichton]

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

[alexcrichton]

I'm personally OK with all stabilization and such here given that the trait to accept multiple ranges remains unstable (which I believe this is suggesting)

[aturon]

@bluss

• Is the design with inherent methods called drain ok, or should there be a Drain trait with a drain method?

Inherent methods are fine -- this isn't the time to add collection traits (still want HKT for that) and I don't think we want something one-off here. A trait can always be added later if the abstraction is called for.

• Is it ok that some collections implement drain(range) and others only drain()?

Yes, I think it's OK.

[alexcrichton]

Thanks again for the RFC @bluss! The libs team discussed this RFC today and the decision was to merge, so I will do so.

I'll also be reusing the stabilization issue as a tracking issue: rust-lang/rust#27711

[bluss]

Great. Thanks everyone for the input.