IndexAssign: overloading the a[b] = c expression

[japaric]

Rendered

Implementation: rust-lang/rust#25628

cc @sfackler @gankro
cc @rust-lang/lang-team cc @nikomatsakis @aturon @huonw @nrc @pnkfelix
cc #997

[Gankra]

I would consider different terminology. subset?

[japaric]

Yeah, subset sounds better

[Gankra]

Clarification request: is this RFC actually proposing we provide foo[a..] = &bar[c..d] as a supported operation, or just stating it as a hypothetical usecase?

[nrc]

There seems to be a distinct level of sugar-iness here that feels a bit unexpected to me in Rust - we've got a single expression on the right and we are setting multiple values on the left of the assignment. I don't think we should allow that (or at least we should not encourage it).

[japaric]

This sugar is common in languages aimed at numerical/scientific computing see NumPy, Julia and Matlab/Octave. It would be helpful for libraries that want to compete in the same space to be able to provide a similar syntax to ease adoption/porting.

I don't think that this form of sugar will become common in other areas/mainstream Rust.

[japaric]

@gankro

Clarification request: is this RFC actually proposing we provide foo[a..] = &bar[c..d] as a supported operation, or just stating it as a hypothetical usecase?

No, the only proposed changes to the standard libraries are the ones to HashMap/BTreeMap.

This syntax is stated as a potential use case for linear algebra libraries. See #1129 (comment)

[Gankra]

This basically LGTM from the what-collections-want perspective. I'm a bit squeemish about the example usages, but such is operator-overloading-lyfe. I don't think the interaction between IndexMut and IndexAssign are particularly worrying in practice, as the RFC notes.

I'm gonna regret this but: why not IndexSet? This would pair more cleanly with the hypothetical IndexGet that e.g. BitVec wants.

[ftxqxd]

I agree with the general idea, but I’d very much prefer IndexAssign to have a higher priority over IndexMut. Unless there’s some technical reason for this, I see no reason for it to be otherwise—it makes sense to use the most specific trait possible in all cases so that the implementer has maximum control over how its collection behaves.

[japaric]

@gankro

I'm gonna regret this but: why not IndexSet? This would pair more cleanly with the hypothetical IndexGet that e.g. BitVec wants.

I'm sort of grouping IndexAssign with AddAssign and co, you know +=, -=, ..., and []=. But, don't worry we can still pair IndexMove (renamed from IndexGet) with DerefMove.

