Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Collections reform #235

Merged
merged 7 commits into from Oct 29, 2014
Merged

Conversation

@aturon
Copy link
Member

@aturon aturon commented Sep 11, 2014

This is a combined conventions and library stabilization RFC. The goal is to establish a set of naming and signature conventions for std::collections.

The major components of the RFC include:

  • Removing the traits in collections.
  • A general proposal for solving the "equiv" problem, as well as improving MaybeOwned.
  • Patterns for overloading on by-need values and predicates.
  • Initial, forwards-compatible steps toward Iterable.
  • A coherent set of API conventions across the full variety of collections.

A big thank-you to @gankro, who helped collect API information and worked through an initial pass of some of the proposals here.

Rendered

@aturon
Copy link
Member Author

@aturon aturon commented Sep 11, 2014

cc #17

impl<T: Eq> Predicate<T> for &T {
fn check(&self, t: &T) -> bool {
self == t
Copy link
Contributor

@Gankra Gankra Sep 11, 2014

Pretty sure these need to be dereffed, unless the semantics of operators have changed from underneath me.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 11, 2014

I skimmed/skipped the first 1000 lines assuming it hasn't changed much since last I saw it. Overall I'm a big 👍 to this proposal's contents. Some misc notes that I don't think you ever discussed/addressed:

  • Predicate as written might be better served as a special case of Comparators (on hold pending full unboxed closures). As it stands, it strikes me as odd that equality is "the" element predicate. In particular a function like lower_bound wouldn't be able to use this kind of predicate. This could be more flexible if what we requested was effectively a "partially applied comparator". Or perhaps we can just have a family of traits EqPred, LePred, GePred, etc?
  • Maps love only yielding V from queries in our current design. Would it be possible to yield (K, V) for more map methods? To my knowledge, when you insert a key-value pair, the key always also gets swapped. Some people might want that Key back? With tuple indexing this should be a trivial ergonomic loss. It also means Sets (which are trivial wrappers around maps) can start telling you about their contents beyond "yeah, I got that" or "Okay, I removed something".

@aturon
Copy link
Member Author

@aturon aturon commented Sep 11, 2014

@gankro I addressed the various typos you pointed out.

Predicate as written might be better served as a special case of Comparators (on hold pending full unboxed closures). As it stands, it strikes me as odd that equality is "the" element predicate. In particular a function like lower_bound wouldn't be able to use this kind of predicate. This could be more flexible if what we requested was effectively a "partially applied comparator". Or perhaps we can just have a family of traits EqPred, LePred, GePred, etc?

I don't think methods like lower_bound make sense to take a more general closure, since they rely intrinsically on ordering. I think that's true more generally: if you have a method that can take an arbitrary predicate on a type, it's unlikely that the natural way to interpret an element of that type is via some ordering.

That said, we could consider using PredEq or some such name, to leave room for adding other predicates with different defaults in the future if they turn out to be wanted.

Maps love only yielding V from queries in our current design. Would it be possible to yield (K, V) for more map methods? To my knowledge, when you insert a key-value pair, the key always also gets swapped. Some people might want that Key back? With tuple indexing this should be a trivial ergonomic loss. It also means Sets (which are trivial wrappers around maps) can start telling you about their contents beyond "yeah, I got that" or "Okay, I removed something".

That's certainly doable. It's a tradeoff, since it makes the API somewhat more complex for a pretty rare use case.

My plan for key recovery was to go through your collection view's RFC, which offers a bit more of a swiss army knife for working with maps. We'll have to think about sets, though.

@spernsteiner
Copy link

@spernsteiner spernsteiner commented Sep 11, 2014

It seems to me that it would be more flexible to define Borrow with both type parameters being "inputs", instead of one input and one output. For example:

trait Borrow<Sized? B> {
    fn borrow(&self) -> &B;
}

impl<T: Sized> Borrow<T> for T { ... }
impl Borrow<str> for String { ... }
impl<T> Borrow<[T]> for Vec<T> { ... }

(I'm not 100% clear about the rules for blanket impls post-trait reform, but the one above seems like it ought to work, based on the blanket Borrow impl that is currently in the RFC.)

Now if you define a custom string type, you can still get the benefits of Borrow without needing to define your own wrapper around str:

struct MyString(String);
impl Borrow<str> for MyString { ... }

It also would let you have String borrow to more than one type:

impl Borrow<[u8]> for String { ... }

Then HashMap would look like this:

struct HashMap<K, V>;
impl<K, V> HashMap<K, V> {
    fn insert(&mut self, k: K, v: V) { ... }
    fn find<B>(&self, k: &B) -> Option<&V> where K: Borrow<B> { ... }
}

And you can still define Cow, though it's not as pretty since you need to specify both the owned and borrowed types:

trait FromBorrow<Sized? B>: Borrow<B> {
    fn from_borrow(b: &B) -> Self;
}

enum Cow<'a, T, B: 'a> where T: FromBorrow<B> {
    Owned(T),
    Shared(&'a B),
}

type MaybeOwned<'a> = Cow<'a, String, str>;

Did you consider implementing Borrow this way? I see the "borrowed -> owned" version in the main body of the RFC, and a sort of "owned -> borrowed" version in the alternatives, but nothing with both "owned" and "borrowed" as inputs.

@zkamsler
Copy link

@zkamsler zkamsler commented Sep 12, 2014

I want to like the idea of Borrow, but it can be limiting. It precludes the use of equiv-like keys for HashMap<MyCoolString, int> and the like. String and Vec<T> are not the only things that could be 'borrowed' to str and [T]. They are good defaults, but it feels strange to bless them to the exclusion of everything else.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 12, 2014

Hmm, this just occurred to me. The proposal states we would "deprecate" the traits. Is this intended literally (as in #[deprecated]), or will they just be removed overnight? Due to naming conflicts I'm pretty sure that the traits can't coexist in a deprecated form with the proposed concrete impls.

Or will we have a brief period where the traits are deprecated as a warning, with no (merged) replacement?

@sfackler
Copy link
Member

@sfackler sfackler commented Sep 12, 2014

@gankro:

This RFC proposes a somewhat radical step for the collections traits: rather than reform them, we should eliminate them altogether -- for now.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 12, 2014

@sfackler I assumed that wording was simply referring to the standard deprecation -> removal path. I was (am?) uncertain about the nature of the transition.

@reem
Copy link

@reem reem commented Sep 13, 2014

I might just be being dense, but is there a reason for the name Cow?

@sfackler
Copy link
Member

@sfackler sfackler commented Sep 13, 2014

Copy on write I'd assume.

@Gankra
Copy link
Contributor

@Gankra Gankra commented Sep 14, 2014

One thing I keep remembering and forgetting, and don't really know where to put it, so I'm just going to put here since it's relevant to this work:

Instead of all this lower_bound and upper_bound nonsense, we could probably just provide one method on sorted collections:

/// Creates an iterator over a sub-range of the collection's items, 
/// starting at min, and ending at max. If min is `None`, then it will
/// be treated as "negative infinity", and if max is `None`, then it will
/// be treated as "positive infinity". Thus range(None, None) will yield
/// the whole collection.
fn range(&self, min: Option<&T>, max: Option<&T>) -> SubItems<'a, T>;

This iterator should be DoubleEnded and therefore reversible. This of course doesn't address the inclusive/exclusive scenario, for which we can either do the same thing as range and include an inclusive/exclusive variant, or we can change the Option to a Tristate enum:

Bound<T> {
    Included(T),
    Excluded(T),
    Unbounded,
}

which is the solution I prefer.

methods would go through a reference iterator (i.e. the `iter` method) rather
than a moving iterator.

It is possible to add such methods using the design proposed above, but there

It shouldn't produce a vector. Creating containers is very expensive and lazy iterators should be encouraged. An API producing temporary container will result in countless accidentally temporaries, as demonstrated by the code conventions in the compiler.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Oct 29, 2014

aturon added a commit to aturon/rust that referenced this issue Nov 13, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
aturon added a commit to aturon/rust that referenced this issue Nov 13, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
aturon added a commit to aturon/rust that referenced this issue Nov 14, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
aturon added a commit to aturon/rust that referenced this issue Nov 14, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
bors added a commit to rust-lang/rust that referenced this issue Nov 16, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
aturon added a commit to aturon/rust that referenced this issue Nov 16, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
bors added a commit to rust-lang/rust that referenced this issue Nov 16, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
aturon added a commit to aturon/rust that referenced this issue Nov 16, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
bors added a commit to rust-lang/rust that referenced this issue Nov 17, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
bors added a commit to rust-lang/rust that referenced this issue Nov 17, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
bors added a commit to rust-lang/rust that referenced this issue Nov 17, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
bors added a commit to rust-lang/rust that referenced this issue Nov 17, 2014
Following [the collections reform RFC](rust-lang/rfcs#235), this PR:

* Adds a new `borrow` module to libcore. The module contains traits for borrowing data (`BorrowFrom` and `BorrowFromMut`), generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).

* Deprecates the `_equiv` family of methods on `HashMap` and `HashSet` by instead generalizing the "normal" methods like `get` and `remove` to use the new `std::borrow` infrastructure.

* Generalizes `TreeMap`, `TreeSet`, `BTreeMap` and `BTreeSet` to use the new `std::borrow` infrastructure for lookups.

[breaking-change]
aturon added a commit to aturon/rust that referenced this issue Nov 17, 2014
Following [the collections reform
RFC](rust-lang/rfcs#235),
this commit adds a new `borrow` module to libcore.

The module contains traits for borrowing data (`BorrowFrom` and
`BorrowFromMut`),
generalized cloning (`ToOwned`), and a clone-on-write smartpointer (`Cow`).
alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 22, 2014
Now that rust-lang#19448 has landed in a snapshot, we can add proper by-value operator overloads for `HashSet`.  The behavior of these operator overloads is consistent with rust-lang/rfcs#235.
bors added a commit to rust-lang/rust that referenced this issue May 1, 2015
According to rust-lang/rfcs#235, `VecDeque` should have this method (`VecDeque` was called `RingBuf` at the time) but it was never implemented.

I marked this stable since "1.0.0" because it's stable in `Vec`.
bors added a commit to rust-lang/rust that referenced this issue May 5, 2015
According to rust-lang/rfcs#235, `VecDeque` should have this method (`VecDeque` was called `RingBuf` at the time) but it was never implemented.

I marked this stable since "1.0.0" because it's stable in `Vec`.
bors added a commit to rust-lang/rust that referenced this issue May 5, 2015
According to rust-lang/rfcs#235, `VecDeque` should have this method (`VecDeque` was called `RingBuf` at the time) but it was never implemented.

I marked this stable since "1.0.0" because it's stable in `Vec`.
bors added a commit to rust-lang/rust that referenced this issue May 6, 2015
According to rust-lang/rfcs#235, `VecDeque` should have this method (`VecDeque` was called `RingBuf` at the time) but it was never implemented.

I marked this stable since "1.0.0" because it's stable in `Vec`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet