From 74391a7f87a74420c9a9bdc201c8c3f85cf318f0 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Wed, 12 Oct 2016 12:03:00 +0100 Subject: [PATCH 01/13] add entry-into-owned rfc --- text/0000-entry-into-owned.md | 188 ++++++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 text/0000-entry-into-owned.md diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md new file mode 100644 index 00000000000..30aa5898d27 --- /dev/null +++ b/text/0000-entry-into-owned.md @@ -0,0 +1,188 @@ +- Feature Name: entry_into_owned +- Start Date: 2016-10-12 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +Enable the map Entry API to take borrowed keys as arguments, cloning only when +necessary. The proposed implementation introduces a new trait +`std::borrow::IntoOwned` which enables the existing `entry` methods to accept +borrows. In effect, it makes the following possible: + +```rust + let string_map: HashMap = ...; + let clone_map: HashMap = ...; + let nonclone_map: HashMap = ...; + + // ... + + *string_map.entry("foo").or_insert(0) += 1; // Clones if "foo" not in map. + *string_map.entry("bar".to_string()) += 1; // By-value, never clones. + + clone_map.entry(&Cloneable::new()); // Clones if key not in map. + clone_map.entry(Cloneable::new()); // By-value, never clones. + + nonclone_map.entry(NonCloneable::new()); // Can't and doesn't clone. +``` + +See [playground](https://is.gd/0lpGej) for a concrete demonstration. + +# Motivation +[motivation]: #motivation + +The motivation for this change is the same as the one laid out in [#1533](https://github.com/rust-lang/rfcs/pull/1533) +by @gereeter. Below is an adapted version of their `Motivation` section: + +The Entry API for maps allows users to save time by allowing them to perform +arbitrary modifications at a given key dependent upon whether that key was +present and if it was, what value was previously there. However, although +insertion is the only action the user might take that requires a by-value key, +there is no way to create an Entry without a fully owned key. When the user only +has a by-reference key on hand, this is inefficient at best, requiring an +unnecessary .to_owned that may involve an expensive allocation and/or copy, and +unusable at worst, if the key cannot be cloned. + +Consider a simple word count example: + +```rust +fn word_count(text: &str) -> HashMap { + let mut map = HashMap::new(); + for word in text.split_whitespace() { + *map.entry(word.to_owned()).or_insert(0) += 1; + } + map +} +``` + +For a large enough text corpus, in the vast majority of cases the entry will be +occupied and the newly allocated owned string will be dropped right away, +wasting precious cycles. We would like the following to work. + +```rust +fn word_count(text: &str) -> HashMap { + let mut map = HashMap::new(); + for word in text.split_whitespace() { + *map.entry(word).or_insert(0) += 1; + } + map +} +``` + +with a conditional `.to_owned` call inside the `Entry` implementation. +Specifically we're looking for a fix which supports the following cases + + 1. `.entry(key)` with `key: K` where `K: !Clone`. + 2. `.entry(key)` with `key: K` where `K: Clone`. + 3. `.entry(&key)` with `key: Q` where `Q: ToOwned`. + +# Detailed design +[design]: #detailed-design + +[Playground Proof of Concept](https://is.gd/0lpGej) + +## Approach +To justify the approach taken by this proposal, first consider the following +(unworkable) solution: + +```rust + pub fn entry<'a, C, Q: ?Sized>(&'a self, k: C) -> Entry<'a, K, V> + where K: Borrow, + Q: Hash + Eq + ToOwned + C: Into> +``` + +This would support (2) and (3) but not (1) because `ToOwned`'s blanket +implementation requires `Clone`. To work around this limitation we take a trick +out of `IntoIterator`'s book and add a new `std::borrow::IntoOwned` trait: + +```rust +pub trait IntoOwned { + fn into_owned(self) -> T; +} + +impl IntoOwned for T { + default fn into_owned(self) -> T { self } +} + +impl IntoOwned for T { + default fn into_owned(self) -> T::Owned { self.ref_into_owned() } +} + +trait RefIntoOwned { + type Owned: Sized; + fn ref_into_owned(self) -> Self::Owned; +} + +impl<'a, T: ?Sized + ToOwned> RefIntoOwned for &'a T { + type Owned = ::Owned; + fn ref_into_owned(self) -> T::Owned { (*self).to_owned() } +} +``` + +The auxilliary `RefIntoOwned` trait is needed to avoid the coherence issues +which an + +```rust +impl<'a, T: ?Sized + ToOwned> IntoOwned for &'a T { + fn into_owned(self) -> T::Owned { (*self).to_owned() } +} +``` + +implementation would cause. Then we modify the `entry` signature to + +```rust + pub fn entry<'a, Q>(&'a self, k: Q) -> Entry<'a, K, V, Q> + where Q: Hash + Eq + IntoOwned +``` + +and add a new `Q: IntoOwned` type parameter to `Entry`. This can be done +backwards-compatibly with a `Q=K` default. The new `Entry` type will store +`key: Q` and call `into_owned` on insert-like calls, while using Q directly on +get-like calls. + +# Drawbacks +[drawbacks]: #drawbacks + +1. The docs of `entry` get uglier and introduce two new traits the user + never needs to manually implement. If there was support for `where A != B` + clauses we could get rid of the `RefIntoOwned` trait, but that would still + leave `IntoOwned` (which is strictly more general than the existing `ToOwned` + trait). + +2. It does not offer a way of recovering a `!Clone` key when no `insert` + happens. This is somewhat orthogonal though and could be solved in a number + of different ways eg. an `into_key` method on `Entry` or via an `IntoOwned` + impl on a `&mut Option`-like. + +3. Further depend on specialisation in its current form for a public API. If the + exact parameters of specialisation change, and this particular pattern + doesn't work anymore, we'll have painted ourselves into a corner. + +# Alternatives +[alternatives]: #alternatives + +1. Keyless entries ([#1533](https://github.com/rust-lang/rfcs/pull/1533)): + + 1. Con: An additional method to the Map API which is strictly more general, + yet less ergonomic than `entry`. + + 2. Con: The duplication footgun around having to pass in the same key twice + or risk corrupting the map. + + 3. Pro: Solves the recovery of `!Clone` keys. + +# Unresolved questions +[unresolved]: #unresolved-questions + +1. Should these traits ever be stabilised? `RefIntoOwned` in particular can go + away with the inclusion of `where A != B` clauses: + + ```rust + impl<'a, T: ?Sized + ToOwned> IntoOwned for &'a T + where T::Owned != &'a T + { + fn into_owned(self) -> T::Owned { (*self).to_owned() } + } + ``` From bf03d12e440d95cc3609a17932227f49f27f97bb Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Wed, 12 Oct 2016 13:48:53 +0100 Subject: [PATCH 02/13] fix example in summary --- text/0000-entry-into-owned.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 30aa5898d27..394b45d51f5 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -18,13 +18,13 @@ borrows. In effect, it makes the following possible: // ... - *string_map.entry("foo").or_insert(0) += 1; // Clones if "foo" not in map. - *string_map.entry("bar".to_string()) += 1; // By-value, never clones. + *string_map.entry("foo").or_insert(0) += 1; // Clones if "foo" not in map. + *string_map.entry("bar".to_string()).or_insert(0) += 1; // By-value, never clones. - clone_map.entry(&Cloneable::new()); // Clones if key not in map. - clone_map.entry(Cloneable::new()); // By-value, never clones. + *clone_map.entry(&Cloneable::new()).or_insert(0) += 1; // Clones if key not in map. + *clone_map.entry(Cloneable::new()).or_insert(0) += 1; // By-value, never clones. - nonclone_map.entry(NonCloneable::new()); // Can't and doesn't clone. + *nonclone_map.entry(NonCloneable::new()).or_insert(0) += 1; // Can't and doesn't clone. ``` See [playground](https://is.gd/0lpGej) for a concrete demonstration. From 6952692acf207953572c90ae3b2e001854f80921 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Wed, 12 Oct 2016 14:04:25 +0100 Subject: [PATCH 03/13] fix a couple typos --- text/0000-entry-into-owned.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 394b45d51f5..cec96e6db6f 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -139,7 +139,7 @@ implementation would cause. Then we modify the `entry` signature to and add a new `Q: IntoOwned` type parameter to `Entry`. This can be done backwards-compatibly with a `Q=K` default. The new `Entry` type will store -`key: Q` and call `into_owned` on insert-like calls, while using Q directly on +`key: Q` and call `into_owned` on insert-like calls, while using `Q` directly on get-like calls. # Drawbacks @@ -154,7 +154,7 @@ get-like calls. 2. It does not offer a way of recovering a `!Clone` key when no `insert` happens. This is somewhat orthogonal though and could be solved in a number of different ways eg. an `into_key` method on `Entry` or via an `IntoOwned` - impl on a `&mut Option`-like. + impl on a `&mut Option`-like type. 3. Further depend on specialisation in its current form for a public API. If the exact parameters of specialisation change, and this particular pattern From af7dc426fbbec98ec560d93bca8d69aa3ee799fc Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Wed, 12 Oct 2016 15:29:18 +0100 Subject: [PATCH 04/13] add note on insta-stable and IntoOwned usefulness --- text/0000-entry-into-owned.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index cec96e6db6f..000707614f7 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -149,7 +149,8 @@ get-like calls. never needs to manually implement. If there was support for `where A != B` clauses we could get rid of the `RefIntoOwned` trait, but that would still leave `IntoOwned` (which is strictly more general than the existing `ToOwned` - trait). + trait). On the other hand `IntoOwned` may be independently useful in generic + contexts. 2. It does not offer a way of recovering a `!Clone` key when no `insert` happens. This is somewhat orthogonal though and could be solved in a number @@ -160,6 +161,9 @@ get-like calls. exact parameters of specialisation change, and this particular pattern doesn't work anymore, we'll have painted ourselves into a corner. +4. The implementation would be insta-stable. There's no real way of + feature-gating this. + # Alternatives [alternatives]: #alternatives From 9e35d5eea35a956975631d72519dbc8b095f9b83 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Thu, 13 Oct 2016 18:17:05 +0100 Subject: [PATCH 05/13] change to AsBorrowRef version --- text/0000-entry-into-owned.md | 203 ++++++++++++++++++++++++---------- 1 file changed, 145 insertions(+), 58 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 000707614f7..3a62f8cb26a 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -7,9 +7,9 @@ [summary]: #summary Enable the map Entry API to take borrowed keys as arguments, cloning only when -necessary. The proposed implementation introduces a new trait -`std::borrow::IntoOwned` which enables the existing `entry` methods to accept -borrows. In effect, it makes the following possible: +necessary (in `VacantEntry::insert`). The proposed implementation introduces a +new trait `std::borrow::AsBorrowOf` which enables the existing `entry` methods +to accept borrows. In effect, it makes the following possible: ```rust let string_map: HashMap = ...; @@ -27,7 +27,8 @@ borrows. In effect, it makes the following possible: *nonclone_map.entry(NonCloneable::new()).or_insert(0) += 1; // Can't and doesn't clone. ``` -See [playground](https://is.gd/0lpGej) for a concrete demonstration. +See [playground](https://is.gd/XBVgDe) and [prototype +implementation](https://github.com/rust-lang/rust/pull/37143). # Motivation [motivation]: #motivation @@ -80,8 +81,6 @@ Specifically we're looking for a fix which supports the following cases # Detailed design [design]: #detailed-design -[Playground Proof of Concept](https://is.gd/0lpGej) - ## Approach To justify the approach taken by this proposal, first consider the following (unworkable) solution: @@ -94,75 +93,92 @@ To justify the approach taken by this proposal, first consider the following ``` This would support (2) and (3) but not (1) because `ToOwned`'s blanket -implementation requires `Clone`. To work around this limitation we take a trick -out of `IntoIterator`'s book and add a new `std::borrow::IntoOwned` trait: +implementation requires `Clone`. To work around this limitation we define a +different trait `std::borrow::AsBorrowOf`: ```rust -pub trait IntoOwned { +pub trait AsBorrowOf: Sized where T: Borrow { fn into_owned(self) -> T; + fn as_borrow_of(&self) -> &B; } -impl IntoOwned for T { - default fn into_owned(self) -> T { self } +impl AsBorrowOf for T { + fn into_owned(self) -> T { self } + fn as_borrow_of(&self) -> &Self { self } } -impl IntoOwned for T { - default fn into_owned(self) -> T::Owned { self.ref_into_owned() } +impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { + fn into_owned(self) -> B::Owned { self.to_owned() } + fn as_borrow_of(&self) -> &B { *self } } +``` -trait RefIntoOwned { - type Owned: Sized; - fn ref_into_owned(self) -> Self::Owned; -} +This trait defines a relationship between three types `T`, `B` and `Self` with +the following properties: -impl<'a, T: ?Sized + ToOwned> RefIntoOwned for &'a T { - type Owned = ::Owned; - fn ref_into_owned(self) -> T::Owned { (*self).to_owned() } -} -``` + 1. There is a by-value conversion `Self` -> `T`. + 2. Both `T` and `Self` can be borrowed as `&B`. + +These properties are precisely what we need an `entry` query: we need (2) to +hash and/or compare the query against exiting keys in the map and we need (1) to +convert the query into a key on vacant insertion. + +The two impl-s capture that -The auxilliary `RefIntoOwned` trait is needed to avoid the coherence issues -which an + 1. `T` can always be converted to `T` and borrowed as `&T`. This enables + by-value keys. + 2. `&B` can be converted to `B::Owned` and borrowed as `&B`, when B: + `ToOwned`. This enables borrows of `Clone` types. + +Then we modify the `entry` signature (for `HashMap`, but similar for `BTreeMap`) +to ```rust -impl<'a, T: ?Sized + ToOwned> IntoOwned for &'a T { - fn into_owned(self) -> T::Owned { (*self).to_owned() } +pub fn entry<'a, Q, B>(&'a self, k: Q) -> Entry<'a, K, V, Q> + where Q: AsBorrowOf + K: Borrow, + B: Hash + Eq { + // use `hash(key.as_borrow_of())` and `key.as_borrow_of() == existing_key.borrow()` + // for comparisions and `key.into_owned()` on `VacantEntry::insert`. } ``` -implementation would cause. Then we modify the `entry` signature to +## Detailed changes: -```rust - pub fn entry<'a, Q>(&'a self, k: Q) -> Entry<'a, K, V, Q> - where Q: Hash + Eq + IntoOwned -``` +Also see [working implementation](https://github.com/rust-lang/rust/pull/37143) +for diff. + + 1. Add `std::borrow::Borrow` as described in previous section. + 2. Change `Entry` to add a `Q` type parameter defaulted to `K` for backwards + compatibility (for `HashMap` and `BTreeMap`). + 3. `Entry::key`, `VacantEntry::key` and `VacantEntry::into_key` are moved to a + separate `impl` block to be implemented only for the `Q=K` case. + 4. `Entry::or_insert`, `Entry::or_insert_with` and `VacantEntry::insert` gain + a `B` type parameter and appropriate constraints: `where Q: AsBorrowOf, K: Borrow, B: Hash + Eq`. -and add a new `Q: IntoOwned` type parameter to `Entry`. This can be done -backwards-compatibly with a `Q=K` default. The new `Entry` type will store -`key: Q` and call `into_owned` on insert-like calls, while using `Q` directly on -get-like calls. # Drawbacks [drawbacks]: #drawbacks -1. The docs of `entry` get uglier and introduce two new traits the user - never needs to manually implement. If there was support for `where A != B` - clauses we could get rid of the `RefIntoOwned` trait, but that would still - leave `IntoOwned` (which is strictly more general than the existing `ToOwned` - trait). On the other hand `IntoOwned` may be independently useful in generic - contexts. +1. The docs of `entry` get uglier and introduce a new trait the user + never needs to manually implement. 2. It does not offer a way of recovering a `!Clone` key when no `insert` happens. This is somewhat orthogonal though and could be solved in a number - of different ways eg. an `into_key` method on `Entry` or via an `IntoOwned` - impl on a `&mut Option`-like type. + of different ways eg. an `into_query` method on `Entry`. + +4. The changes to `entry` would be insta-stable (not the new traits). There's + no real way of feature-gating this. -3. Further depend on specialisation in its current form for a public API. If the - exact parameters of specialisation change, and this particular pattern - doesn't work anymore, we'll have painted ourselves into a corner. +5. May break inference for uses of maps where `entry` is the only call (`K` can + no longer be necessarily inferred as the arugment of `entry`). May also hit + issue [#37138](https://github.com/rust-lang/rust/issues/37138). -4. The implementation would be insta-stable. There's no real way of - feature-gating this. +6. The additional `B` type parameter on `on_insert_with` is a backwards + compatibility hazard, since it breaks explicit type parameters + (e.g. `on_insert_with::` would need to become `on_insert_with::`). + This seems very unlikely to happen in practice: F is almost always a closure + and even when it isn't `on_insert_with` can always infer the type of `F`. # Alternatives [alternatives]: #alternatives @@ -177,16 +193,87 @@ get-like calls. 3. Pro: Solves the recovery of `!Clone` keys. +2. Add a `entry_or_clone` with an `Q: Into>` bound. + + 1. Con: Adds a new method as well as new `Entry` types for all maps. + + 2. Con: Passes on the problem to any generic users of maps with every layer + of abstraction needing to provide an `or_clone` variant. + + 3. Pro: probably clearest backwards compatible solution, doesn't introduce + any new traits. + +3. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the + original proposal: + + 1. Con: Requires introducing three new traits. + + 2. Con: Requires specialisation to implement a public API, tying us closer + to current parameters of specialisation. + + 3. Pro: `IntoOwned` may be independently useful as a more general + `ToOwned`. + + 4. Pro: no additional `B` type parameter on `on_insert` and + `on_insert_with`. + +Code: +```rust +pub trait IntoOwned { + fn into_owned(self) -> T; +} + +impl IntoOwned for T { + default fn into_owned(self) -> Self { + self + } +} + +impl IntoOwned for T + where T: RefIntoOwned +{ + default fn into_owned(self) -> T::Owned { + self.ref_into_owned() + } +} + +pub trait AsBorrowOf: IntoOwned where T: Borrow { + fn as_borrow_of(&self) -> &B; +} + +impl AsBorrowOf for T { + default fn as_borrow_of(&self) -> &Self { + self + } +} + +impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { + default fn as_borrow_of(&self) -> &B { + *self + } +} + +// Auxilliary trait to get around coherence issues. +pub trait RefIntoOwned { + type Owned: Sized; + + fn ref_into_owned(self) -> Self::Owned; +} + +impl<'a, T: ?Sized> RefIntoOwned for &'a T + where T: ToOwned +{ + type Owned = ::Owned; + + fn ref_into_owned(self) -> T::Owned { + (*self).to_owned() + } +} + +``` + # Unresolved questions [unresolved]: #unresolved-questions -1. Should these traits ever be stabilised? `RefIntoOwned` in particular can go - away with the inclusion of `where A != B` clauses: - - ```rust - impl<'a, T: ?Sized + ToOwned> IntoOwned for &'a T - where T::Owned != &'a T - { - fn into_owned(self) -> T::Owned { (*self).to_owned() } - } - ``` +1. Are the backwards compatibility hazards acceptable? +2. Is the `IntoOwned` version preferable? From 10f8b13f7feed800190ef3c0633463b96726f76a Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Thu, 13 Oct 2016 22:00:06 +0100 Subject: [PATCH 06/13] fix some phrasing here and there --- text/0000-entry-into-owned.md | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 3a62f8cb26a..8444d180ecf 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -119,16 +119,16 @@ the following properties: 1. There is a by-value conversion `Self` -> `T`. 2. Both `T` and `Self` can be borrowed as `&B`. -These properties are precisely what we need an `entry` query: we need (2) to -hash and/or compare the query against exiting keys in the map and we need (1) to -convert the query into a key on vacant insertion. +These properties are precisely what we need from an `entry` query: we need (2) +to hash and/or compare the query against existing keys in the map and we need (1) +to convert the query into a key on `VacantEntry::insert`. The two impl-s capture that 1. `T` can always be converted to `T` and borrowed as `&T`. This enables - by-value keys. + by-value queries regardless of their `Clone`-ability. 2. `&B` can be converted to `B::Owned` and borrowed as `&B`, when B: - `ToOwned`. This enables borrows of `Clone` types. + `ToOwned`. This enables queries via borrows of `Clone` types. Then we modify the `entry` signature (for `HashMap`, but similar for `BTreeMap`) to @@ -148,13 +148,19 @@ pub fn entry<'a, Q, B>(&'a self, k: Q) -> Entry<'a, K, V, Q> Also see [working implementation](https://github.com/rust-lang/rust/pull/37143) for diff. - 1. Add `std::borrow::Borrow` as described in previous section. + 1. Add `std::borrow::AsBorrowOf` as described in previous section. + 2. Change the signature of `{HashMap,BTreeMap}::entry` to the one described + above. Change the implementation to use `key.as_borrow_of()` to search the + map. 2. Change `Entry` to add a `Q` type parameter defaulted to `K` for backwards - compatibility (for `HashMap` and `BTreeMap`). - 3. `Entry::key`, `VacantEntry::key` and `VacantEntry::into_key` are moved to a + compatibility (for `HashMap` and `BTreeMap`). `VacantEntry` will now store + a query of type `Q` rather than an actual key of type `K`. On `insert` a + call to `Q::into_owned` is made to convert the query into an owned key to + use in the map. + 3. Move `Entry::key`, `VacantEntry::key` and `VacantEntry::into_key` to a separate `impl` block to be implemented only for the `Q=K` case. - 4. `Entry::or_insert`, `Entry::or_insert_with` and `VacantEntry::insert` gain - a `B` type parameter and appropriate constraints: `where Q: AsBorrowOf, K: Borrow, B: Hash + Eq`. + 4. Add to`Entry::or_insert`, `Entry::or_insert_with` and `VacantEntry::insert` + a `B` type parameter with appropriate constraints: `where Q: AsBorrowOf, K: Borrow, B: Hash + Eq`. # Drawbacks @@ -171,7 +177,7 @@ for diff. no real way of feature-gating this. 5. May break inference for uses of maps where `entry` is the only call (`K` can - no longer be necessarily inferred as the arugment of `entry`). May also hit + no longer be necessarily inferred as the argument of `entry`). May also hit issue [#37138](https://github.com/rust-lang/rust/issues/37138). 6. The additional `B` type parameter on `on_insert_with` is a backwards @@ -193,7 +199,7 @@ for diff. 3. Pro: Solves the recovery of `!Clone` keys. -2. Add a `entry_or_clone` with an `Q: Into>` bound. +2. Add a new `entry_or_clone` method with an `Q: Into>` bound. 1. Con: Adds a new method as well as new `Entry` types for all maps. @@ -204,9 +210,9 @@ for diff. any new traits. 3. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the - original proposal: + original proposal in this RFC: - 1. Con: Requires introducing three new traits. + 1. Con: Requires introducing three new traits instead of one. 2. Con: Requires specialisation to implement a public API, tying us closer to current parameters of specialisation. From c5d9df9c38f8e849fa074f2be62971f67c49df6e Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Fri, 14 Oct 2016 14:04:46 +0100 Subject: [PATCH 07/13] add bit about deref coercions --- text/0000-entry-into-owned.md | 38 +++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 8444d180ecf..dd7f36f4e52 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -143,6 +143,42 @@ pub fn entry<'a, Q, B>(&'a self, k: Q) -> Entry<'a, K, V, Q> } ``` +### Deref coercions and backwards compatibility. + +An unexpected backwards compatibility hazard comes from deref coercions. +Consider: + +``` +fn increment<'a>(map: &mut HashMap<&'a str, u32>, key: &'a String) { + *map.entry(key).or_insert(0) += 1; +} +``` + +Currently this compiles just fine: `&'a String` is coerced to `&'a str` because +`String: Deref`, but if `entry` becomes generic, deref coercions stop +working automatically. We can either accept this backwards incompatibility, or +we can use specialisation and introduce a new `AsBorrowOf` impl: + +```rust +// Same as before, but with specialisable `default` methods. +impl AsBorrowOf for T { + default fn into_owned(self) -> T { self } + default fn as_borrow_of(&self) -> &Self { self } +} + +// Allow `&'a T` to be used as queries in a map with `&'a U` keys as long as +// `T: Deref`. +impl<'a, T: Deref> AsBorrowOf<&'a T::Target, T::Target> for &'a T { + default fn into_owned(self) -> &'a T::Target { self.deref() } + default fn as_borrow_of(&self) -> &T::Target { self.deref() } +} + +// ... (impl for `ToOwned` stays unchanged) ... +``` + +I think this `impl` is worth the downside of bringing in specialisation into the +mix, compared to the downside of backwards incompatibility. + ## Detailed changes: Also see [working implementation](https://github.com/rust-lang/rust/pull/37143) @@ -283,3 +319,5 @@ impl<'a, T: ?Sized> RefIntoOwned for &'a T 1. Are the backwards compatibility hazards acceptable? 2. Is the `IntoOwned` version preferable? +3. Do we include the `Deref` impl for `AsBorrowOf` to keep deref coercions + working? From 1e329b66e01ccc99de5e70d05f67ec501ac71eb7 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Fri, 14 Oct 2016 16:32:49 +0100 Subject: [PATCH 08/13] update with knowledge from crater --- text/0000-entry-into-owned.md | 40 ++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index dd7f36f4e52..59f7d0bf963 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -188,15 +188,13 @@ for diff. 2. Change the signature of `{HashMap,BTreeMap}::entry` to the one described above. Change the implementation to use `key.as_borrow_of()` to search the map. - 2. Change `Entry` to add a `Q` type parameter defaulted to `K` for backwards - compatibility (for `HashMap` and `BTreeMap`). `VacantEntry` will now store - a query of type `Q` rather than an actual key of type `K`. On `insert` a - call to `Q::into_owned` is made to convert the query into an owned key to - use in the map. + 2. Change `Entry` to add `Q` and `B` type parameters defaulted to `K` for + backwards compatibility (for `HashMap` and `BTreeMap`). `VacantEntry` will + now store a query of type `Q` rather than an actual key of type `K`. On + `insert` a call to `AsBorrowOf::::into_owned` is made to convert the + query into an owned key to use in the map. 3. Move `Entry::key`, `VacantEntry::key` and `VacantEntry::into_key` to a separate `impl` block to be implemented only for the `Q=K` case. - 4. Add to`Entry::or_insert`, `Entry::or_insert_with` and `VacantEntry::insert` - a `B` type parameter with appropriate constraints: `where Q: AsBorrowOf, K: Borrow, B: Hash + Eq`. # Drawbacks @@ -212,15 +210,18 @@ for diff. 4. The changes to `entry` would be insta-stable (not the new traits). There's no real way of feature-gating this. -5. May break inference for uses of maps where `entry` is the only call (`K` can - no longer be necessarily inferred as the argument of `entry`). May also hit - issue [#37138](https://github.com/rust-lang/rust/issues/37138). +5. Like any change of this nature (changing a concrete parameter to trait bound + type), it is not fully backwards compatibile since it may break inference in + a few places: + * Uses of maps where `entry` is the only call (`K` can no longer be + inferred as the argument of `entry` if `K` is a reference type). + * Uses of `entry(something.into())` may become ambiguous if `something` is + a reference. + * Inference may also hit issue [#37138](https://github.com/rust-lang/rust/issues/37138). -6. The additional `B` type parameter on `on_insert_with` is a backwards - compatibility hazard, since it breaks explicit type parameters - (e.g. `on_insert_with::` would need to become `on_insert_with::`). - This seems very unlikely to happen in practice: F is almost always a closure - and even when it isn't `on_insert_with` can always infer the type of `F`. +6. The additional `B` type parameter on `Entry` is superfluous and exposed to + any non-`Entry` wrappers of `Entry`. It would be great if we + could make `B` an associated type (see 'Unresolved Questions'). # Alternatives [alternatives]: #alternatives @@ -259,6 +260,9 @@ for diff. 4. Pro: no additional `B` type parameter on `on_insert` and `on_insert_with`. +4. Be more honest about the purpose of the trait and call it + `std::collections::Query` with `into_key`, `borrow_as_key` methods. + Code: ```rust pub trait IntoOwned { @@ -321,3 +325,9 @@ impl<'a, T: ?Sized> RefIntoOwned for &'a T 2. Is the `IntoOwned` version preferable? 3. Do we include the `Deref` impl for `AsBorrowOf` to keep deref coercions working? +4. The `B` parameter of `AsBorrowOf` is really inconvenient, especially because + it propagates to the `Entry` types. Conceptually, it's an associated type + (for a `K, Q` pair, there's only one `B` for which `Q: AsBorrowOf`, but + I wasn't able to get coherence to work with a `type Borrowed`. Can it be + done? + From eb63ff84a0e0925802fe65d50b8be989ef855690 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Fri, 14 Oct 2016 18:33:18 +0100 Subject: [PATCH 09/13] fix rust highlighting in deref block --- text/0000-entry-into-owned.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 59f7d0bf963..ed569117cc9 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -143,12 +143,12 @@ pub fn entry<'a, Q, B>(&'a self, k: Q) -> Entry<'a, K, V, Q> } ``` -### Deref coercions and backwards compatibility. +### Deref coercions and backwards compatibility An unexpected backwards compatibility hazard comes from deref coercions. Consider: -``` +```rust fn increment<'a>(map: &mut HashMap<&'a str, u32>, key: &'a String) { *map.entry(key).or_insert(0) += 1; } From 8f41677da9ffb7066387e408e891d6fe1dd7d277 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Fri, 14 Oct 2016 18:34:26 +0100 Subject: [PATCH 10/13] order alternatives correctly with code --- text/0000-entry-into-owned.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index ed569117cc9..943dbf529c0 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -246,7 +246,10 @@ for diff. 3. Pro: probably clearest backwards compatible solution, doesn't introduce any new traits. -3. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the +3. Be more honest about the purpose of the trait and call it + `std::collections::Query` with `into_key`, `borrow_as_key` methods. + +4. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the original proposal in this RFC: 1. Con: Requires introducing three new traits instead of one. @@ -260,9 +263,6 @@ for diff. 4. Pro: no additional `B` type parameter on `on_insert` and `on_insert_with`. -4. Be more honest about the purpose of the trait and call it - `std::collections::Query` with `into_key`, `borrow_as_key` methods. - Code: ```rust pub trait IntoOwned { From a4b35b708295fd745b4c8da574c01d0bce4eb602 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Fri, 14 Oct 2016 21:33:24 +0100 Subject: [PATCH 11/13] introduce another option of getting rid of B --- text/0000-entry-into-owned.md | 160 ++++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 66 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 943dbf529c0..3597db4a76a 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -221,7 +221,9 @@ for diff. 6. The additional `B` type parameter on `Entry` is superfluous and exposed to any non-`Entry` wrappers of `Entry`. It would be great if we - could make `B` an associated type (see 'Unresolved Questions'). + could make `B` an associated type but it doesn't seem possible. Another + option is to remove them only from `Entry`, by replacing `Q, B` with + `Q: Into` with a default of `Q=Query`, where # Alternatives [alternatives]: #alternatives @@ -246,88 +248,114 @@ for diff. 3. Pro: probably clearest backwards compatible solution, doesn't introduce any new traits. -3. Be more honest about the purpose of the trait and call it - `std::collections::Query` with `into_key`, `borrow_as_key` methods. - -4. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the +3. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the original proposal in this RFC: 1. Con: Requires introducing three new traits instead of one. - 2. Con: Requires specialisation to implement a public API, tying us closer - to current parameters of specialisation. - - 3. Pro: `IntoOwned` may be independently useful as a more general - `ToOwned`. - - 4. Pro: no additional `B` type parameter on `on_insert` and - `on_insert_with`. - -Code: -```rust -pub trait IntoOwned { - fn into_owned(self) -> T; -} - -impl IntoOwned for T { - default fn into_owned(self) -> Self { - self - } -} - -impl IntoOwned for T - where T: RefIntoOwned -{ - default fn into_owned(self) -> T::Owned { - self.ref_into_owned() - } -} - -pub trait AsBorrowOf: IntoOwned where T: Borrow { - fn as_borrow_of(&self) -> &B; -} - -impl AsBorrowOf for T { - default fn as_borrow_of(&self) -> &Self { - self - } -} - -impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { - default fn as_borrow_of(&self) -> &B { - *self - } -} - -// Auxilliary trait to get around coherence issues. -pub trait RefIntoOwned { - type Owned: Sized; - - fn ref_into_owned(self) -> Self::Owned; -} + 2. Con: Requires introducing three new traits instead of one. -impl<'a, T: ?Sized> RefIntoOwned for &'a T - where T: ToOwned -{ - type Owned = ::Owned; + 3. Con: Doesn't support the `Deref` trick (might work with lattice rule). - fn ref_into_owned(self) -> T::Owned { - (*self).to_owned() - } -} + 4. Pro: `IntoOwned` may be independently useful as a more general + `ToOwned`. -``` + 5. Pro: no additional `B` type parameter in `Entry`. + + Code: + ```rust + pub trait IntoOwned { + fn into_owned(self) -> T; + } + + impl IntoOwned for T { + default fn into_owned(self) -> Self { + self + } + } + + impl IntoOwned for T + where T: RefIntoOwned + { + default fn into_owned(self) -> T::Owned { + self.ref_into_owned() + } + } + + pub trait AsBorrowOf: IntoOwned where T: Borrow { + fn as_borrow_of(&self) -> &B; + } + + impl AsBorrowOf for T { + default fn as_borrow_of(&self) -> &Self { + self + } + } + + impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { + default fn as_borrow_of(&self) -> &B { + *self + } + } + + // Auxilliary trait to get around coherence issues. + pub trait RefIntoOwned { + type Owned: Sized; + + fn ref_into_owned(self) -> Self::Owned; + } + + impl<'a, T: ?Sized> RefIntoOwned for &'a T + where T: ToOwned + { + type Owned = ::Owned; + + fn ref_into_owned(self) -> T::Owned { + (*self).to_owned() + } + } + + ``` # Unresolved questions [unresolved]: #unresolved-questions 1. Are the backwards compatibility hazards acceptable? + 2. Is the `IntoOwned` version preferable? + 3. Do we include the `Deref` impl for `AsBorrowOf` to keep deref coercions working? -4. The `B` parameter of `AsBorrowOf` is really inconvenient, especially because + +4. Should we be more honest about the purpose of the trait and call it + `std::collections::Query` with `into_key`, `borrow_as_key` methods? + +5. The `B` parameter of `AsBorrowOf` is really inconvenient, especially because it propagates to the `Entry` types. Conceptually, it's an associated type (for a `K, Q` pair, there's only one `B` for which `Q: AsBorrowOf`, but I wasn't able to get coherence to work with a `type Borrowed`. Can it be done? + Another option to get rid of it only in the `Entry` types is to add a + `Q: Into` parameter to `Entry` instead of `Q, B`. The default becomes + `Q=Query` where `Query` is + + ```rust + struct Query { + value: Q, + _phantom: PhantomData, + } + + // Abuse our std-powers to provide a generic `From` impl. + impl From> for K + where K: Borrow, Q: AsBorrowOf + { + fn from(query: Query) -> K { + query.value.into_owned() + } + } + + impl Debug for Query { /* ... */ } + + // etc. + ``` From 12ba17a193000d4e50cc8d12abffa538de40fde4 Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Sat, 15 Oct 2016 18:15:19 +0100 Subject: [PATCH 12/13] fix a duplicated paragraph --- text/0000-entry-into-owned.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 3597db4a76a..9ee56d1dd3f 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -220,10 +220,8 @@ for diff. * Inference may also hit issue [#37138](https://github.com/rust-lang/rust/issues/37138). 6. The additional `B` type parameter on `Entry` is superfluous and exposed to - any non-`Entry` wrappers of `Entry`. It would be great if we - could make `B` an associated type but it doesn't seem possible. Another - option is to remove them only from `Entry`, by replacing `Q, B` with - `Q: Into` with a default of `Q=Query`, where + any non-`Entry` wrappers of `Entry`. See `Unresolved Questions` + for some imperfect ways of removing it. # Alternatives [alternatives]: #alternatives From 0d027ef564629eca058e24213cf0c960829bf5af Mon Sep 17 00:00:00 2001 From: Cristi Cobzarenco Date: Tue, 28 Feb 2017 21:42:46 +0000 Subject: [PATCH 13/13] change to Query and add explicit impl alternative --- text/0000-entry-into-owned.md | 217 ++++++++++++---------------------- 1 file changed, 74 insertions(+), 143 deletions(-) diff --git a/text/0000-entry-into-owned.md b/text/0000-entry-into-owned.md index 9ee56d1dd3f..274feae64e2 100644 --- a/text/0000-entry-into-owned.md +++ b/text/0000-entry-into-owned.md @@ -8,7 +8,7 @@ Enable the map Entry API to take borrowed keys as arguments, cloning only when necessary (in `VacantEntry::insert`). The proposed implementation introduces a -new trait `std::borrow::AsBorrowOf` which enables the existing `entry` methods +new trait `std::collections::Query` which enables the existing `entry` methods to accept borrows. In effect, it makes the following possible: ```rust @@ -27,14 +27,15 @@ to accept borrows. In effect, it makes the following possible: *nonclone_map.entry(NonCloneable::new()).or_insert(0) += 1; // Can't and doesn't clone. ``` -See [playground](https://is.gd/XBVgDe) and [prototype +See [playground](https://is.gd/w2GrUH) and (a slightly out of date) [prototype implementation](https://github.com/rust-lang/rust/pull/37143). # Motivation [motivation]: #motivation -The motivation for this change is the same as the one laid out in [#1533](https://github.com/rust-lang/rfcs/pull/1533) -by @gereeter. Below is an adapted version of their `Motivation` section: +The motivation for this change is the same as the one laid out in +[#1533](https://github.com/rust-lang/rfcs/pull/1533) by @gereeter. Below is an +adapted version of their `Motivation` section: The Entry API for maps allows users to save time by allowing them to perform arbitrary modifications at a given key dependent upon whether that key was @@ -59,7 +60,7 @@ fn word_count(text: &str) -> HashMap { For a large enough text corpus, in the vast majority of cases the entry will be occupied and the newly allocated owned string will be dropped right away, -wasting precious cycles. We would like the following to work. +wasting precious cycles. We would like the following to work: ```rust fn word_count(text: &str) -> HashMap { @@ -94,38 +95,38 @@ To justify the approach taken by this proposal, first consider the following This would support (2) and (3) but not (1) because `ToOwned`'s blanket implementation requires `Clone`. To work around this limitation we define a -different trait `std::borrow::AsBorrowOf`: +different trait `std::collections::Query`: ```rust -pub trait AsBorrowOf: Sized where T: Borrow { - fn into_owned(self) -> T; - fn as_borrow_of(&self) -> &B; +pub trait Query: Sized where K: Borrow { + fn into_key(self) -> K; + fn borrow_as_key(&self) -> &B; } -impl AsBorrowOf for T { - fn into_owned(self) -> T { self } - fn as_borrow_of(&self) -> &Self { self } +impl Query for K { + fn into_key(self) -> K { self } + fn borrow_as_key(&self) -> &Self { self } } -impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { - fn into_owned(self) -> B::Owned { self.to_owned() } - fn as_borrow_of(&self) -> &B { *self } +impl<'a, B: ToOwned + ?Sized> Query for &'a B { + fn into_key(self) -> B::Owned { self.to_owned() } + fn borrow_as_key(&self) -> &B { *self } } ``` -This trait defines a relationship between three types `T`, `B` and `Self` with +This trait defines a relationship between three types `K`, `B` and `Self` with the following properties: - 1. There is a by-value conversion `Self` -> `T`. - 2. Both `T` and `Self` can be borrowed as `&B`. + 1. There is a by-value conversion `Self` -> `K`. + 2. Both `K` and `Self` can be borrowed as `&B`. These properties are precisely what we need from an `entry` query: we need (2) -to hash and/or compare the query against existing keys in the map and we need (1) -to convert the query into a key on `VacantEntry::insert`. +to hash and/or compare the query against existing keys in the map and we need +(1) to convert the query into a key on `VacantEntry::insert`. The two impl-s capture that - 1. `T` can always be converted to `T` and borrowed as `&T`. This enables + 1. `K` can always be converted to `K` and borrowed as `&K`. This enables by-value queries regardless of their `Clone`-ability. 2. `&B` can be converted to `B::Owned` and borrowed as `&B`, when B: `ToOwned`. This enables queries via borrows of `Clone` types. @@ -134,12 +135,12 @@ Then we modify the `entry` signature (for `HashMap`, but similar for `BTreeMap`) to ```rust -pub fn entry<'a, Q, B>(&'a self, k: Q) -> Entry<'a, K, V, Q> - where Q: AsBorrowOf +pub fn entry<'a, Q, B>(&'a self, query: Q) -> Entry<'a, K, V, Q> + where Q: Query K: Borrow, B: Hash + Eq { - // use `hash(key.as_borrow_of())` and `key.as_borrow_of() == existing_key.borrow()` - // for comparisions and `key.into_owned()` on `VacantEntry::insert`. + // use `hash(query.borrow_as_key())` and `query.borrow_as_key() == existing_key.borrow()` + // for comparisions and `query.into_key()` on `VacantEntry::insert`. } ``` @@ -157,20 +158,20 @@ fn increment<'a>(map: &mut HashMap<&'a str, u32>, key: &'a String) { Currently this compiles just fine: `&'a String` is coerced to `&'a str` because `String: Deref`, but if `entry` becomes generic, deref coercions stop working automatically. We can either accept this backwards incompatibility, or -we can use specialisation and introduce a new `AsBorrowOf` impl: +we can use specialisation and introduce a new `Query` impl: ```rust // Same as before, but with specialisable `default` methods. -impl AsBorrowOf for T { - default fn into_owned(self) -> T { self } - default fn as_borrow_of(&self) -> &Self { self } +impl Query for K { + default fn into_key(self) -> K { self } + default fn borrow_as_key(&self) -> &Self { self } } // Allow `&'a T` to be used as queries in a map with `&'a U` keys as long as // `T: Deref`. -impl<'a, T: Deref> AsBorrowOf<&'a T::Target, T::Target> for &'a T { - default fn into_owned(self) -> &'a T::Target { self.deref() } - default fn as_borrow_of(&self) -> &T::Target { self.deref() } +impl<'a, T: Deref> Query<&'a T::Target, T::Target> for &'a T { + default fn into_key(self) -> &'a T::Target { self.deref() } + default fn borrow_as_key(&self) -> &T::Target { self.deref() } } // ... (impl for `ToOwned` stays unchanged) ... @@ -184,19 +185,18 @@ mix, compared to the downside of backwards incompatibility. Also see [working implementation](https://github.com/rust-lang/rust/pull/37143) for diff. - 1. Add `std::borrow::AsBorrowOf` as described in previous section. + 1. Add `std::collections::Query` as described in previous section. 2. Change the signature of `{HashMap,BTreeMap}::entry` to the one described - above. Change the implementation to use `key.as_borrow_of()` to search the - map. + above. Change the implementation to use `query.borrow_as_key()` to search + the map. 2. Change `Entry` to add `Q` and `B` type parameters defaulted to `K` for backwards compatibility (for `HashMap` and `BTreeMap`). `VacantEntry` will now store a query of type `Q` rather than an actual key of type `K`. On - `insert` a call to `AsBorrowOf::::into_owned` is made to convert the + `insert` a call to `Query::::into_key` is made to convert the query into an owned key to use in the map. 3. Move `Entry::key`, `VacantEntry::key` and `VacantEntry::into_key` to a separate `impl` block to be implemented only for the `Q=K` case. - # Drawbacks [drawbacks]: #drawbacks @@ -217,16 +217,46 @@ for diff. inferred as the argument of `entry` if `K` is a reference type). * Uses of `entry(something.into())` may become ambiguous if `something` is a reference. - * Inference may also hit issue [#37138](https://github.com/rust-lang/rust/issues/37138). + * Inference may also hit issue + [#37138](https://github.com/rust-lang/rust/issues/37138). 6. The additional `B` type parameter on `Entry` is superfluous and exposed to - any non-`Entry` wrappers of `Entry`. See `Unresolved Questions` - for some imperfect ways of removing it. + any non-`Entry` wrappers of `Entry`. + +7. The `ToOwned` blanket implementation limits some potentially desirable + `impl`-s like `&[T]: Query, [T]>` since it conflicts with the `Vec` + imlementation # Alternatives [alternatives]: #alternatives -1. Keyless entries ([#1533](https://github.com/rust-lang/rfcs/pull/1533)): +1. A variation on this design would define the `Query` trait as + + ```rust + pub trait Query: Sized where K: Borrow { + type Borrowed: ?Sized; + + fn into_key(self) -> K; + fn borrow_as_key(&self) -> &B; + } + + impl Query for K { + fn into_key(self) -> K { self } + fn borrow_as_key(&self) -> &Self { self } + } + ``` + + instead and dropping the blanket `ToOwned` implementation ([playground link](https://play.rust-lang.org/?gist=9077bce21c3fc05b29cc04dcda6056e8&version=nightly&backtrace=1)). + + This would require explicit `impl`-s for `&[T]: Vec`, `&[T]: Box<[T]>` etc. + but it completely solves drawbacks 6 (superfluous `B` type parameter) and 7 + (inability to define certain `Query`-s like the one for `Box<[T]>`). + + Writing the explicit `impl`-s is a bit of a pain though and adds to the + growing mountain of traits required for a `str`-`String`-like pair: + `As{Ref,Mut}`, `Borrow{,Mut}`, `Deref{,Mut}` and now `Query`. + +2. Keyless entries ([#1533](https://github.com/rust-lang/rfcs/pull/1533)): 1. Con: An additional method to the Map API which is strictly more general, yet less ergonomic than `entry`. @@ -236,7 +266,7 @@ for diff. 3. Pro: Solves the recovery of `!Clone` keys. -2. Add a new `entry_or_clone` method with an `Q: Into>` bound. +3. Add a new `entry_or_clone` method with an `Q: Into>` bound. 1. Con: Adds a new method as well as new `Entry` types for all maps. @@ -246,74 +276,6 @@ for diff. 3. Pro: probably clearest backwards compatible solution, doesn't introduce any new traits. -3. Split `AsBorrowOf` into `AsBorrowOf` and `IntoOwned`. This is closer to the - original proposal in this RFC: - - 1. Con: Requires introducing three new traits instead of one. - - 2. Con: Requires introducing three new traits instead of one. - - 3. Con: Doesn't support the `Deref` trick (might work with lattice rule). - - 4. Pro: `IntoOwned` may be independently useful as a more general - `ToOwned`. - - 5. Pro: no additional `B` type parameter in `Entry`. - - Code: - ```rust - pub trait IntoOwned { - fn into_owned(self) -> T; - } - - impl IntoOwned for T { - default fn into_owned(self) -> Self { - self - } - } - - impl IntoOwned for T - where T: RefIntoOwned - { - default fn into_owned(self) -> T::Owned { - self.ref_into_owned() - } - } - - pub trait AsBorrowOf: IntoOwned where T: Borrow { - fn as_borrow_of(&self) -> &B; - } - - impl AsBorrowOf for T { - default fn as_borrow_of(&self) -> &Self { - self - } - } - - impl<'a, B: ToOwned + ?Sized> AsBorrowOf for &'a B { - default fn as_borrow_of(&self) -> &B { - *self - } - } - - // Auxilliary trait to get around coherence issues. - pub trait RefIntoOwned { - type Owned: Sized; - - fn ref_into_owned(self) -> Self::Owned; - } - - impl<'a, T: ?Sized> RefIntoOwned for &'a T - where T: ToOwned - { - type Owned = ::Owned; - - fn ref_into_owned(self) -> T::Owned { - (*self).to_owned() - } - } - - ``` # Unresolved questions [unresolved]: #unresolved-questions @@ -322,38 +284,7 @@ for diff. 2. Is the `IntoOwned` version preferable? -3. Do we include the `Deref` impl for `AsBorrowOf` to keep deref coercions +3. Do we include the `Deref` impl for `Query` to keep deref coercions working? -4. Should we be more honest about the purpose of the trait and call it - `std::collections::Query` with `into_key`, `borrow_as_key` methods? - -5. The `B` parameter of `AsBorrowOf` is really inconvenient, especially because - it propagates to the `Entry` types. Conceptually, it's an associated type - (for a `K, Q` pair, there's only one `B` for which `Q: AsBorrowOf`, but - I wasn't able to get coherence to work with a `type Borrowed`. Can it be - done? - - Another option to get rid of it only in the `Entry` types is to add a - `Q: Into` parameter to `Entry` instead of `Q, B`. The default becomes - `Q=Query` where `Query` is - - ```rust - struct Query { - value: Q, - _phantom: PhantomData, - } - - // Abuse our std-powers to provide a generic `From` impl. - impl From> for K - where K: Borrow, Q: AsBorrowOf - { - fn from(query: Query) -> K { - query.value.into_owned() - } - } - - impl Debug for Query { /* ... */ } - - // etc. - ``` +4. Do we do alternative 1 or not?