(I'm actually fine with either name.)

---

@P1start Now that you mention it, yes, it makes sense to reverse the priorities. I'll test that locally and let you know, but I don't expect any difficulties.

[sfackler]

It does seem a bit footgunny that foo[&key] = value and foo[key] = value look so similar but one will panic if key isn't in foo.

[pythonesque]

I'm in favor of this RFC, because I view it as a special case of *Assign (which I'd like for most of Rust's exposed operators). In fact, I'm wondering whether this wouldn't be a good opportunity to add assignment traits for all the current operators (though I realize this is not in scope for this RFC at present, it seems a bit arbitrary to just add IndexAssign to me).

[japaric]

@pythonesque FYI, there's already an RFC about overloading all the other augmented assignment operators (+= and co).

@sfackler Yeah. I don't know how much would help but if we give IndexAssign higher priority than IndexMut (as @P1start suggested) then we could show a custom panic message when map[&key] = value fails, perhaps "can't update entry, key not found. (hint: if you want to add a new entry, pass the key by value, not by reference)".

[japaric]

@P1start FYI, I tested changing priorities in this branch, and all test continue to pass.

[petrochenkov]

I agree with the general idea, but I’d very much prefer IndexAssign to have a higher priority over IndexMut. Unless there’s some technical reason for this, I see no reason for it to be otherwise—it makes sense to use the most specific trait possible in all cases so that the implementer has maximum control over how its collection behaves.

+1

And a small question: does (a[b]) = c trigger IndexAssign? (I guess no.) Or index_macro!(a, b) = c (I guess yes.)

[japaric]

And a small question: does (a[b]) = c trigger IndexAssign? (I guess no.)

It doesn't work in the current implementation, but given that (a[b]) = c does work with IndexMut, then it should also work with IndexAssign. Otherwise, a[b] = c would use IndexAssign, and (a[b]) = c) would use IndexMut.

What doesn't work and won't work with IndexMut is { a[b] } = c, because the block will be evaluated to an rvalue, and you can't have an rvalue in the LHS of an assignment. Likewise, IndexAssign shouldn't trigger in that case either.

Or index_macro!(a, b) = c (I guess yes.)

Yes, if the macro expands to a[b]. Macros are expanded early on, so the type checker only sees the expanded code and not the macro.

[cristicbz]

Thanks for another great RFC @japaric!

I think the map[&k] vs map[k] footgun is very worrying. In particular since whether k is a reference or not is not always immediately obvious and can change when refactoring things e.g. inside iterator chains (or anywhere else where a reference type is inferred).

I am for this RFC, but also for recommending that any implementation of a[k]=b should be functionally equivalent to *(&mut a[k]) = b (this is similar to how AddAssign a+=b should be functionally equivalent to a = a + b). So I think the IndexAssign impl for HashMap should panic for missing keys (similar to how for a Vec an out of bounds assignment panics just as much as out of bounds access).

EDIT: Removed broken example code; it wasn't hugely enlightening anyway.

[Gankra]

If IndexAssign panics on missing keys, then it's basically useless. The whole point is that it's sugar for insertion.

[cristicbz]

@gankro: It would be useless for standard maps. BitVec and the other use cases @japaric mentions (linear algebra etc) would stay.

How about IndexAssign taking Cow? There would be no silent ambiguity, requiring map[key.into_cow()] = value, and it would only perform the allocation if needed.

• The risk of making by-value vs by-ref indexing have wildly different behaviour (to the point of crashing) for me outweighs any advantages of insertion sugar; maybe other people are better at this, but I always forget to ref/deref things and I'm glad the compiler points it out.
• I also think the consistency argument with out of bounds access for Vec<> is also a salient one.
• Also having a[x] = b be different to *(&mut a[x]) = b in general, spells trouble to me.

[Florob]

Are there any thoughts on how this interacts with placement-new?
As I understand this proposal map[key] = C::new() would move the value around twice in a worst case scenario (calling index_assign() and calling insert(), though one would usually expect some inlining). Ideally we'd want the C to be constructed in place, without being moved. The current proposal for that is along the lines of in map.place(key) { C::new() }. This is getting into a region where C++ seems far better of with push_back() vs. emplace_back().

[nikomatsakis]

Regarding the map[&k] vs map[k] distinction, I don't necessarily consider that a footgun. In particular, I rather like that the notation makes it very clear whether the key is being consumed (map[k] = ...) or merely borrowed (map[&k]).

I am interested in the comments by @Florob about placement-new, though. It's a good thing to consider. It's true that it might be nice to write in map[key] {expr} instead.

cc @pnkfelix

[bluss]

The notation map[k] does not remind you of what the type of k is though, if it's pointer-to-key or key by value, that's the trap.

[gsingh93]

@nikomatsakis That is unless the k in map[k] is already borrowed, in which case it's still a footgun.

[nikomatsakis]

@bluss ah, ok, I see. You're right, I was overlooking that (or missed it).

[nikomatsakis]

So it seems like basically the fate of this RFC boils down to the question of the "hashmap footgun", where map[k] = v has subtly different semantics depending on whether k is a &K or a K? Are there other objections?

If the footgun is a concern, one possible fix is to say that if we see an expression where IndexSet is applicable (i.e., map[k] = v), then we search for an impl of IndexSet using just the type of the map, with a fresh type variable for the key. If this succeeds, we check that k matches the type of this type variable. We do not attempt to fallback to IndexMut. This means it will be an error to do map[k] = v if k: &K. You would have to do *&mut map[k] = v to get those semantics.

[nikomatsakis]

ps I've not re-read the RFC so I apologize if I'm misremembering something. Just trying to get conversation going again here. I personally... think I prefer the "do not fallback to IndexMut" variant. Basically collections fall into two categories: maps, where map[k] = v is an IndexSet, and vecs, where vec[k] = v is *&mut vec[k] = v. I don't see a lot of likely confusion between the two, and overwriting an existing entry in a map is a relatively unusual occurrence, so it's okay that it's done more explicitly.

[sfackler]

I think that seems reasonable to me.

[bluss]

(Using type ascription syntax to illustrate.)

None of the two ways presented to go about map[k: K] = v vs map[k: &K] = v are perfect, but I think @nikomatsakis's suggestion seems to be ok. It has less traps, but is it too annoying to encounter when you don't know it yet?

Can we allow this: *map[k: &K] = v to trigger IndexMut? (Edit: No, that would be weird, didn't realize. I'm not so fond of *&mut map[k] = v.)

[Gankra]

One interesting subtlety is that IndexMut is potentially more efficient than IndexSet on maps: you don't need to maintain the state to maybe insert. I believe bluss's BTreeMap rewrite would nullify the difference (state need not be maintained because we have parent pointers). I believe any differences are also negligible for HashMap.

[japaric]

I've updated the RFC! The main proposal is now the "no fallback" alternative proposed by @nikomatsakis.

I'm not so fond of *&mut map[k] = v.

@bluss Check the updated drawbacks section, I suggest using map[k].swap(v) instead of *&mut map[k] = v.

My dreams are really for an IndexGet.

@retep998 Could you post your use case(s) (and desired IndexGet/index_get signature: self or &mut self?) at #997? I've thought about IndexGet before but it wasn't clear to me how to handle ambiguous cases like a[b].consume() where both IndexGet and Index may be applicable.

[sfackler]

This is totally unrelated to the current RFC, but the use cases I see for IndexGet would be in places where no Index impl could exist. We could make them mutually exclusive and avoid weird ambiguity issues.

[retep998]

@japaric https://github.com/NoLifeDev/nx-rs/blob/master/src/node.rs#L72-L92
Since I cannot return elements from the array in memory directly, but rather I have to use a wrapper type with some other extra data, I am unable to return a &, so Index is completely unusable for me. IndexGet, which as I imagine is the same as Index but without the &, would allow me to get [] indexing on that type. Even if it's mutually exclusive with Index, that's enough for me.

[japaric]

@retep998 Thanks. In that case, indexing doesn't freeze self so that feature can be implemented today without HKT. Something like this:

trait IndexNoHKT<Idx> {
    type Output;

    /// `self[i]` === `self.index(i)`
    fn index(&self, i: Idx) -> Self::Output;
}

trait IndexMutNoHKT<Idx> {
    type Output;

    /// `self[mut i]` === `self.index_mut(i)`
    fn index_mut(&mut self, i: Idx) -> Self::Output;
}

// Your example
impl<'a, 's> IndexNoHKT<&'s str> for Node<'a> {
    type Output = Option<Node<'a>>;
    // `index` would be same as your `Node::get` method
}

Plus the restriction that @sfackler mentioned: if you implement any of Index*NoHKT on a type then you can't implement Index on it and vice versa (I think this would be the trickiest part of implementing the feature).

If we had HKT we could define a more general set of traits:

trait IndexHKT<Idx> {
    // Hypothetical syntax. This is a type constructor that takes a lifetime
    // as input and returns a concrete type
    type Output<'a>;

    /// `self[i]` === `self.index(i)`
    fn index<'s>(&'s self, i: Idx) -> Self::Output<'s>;
}

trait IndexMutHKT<Idx> {
    type Output<'a>;

    /// `self[mut i]` === `self.index_mut(i)`
    fn index_mut<'s>(&'s mut self, i: Idx) -> Self::Output<'s>;
}

These would let us implement indexing that does freeze self, like Index does today, but would allow returning something else than a reference.

Below, an example that can't be implemented with Index or with IndexNoHKT

struct BoxedSlice<T>(Box<[T]>);
struct Slice<'a, T>(&'a [T]);

impl<T> IndexHKT<Range<usize>> for BoxedSlice<T> {
    type Output<'a> = Slice<'a, T>;

    fn index<'s>(&'s self, _: Range<usize>) -> Slice<'s, T> { .. }
    // Note tha `'s` appears in the output so this operation freezes `self`
}

This set of traits can also handle your non-freezing use case.

impl<'a, 's> IndexNoHKT<&'s str> for Node<'a> {
    type Output<'_> = Option<Node<'a>>;

    fn index<'e>(&'e self, _: &'s str) -> Option<Node<'a>> { .. }
}

Given that the set of HKT traits is more flexible than the non-HKT one, I wonder if we can add the non-HKT set now and deprecate it later in favor of the HKT one, or somehow just upgrade the non-HKT traits to use HKT in a backward compatible way.

