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

Already on GitHub? Sign in to your account

split Index into Index, IndexMut and IndexAssign #6515

Closed
thestinger opened this Issue May 15, 2013 · 21 comments

Comments

Projects
None yet
Contributor

thestinger commented May 15, 2013

The current Index isn't great because you have to use &self and return &T or T (via clone) for a map/vector type.

I think it would be best to split it into Index (with &self), IndexMut (with &mut self) and IndexAssign (&mut self, inserting a value).

I'm unsure about the syntax. It would be nice to make it uniform with vector syntax and have &a["foo"] call Index for &T, etc.

Contributor

graydon commented May 23, 2013

Relative of #5992 -- dupe?

Contributor

thestinger commented May 29, 2013

@graydon: not quite, that's more about adding in-place versions of the binary operators with a mention of index-assign

Member

chris-morgan commented Sep 27, 2013

How about foo[bar] += baz? Where will that fit in (with regards to #5992; probably more appropriate there)?

Owner

huonw commented Sep 27, 2013

@chris-morgan I imagine that would be handled by IndexMut i.e. "desugared" to something like foo.index_mut(bar).add_assign(&bar).

Contributor

bluss commented Sep 28, 2013

I'll mention the role of RandomAccessIterator (that is an experimental feature yet to find its proper formulation I think); RAI could be unified with the Index traits. However if traits like RandomAccess/RandomAccessMut are fleshed out, we will also see that the vec iterator and vec slice will start to look very similar.. (But, the random access traits are of course there to apply to much more than just vec iterators).

Edit: To give the traits purpose, for RandomAccess, think binary search, for RandomAccessMut, think generic sort.

Contributor

nikomatsakis commented Nov 15, 2013

cc me, I am working on overloading deref and so on, and this is related. I'll put in the specifics of my proposal in a bit.

Contributor

brson commented Jan 24, 2014

To move vec builders into the library we need overloadable index to be working correctly. Nominating.

@thestinger thestinger referenced this issue Jan 28, 2014

Closed

port from ~[T] to Vec<T> #11875

1 of 3 tasks complete
Member

pnkfelix commented Jan 30, 2014

Accepted for 1.0, P-high.

Contributor

nikomatsakis commented Jan 30, 2014

To be concrete, I think it should work like this.

// 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);
}

These specifications make one crucial assumption: that the implementer can return a reference to the result. This is suitable for many if not all uses cases, but it disallows e.g. an implementer that was going to synthesize the result.

An alternative is to add a fourth trait, corresponding to self[element] used an rvalue:

// self[element] in an rvalue context
trait IndexGet<E,R> {
    fn index_get(&self, element: &E) -> R;
}

which would be intended for those cases where self[element] is used as an rvalue.

I am more-or-less indifferent as to which of these would be best.

Contributor

thestinger commented Jan 30, 2014

@nikomatsakis: I think it would be fine to reserve the syntax for in-memory data structures able to return a reference. As long as it's possible to implement what slices do today in a user-defined type, I'll be happy!

Contributor

wycats commented Jan 30, 2014

@nikomatsakis this looks fine as a starting point.

I think I want this as well:

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

I'm happy to pick up the work for your three traits and see how far I get.

Contributor

nikomatsakis commented Jan 30, 2014

I think it's plausible to have various set traits so long as a type implements at most one (or rather we try them in a specific order, so there's no point in implementing more than one)

Contributor

nikomatsakis commented Jan 30, 2014

I am happy to mentor this bug fyi

Contributor

nikomatsakis commented Feb 11, 2014

With respect to PR #11977, @wycats and I talked privately over IRC some time ago. This current patch doesn't implement the desired semantics. For example, if x implements IndexRef to act like a "vector of T", then x[y] in this patch would have the type &T rather than T (and &x[y] would have type &&T). I'll add a comment shortly detailing what needs to be done (I believe) and in what order.

Contributor

nikomatsakis commented Feb 14, 2014

I've given some more thought to this and I'm not sure that the IndexGet trait that I listed makes sense or is possible. The problem is that, if the type E is POD, you want fn(&self), but the type E is linear, you want fn(self). This seems to be a general difficulty (also with deref traits).

Contributor

nikomatsakis commented Feb 14, 2014

See comments here:

mozilla#7141 (comment)

Most of this applies equally to index. The only difference is the IndexSet trait, intended to support hashtables.

Owner

huonw commented Mar 15, 2014

With the new Vec type, having this working properly would be really nice (although it is still just sugar).

Member

sfackler commented Mar 20, 2014

I'd appreciate the ability to return things by value, personally. It allows for some nice sugar:
https://github.com/sfackler/rust-postgres/blob/537203d3cbd9fe0bf246c65b8d81d7af66568691/src/stmt.rs#L516

Contributor

pcwalton commented Apr 22, 2014

Nominating for backcompat-lang since this will change the defn. of the Index trait.

@brson brson added P-backcompat-lang and removed P-high labels Apr 25, 2014

Contributor

seanmonstar commented May 29, 2014

Does this need a real RFC?

Contributor

pcwalton commented Jun 9, 2014

This is RFC #111.

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 4, 2014

librustc (RFC #34): Implement the new `Index` and `IndexMut` traits.
This will break code that used the old `Index` trait. Change this code
to use the new `Index` traits. For reference, here are their signatures:

    pub trait Index<Index,Result> {
        fn index<'a>(&'a self, index: &Index) -> &'a Result;
    }
    pub trait IndexMut<Index,Result> {
        fn index_mut<'a>(&'a mut self, index: &Index) -> &'a mut Result;
    }

Closes #6515.

[breaking-change]

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 4, 2014

librustc (RFC #34): Implement the new `Index` and `IndexMut` traits.
This will break code that used the old `Index` trait. Change this code
to use the new `Index` traits. For reference, here are their signatures:

    pub trait Index<Index,Result> {
        fn index<'a>(&'a self, index: &Index) -> &'a Result;
    }
    pub trait IndexMut<Index,Result> {
        fn index_mut<'a>(&'a mut self, index: &Index) -> &'a mut Result;
    }

Closes #6515.

[breaking-change]

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 4, 2014

librustc (RFC #34): Implement the new `Index` and `IndexMut` traits.
This will break code that used the old `Index` trait. Change this code
to use the new `Index` traits. For reference, here are their signatures:

    pub trait Index<Index,Result> {
        fn index<'a>(&'a self, index: &Index) -> &'a Result;
    }
    pub trait IndexMut<Index,Result> {
        fn index_mut<'a>(&'a mut self, index: &Index) -> &'a mut Result;
    }

Closes #6515.

[breaking-change]

pcwalton added a commit to pcwalton/rust that referenced this issue Jul 7, 2014

librustc (RFC #34): Implement the new `Index` and `IndexMut` traits.
This will break code that used the old `Index` trait. Change this code
to use the new `Index` traits. For reference, here are their signatures:

    pub trait Index<Index,Result> {
        fn index<'a>(&'a self, index: &Index) -> &'a Result;
    }
    pub trait IndexMut<Index,Result> {
        fn index_mut<'a>(&'a mut self, index: &Index) -> &'a mut Result;
    }

Closes #6515.

[breaking-change]

bors added a commit that referenced this issue Jul 7, 2014

auto merge of #15394 : pcwalton/rust/new-index-traits, r=nick29581
This will break code that used the old `Index` trait. Change this code
to use the new `Index` traits. For reference, here are their signatures:

    pub trait Index<Index,Result> {
        fn index<'a>(&'a self, index: &Index) -> &'a Result;
    }
    pub trait IndexMut<Index,Result> {
        fn index_mut<'a>(&'a mut self, index: &Index) -> &'a mut Result;
    }

Closes #6515.

[breaking-change]

r? @nick29581

@pcwalton pcwalton closed this in 7e4e991 Jul 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment