-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce remaining Index traits #159
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
- Start Date: 2014-07-02 | ||
- RFC PR #: | ||
- Rust Issue #: | ||
|
||
# Summary | ||
|
||
[RFC #34](https://github.com/rust-lang/rfcs/pull/111) added two traits for | ||
index-operator overloading: `Index` and `IndexMut`. However, some critical | ||
functionality remains missing. Specifically, the ability to assign to an index | ||
into a structure, the ability to move out of an index into a structure and the | ||
ability to return by value from an index into a structure. | ||
|
||
# Motivation | ||
|
||
There are two distinct cases to handle: collections which can return references | ||
to elements and those that cannot. For example, the `Vec` type can return | ||
references to its elements, but `BitvSet` cannot as the boolean elements are | ||
stored in a non-addressable compressed form. A cache is another example of a | ||
non-addressable but indexable collection. References cannot be handed out | ||
since cached values will be purged at arbitrary times, so it would probably | ||
need to return a smart pointer such as `Rc` instead. | ||
|
||
# Detailed design | ||
|
||
RFC #34 defines two traits: | ||
```rust | ||
pub trait Index<E, R> { | ||
fn index<'a>(&'a self, element: &E) -> &'a R; | ||
} | ||
|
||
pub trait IndexMut<E, R> { | ||
fn index_mut<'a>(&'a mut self, element: &E) -> &'a mut R; | ||
} | ||
``` | ||
|
||
Two additional traits will be added: `IndexGet` and `IndexSet`. In addition, | ||
the type of the second argument to the `index` and `index_mut` methods will | ||
be changed from `&E` to `E`. This is not strictly necessary, but will make the | ||
traits more flexible than they were before (E can always be set to `&'a Foo` to | ||
match the old functionality). In addition, `index_set` *must* take its second | ||
argument by value if it is to work with types like `HashMap`. | ||
Here are the four `Index` traits: | ||
```rust | ||
pub trait Index<E, R> { | ||
fn index<'a>(&'a self, element: E) -> &'a R; | ||
} | ||
|
||
pub trait IndexMut<E, R>: Index<E, R> { | ||
fn index_mut<'a>(&'a mut self, element: E) -> &'a mut R; | ||
} | ||
|
||
pub trait IndexGet<E, R> { | ||
fn index_get(self, element: E) -> R; | ||
} | ||
|
||
pub trait IndexSet<E, R> { | ||
fn index_set(&mut self, element: E, value: R); | ||
} | ||
``` | ||
|
||
Note that `IndexGet` handles both the move and by-value use cases, as one can | ||
implementing it for either `T` or `&T`. It's theoretically possible to remove | ||
`Index` and `IndexMut` in favor of `IndexGet` but I feel that it | ||
overcomplicates the implementation (especially as to how the compiler's | ||
supposed to handle `&foo[idx]` and `&mut foo[idx]`. | ||
|
||
# Drawbacks | ||
|
||
This adds several new lang items and adds complexity to the language as a | ||
whole. | ||
|
||
# Alternatives | ||
|
||
There was some discussion in the previous RFC about how index-set should be | ||
implemented. One alternative mentioned would be to add `index_set` as a method | ||
in `IndexMut` with a default implementation: | ||
```rust | ||
pub trait IndexMut<E, R> { | ||
fn index_mut<'a>(&mut self, element: &E) -> &'a mut V; | ||
|
||
fn index_set(&mut self, element: E, value: V) { | ||
*self.index_mut(&element) = value; | ||
} | ||
} | ||
``` | ||
|
||
However, this does not handle the by-value case. We can restructure and rename | ||
the traits a bit and end up with this: | ||
```rust | ||
pub trait IndexRef<E, R> { | ||
fn index<'a>(&'a self, element: &E) -> &'a V; | ||
} | ||
|
||
pub trait IndexRefMut<E, R>: IndexRef<E, R> { | ||
fn index_mut<'a>(&mut self, element: &E) -> &'a mut V; | ||
|
||
fn index_set(&mut self, element: E, value: V) { | ||
*self.index_mut(&element) = value; | ||
} | ||
} | ||
|
||
pub trait IndexValue<E, R> { | ||
fn index(&self, element: &E) -> R; | ||
} | ||
|
||
pub trait IndexValueMut<E, R>: IndexValue<E, R> { | ||
fn index_set(&mut self, element: E, value: V); | ||
} | ||
|
||
pub trait IndexMove<E, R> { | ||
fn index_move(self, element: &E) -> R; | ||
} | ||
``` | ||
|
||
The compiler would forbid the implementation of both `IndexRef` and | ||
`IndexValue` for the same type. | ||
|
||
This is a bit more straightforward - there is a clear separation between the | ||
by-ref collections and the by-value collections. It's also immediately clear | ||
how one would implement a by-value set - the pattern of implementing a trait | ||
with a method that takes `self` by value on `&T` has no parallel in the | ||
standard library that I'm aware of at this time. On the other hand, it is a | ||
larger set of traits. On the other hand, it is significantly less flexible. | ||
|
||
# Unresolved questions | ||
|
||
How should compound assignment be handled? For types that implement `IndexMut`, | ||
we can easily expand `a[b] += c` to `*a.index_mut(b) += c`. The implementation | ||
for types which only implement `IndexGet` and `IndexSet` is a bit more | ||
complicated. One option is to simply expand to `a.index_set(b, a.index_get(b) + | ||
c)`, but I'd imagine there are situations in which that is needlessly | ||
inefficient. It's similar to the old desugaring of `a += b` to `a = a + b` | ||
which was removed because of the inefficiency. It may be best to punt on this | ||
question until the situation with compound assignment in general is worked out. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
IndexMut<E, R>
requireIndex<E, R>
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the upgrade mechanism required to fix the
DerefMut
-in-immutable-contexts issue.Or is it downgrade? In any case, one of
IndexMut
orIndex
is picked, and depending on whether the result is required to be a mutable lvalue (e.g. method call with&mut self
) or not, the operation will be adjusted to use the more appropriate trait.