[nikomatsakis]

I'm of two minds about DerefGet/IndexGet. On the one hand, I see the
appeal; on the other hand, I like that we can syntactically identify
lvalues now. An alternative to IndexGet would be for people to simply
overload () notation: so instead of DerefGet, you could implement Fn(),
and instead of IndexGet, you could implement Fn(I) (or even multiple
arguments). So you could access matrix(22, 33) or matrix(22)(33)
instead of matrix[22][33]. However, it would not work for lvalues like
matrix(22, 33) = 1, we'd need some sort of sugar for that.

On Wed, Sep 23, 2015 at 1:55 AM, Jorge Aparicio [email protected]
wrote:

@retep998 https://github.com/retep998 Thanks. In that case, indexing
doesn't freeze self so that feature can be implemented today without HKT.
Something like this:

trait IndexNoHKT {
type Output;

/// `self[i]` === `self.index(i)`
fn index(&self, i: Idx) -> Self::Output;

}
trait IndexMutNoHKT {
type Output;

/// `self[mut i]` === `self.index_mut(i)`
fn index_mut(&mut self, i: Idx) -> Self::Output;

}
// Your exampleimpl<'a, 's> IndexNoHKT<&'s str> for Node<'a> {
type Output = Option<Node<'a>>;
// index would be same as your Node::get method
}

Plus the restriction that @sfackler https://github.com/sfackler
mentioned: if you implement any of Index*NoHKT on a type then you can't
implement Index on it and vice versa (I think this would be the trickiest
part of implementing the feature).

If we had HKT we could define a more general set of traits:

trait IndexHKT {
// Hypothetical syntax. This is a type constructor that takes a lifetime
// as input and returns a concrete type
type Output<'a>;

/// `self[i]` === `self.index(i)`
fn index<'s>(&'s self, i: Idx) -> Self::Output<'s>;

}
trait IndexMutHKT {
type Output<'a>;

/// `self[mut i]` === `self.index_mut(i)`
fn index_mut<'s>(&'s mut self, i: Idx) -> Self::Output<'s>;

}

These would let us implement indexing that does freeze self, like Index
does today, but would allow returning something else than a reference.

Below, an example that can't be implemented with Index or with IndexNoHKT

struct BoxedSlice(Box<[T]>);struct Slice<'a, T>(&'a [T]);
impl IndexHKT<Range> for BoxedSlice {
type Output<'a> = Slice<'a, T>;

fn index<'s>(&'s self, _: Range<usize>) -> Slice<'s, T> { .. }
// Note tha `'s` appears in the output so this operation freezes `self`

}

This set of traits can also handle your non-freezing use case.

impl<'a, 's> IndexNoHKT<&'s str> for Node<'a> {
type Output<'_> = Option<Node<'a>>;

fn index<'e>(&'e self, _: &'s str) -> Option<Node<'a>> { .. }

}

Given that the set of HKT traits is more flexible than the non-HKT one, I
wonder if we can add the non-HKT set now and deprecate it later in favor of
the HKT one, or somehow just upgrade the non-HKT traits to use HKT in a
backward compatible way.

—
Reply to this email directly or view it on GitHub
#1129 (comment).

[nikomatsakis]

Seems like this RFC has been hanging around. To be honest I've sort of forgotten the trade-offs, but from what I can see the RFC has been updated. @aturon what do you think, shall we try to move forward with this now? I think one question is whether there are people who are interested in implementing it. It's not a blocker if there are not, of course, but most of the main rustc hackers seem to have their hands full at the moment.

[retep998]

I think one question is whether there are people who are interested in implementing it.

Have you not noticed the implementation conveniently provided by the author of this RFC?

[brendanzab]

I'm in favor - would be most useful for cgmath. Currently you have to do ugly derefs in order to assign using IndexMut.

[nikomatsakis]

On Thu, Nov 05, 2015 at 03:12:10PM -0800, Peter Atashian wrote:

I think one question is whether there are people who are interested in implementing it.

Have you not noticed the implementation conveniently provided by the author of this RFC?

I've noticed it, yes, but last time I checked it needed to be rebased.

[aturon]

This RFC is entering its week-long Final Comment Period. 💬

[Florob]

I'm still very wary of this proposals interaction with placement. I'd rather not have:

• map.insert(k, v)
• map[k] = v
• map.place(k) <- v

as three roughly equivalent alternatives.
In particular this likely adds a new non-preferred way to add a new entry in a data structure in the long run.

[aturon]

@Florob And of course, that's not counting the entry API. Though, on reflection, I think that the Entry type could actually just be a placer, so you'd have map.entry(k) <- v (which reads pretty nicely imo, once you know what entry is).

It is interesting that, in the same way this RFC special-cases indexing and assignment as a pair, you could imagine special-casing indexing and placement: map[k] <- v. Having that and the map[k] = v syntaxes both available doesn't seem like the worst outcome to me, though I take your point that most experience Rustaceans will use <- due to performance perceptions.

To me there are two main motivations for this RFC:

• Fixing a papercut for newcomers. Once you learn how [] works for vectors and that you use the same notation to read from maps, it's natural to expect to be able to insert into maps with the same notation (as you find in many other languages). Maps being an extremely common data structure, we've worked hard to make them as ergonomic and unsurprising as we can, and this is an additional avenue for doing so.
• More interesting overloads. The RFC talks about matrix assignments, for example, which of course have less overlap with the other mechanisms you mention but are also fairly subtle uses of the syntax.

I think both of these motivations point to needs that cannot be addressed by placement alone. Personally, I find both motivations reasonably compelling, and am willing to trade that against the "more than one way to do it" with placement (especially if we can get a symmetric placement syntax like map[k] <- v).

My biggest worry about the RFC is the subtle rules regarding when to use IndexMut and when to use IndexAssign. I remember some previous iteration just disallowing both traits to be implemented on overlapping Self types, resulting in a less expressive but simpler spec. I wonder if we might at least start there, and see how often we want to reach for IndexMut on things like maps. I suspect that with swiss army knives like entry, there are more clear ways to get a pointer out anyway (and we could streamline doing so).

[nagisa]

My biggest worry about the RFC is the subtle rules regarding when to use IndexMut and when to use IndexAssign.

We already have some troubles selecting between Intex and IndexMut. The relevant code (and thus associated corner cases) are surely to become more complex with addition of IndexAssign.

---

Being a proponent of placement-in, I’d like to wait and see how emplacement works out for this use-case, and if it doesn’t IndexAssign could then be implemented as necessary (we would also know the requirements better then).

I also do not see how IndexAssign covers more cases than placement even with @aturon’s comment above, but I didn’t digest this RFC for long enough either.

[bstrie]

