Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

RFC for index traits #111

Merged
merged 2 commits into from
Jun 25, 2014
Merged

RFC for index traits #111

merged 2 commits into from
Jun 25, 2014

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jun 9, 2014

  • Start Date: 2014-06-09
  • RFC PR #: (leave this empty)
  • Rust Issue #: #6515

Summary

Index should be split into Index, IndexMut, and IndexAssign

Motivation

Currently, the Index trait is not suitable for most array indexing tasks. The slice functionality cannot be replicated using it, and as a result the new Vec has to use .get() and .get_mut() methods.

Additionally, this simply follows the Deref/DerefMut split that has been implemented for a while.

Detailed design

We split Index into three traits (borrowed from @nikomatsakis):

// self[element] -- if used as rvalue, implicitly a deref of the result
trait Index<E,R> {
    fn index<'a>(&'a self, element: &E) -> &'a R;
}

// &mut self[element] -- when used as a mutable lvalue
trait IndexMut<E,R> {
    fn index_mut<'a>(&'a mut self, element: &E) -> &'a mut R;
}

// self[element] = value
trait IndexSet<E,V> {
    fn index_set(&mut self, element: E, value: V);
}

Drawbacks

  • The number of lang. items increases.
  • This design doesn't support moving out of a vector-like object.

Alternatives

The impact of not doing this is that the [] notation will not be available to Vec.

Unresolved questions

None that I'm aware of.

@lilyball
Copy link
Contributor

lilyball commented Jun 9, 2014

If a type implements IndexMut but not IndexSet, I take it this means foo[idx] = value is a compile-time error? That seems somewhat unfortunate, because presumably *&mut foo[idx] = value would work for most indexable types, and that looks like it should be semantically equivalent to foo[idx] = value. Could we possibly fall back to *&mut foo[idx] = value behavior if IndexSet is not implemented, so types only need to implement it when they need to special-case the behavior? Alternatively, could we have #[deriving(IndexSet)] that just derives it based on IndexMut?

Note here that I'm expecting that most indexable types will not support setting a value with an index that is not otherwise valid for use with IndexMut. Offhand, only key-value mappings seem like they benefit from the IndexMut/IndexSet split.

I'd also like to know how how e.g. foo[idx] += value is handled. The two possible approaches are using Index + IndexSet, as in foo[idx] = foo[idx] + value, or IndexMut, as *&mut foo[idx] += value. The latter seems like the correct approach (for multiple reasons, including the fact that += and + are not equivalent operators and that user-defined types can't even implement += today), but I would like the RFC to make this clear.

@pcwalton
Copy link
Contributor Author

pcwalton commented Jun 9, 2014

We could do #[deriving(IndexSet)].

Yes, I was assuming that index-assign would desugar into the *&mut foo[idx] thing, as that's forward-compatible with whatever AddSet-like traits we want to add in the future.

@lilyball
Copy link
Contributor

lilyball commented Jun 9, 2014

My preference is to default to *&mut foo[idx] = val automatically if IndexSet isn't implemented, instead of requiring #[deriving(IndexSet)], but I don't know if there are any issues with doing that.

@sfackler
Copy link
Member

sfackler commented Jun 9, 2014

It'd be nice to keep around some trait with the same functionality as the current Index trait, since it isn't always possible to return a reference to something contained in the Self object.

@brendanzab
Copy link
Member

Yes, I was assuming that index-assign would desugar into the *&mut foo[idx] thing, as that's forward-compatible with whatever AddSet-like traits we want to add in the future.

I like that. Could that have a default method implementation? Also, let me put forth IndexAssign as an alternative name.

@lilyball
Copy link
Contributor

lilyball commented Jun 9, 2014

@sfackler I was going to say that there are zero clients of Index, but I checked and it looks like std::collections::bitv::Bitv is now actually the sole client of Index, and relies precisely on what you just mentioned (to return bool).

