libs: Add borrow module, deprecate _equiv and friends #18910

aturon · 2014-11-13T00:18:56Z

Following the collections reform RFC, 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 · 2014-11-13T00:19:16Z

aturon · 2014-11-13T00:21:18Z

Note that the design here is somewhat different than the one in the RFC. This was necessary in order to overcome some language limitations: using Borrow instead of BorrowFrom requires full method-level where clauses, which are not yet available.

This will be noted in the tracking issue, and made part of a general amendment to the RFC with implementation tweaks.

aturon · 2014-11-13T00:22:43Z

Also, the deprecation of MaybeOwned in favor of Cow will be left for follow-up work.

nikomatsakis · 2014-11-13T00:26:37Z

Nifty.

aturon · 2014-11-13T00:31:24Z

Another note: the RFC did not mention generalizing the indexing notation using the borrow traits, but of course that only makes sense. This PR does that as well. (Thanks @nikomatsakis for pointing out that it should.)

alexcrichton · 2014-11-13T00:32:20Z

src/libcollections/tree/map.rs

-    pub fn get(&self, key: &K) -> Option<&V> {
-        tree_find_with(&self.root, |k2| key.cmp(k2))
+    pub fn get<Sized? Q>(&self, key: &Q) -> Option<&V>
+    where Q: BorrowFrom<K> + Ord {


This is an interesting stylistic precedent to line up the where and fn keywords, I would have expected the alignment of where to match the alignment of -> if it came on the next line which I thought was the ( in the argument list.

Yeah, I know :-) generally we need to actually firm up all our style conventions, but that's of course lower priority than API issues. I'm happy to go with your recommended style here.

Personally I'd just do a double-indent, like a normal line continuation.

alexcrichton · 2014-11-13T00:34:44Z

Is there a reason you chose BorrowFrom{,Mut} instead of Borrow{,Mut}? (like in the RFC)

Gankra · 2014-11-13T00:56:08Z

src/libcollections/slice.rs

+}
+
+#[unstable = "trait is unstable"]
+impl<T> BorrowFromMut<Vec<T>> for [T] {


Should these be in vec.rs? Genuinely unsure.

Since the BorrowFrom and BorrowFromMut traits are in libcore, they cannot implement for Vec there. Sincce these implementations can't be anywhere else, surely this is the right place?

They could be in the vec module in libcollections. I put them here because they are being applied to the slice type, and I feel like putting impls in the module defining their Self type is a good default.

nikomatsakis · 2014-11-13T01:20:53Z

@aturon it occurs to me that we may be able to implement PathBuf : BorrowFrom<Path> and thus have a hashmap keyed on PathBuf be indexable with a Path. The blanket impl wouldn't conflict because it requires that the two types be equal, and PathBuf != Path. I'm not sure how it would interact with inference.

Gankra · 2014-11-13T01:25:53Z

src/libcore/borrow.rs

+//!
+//! Some types make it possible to go from borrowed to owned, usually by
+//! implementing the `Clone` trait. But `Clone` works only for going from `&T`
+//! to `T`. The `ToOwned` trait generalizes `Clone` to contruct owned data


s/contruct/construct

aturon · 2014-11-13T01:49:35Z

@alexcrichton Note the comments above re: differences from the RFC.

aturon · 2014-11-13T01:50:12Z

@nikomatsakis

@aturon it occurs to me that we may be able to implement PathBuf : BorrowFrom<Path> and thus have a hashmap keyed on PathBuf be indexable with a Path. The blanket impl wouldn't conflict because it requires that the two types be equal, and PathBuf != Path. I'm not sure how it would interact with inference.

Yes! That's very much the plan. Note that the blanket impl implicitly requires sized types, which is enough to avoid the overlap.

Gankra · 2014-11-13T01:57:31Z

src/libcore/borrow.rs

+    fn borrow_from(owned: &T) -> &T { owned }
+}
+
+impl BorrowFrom<&'static str> for str {


What is this necessary for?

This allows you to use things like HashMap<&'static str, T> effectively. However, I don't see any reason to not generalize this to impl<'a, Sized? T> BorrowFrom<&'a T> for T.

Note: there was an issue a while ago (that I just spent far too long failing to find) about the fact that there was no good way to use data structures keyed on reference types like &'static str.

Gankra · 2014-11-13T02:08:15Z

\o/ This is wonderful! 😍

gereeter · 2014-11-13T02:43:04Z

src/libcore/borrow.rs

+    fn borrow_from_mut(owned: &mut Owned) -> &mut Self;
+}
+
+impl<T: Sized> BorrowFrom<T> for T {


I know that the RFC said to use T: Sized here to prevent overlap with the other impls, but now that we have multidispatch this shouldn't be a problem, right?

Gankra · 2014-11-13T02:49:16Z

You seem to have forgotten btree.

Gankra · 2014-11-13T04:05:43Z

Possible thing to include: tuple of Owned -> tuple of Borrowed impls.

SimonSapin · 2014-11-13T15:51:59Z

src/libstd/collections/hash/map.rs

@@ -1045,7 +997,7 @@ impl<K: Eq + Hash<S>, V, S, H: Hasher<S>> HashMap<K, V, H> {
    /// assert_eq!(map.get(&2), None);
    /// ```
    #[unstable = "matches collection reform specification, waiting for dust to settle"]
-    pub fn get(&self, k: &K) -> Option<&V> {
+    pub fn get<Sized? Q>(&self, k: &Q) -> Option<&V> where Q: Hash<S> + Eq + BorrowFrom<K> {


If I understand correctly, this assumes that the implementations of Hash and Eq for Q and K are compatible, right? That is, for any k1 and k2 of type K:

let q1: Q = BorrowFrom::borrow_from(k1); let q2: Q = BorrowFrom::borrow_from(k2); assert_eq!(make_hash(q1), make_hash(k1)); assert_eq!(q1 == q2, k1 == k2);

That invariant unfortunately doesn’t seem like it can be enforced by the type system, but maybe it should be documented?

The problem was the same with Equiv. We ran into it in Servo when trying to make hashing of interned strings O(1) by only hashing the internal u64.

Yes, this was a tradeoff brought up in the RFC. It's possible to enforce this invariant, but at some loss in flexibility: it would force you to consistently use a single kind of borrowed value for lookups.

Agreed about documentation.

aturon · 2014-11-14T00:20:20Z

@gankro @alexcrichton

Pushed a new version; I believe I've addressed all comments.

thestinger · 2014-11-14T01:31:41Z

(C++14 provides methods like find_with, so there is precedence for it)

pythonesque · 2014-11-14T04:38:18Z

Any way Cow could not be done through deref(mut)? It was pointed out on IRC that this would be Rust's first "costly" use of those traits, which at least to me seems like it's sufficient to negate the syntactic simplicity of magic.

alexcrichton · 2014-11-14T05:34:10Z

@thestinger with rust-lang/rfcs#439 we'll have a trait definition that looks like:

pub trait Ord<Rhs = Self>: Eq<Rhs> + PartialOrd<Rhs> {
    fn cmp(&self, other: &Rhs) -> Ordering;
}

in which case I think we can leverage the Rhs type parameter to compare any type to the key type:

impl<K, V> TreeMap<K, V> {
    fn get<Q>(&self, q: &Q) -> Option<&V> where K: Ord<Q> { /* ... */ }
}

impl<T, U: BorrowFrom<T>> Ord<U> for T { /* ... */ }

I'm not entirely sure how much coherence would disallow, but it seems like it should be possible.

aturon · 2014-11-14T17:39:27Z

@pythonesque yes, you're right, supporting DerefMut here does not fit will with our general strategy of making things like clone explicit. (Deref is fine.) I will revise.

aturon · 2014-11-14T17:45:42Z

@pythonesque I've removed the DerefMut impl.

@thestinger I reinstated the find_with methods as #[experimental]; I have no objection to keeping them around under that status for now.

@alexcrichton re-r?

thestinger · 2014-11-14T17:47:01Z

@aturon: Thanks, I'm in full agreement with this pull request then.

TeXitoi · 2014-11-17T15:03:31Z

src/libcore/borrow.rs

+/// A generalization of Clone to borrowed data.
+pub trait ToOwned<Owned> for Sized?: BorrowFrom<Owned> {
+    /// Create owned data from borrowed data, usually by copying.
+    fn to_owned(&self) -> Owned;


Why to_owned must takes a reference?

I think that HashMap::entry() can take a ToOwned<K> as key, and create a owned object if needed. But to have that to work, we should have to_owned taking self to allow T: ToOwned<T> to just move.

Example: http://is.gd/YORUei

I didn't tried to modify this pull request with this change, so maybe there is problem in Cow with this change.

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`).

This commit generalizes methods like `get` and `remove` for `TreeMap` and `TreeSet` to use the new `std::borrow` infrastructure. [breaking-change]

This commit 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. [breaking-change]

This commit handles the fallout from deprecating `_with` and `_equiv` methods.

Generalizes the BTree-based collections to use the new BorrowFrom infrastructure for more flexible lookups and removals.

aturon force-pushed the borrow-traits branch from 6c23d67 to c0e7a32 Compare November 13, 2014 00:30

alexcrichton reviewed Nov 13, 2014
View reviewed changes

Gankra reviewed Nov 13, 2014
View reviewed changes

gereeter reviewed Nov 13, 2014
View reviewed changes

SimonSapin reviewed Nov 13, 2014
View reviewed changes

aturon force-pushed the borrow-traits branch 3 times, most recently from b55a578 to 9c9615a Compare November 14, 2014 00:20

aturon force-pushed the borrow-traits branch from 9c9615a to bbc53d7 Compare November 14, 2014 17:44

aturon force-pushed the borrow-traits branch from bbc53d7 to 2174e57 Compare November 16, 2014 06:46

aturon force-pushed the borrow-traits branch from 2174e57 to 7cf8589 Compare November 16, 2014 22:13

aturon force-pushed the borrow-traits branch from 7cf8589 to 1d88e73 Compare November 17, 2014 00:53

aturon force-pushed the borrow-traits branch 2 times, most recently from 168d74a to a8a8b23 Compare November 17, 2014 03:44

TeXitoi reviewed Nov 17, 2014
View reviewed changes

aturon added 7 commits November 17, 2014 11:26

libcollections: use BorrowFrom in TreeSet, Map

5eec666

This commit generalizes methods like `get` and `remove` for `TreeMap` and `TreeSet` to use the new `std::borrow` infrastructure. [breaking-change]

libstd: Deprecate _equiv methods

80a2867

This commit 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. [breaking-change]

Fallout from deprecation

7213de1

This commit handles the fallout from deprecating `_with` and `_equiv` methods.

libcollections: generalize BTree* to use BorrowFrom

ff88510

Generalizes the BTree-based collections to use the new BorrowFrom infrastructure for more flexible lookups and removals.

Further DSTify Index traits

9f1217d

Remove bogus Duration::span test

46be8eb

aturon force-pushed the borrow-traits branch from 375b205 to 46be8eb Compare November 17, 2014 19:26

bors merged commit 46be8eb into rust-lang:master Nov 18, 2014

aturon mentioned this pull request Nov 18, 2014

Equiv and HashMap need a better interface #12135

Closed

alexcrichton mentioned this pull request Nov 18, 2014

Implement collections reform #18424

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libs: Add borrow module, deprecate _equiv and friends #18910

libs: Add borrow module, deprecate _equiv and friends #18910

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

nikomatsakis commented Nov 13, 2014

aturon commented Nov 13, 2014

alexcrichton Nov 13, 2014

aturon Nov 13, 2014

Gankra Nov 13, 2014

alexcrichton commented Nov 13, 2014

Gankra Nov 13, 2014

Thiez Nov 13, 2014

aturon Nov 13, 2014

nikomatsakis commented Nov 13, 2014

Gankra Nov 13, 2014

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

Gankra Nov 13, 2014

gereeter Nov 13, 2014

gereeter Nov 13, 2014

Gankra commented Nov 13, 2014

gereeter Nov 13, 2014

Gankra commented Nov 13, 2014

Gankra commented Nov 13, 2014

SimonSapin Nov 13, 2014

aturon Nov 13, 2014

aturon commented Nov 14, 2014

thestinger commented Nov 14, 2014

pythonesque commented Nov 14, 2014

alexcrichton commented Nov 14, 2014

aturon commented Nov 14, 2014

aturon commented Nov 14, 2014

thestinger commented Nov 14, 2014

TeXitoi Nov 17, 2014

libs: Add borrow module, deprecate _equiv and friends #18910

libs: Add borrow module, deprecate _equiv and friends #18910

Conversation

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

nikomatsakis commented Nov 13, 2014

aturon commented Nov 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Nov 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 13, 2014

Choose a reason for hiding this comment

aturon commented Nov 13, 2014

aturon commented Nov 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Nov 13, 2014

Choose a reason for hiding this comment

Gankra commented Nov 13, 2014

Gankra commented Nov 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aturon commented Nov 14, 2014

thestinger commented Nov 14, 2014

pythonesque commented Nov 14, 2014

alexcrichton commented Nov 14, 2014

aturon commented Nov 14, 2014

aturon commented Nov 14, 2014

thestinger commented Nov 14, 2014

Choose a reason for hiding this comment