Second pass stabilization: slice and vec

[aturon]

Second pass stabilization: vec

This PR takes a second pass through the vec module to
stabilize its API. The changes are as follows:

Stable:

• dedup
• from_raw_parts
• insert
• into_iter
• is_empty
• remove
• reserve_exact
• reserve
• retain
• swap_remove
• truncate

Deprecated:

• from_fn, from_elem, grow_fn and grow, all deprecated in
  favor of iterators. See Collections Reform Part 2 rfcs#509
• partition, partitioned, deprecated in favor of a new, more
  general iterator consumer called partition.
• unzip, deprecated in favor of a new, more general iterator
  consumer called unzip.

A few remaining methods are left at experimental status.

Second pass stabilization: slice

This PR takes a second pass through the slice module to
stabilize its API. The changes are as follows:

Stable:

• as_mut_slice
• as_ptr, as_mut_ptr
• binary_search_by (was: binary_search)
• binary_search (was: binary_search_elem)
• chunks, chunks_mut
• contains
• ends_with
• first, first_mut (was: head)
• get_unchecked, get_unchecked_mut (was: unsafe_get)
• get
• is_empty
• iter, iter_mut
• len
• permutations
• reverse
• sort_by
• sort
• split_at, split_at_mut
• split_mut, splitn_mut, rsplitn_mut
• split, splitn, rsplitn
• starts_with
• swap
• to_vec
• windows

Deprecated:

• head, head_mut (renamed as above)
• unsafe_get, unsafe_mut (renamed as above)
• binary_search_elem (renamed as above)
• partitioned, deprecated in favor of a new, more
  general iterator consumer called partition.
• BinarySearchResult, deprecated in favor of Result<uint, uint>

Unify concat and concat_vec

We've long had traits StrVector and VectorVector providing
concat/connect and concat_vec/connect_vec respectively. The
reason for the distinction is that coherence rules did not used to be
robust enough to allow impls on e.g. Vec<String> versus Vec<&[T]>.

This PR consolidates the traits into a single SliceConcatExt trait
provided by slice and the prelude (where it replaces StrVector,
which is removed.)

[breaking-change]

r? @alexcrichton

[aturon]

Note @alexcrichton: I made the mistake of organizing the vec and slice files (they were previously totally haphazard). While that's a nice cleanup, it makes this somewhat rebase-hostile. I'd suggest high priority. Also, feel free to take over this PR and push it through if you like while I'm away.

[aturon]

@MrFloya @gankro: this is the PR I was mentioning

[Gankra]

I wonder if we should perhaps hold off on properly deprecating from_fn, from_elem, grow_fn and grow while we figure out the trusted_len story. It's an unfortunate perf regression to cause in the interim.

[reem]

Was back vs. last discussed in an RFC? I'm not a huge fan of the renaming.

[Gankra]

It was not part of any collections RFC, to my knowledge. Likely dark work-week cabals.

[aturon]

@gankro

I wonder if we should perhaps hold off on properly deprecating from_fn, from_elem, grow_fn and grow while we figure out the trusted_len story. It's an unfortunate perf regression to cause in the interim.

I'm open to that -- we could leave them as experimental with a message indicating that they're likely to be removed. It's a bit frustrating because we could add trusted_len today to recover the performance, but it's likely that a cleaner solution will be available later.

@reem

Was back vs. last discussed in an RFC? I'm not a huge fan of the renaming.

No, this was a bit of bikeshedding that came up during the first round of vec stabilization. The main problem is that head and last are inconsistent. Contenders are: first/last or front/back. I went with the latter because it's consistent with APIs elsewhere, e.g. for DList.

[alexcrichton]

I may have expected this to be get_mut_unchecked, although I may not quite have the ordering of suffixes right in my head.

[aturon]

https://github.com/aturon/rfcs/blob/conventions-galore/active/0000-conventions-galore.md#suffix-ordering

[alexcrichton]

Ah ok, cool!

[alexcrichton]

Sorry for being a little slow getting to this @aturon! I've looked things over and I'm quite happy with this! Just a few nits here and there and I think it's good to go.

[aturon]

I've added almost all your suggestions, except for the change to binary search's return type (which didn't seem worthwhile to me).

[reem]

@aturon I prefer first and last because they are strongly related to ordering. I dislike front/back because it's a nebulous spatial metaphor.

[aturon]

@reem

@aturon I prefer first and last because they are strongly related to ordering. I dislike front/back because it's a nebulous spatial metaphor.

OK, that's a pretty compelling argument, since the use of front/back elsewhere is tied to queues where the term is more applicable.

I will adjust accordingly, thanks!

[alexcrichton]

Landed in #20360

[bluss]

Why did you change .remove() and .swap_remove to not return Option? It seems to contradict the RFC.

[bluss]

Also, why do we mark methods stable even if they were just changed?

[tomaka]

The RFC in question: https://github.com/rust-lang/rfcs/blob/master/text/0235-collections-conventions.md#deletion

[Gankra]

Yes, this change flew under my radar, I would be interested in the detailed rationale, as this is technically a functionality regression (even though it improves ergonomics in the expected case).

[aturon]

@bluss Thanks for bringing this to my attention! The RFC mistakenly had remove on Vec returning an Option, even though our intended convention for Vec operations was:

If it takes an index, panic on out-of-bounds, with the sole exception of get/get_mut

This was part of rolling out error conventions for Vec, after a lot of experimentation with different options (e.g. returning Option everywhere). Initially Vec was very inconsistent.

The reason for marking it stable here was that I believed this was bringing into line with the old conventions RFC. However, as I said above, that RFC had a mistake.

I'm going to file an amendment to clarify the intended conventions and their rationale. If discussion there determines it should be otherwise, there's still time to change before 1.0-beta.

[SimonSapin]

If it takes an index, panic on out-of-bounds, with the sole exception of get / get_mut

@aturon, Where was this discussed? What is the rationale? It seems opposite with the earlier trend of avoiding panic when possible, e.g. #11129. And taking an index seems a fairly arbitrary criteria for different error handling.

If you want to panic on failure it’s very easy to add .unwrap() on an Option<T>, but doing something else on failure with a method that panics is much more difficult since panic cannot be caught/handled.

[aturon]

@SimonSapin Some of the early discussion was here, although the repo seems to be missing the follow-up discussion after doing the experiment. This was also part of the (many, many) discussions about error conventions, though I don't recall how much was on IRC vs other forums.

In any case, two things:

Why I believe this is the right guideline

I stand by the guideilne. It has a very simple rationale: almost all of the time, if you have managed to produce an index, either it's "special" (in practice, 0), or it was computed in a way that guarantees it's in bounds.

For special indexes (0 and len() - 1) we tend to provide convenience methods (like pop) which then return Option to allow you to fold in the check. We don't currently have a convenience for remove(0) -- it was removed earlier on, before these conventions -- but we should.

FWIW, I tried many variations on the error conventions for Vec during the first stabilization pass. In particular, I tried a variant that returned Result everywhere. Rolling it out in std and the compiler, I found that there were very, very few times where I could end up taking advantage of that result if the code was already computing an index -- as above, unless it was a "special" index I already knew it was in bounds. So it ended up cluttering a lot of code with extra unwraps for very little benefit.

While we have moved away from panicicking in general, we are not doing so blindly. The accepted conventions leave it to the API designer to choose what to treat as contracts (resulting in panics) and when to return results. It depends a lot on how a given API is used in practice and what knowledge clients typically have. The above guideline for Vec, besides being IMO the right tradeoff ergonomically, also makes Vec's error handling story relatively simple, consistent and easy to predict.

To my mind, the pushback against changing remove really amounts to a request to add back unshift.

But I could be wrong!

As I mentioned above, I will be posting an amendment to the RFC to make sure these considerations are recorded highly visibly and to give people a chance to weigh in. If there is strong disagreement, there is still time to change the API before 1.0-final.

[SimonSapin]

almost all of the time, if you have managed to produce an index, either it's "special" (in practice, 0), or it was computed in a way that guarantees it's in bounds.

That makes sense. Thanks for the explanation.

We don't currently have a convenience for remove(0) -- it was removed earlier on, before these conventions -- but we should.

Yes please. remove(0) in my code is what broke and lead me here.