I generally like the idea behind this RFC, but I second (third) the concern that this has overlap with placement-in. Given that we already know that we definitely want placement-in regardless, I'd be willing to postpone this until after we've had time to experiment with placement-in and determine if this RFC is still necessary.

However, that's working under the assumption that placement-in is coming "sometime soon" (say... six months or less?), which I really really hope is true.

[pnkfelix]

@bstrie

that's working under the assumption that placement-in is coming "sometime soon" (say... six months or less?)

I infer you must be talking about support for placement-in on the various stdlib types?

(See also rust-lang/rust#30172 )

Update: Or are you asking about when the placement-in feature will be stabilized ?

[nikomatsakis]

We discussed this in the @rust-lang/lang meeting yesterday. In general, there are some mixed feelings about the design:

• Having an "if-else-if" style rule in the typechecker can complicate inference and other features, and so is to be approached with caution.
  • However, processing expressions like foo[] already requires that e.g. the type of foo is sufficiently known to resolve the Index trait, so it is likely that the impact here would be minimal.
• The potential proliferation of different ways to insert into maps, as described by @retep998 here, remains a concern.
  • However, the fact remains that as long as you can write foo[key], people are likely to attempt foo[key] = value. Though @aturon suggested that merely detecting this scenario and giving an error that led the user to call insert might be enough to address this papercut in practice.

All that said, it seems clear that HashMap should never implement IndexMut unless we add a trait like this, since otherwise map[k] = v represents a very serious footgun. Furthermore, nobody has any ideas for an alternative design that would avoid any of the obstacles above.

Overall, this seems like a case where accepting the RFC might allow us to gain the experience we need to reach a final decision. However, even if we had a feature gate, we still couldn't add IndexMut or IndexAssign impls for HashMap, since there is currently no way to mark impls as unstable. This means that if we added an IndexMut impl, code like &mut map[k] would work, even on stable code -- and then, if we decided against adopting IndexAssign permanently, we'd still be stuck with the IndexMut impl.

Even if a feature-gated impl is not used in libstd, though, it might allow experimentation in external libraries. Having convincing (and working) examples of matrix abstractions, or other types that rely on IndexAssign for usability, would probably make a big difference here, since currently the motivation for HashMap feels relatively thin and is clouded by the proliferation of "insert-like" APIs.

Ultimately, we didn't really reach a final decision, but we plan to discuss a bit more next week. In the meantime, any thoughts on the above comments would be most appreciated.

Other notes:

• One idea that was floated is that perhaps we could simply disallow a single type from implementing both IndexMut and IndexAssign, but @huonw pointed out that this is not desirable, since hashmaps want IndexAssign to support map[k] = v as well as IndexMut to support &mut map[k].

[daniel-vainsencher]

+1 to adding the RFC, feature gating, experimenting in libraries.

I use Rust for numerics, hence also matrices (currently scirust, also looking at others). I think that full, thoughtful support for convenient slicing, including stuff like

X[r,0..] = a_row 

and
mut row = X[r,0..]
would be a serious boon to my work; numerical algorithms are complicated enough even when the code is pretty transparent.

Multidimensional arrays, dataframes make completely unreasonable demands on this syntax in other language; finding the right tradeoffs for Rust will not happen in a week, maybe even not in a year, but it is important work.

[pliniker]

I'm also on favor of adding this behind a feature gate for library experimentation. I totally see the hesitation in adding this, particularly in the context of HashMap etc but for newer data structures that do not have an already-established api, this could be ergonomic.

[nikomatsakis]

We discussed this RFC in the @rust-lang/lang meeting last night. We've decided to postpone this RFC under Issue #997. The concerns are the same as the ones I documented before. The thought was that it would be good to push on the emplacement syntax and API and see how that plays out before making a firm decision here.

One observation that @huonw made which I don't see in my previous post is that a syntax like map[k] <- v is ambiguous in the case where the map has "placer objects" as its values -- are you intending to emplace into the map, or the value?