collections: Stabilize Vec

[alexcrichton]

The following methods, types, and names have become stable:

• Vec
• Vec::as_mut_slice
• Vec::as_slice
• Vec::capacity
• Vec::clear
• Vec::default
• Vec::grow
• Vec::insert
• Vec::len
• Vec::new
• Vec::pop
• Vec::push
• Vec::remove
• Vec::set_len
• Vec::shrink_to_fit
• Vec::truncate
• Vec::with_capacity
• vec::raw
• vec::raw::from_buf
• vec::raw::from_raw_parts

The following have become unstable:

• Vec::dedup // naming
• Vec::from_fn // naming and unboxed closures
• Vec::get_mut // will be removed for IndexMut
• Vec::grow_fn // unboxed closures and naming
• Vec::retain // unboxed closures
• Vec::swap_remove // uncertain naming
• Vec::from_elem // uncertain semantics
• vec::unzip // should be generic for all collections

The following have been deprecated

• Vec::append - call .extend()
• Vec::append_one - call .push()
• Vec::from_slice - call .to_vec()
• Vec::grow_set - call .grow() and then .push()
• Vec::into_vec - move the vector instead
• Vec::move_iter - renamed to iter_move()
• Vec::push_all - call .extend()
• Vec::to_vec - call .clone()
• Vec:from_raw_parts - moved to raw::from_raw_parts

This is a breaking change in terms of the signature of the Vec::grow function.
The argument used to be taken by reference, but it is now taken by value. Code
must update by removing a leading & sigil or by calling .clone() to create a
value.

[breaking-change]

[alexcrichton]

oops, wrong title!

[thestinger]

I don't understand the rationale for moving unsafe static methods to free functions. They are no longer tied to the type and it makes re-exports work poorly because despite having Vec in scope you need to use a ridiculously long path to get at them.

Why go out of the way to make unsafe code painful? It already requires an explicit unsafe block. It's not like Go where everything needs to be tucked away in an unsafe module because there's no explicit boundary.

Why only do it for static methods? It's just inconsistent and needlessly verbose. The names will collide In a module with more than one collection type (map + set), which is another reason why static methods are useful.

AFAIK, it never went through the RFC process (at least not anything I saw) and goes against the previous consensus that led to converting these from free functions into methods in many modules.

[lilyball]

@thestinger When was there consensus that these should be methods? I thought they became methods purely because of the move to Vec / String, but I don't recall any actual discussion on the matter. Personally, I'm in favor of putting these things in a raw module.

[lilyball]

@alexcrichton If from_raw_parts() is moving to vec::raw, does it need the word "raw" to stay in the name? vec::raw::from_raw_parts() seems kind of redundant, as opposed to just vec::raw::from_parts().

[thestinger]

@kballard: How do you propose dealing with modules with more than one container type then? Why should static methods be in a module but not any of the other methods? They became methods because we made the decision to purge the free functions and a lot of work went into migrating everything. The unsafe functions were given the same treatment. Rust relies heavily on re-exports and local use statements to avoid writing out enormous paths, and static methods work well in that scheme.

Referring to Vec::from_buf is easy, but std::vec::raw::from_raw_parts or collection::vec::raw::from_raw_parts is not. You can chop off the crate name, but you can't get rid of vec::raw:: or it becomes ambiguous - and it turns into an extra use statement just to get there. Making unsafe Rust more verbose does not make it safer, it makes it harder to write and read the code and pushes people away to languages with sugar for low-level code.

[lilyball]

What's the motivation for removing append() and append_one()? They exist because they're useful.

Similarly, push_all() being replaced with extend() has worse ergonomics. The previously simple code vec.push_all(slice) is now vec.extend(slice.iter().map(|x| x.clone())). That's really kind of sucky.

[lilyball]

Also, when you're deprecating .to_vec() and .into_vec(), those are actually provided by the impl of CloneableVector. How are you proposing to deprecate those? Or do you actually want to remove the CloneableVector impl entirely? (and can you deprecate an entire trait impl?)

[thestinger]

(push_all is likely faster too, extend can't trust the size hint to remove bounds checks)

[lilyball]

@thestinger You can't put instance methods in a module, unless you want to provide a single-implementation trait for them. And actually, using a single-implementation trait that needs to be explicitly used in order to get unsafe "raw" methods is not a terrible idea.

[thestinger]

@kballard: Making unsafe code more verbose doesn't make it any safer. It is just going to push away the people who want to use Rust as a systems language. Concise and readable code is easier to secure, adding verbosity is cargo cult security. Raw pointer dereferencing is built-in so there's no added boundary from making other unsafe code more verbose.

[lilyball]

What's the motivation for removing the & in grow()? I'm not saying I necessarily oppose it, but it does make the edge case of grow(0, val.clone()) unfortunate (as the clone is wasted).

And the suggested replacement for grow_set() is incorrect. grow() + push() only works if the vector needs growing; if it's already large enough, you need to replace the existing value instead. The code vec.grow_set(5, &0u, 42u) becomes

if vec.len() < 4 {
    vec.grow(4 - vec.len(), 0u);
    vec.push(42u);
} else {
    *vec.get_mut(5) = 42u; // will be vec[5] = 42u
}

I'm willing to believe that grow_set() is an esoteric function that almost nobody uses, and should be deprecated on those grounds. But the written suggestion of "call .grow() and then .push()" is just wrong.

[lilyball]

@thestinger It's not about verbosity, it's about simplifying the API.

[thestinger]

@kballard: How is adding a module or adding a trait simplifying the API? Please just add a filter for unsafe functions to rustdoc if you want a way to hide them. Making unsafe code more verbose is not the solution.

[lilyball]

@thestinger Note that I am not saying we should actually move instance methods into traits in the raw module. I only said it's not a terrible idea, and it's something I would not oppose if it was proposed. But I am not proposing it myself.

I also disagree that moving static methods into the raw module is more verbose. vec::raw::from_buf() is no more verbose than Vec::from_raw_buf() (which is what it would probably be called if it was turned into a static method). The only thing that makes this more verbose from a practical perspective is the fact that Vec is in the prelude but std::vec is not, but I don't think that's a good justification for keeping an extremely rarely-used static function on Vec. Better to maintain the precedent already established with the previous raw modules (such as std::str::raw), especially because this same precedent will presumably apply to other standard library types that may not be in the prelude.

[thestinger]

Note that I am not saying we should actually move instance methods into traits in the raw module. I only said it's not a terrible idea, and it's something I would not oppose if it was proposed. But I am not proposing it myself.

It's a terrible idea because it doesn't work for modules with more than one type like the key-value data structures so it cannot be used consistently. You've avoided addressing that point. No one has given an explanation for the reasoning behind moving the static methods but not the instance methods. I think your point about a clean view of the safe methods was good, but I think it would be fully addressed by adding a filter to rustdoc.

The only thing that makes this more verbose from a practical perspective is the fact that Vec is in the prelude but std::vec is not, but I don't think that's a good justification for keeping an extremely rarely-used static function on Vec.

That's not true. It's an extra namespace combined with an extra use statement for anything not in the prelude. There will be a use statement the type itself and then use module_name in order to have module_name::raw instead of crate::module_name::raw.

Better to maintain the precedent already established with the previous raw modules (such as std::str::raw), especially because this same precedent will presumably apply to other standard library types that may not be in the prelude.

That's a misrepresentation of the current state of affairs. The vast majority of these unsafe functions associated with types are implemented on the types. The str module had one or two functions left over that hadn't been migrated to methods. The raw module in String and elsewhere + the deprecation of the unsafe static methods was done without an RFC very recently and I filed a bug for it.

I don't think that's a good justification for keeping an extremely rarely-used static function on Vec.

Not all static unsafe methods are rarely used. This kind of style decision needs to be applied consistently in order for the API to be easy to learn and navigate so the issue of modules with mutliple types needs to be addressed. IMO, crate_name::module_name::raw::type_name_foo is incredibly bad, because the only part you can sanely strip away is crate_name. With crate_name::module_name::TypeName::foo you can cut it down to TypeName::foo just by importing the type, which is likely going to be done anyway to get any the safe static methods and type name easily. The from_raw_parts method isn't the only thing at stake here.

[emberian]

I fully agree with @thestinger.

[alexcrichton]

@thestinger, the convention for "raw" operations is a convention under development. The current mention in the guidelines isn't as specific as it could be, and I now that @aturon is working on fleshing it out to make it more concrete.

This PR represents our current understanding of how "raw" operations should work, and you bring up some good points that need to be taken into consideration when developing the guideline around operations such as these. Our thinking was something like:

• Rustdoc today shows these methods with the same prominence as the other constructors, which seems like a bug. This is fixable both at the API level and the rustdoc level.
• "Inconvenient access" to these functions is both desirable and undesirable. If using the function for the first time, you should be aware of the gotchas about it, but if using it for the Nth time you don't want to keep typing the same thing over and over. These two concerns are often at odds with one another.
• The primary concern for these raw operations is consistency across all APIs, and as @kballard mentioned we're leaning more today towards raw submodules as opposed to top-level methods.

Due to a concrete guideline not currently existing, I may leave these methods in the submodule as #[unstable] for now rather than going to #[stable] as there are still lingering concerns.

---

@kballard,

What's the motivation for removing append() and append_one()?

Right now we have a number of "push some elements onto this vector" functions, all of which are pretty inconsistent:

• push - takes a value and pushes it onto the receiver.
• push_all - takes a slice and clones values onto the receiver.
• push_all_move - takes a Vec and moves the values into the receiver
• append_one - takes one value and pushes it, returning the vector.
• append - takes a slice and clones the values, returning the vector.

Some example inconsistencies:

• The _move suffix is still up for debate whether it's going to stick around, this is one of the very few functions in the standard library that has the suffix.
• The append functions are some of the only functional-style apis for collections in the standard library, they stick out.
• The append_one method has a _one suffix, but push (the analgous operation) does not.
• The analagous push_all and append operations have differing suffixes

We ended up deciding that there are so many convention questions here that the best thing to do would be to deprecate all of them and start from scratch. I believe that using iterators can be more ergonomic than it is today, and I agree that the current method of "pushing all by clone" is a little un-ergonomic.

To maintain consistence across all collections we need to make sure they all provide similar interfaces (wherever possible). The sugar of "push all by clone" only exists for vectors, and we'd also like to extend that option to containers like set, maps, etc.

So, all in all, we would like to deprecate the functions for now and start afresh. Does that make sense?

---

@thestinger,

push_all is likely faster too, extend can't trust the size hint to remove bounds checks

This was indeed considered when deciding what to do about these functions. We figured that these functions are not being removed, and will likely not for awhile, so the performance is not lost for now. If we are unable to come up with a suitable alternative when these functions are slated for deletion, it is likely that some may remain for this concern.

---

@kballard,

Also, when you're deprecating .to_vec() and .into_vec(), ... How are you proposing to deprecate those?

I've placed the #[deprecated] annotations on the implementation for Vec. Whether or not the stability attributes pick that up is largely a question for the stability attributes. I believe the trait will stick around now because these are useful methods for slices. When we get to stabilizing the trait we may remove the into_vec method, however (just a guess).

Does that makes sense?

---

@kballard,

What's the motivation for removing the & in grow()?

This was a bit of a toss up. The other method in question was Vec::from_elem (which took the argument by value), and we wanted to unify the two of these. We ended up deciding that by-value had the possible optimization of actually using the value given for one of the pushes, so we ended up deciding to remove the &.

We couldn't really think of a super-compelling argument in either direction, so we just made a decision.

---

@kballard,

And the suggested replacement for grow_set() is incorrect

Thanks for pointing that out! I'll change it.

[thestinger]

and you bring up some good points that need to be taken into consideration when developing the guideline around operations such as these

That's the problem with making all the decision making behind closed doors. There are many good points (not just from me) that are being missed and then the language has to go through many iterations of breaking changes. The problem with making stability decisions in the dark is that there's no going back after a mistake. There should at least be a cool off period of a week after the last API change before something can be marked #[stable]. If it just changed, then it's hardly stable. It could have a terrible flaw that's only discovered after the change is made.

"Inconvenient access" to these functions is both desirable and undesirable. If using the function for the first time, you should be aware of the gotchas about it, but if using it for the Nth time you don't want to keep typing the same thing over and over. These two concerns are often at odds with one another.

That's what unsafe is supposed to be for, nothing else is needed. It's already clear that when using an unsafe you need to very carefully read the documentation and consider the consequences. Rust is already more verbose than C++ as a systems language, and intentionally adding verbosity will just mean it loses by a larger margin.

There are incredibly simple ways to shoot yourself in the foot like transmute and raw pointers so there's nothing to gain by going out of the way to make other unsafe code painful to use. I can't see why a programming language going out of the way to make your life difficult and your code unreadable is a good thing. Unsafe code is already discouraged by the need to use unsafe, and if someone decides they do need it then the standard libraries should make their life as easy as possible. The language needs to make unsafe code clean and readable if it's going to compete with C / C++. It's already missing a lot of the sugar.

This was indeed considered when deciding what to do about these functions. We figured that these functions are not being removed, and will likely not for awhile, so the performance is not lost for now. If we are unable to come up with a suitable alternative when these functions are slated for deletion, it is likely that some may remain for this concern.

If it's useful, it shouldn't be deprecated. Just make it #[unstable] or #[experimental]. Deprecation implies it is going away and will cause people to avoid it and migrate away from it. The only good reasons to get rid of something that's useful are if it has a high maintenance burden or a negative impact elsewhere.

The current mention in the guidelines isn't as specific as it could be, and I now that @aturon is working on fleshing it out to make it more concrete.

One person's personal style guidelines isn't the same thing as consensus-based decisions. Writing them up and then proposing them via the RFC process is great, but it currently doesn't really hold any weight. Even though the RFC decisions are made by a small group of people, it's done after people have a venue to give input and criticize. The decision might not be something that most people in the community agree with, but at least it's a very well informed decision.

This PR represents our current understanding of how "raw" operations should work, and you bring up some good points that need to be taken into consideration when developing the guideline around operations such as these.

It's not the community's understanding of how unsafe should work. It's a minority opinion and is contrary to all of the decisions that were made in the open.

[lilyball]

So, all in all, we would like to deprecate the functions for now and start afresh. Does that make sense?

[...]

This was indeed considered when deciding what to do about these functions. We figured that these functions are not being removed, and will likely not for awhile, so the performance is not lost for now.

This is not appropriate. As @thestinger says, deprecation means "this is going away" and is a very strong encouragement for people to stop using the APIs immediately. The only alternatives are a) live with compiler warnings (which sucks, and we definitely don't want to encourage this), or b) disable the warnings (which also sucks, we don't want people to disable deprecation warnings).

