Implement drain RFC

[mahkoh]

Closes #23055

[breaking-change]

r? @gankro

[Gankra]

Oh woah the RFC landed.

[Gankra]

I believe the lifetimes can be elided here?

[mahkoh]

Updated.

[nikomatsakis]

If I were going to write Drain in pure safe code, it would look something like:

pub struct Drain<'a, T:'a> {
    start: uint,
    len: uint,
    victim: &'a mut Vec<T>
}

then on each call to next I could call remove on the item at self.start and decrement self.len. When dropping I would keep calling next self.len times. This should be roughly equivalent to the current behavior, though the modifications to the vector happen at different times, but I believe that is unobservable.

Therefore, I believe the marker you want is PhantomData<&'a mut Vec<T>>.

It is reasonable to want to put PhantomData<T>, because the destructor will drop instances of T, but not necessary -- the dropck will already be imposing conservative rules on Drain because it contains borrowed data. Basically, if you model what you would have written in safe code, you should be ok (unless the language itself is flawed).

cc @pnkfelix

UPDATE: Fixed a reference to &'a Vec<T> that should have been &'a mut Vec<T>

[nikomatsakis]

To summarize a length discussion on IRC about markers:

1. What I wrote here is definitely the most straight-forward annotation: Implement drain RFC #23071 (comment)
2. However, it would lead to invariance, which given the public API of Drain is stricter than necessary.
3. &'a T would induce covariance and suggests the correct relationship between 'a and T
4. However, @mahkoh argues that in principle some sufficiently smart compiler might consider the substituted form, and hence it would see &'a &mut U, and thus consider that since &mut is affine and you can't move from a & reference, it is safe to approximate U (be covariant w/r/t U). This is true as far as it goes (it's not what our compiler does, and I'm not sure that it would ever be a reasonable thing to do). For this reason, he argues for &'a () (to capture the lifetime) and fn() -> T (to capture the covariance). I don't know that this distinction is so important, but I agree that this would be a sound formulation, which is my primary concern here.

[mahkoh]

Updated.

[nikomatsakis]

ps this API is pretty nifty.

[mahkoh]

Updated.

[Gankra]

I believe this doesn't closr the rfc issue proper. At least VecDeque is outstanding.

[mahkoh]

VecDeque was not part of the RFC. It talked only about Vec and String.

[aturon]

cc me

[aturon]

@mahkoh @gankro

VecDeque was not part of the RFC. It talked only about Vec and String.

I think there was some oversight in terms of the full force of the RFC. Although the RFC was explicitly only talking about String and Vec, I had assumed that the drain convention would apply to any collection for which ranges make sense, at least. For things like HashMap, I expect we won't have a range parameter.

If needed, we can file an amendment to the RFC to clarify this point.

[aturon]

Also, the RFC left unspecified the exact trait used for capturing the various range types (which was a point of frustration, since it's much better to specify these details up front), but I personally would strongly prefer to introduce a general trait that covers the various ranges over a given type T. This is needed in a few other places as well.

cc @alexcrichton

[Gankra]

By contrast I'd rather favour drain be a trait that the collection implements for particular inputs. This allows the iterator to be specialized for the different kind of range for performance reasons as necessary (e.g. FullRange or usize can be quite simpler).

However I think this needs HKT.

[Gankra]

For reference, I picture:

trait Drain<Input> {
  type Drain<'*>; // HKT
  fn drain<'a>(&'a mut self, input: Input) -> Drain<'a>;
}

[mahkoh]

What's stopping this PR from being merged right now? A new RFC can get a new PR and for the most common use case, calling drain with an explicitly typed range parameter, all changes will be backwards compatible.

[liigo]

The elements of Vec are removed when dropping Drain, but the length of Vec is changed before creating Drain. I'm not sure the state of the Vec is valid in Drain's lifetime. Let me know if I'm wrong.

[alexcrichton]

I personally would strongly prefer to introduce a general trait that covers the various ranges over a given type T

I agree, but this is all #[unstable] for now so it's fine by me to land this in the interim. We definitely don't want to go too crazy in adding 4 of these, however.

[pnkfelix]

@liigo I think the Vec is borrowed for the lifetime.of the drain, so one cannot push onto it or otherwise cause a realloc , and it looks like the drain drop implementation will clean up its part if we hit a panic.

Still, a compile-fail test for the former and a run-pass test for the latter seem like good exercises

[bluss]

@gankro, you don't need HKT, trait Drain<'a, Input> { type Drain; fn drain(&'a self, Input) -> Self::Drain } works today. Bound is T: for<'a> Drain<'a, Range<K>>. Note that implementor decides if the Drain type needs to use the lifetime or not though..

I also think it is silly to implement Drain for other specifiers than ranges. Skip single indices and leave that to other plain remove methods. It's not good API design to combine different functionality. Of course constructing a range that spans just a single key will often be possible.

[mahkoh]

I also think it is silly to implement Drain for other specifiers than ranges. Skip single indices and leave that to other plain remove methods. It's not good API design to combine different functionality.

Should have voiced your concerns before the RFC was accepted.

[nikomatsakis]

It's not clear to me how much this design discussion concerns this PR vs the overall end point of the design. Seems like a thread on internals would be a better venue for discussing the latter.