From b95ae6b514cd3f21fa77c46c59c7a9a3e755da45 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Tue, 19 May 2015 18:29:13 -0500 Subject: [PATCH 1/2] IndexAssign: overloading the `a[b] = c` expression --- text/0000-index-assign.md | 325 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 325 insertions(+) create mode 100644 text/0000-index-assign.md diff --git a/text/0000-index-assign.md b/text/0000-index-assign.md new file mode 100644 index 00000000000..cb5c7dcd686 --- /dev/null +++ b/text/0000-index-assign.md @@ -0,0 +1,325 @@ +- Feature Name: indexed_assignment +- Start Date: 2015-05-19 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary + +Add a `IndexAssign` trait that allows overloading "indexed assignment" expressions like `a[b] = c`. + +# Motivation + +Let users define syntactic sugar for operations like these: + +- Inserting a *new* key-value pair into a map + +``` rust +let mut map = HashMap::new(); +map[key] = value; // equivalent to `map.insert(key, value);` +``` + +- Setting each element of a "slice" to some value + +(slice, as in a fraction of a collection, which may not necessarily be stored in contiguous memory) + +``` rust +let mut matrix = { .. }; + +// Set each element of the second row of `matrix` to `1` +matrix[1] = 1; + +// or + +let mut vector = { .. }; + +// set first 4 elements of `vector` to zero +vector[..4] = 0; +``` + +- Copying a slice into another + +``` rust +// Copy the third row of `another_matrix` into the second row of `matrix` +matrix[1] = &another_matrix[2] + +// or + +// Copy the first four elements of `another_vector` in the middle of `vector` +vector[2..6] = &another_vector[..4] +``` + +Also, before 1.0, `BTreeMap` and `HashMap` lost their `IndexMut` implementations to +[future-proof indexing on maps]. With this feature, it would be possible to re-implement `IndexMut` +on these maps and additionally implement `IndexAssign` on them, such that all these operations will +work: + +[future-proof indexing on maps]: https://github.com/rust-lang/rust/pull/23559 + +``` rust +// insert new entry (`IndexAssign`) +map[key] = value; + +// apply mutating method to the value associated with `key` (`IndexMut`) +map[&key].foo_mut(); + +// get a mutable reference to the value associated with `key` (`IndexMut`) +&mut map[&key]; +``` + +# Detailed design + +## The trait + +The `IndexAssign` trait will be added to the core crate, and re-exported in the std crate. Its +signature is shown below. + +``` rust +#[lang = "index_assign"] +trait IndexAssign { + /// `self[index] = rhs` + fn index_assign(&mut self, index: Index, rhs: Rhs); +} +``` + +## Type checking `a[b] = c` + +Today, the expression `a[b] = c` is always "evaluated" as an assignment, where the LHS may be +evaluated: + +- using "built-in" indexing (which is only applicable to the types `[T]` and `[T; N]`), or +- using the `IndexMut` trait, i.e. as `*a.index_mut(b)` + +The type check section of the compiler will choose which evaluation form to use based on the types +of `a`, `b` and `c`, and the traits that `a` implements, or raise an error if neither form is +applicable. + +To additionally support evaluating `a[b] = c` as `a.index_assign(b, c)`, the type checking logic +will be *extended* as follows: + +> Just like today, try to evaluate `a[b] = c` as an assignment, if the expression can't be +> evaluated as an assignment, then instead of raising an error, try to evaluate the expression +> as an indexed assignment using the `IndexAssign` trait. + +Here's an example of how type checking will work: + +``` rust +struct Array([i32; 32]); + +impl IndexMut> for Array { + fn index_mut(&mut self, r: Range) -> &mut [i32] { + &mut self.0[r] + } +} + +impl IndexAssign, i32> for Array { + fn index_assign(&mut self, r: Range, rhs: i32) { + for lhs in &mut self[r] { + *lhs = rhs; + } + } +} + +// type check as assignment +// `IndexMut>` is not applicable because RHS is `i32`, expected `[i32]` +// type check as indexed assignment +// `IndexAssign, i32>` is applicable +// -> Expression will be evaluated as `array.index_assign(4..10, 0)` +array[4..10] = 0; +``` + +From the extended type check logic, it follows that in the case that both `IndexMut` and +`IndexAssign` are applicable, the `IndexMut` implementation will be favored [1]. + +``` rust +impl IndexMut for Array { + fn index_mut(&mut self, i: usize) -> &mut i32 { + &mut self.0[i] + } +} + +impl IndexAssign for Array { + fn index_assign(&mut self, _: usize, _: i32) { + unreachable!() + } +} + +// type check as assignemnt +// `IndexMut` is applicable +// -> Expression will be evaluated as `*array.index_mut(0) = 1` +array[0] = 1; +``` + +## Feature gating + +The feature itself will land behind a `indexed_assignment` feature gate, and the `IndexAssign` +trait will be marked as unstable under a `index_assign` feature gate. + +The expression `a[b] = c` will be gated only if it must be evaluated using the `IndexAssign` trait. + +An example below: + +``` rust +// aux.rs + +// required to implement the unstable `IndexAssign` trait +#![feature(index_assign)] + +pub struct Map(HashMap); + +impl IndexAssign for Map { .. } +``` + +``` rust +// main.rs + +extern crate aux; + +use aux::Map; + +fn main() { + let map: Map = { .. }; + + // would be evaluated as `map.index_assign(0, 1)` + map[0] = 1; //~ error: overloaded indexed assignment is unstable + //~^ help: add `#![feature(indexed_assignment)]` to enable + + let mut v = vec![0, 1, 2]; + // This is OK, because `v[0]` goes through the `IndexMut` trait + // will be evaluated as `*v.index_mut(0) = 1` + v[0] = 1; +} +``` + +## Changes in the standard library + +The `IndexMut` implementations of `HashMap` and `BTreeMap` will be restored, and additionally both +maps will implement the `IndexAssign` trait such that `map[key] = value` will become sugar +for `map.insert(key, value)`. + +## Backward compatibility + +Adding this feature is a backward compatible change because expressions like `a[b] = c` that work +today, will continue to work with unaltered semantics. + +The proposed library changes are also backward compatible, because they will enable expressions +like `map[&key] = value` and `map[key] = value` which don't compile today. + +# Drawbacks + +None that I can think of + +# Alternatives + +## Bridge `IndexAssign` and `IndexMut` + +Because `IndexMut` has "higher priority" than `IndexAssign`, it's possible to (unintentionally?) +change the semantics of the `a[b] = c` expression when a `IndexMut` implementation is added [2]. +For example: + +``` rust +struct Map(..); + +impl IndexAssign for Map { + fn index_assign(&mut self, key: i32, value: i32) { + println!("via IndexAssign"); + .. + } +} + +// Expression will be evaluated as `map.index_assign(0, 1)` +map[0] = 1; // prints "via IndexAssign" + +// But if this implementation is added +impl IndexMut for Map { + fn index_mut(&mut self, k: i32) -> &mut i32 { + panic!("no indexing for you") + } +} + +// Now the expression will be evaluated as `*map.index_mut(0) = 1` +map[0] = 1; // nows panics +``` + +This hazard (?) can be avoided by "bridging" the `IndexMut` and `IndexAssign` traits with a blanket +implementation: + +``` rust +impl IndexAssign for T where + T: IndexMut, +{ + fn index_assign(&mut self, idx: Idx, rhs: T::Output) { + *self.index_mut(idx) = rhs; + } +} +``` + +Now it's impossible to implement `IndexMut` on a type `A`, if it already implements +`IndexAssign` and vice versa. + +However this blanket implementation creates coherence problems for the planned changes to +`BTreeMap` and `HashMap`: + +``` rust +// NOTE Omitting all the bounds for simplicity + +// "Mutable" lookup +impl<'a, K, V> IndexMut<&'a K> for HashMap { + // Output = V; + fn index_mut(&mut self, k: &K) -> &mut V { .. } +} + +// By extension: HashMap also implements IndexAssign<&K, V> + +// Insertion +impl IndexAssign for HashMap { +//~^ this conflicts with the other `IndexAssign` implementation, because the `K` in this +// `IndexAssign` includes all the references of the form `&'a _` + fn index_assign(&mut self, k: K, v: V) { .. } +} +``` + +So it's not a viable alternative. + +# Unresolved questions + +None so far + +--- + +### Author notes + +[1] The compiler does something similar when type checking an expression where both built-in +indexing and the `Index[Mut]` trait are applicable - it favors built-in indexing. + +[2] `a[b]` is another expression where one can change its semantics by implementing a trait: + +``` rust +struct Array([i32; 32]); + +impl Deref for Array { + type Target = [i32; 32]; + + fn deref(&self) -> &[i32; 32] { + println!("via Deref") + &self.0 + } +} + +// Will be evaluated as `array.deref()[0]` +array[0]; // prints "via Deref" + +impl Index for Array { + type Output = i32; + + fn index(&self, _: usize) -> &i32 { + panic!("no indexing for you") + } +} + +// Now will be evaluated as `*array.index(0)` +array[0]; // now panics +``` + +However, I don't think either case is a problem in practice. It seems unlikely that a library +author will purposefully override the semantics of an operator, and it seems less likely that they +would do it unintentionally, without triggering a unit test failure. From 7a32545504ab184925c0c5a123d93a2c4effb6f9 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Mon, 21 Sep 2015 23:17:42 -0500 Subject: [PATCH 2/2] change main proposal to the no fallback alternative --- text/0000-index-assign.md | 233 ++++++++++++++++++-------------------- 1 file changed, 112 insertions(+), 121 deletions(-) diff --git a/text/0000-index-assign.md b/text/0000-index-assign.md index cb5c7dcd686..0815108b08d 100644 --- a/text/0000-index-assign.md +++ b/text/0000-index-assign.md @@ -83,72 +83,68 @@ trait IndexAssign { ## Type checking `a[b] = c` -Today, the expression `a[b] = c` is always "evaluated" as an assignment, where the LHS may be -evaluated: +The type checker logic will be extended as follows: -- using "built-in" indexing (which is only applicable to the types `[T]` and `[T; N]`), or -- using the `IndexMut` trait, i.e. as `*a.index_mut(b)` +> Whenever the expression `a[b] = c` is encountered, the compiler will check if `A` (`a` has type +> `A`) has *any* implementation of the `IndexAssign` trait; if that's the case then it will proceed +> to look for an applicable implementation and evaluate the expression as `a.index_assign(b, c)`, +> or, if no implementation is applicable, it will raise an error. On the other hand, if `A` doesn't +> have any `IndexAssign` implementation then the compiler will use today's logic: evaluate the +> expression as an assignment where the LHS is evaluated as an lvalue using either built-in +> indexing or the `IndexMut` trait. -The type check section of the compiler will choose which evaluation form to use based on the types -of `a`, `b` and `c`, and the traits that `a` implements, or raise an error if neither form is -applicable. - -To additionally support evaluating `a[b] = c` as `a.index_assign(b, c)`, the type checking logic -will be *extended* as follows: - -> Just like today, try to evaluate `a[b] = c` as an assignment, if the expression can't be -> evaluated as an assignment, then instead of raising an error, try to evaluate the expression -> as an indexed assignment using the `IndexAssign` trait. - -Here's an example of how type checking will work: +Three cases are worth analyzing: ``` rust -struct Array([i32; 32]); - -impl IndexMut> for Array { - fn index_mut(&mut self, r: Range) -> &mut [i32] { - &mut self.0[r] - } -} +// A +impl IndexAssign for Foo { .. } +impl Index for Foo { type Output = Baz; .. } +impl IndexMut for Foo { .. } + +let (foo, bar, baz): (Foo, Bar, Baz); +// .. +foo[bar] = baz; +``` -impl IndexAssign, i32> for Array { - fn index_assign(&mut self, r: Range, rhs: i32) { - for lhs in &mut self[r] { - *lhs = rhs; - } - } -} +Here `Foo` has an applicable `IndexAssign` implementation, so `foo[bar] = baz` is evaluated as +`foo.index_assign(bar, baz)`. Note that the `IndexMut` implementation is ignored even though +`*foo.index_mut(bar) = baz` is a valid evaluation form of `foo[bar] = baz`. Finally, one can use +the `*&mut foo[bar] = baz` expression to use `IndexMut` instead of `IndexAssign`. -// type check as assignment -// `IndexMut>` is not applicable because RHS is `i32`, expected `[i32]` -// type check as indexed assignment -// `IndexAssign, i32>` is applicable -// -> Expression will be evaluated as `array.index_assign(4..10, 0)` -array[4..10] = 0; +``` rust +// B +impl IndexAssign for Foo { .. } +impl Index for Foo { type Output = Quux; .. } +impl IndexMut for Foo { .. } + +let (foo, baz, quux): (Foo, Baz, Quux); +// .. +foo[baz] = quux; +//~^ error: expected `Bar`, found `Baz` ``` -From the extended type check logic, it follows that in the case that both `IndexMut` and -`IndexAssign` are applicable, the `IndexMut` implementation will be favored [1]. +In this case, `Foo` has an `IndexAssign` implementation but it's not applicable to +`foo[baz] = quux` so a compiler error is raised. Although the expression could have been evaluated +as `*foo.index_mut(baz) = quux`, the compiler won't attempt to "fall back" to the `IndexMut` trait. +See the alternatives section for a version of this RFC where the compiler does fall back to +`IndexMut`. ``` rust -impl IndexMut for Array { - fn index_mut(&mut self, i: usize) -> &mut i32 { - &mut self.0[i] - } -} - -impl IndexAssign for Array { - fn index_assign(&mut self, _: usize, _: i32) { - unreachable!() - } -} +// C +impl Index for Foo { type Output = Baz; .. } +impl IndexMut for Foo { .. } -// type check as assignemnt -// `IndexMut` is applicable -// -> Expression will be evaluated as `*array.index_mut(0) = 1` -array[0] = 1; +let (foo, bar, baz): (Foo, Bar, Baz); +// .. +foo[bar] = baz; ``` +The third case points out a breaking-change hazard to library authors. If the author adds e.g. +`impl IndexAssign for Foo` to their library, the change will break all the downstream +crates that use the `IndexMut` implementation in the form of `foo[bar] = baz`. To prevent +breakage, the author must add a `IndexAssign` implementation (that preserves the +semantics of `foo[bar] = baz`) to `Foo` before adding any other `IndexAssign` implementation. + ## Feature gating The feature itself will land behind a `indexed_assignment` feature gate, and the `IndexAssign` @@ -202,46 +198,80 @@ Adding this feature is a backward compatible change because expressions like `a[ today, will continue to work with unaltered semantics. The proposed library changes are also backward compatible, because they will enable expressions -like `map[&key] = value` and `map[key] = value` which don't compile today. +like `map[&key].foo_mut()` and `map[key] = value` which don't compile today. # Drawbacks -None that I can think of +There is sugar for map insertion (`map[xey] = value`), but not for updating the value associated to +an existing key (some people actually consider that this is actually a good thing). The closest +thing to sugar for the update operation is `*&mut map[&key] = value`, which is totally not obvious. -# Alternatives - -## Bridge `IndexAssign` and `IndexMut` - -Because `IndexMut` has "higher priority" than `IndexAssign`, it's possible to (unintentionally?) -change the semantics of the `a[b] = c` expression when a `IndexMut` implementation is added [2]. -For example: +This situation could be improved using an extension trait (which doesn't even need to be defined in +the `std` crate): ``` rust -struct Map(..); +/// `mem::replace` as a method +trait Swap { + /// `mem::replace(self, new_value)` + fn swap(&mut self, new_value: Self) -> Self; +} -impl IndexAssign for Map { - fn index_assign(&mut self, key: i32, value: i32) { - println!("via IndexAssign"); - .. +impl Swap for T { + fn swap(&mut self, value: T) -> T { + mem::replace(self, value) } } -// Expression will be evaluated as `map.index_assign(0, 1)` -map[0] = 1; // prints "via IndexAssign" +let mut map: HashMap; +let (new_thing, another_new_thing) = (Thing, Thing); +// .. +// Update the value associated to `"foo"`, the old value is discarded +map["foo"].swap(new_thing); // instead of the more obscure `*&mut map["foo"] = new_thing` -// But if this implementation is added -impl IndexMut for Map { - fn index_mut(&mut self, k: i32) -> &mut i32 { - panic!("no indexing for you") - } -} +// As a bonus, you can actually retrieve the old value +let old_thing = map["bar"].swap(another_new_thing); +``` + +# Alternatives + +## Fall back to `IndexMut` + +As shown in the case B of the type checking section, when checking `a[b] = c` the compiler will +error if none of the `IndexAssign` implementations is applicable, even if a `IndexMut` +implementation could have been used. This alternative proposes falling back to the `IndexMut` +trait in such scenario. Under this alternative the case B example would compile and `foo[baz] = +quux` would be evaluated as `*foo.index_mut(baz) = quux`. -// Now the expression will be evaluated as `*map.index_mut(0) = 1` -map[0] = 1; // nows panics +The most visible consequence of this change is that we'd have sugar for updating a key value pair +in a map: + +``` rust +map[key] = value; // insert a `(key, value)` pair +map[&key] = new_value; // update the value associated to `key` ``` -This hazard (?) can be avoided by "bridging" the `IndexMut` and `IndexAssign` traits with a blanket -implementation: +However, some people deem this as a footgun because its easy to confuse the insertion operation +and the update one resulting in programs that always panic: + +``` rust +let (key, value): (&Key, Value); +// .. +let mut map: HashMap = HashMap::new(); +map[key] = value; // Compiles, but always panics! +``` + +The programmer meant to write `map[key.clone()] = value` to use the insertion operation, but they +won't notice the problem until the program crashes at runtime. + +For more details about this alternative check the previous git revision of this RFC, where this +alternative was the main proposal. + +## Bridge `IndexAssign` and `IndexMut` + +As shown in the case C of the type checking section adding an `IndexAssign` implementation to a +struct that already implements the `IndexMut` trait can cause breakage of downstream crates if one +is not careful. This hazard can be eliminated by "bridging" the `IndexMut` and `IndexAssign` traits +with a blanket implementation: ``` rust impl IndexAssign for T where @@ -253,8 +283,9 @@ impl IndexAssign for T where } ``` -Now it's impossible to implement `IndexMut` on a type `A`, if it already implements -`IndexAssign` and vice versa. +Now it's impossible to forget to implement `IndexAssign` on a type `A` that already +implements `IndexMut` because it's automatically handled by the blanket +implementation. However this blanket implementation creates coherence problems for the planned changes to `BTreeMap` and `HashMap`: @@ -283,43 +314,3 @@ So it's not a viable alternative. # Unresolved questions None so far - ---- - -### Author notes - -[1] The compiler does something similar when type checking an expression where both built-in -indexing and the `Index[Mut]` trait are applicable - it favors built-in indexing. - -[2] `a[b]` is another expression where one can change its semantics by implementing a trait: - -``` rust -struct Array([i32; 32]); - -impl Deref for Array { - type Target = [i32; 32]; - - fn deref(&self) -> &[i32; 32] { - println!("via Deref") - &self.0 - } -} - -// Will be evaluated as `array.deref()[0]` -array[0]; // prints "via Deref" - -impl Index for Array { - type Output = i32; - - fn index(&self, _: usize) -> &i32 { - panic!("no indexing for you") - } -} - -// Now will be evaluated as `*array.index(0)` -array[0]; // now panics -``` - -However, I don't think either case is a problem in practice. It seems unlikely that a library -author will purposefully override the semantics of an operator, and it seems less likely that they -would do it unintentionally, without triggering a unit test failure.