Starting from scratch and building a more consistent API is fine. But In the meantime, the functions that are candidates for being replaced but do not yet have a replacement should be #[unstable] or #[experimental], not #[deprecated]. We can deprecate them once we actually have a replacement. And heck, maybe the new from-scratch API will re-use some of the same function names anyway and so they won't even need to be deprecated.

[lilyball]

@thestinger Thinking about it some more, the precedent of raw modules isn't actually "unsafe static methods", but "functions that construct a value out of raw * pointers". Hence the name "raw". In light of that, Vec::from_raw_parts() still makes sense to turn into vec::raw::from_parts(), but this would not set a precedent of "all unsafe static methods must become free functions in a child module".

[thestinger]

It's still a more complex API (another module to look through), noisier to use, inconsistent (everything else is a method) and is an incomplete solution (what do you do with multiple types in a module?). To add another issue, what about CVec and CStr?

[aturon]

@alexcrichton Yes, I'd recommend keeping the raw-related bits as unstable until we go through a formal conventions process for it. I'll try to get that RFC out ASAP. I don't personally have strong feelings about this, other than that we need a clear convention.

As to push_all and friends, I am working on a broad RFC for conventions, stabilization, and some new features for libcollections as a whole. This includes specific plans for recovering the ergonomics and performance. I wouldn't say that the plan is to "start from scratch" with these APIs, but rather to increase the power and ergonomics of the Iterator API so that extend covers all use cases. As @alexcrichton says, in the end we can always re-introduce the methods if necessary.