I'm not sure what the right way to handle this is, however. We could conceivably make Index and IndexMut take the full return value (e.g. &'a R and &'a mut R), but then you could theoretically implement an Index that returns a mutable reference somehow, or an IndexMut that returns a non-mutable reference. There's also the case of e.g. Index returning, say, a RefMut, which seems like a potentially legitimate operation (e.g. an immutable object that contains a RefCell could want to return a RefCell to allow mutation, but it can't implement IndexMut for this because that takes &mut self).

Theoretically, we could even use this to implement Index/IndexMut on RefMut itself, to index into the contained type and wrap the resulting value back up in another Ref/RefMut (to keep the dynamic borrow alive).

@sfackler
Copy link
Member

sfackler commented Jun 9, 2014

}

// self[element] = value
trait IndexSet<E,V> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary said IndexAssign—which is it to be?

For reference, in augmented assignment operators (rust-lang/rust#5992), *Assign is the direction taken thus far, though that can certainly change

@seanmonstar
Copy link
Contributor

Could not an IndexMove trait be included, which is basically the current
Index, to satisfy @sfackler's request?

@sfackler
Copy link
Member

IndexMove is different from the current Index trait in that Index takes &self where IndexMove would take self.

@schmee
Copy link

schmee commented Jun 10, 2014

Will this work for Map types as well? So you could do map[key] = value?

@chris-morgan
Copy link
Member

I was thinking further about how this would work with Teepee’s header representation scheme; I’m presuming that this along with #48 will allow me to use proper index assignation at the least. But I won’t be able to use Index, because my getting function necessarily takes &mut self. Applying this generally, I presume that if one was to implement IndexMut but not Index, foo[bar] as an rvalue would not work?

I think I’m going to be stuck with being able to write headers[FOO] = Foo but not being able to access it with headers[FOO]. Ah well; such is life. (This is certainly not an argument against these indexing traits at all.)

@bill-myers
Copy link

What about changing the syntax from a[i] to a(i) and then using the Fn and FnMut traits from unboxed closures, to which one could add an FnMutSet for setting?

This would also let Bitv return bool

@nikomatsakis
Copy link
Contributor

On Thu, Jun 12, 2014 at 09:47:08PM -0700, bill-myers wrote:

What about changing the syntax from a[i] to a(i) and
then using the Fn and FnMut traits from unboxed closures, to which
one could add an FnMutSet for setting?

The problem with this is that indexing expressions are lvalues,
whereas calls are not. This leads to a rather different signature for
indexing vs calls (indexing implicitly reference a reference with the
same lifetime as its input). So we'd need to have more closure traits,
basically.

I had originally thought we might want to add a fourth kind of Index
trait (IndexGet?) for indexable things are not lvalues, like bitv,
but your suggestion has me thinking that it might be better to just
use the closure traits, as you suggest (though this doesn't permit
bitv(i) = 10, but we could add that as an extension at some point in
the future if we wanted).

The other impact of your proposed change, of course, would be to move
away from the C notion of [] for indexing and go towards the more
functional notion of () that is used by scala, clojure, etc. I see
some appeal in that but it is a change to the character of the
language somehow.

pcwalton added a commit that referenced this pull request Jun 25, 2014
@pcwalton pcwalton merged commit c9c0fdb into rust-lang:master Jun 25, 2014
@pcwalton pcwalton deleted the index-traits branch June 25, 2014 00:30
@alexcrichton
Copy link
Member

This was discussed today and we decided to merge with just two traits.

@wycats
Copy link
Contributor

wycats commented Jun 25, 2014

I really would like IndexSet. I don't see a strong rationale in the notes not to include it.

@huonw
Copy link
Member

huonw commented Jun 25, 2014

@wycats mainly because the exact design hasn't been worked out, and we're not letting that get in the way of progress (since having Index and IndexMut for [] is a definite and really really wanted now). That is, it's not immediately clear if we want a separate trait or something else like a (default) method on IndexMut.

@sfackler
Copy link
Member

Is the current Index going to stick around under another name?

@Centril Centril added A-traits Trait system related proposals & ideas A-operator Operators related proposals. A-traits-libstd Standard library trait related proposals & ideas and removed A-traits Trait system related proposals & ideas labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-operator Operators related proposals. A-traits-libstd Standard library trait related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.