We were on the fence as to whether to mark these methods as deprecated or experimental. Since our plan is to remove them, deprecation seemed to send the clearest signal, but marking experimental may indeed be the better choice in cases like this (i.e. when the migration is not yet clear.)

As @alexcrichton explained, append and append_one are not consistent with any of our other APIs; they are there only by a long historical accident. Removing them may be a slight ergonomic regression in some cases, but we'd much rather have a general way of doing this kind of mutate-and-return-self pattern.

As a general point, during stabilization we are trying to reduce API surface area where we can, both to minimize the promises we're making and to simplify APIs. For the case of convenience methods, this is sometimes a matter of taste and/or conventions.

We also want to avoid leaving too much around as experimental, lest those APIs become de facto stable.

[lilyball]

@alexcrichton I'm not sure where to find it anymore but somewhere there's a suggestion that the syntax foo(a,_,b) should implicitly create a closure |x| foo(a,x,b) (not sure about which flavor of closure this should be). If you take that suggestion and combine it with UFCS, you could produce an API like

fn with<T>(val: T, f: |&mut T|) -> T {
    let mut val = val;
    f(&mut val);
    val
}

and then call it like

let x = with(make_vec(), Vec::push(_, 42u));

Not as nice or straightforward as let x = make_vec().append(42u) but it is something to consider.

Note: the with() func should probably take something like F: FnOnce but I'm not sure where we stand on that.

[aturon]

@thestinger

One person's personal style guidelines isn't the same thing as consensus-based decisions. Writing them up and then proposing them via the RFC process is great, but it currently doesn't really hold any weight. Even though the RFC decisions are made by a small group of people, it's done after people have a venue to give input and criticize. The decision might not be something that most people in the community agree with, but at least it's a very well informed decision.

To be clear, the guidelines that @alexcrichton cited are not personal -- they are official. In particular, new guidelines for things like raw go through the RFC process before ending up in the official guidelines. The current guidelines include a number of this with "RFC" status which means they are pending this RFC process and are therefore not yet official. See this for an example conventions RFC.

That's the problem with making all the decision making behind closed doors. There are many good points (not just from me) that are being missed and then the language has to go through many iterations of breaking changes. The problem with making stability decisions in the dark is that there's no going back after a mistake. There should at least be a cool off period of a week after the last API change before something can be marked #[stable]. If it just changed, then it's hardly stable. It could have a terrible flaw that's only discovered after the change is made.

We've been trying to strike a balance between going through the RFC process for conventions issues and making more local API decisions in meetings. The RFC process is lengthy and heavy weight, and we have to balance it against the need to get APIs stabilized leading up to 1.0. Please understand that this is not about a power trip, but rather the simple fact that decisions need to be made in a timely fashion and the resulting process is not always perfect.

That said, I think the process can certainly be improved and allow for more community input prior to final decisions. (For example, I'm currently writing a large RFC for more general API design around libcollections). I will discuss this further with the rest of the core team to see what's feasible balanced against the 1.0 deadline.

And thanks @kballard and @thestinger both for your feedback.

[lilyball]

@thestinger

inconsistent (everything else is a method)

If all type constructors that take raw pointers are done as functions in a raw module, that's consistent.

I get what you're saying here, but I'm not sure it's really that important to require that all static type constructors be static methods of the type. Note here that we're actually removing Vec::from_slice() in favor of just calling .to_vec() on the slice; we're losing a static type constructor in favor of a method on another type. That's not that different than losing it in favor of a static function in another module.

what do you do with multiple types in a module?

Use your best judgement?

I'd actually be in favor of extending associated items to allow modules as well, so you can say Vec::raw::from_parts(), although that's probably extremely problematic. In the absence of that, you could just use more descriptive function names. I'd give you an example but I actually can't find any modules that define multiple types and have static type constructors that use raw pointers (e.g. neither HashMap nor HashSet seem to have any methods that deal with raw pointers).

---

It occurs to me that we actually could simulate the embedded module thing with the current proposed associated items, by having an associated item that is a static value of a zero-sized type that provides the constructors as methods. The only difference is you'd use . instead of :: before the function. Something like

impl<T> Vec<T> {
    pub static raw: VecRawConstructor<T> = VecRawConstructor;
    // ...
}

struct VecRawConstructor<T>;

impl<T> VecRawConstructor<T> {
    pub fn from_parts(&self, length: uint, capacity: uint, ptr: *mut T) -> Vec<T> { ... }
}

fn example_usage() {
    let (len, cap, ptr) = ...;
    let v = unsafe { Vec::raw.from_parts(len, cap, ptr) };
    // ...
}

[lilyball]

@aturon I am quite sympathetic to the idea that we should make it easier to user iterators for things like .push_all(). However, appending a slice onto a vector seems like really core functionality, and should be a) really obvious how to do it, and b) doable with as little code as possible. Even a hypothetical vec.extend(slice.iter_clone()) is pushing it a little. I could imagine something like a trait CloneIterable<T> with a method iter_clone() that's basically equivalent to iter().map(|x| x.clone()), because then we could have fn push_from<I: CloneIterable<T>>(&mut self, source: &I). This could then be implemented on [T] where T: Clone, and we'd get vec.push_from(slice). But it is a bit complicated.

In any case, while the actual design of these APIs are being hammered out, I think it's really important that we don't use #[deprecated] until we actually have a replacement. Using .extend() instead of .push_all() is a workaround, not a replacement.

Perhaps we need another stability level, something that means "this is a candidate for deprecation, but we don't have a replacement yet". This way we can still mark these APIs appropriately without triggering the deprecation warnings. But I personally think #[experimental = "candidate for deprecation"] is reasonable; APIs marked #[experimental] are already subject to being changed later. The only real difference is new APIs introduced as #[experimental] are candidates for stabilization, whereas we're talking about APIs that are candidates for deprecation. But I don't know if that difference actually matters in any practical sense.

[thestinger]

Please understand that this is not about a power trip, but rather the simple fact that decisions need to be made in a timely fashion and the resulting process is not always perfect.

@aturon: I don't think it's a power trip. I think the chosen development process is flawed. Major overhauls of existing library APIs or conventions should be going through an RFC so the community has a chance to give input. A steering committee should make the final call after hearing all of the facts / opinions, not micro-manage the whole design.

[lilyball]

Oh what I thinking, the associated items RFC includes associated types, that can be used instead of an associated static:

impl<T> Vec<T> {
    pub type raw = VecRawConstructor<T>;
    // ...
}

pub struct VecRawConstructor<T>;

impl<T> VecRawConstructor<T> {
    pub fn from_parts(&self, length: uint, capacity: uint, ptr: *mut T) -> Vec<T> { ... }
}

fn example_usage() {
    let (len, cap, ptr) = ...;
    let v = unsafe { Vec::raw::from_parts(len, cap, ptr) };
    // ...
}

[bluss]

It seems to be a simple tooling issue -- fix it in rustdoc. You wouldn't put "hard to use" but safe methods in a separate submodule, so why here.