overloaded assignment operations a += b

[japaric]

Rendered

Implementation

cc @bjz @aturon @nikomatsakis

[huonw]

I would like to see discussion of why the proposed design differs from the plain Add, Sub etc. traits. (I don't think it's a bad idea, but it seems worthy of at least mentioning briefly.)

[japaric]

Added a section on by-ref vs by-value.

[kennytm]

Alternative would be delegate x += y to x = x + y, and rely on the optimizer to eliminate the extra memcpys. The Pro is we don't need 10 more traits. However I don't know if LLVM and other backends are good enough to optimize these, and also we cannot += to an unsized type with this alternative.

[CloudiDust]

I'd prefer taking RHS by value, like the "normal" binary operator traits do, for consistency.

I think we can deal with the ergonomics loss with another RFC (auto-referencing for operands), which will work uniformly across "normal" binary operators and their assignment counterparts.

(Actually currently I am not quite convinced that we should add auto-referencing, but I'd prefer "uniform auto-referencing for operands" to "assignment operators taking RHS by reference".)

[eddyb]

@kennytm I doubt that's not the primary concern - what about *x += 1; where x is a &mut BigInt?
We can't move out of &mut T right now at all to get the by-value self for efficient operation - and if it's not by-value, then you're cloning a heap allocation, which is a much bigger problem than the memcpys involved, from both a semantics and optimization point of view.

👍 to this proposal, and even though I prefer by-value RHS, most of my places where I've wanted to use this feature could've worked fine with by-ref.

I am (pleasantly) surprised to see this get a chance before 1.0, for some reason I thought it wouldn't even be considered for now.

[CloudiDust]

@eddyb,

and even though I prefer by-value RHS, most of my places where I've wanted to use this feature could've worked fine with by-ref.

I think the same can also be said about the "normal" binary operators. We generally want by-value for primitives and by-ref for custom types. And the actual problem is our "operator traits" approach cannot support both (without ergonomics loss when calling).

I still prefer by-value traits (and maybe with uniform auto-referencing support for operands).

Besides consistency, suppose we have a BigInt type, how do we implement big_int += primitive_int? I don't think using a by-ref trait feels more natural than using a by-value one. (If the trait is by-value, we can opt-in by-ref, but if the trait is by-ref, we cannot opt-in by-value.)

But if we decide to use by-ref traits, then I suggest adding a Ref suffix to the trait names.

It is as if we have:

trait Add { ... }  // binary `+`, which takes both operands by value. (Existing.)
trait AddRef { ... } // binary `+`, which takes both operands by reference. (Not added.)
trait AddAssign { ... } // `+=`, which takes the RHS by value. (Not added.)
trait AddAssignRef { ... } // `+=`, which takes the RHS by reference. (Added.)

This also means renaming BitAndAssign to BitAndAssignRef, and so on.

[kennytm]

@eddyb Ah right. Unfortunately add(lhs: &mut BigInt, rhs: BigInt) -> &mut BigInt can't make sense, so we really gotta add these traits to support += 😞

[shepmaster]

Alternative would be delegate x += y to x = x + y, and rely on the optimizer to eliminate the extra memcpys

Do we need to worry about supporting atomicity ever? Maybe we want to implement += for AtomicInt? If so, then I'd assume that splitting into two steps would be A Bad Thing.

[eddyb]

@shepmaster that wouldn't be doable because a shared atomic would be &AtomicFoo, not &mut AtomicFoo - if you have mutable access, there isn't much point to doing atomic operations.
The same goes for Cell, which would also be more usable, but it's not covered by this proposal.

[gsingh93]

Using the current Add, Sub, etc. traits seems like a better option to me. Having the option to implement Add but not AddAssign (or vice versa) could lead to confusing API designs, and I don't see a reason why having one wouldn't imply both. Is the only reason against this the inability to optimize properly (I don't completely understand why, btw)?

[oli-obk]

Question: Should the XYZAssign trait depend on XYZ? Is there ever a situation where one would need AddAssign but not Add (an example I can think of is C# event-handlers)?
The decision cannot be made in a backwards compatible way. Not having AddAssign depend on Add will break code if in the future it depends on Add. If it depends now on Add, it can never be removed (or can it?, rust doesn't have dynamic_cast).

Are there any use cases of assignment operations where the RHS has to be taken by value for the operation to be performant (e.g. to avoid internal cloning)?

Both might be abuse of the operator, and I don't know of rust-versions

1. C# event handlers need to get a moved closure.
2. Set-operations for sets of Vec or similar

[japaric]

@kennytm @gsingh93

Alternative would be delegate x += y to x = x + y

For a linear algebra library, this would lead to an API with unnecessary cloning and slower operations, for example if you want to write

// x, y, z: are owned matrices (`Mat`) that contain `f32` elements
// `impl Mul<f32> for Mat { type Output = Mat; .. }` eager scaling - O(N) time
// `impl Add<Mat> for Mat { type Output = Mat>; .. }` saxpy call - O(N) time
y *= 2.;      // y = y * 2.;
y += x * 3.;  // y = y + x * 3.;
z += x * 4.;  // z = z + x * 4.;  error: `x` has been moved

Instead you need to write:

y *= 2.;              // O(N) time from eager scaling
y += x.clone() * 3.;  // O(N) memory use from clone
                      // O(N) time from eager scaling (3.0 * x.clone())
                      // O(N) time from saxpy (y += temp)
                      // ???? time from calling destructor on temp
z += x * 4.;          // O(N) time from eager scaling (4.0 * x)
                      // O(N) time from saxpy (z += temp)
                      // ???? time calling destructor on temp
// and you can't use `x` afterwards

However with the OpAssign traits, you can avoid the allocation and do less work overall:

// `struct Scaled<'a>(f32, &'a Mat)`
// `impl MulAssign<f32> for Mat { .. }`  eager scaling - O(N) time
// `impl<'a> Mul<f32> for &'a Mat { type Output = Scaled<'a>; .. }` lazy scaling - O(1) time
// `impl AddAssign<Scaled<'a>> for Mat { .. }` saxpy call - O(N) time
y *= 2.;
y += x * 3.;  // error: `Mul<f32>` not implemented for `Mat`
z += x * 4.;  // error: `Mul<f32>` not implemented for `Mat`

Here the compiler errors lead the user to more efficient operations:

y *= 2.;       // O(N) time from eager scaling
y += &x * 3.;  // O(1) time from lazy scaling (3.0 * &x)
               // O(N) from saxpy (y += scaled)
z += &x * 4.;  // O(1) time from lazy scaling (4.0 * &x)
               // O(N) from saxpy (z += scaled)
// and `x` *can* be used afterwards

Note that here is important to provide Mat *= f32 but not Mat * f32 because the latter operation creates a temporary. For linear algebra libraries is important to reduce the creation of temporaries, minimize memory allocations and reduce the number of BLAS calls. Having separate OpAssign traits is required to achieve these goals while providing sugared operations.

---

@shepmaster

Even if we changed the signature of op_assign to allow calling op_assign(&atomic_isize, isize). I don't think it woul be that useful because e.g. fetch_add takes an Ordering and there are multiple choices for it, and the sugar would always use the same ordering (then you have the question of what Ordering should add_assign use).

---

@oli-obk

Question: Should the XYZAssign trait depend on XYZ?

No, I don't think so. In fact, I usually do it the other way and define XYZ in terms of XYZAssign:

// X: An owned value (`Mat`)
// Y: A reference-like value (`Scaled<'a>`)
impl Add<Y> for X {
     type Output = X;
     fn add(mut self, rhs: Y) -> X {
         self += rhs;
         self
     }
}

But I don't think one trait should depend on the other. Nor do I think that if one impl AddAssign<Y> for X then necessarily one must impl Add<Y> for X or viceversa (see above example).

1. C# event handlers need to get a moved closure.

Could you link to examples of this sugar?

1. Set-operations for sets of Vec or similar

What do you have in mind here? AFAIK, the set operations provided by stdlib are lazy (return an iterator).

[Gankra]

@japaric We currently provide a | b as sugar for a.union(b).collect::<Self>(). (and a & b, a ^ b, and a - b for intersect, symmetric-diff, and diff). Though I'm growing kinda nervous of these convenience overloads as dubious/borderline operator abuse. Would be willing to discuss killing them.

[CloudiDust]

@gankro, I think set operations are a fine use case of operator overloading, that said, having lazy named methods but eager operators may or may not surprise people. (Again I think the status quo is okay.)

[nikomatsakis]

This looks good to me, basically like what I had in mind, modulo the question of by-value or by-ref. I think I am sympathetic to @CloudiDust's point that we should handle by-ref uniformly (and I am vaguely in favor of doing so). It seems to me that consuming the rhs could be more efficient in some cases. For example, if you were to write vec1 += vec2, and it so happens that vec1 is short but vec2 has lots of extra capacity, you could choose to use the memory from vec2 and free the memory from vec1, avoiding an allocation.

[CloudiDust]

@nikomatsakis, an assignment operator that consumes its RHS would be quite surprising. I'd expect that only Copy types (references/slices, primitive numeric types, small Copy structs ...) would go on the RHS.

[glaebhoerl]

@CloudiDust, why? Consuming is the default just about everywhere in Rust. Including the addition operator + and the unadorned assignment operator =, which are lexically combined to form +=.

[CloudiDust]

@glaebhoerl, I'd say arithmetic operators are a bit different from normal named functions, their operands are not expected to be mutated/consumed. (For an assignment operator, only the LHS is expected to be mutated.)

I think on the doc page of the Add trait, all implementors are taking RHS that are Copy.

[glaebhoerl]

(For an assignment operator, only the LHS is expected to be mutated.)

I do think we should clearly distinguish between "mutate" ("by &mut") and "consume" ("by value"). It would be very weird if a = b were to leave b accessible while mutating it. On the other hand it is natural for it to consume b, leaving it inaccessible. I think += is the same way. (If the operand is usually Copy in practice, that's a separate matter.)

(We may have been talking past each other? As far as I can tell, you came out in favor of += taking the RHS by value, which feels like the right way to me as well.)

[CloudiDust]

@glaebhoerl, oops, what was I thinking.

(Note: In the two previous comments, when I talked about "assignment operators", I always meant "compound assignment operators", stressing the "arithmetic operator" parts. Not that this correction makes my previous statements true, though.)

I thought that vec1 += vec2 was just sugar for vec1 = vec1 + vec2 and it would be weird for vec2 to be consumed. But then I realized that vec1 = vec1 + vec2 was not even valid. It should have been vec1 = vec1 + &vec2, which in the future, may also be written vec1 += &vec2.

vec1 += vec2 and vec1 += &vec2 are different.

Time to get some rest.

[japaric]

@gankro That's operator abuse, because | is BitOr and ^ is BitXor, but the operation doesn't do what the trait documentation says. However, I don't think we can prevent people from doing it, even if we set the example of not abusing operators in the stdlib. (At least I'm glad that nobody has asked (yet) for ShrAssign to return a value, so they can hack it into the bind operator >>=)

@nikomatsakis Thanks for the example, I agree that taking RHS by value would lead to better performance in that case. Though I'd expect vec1 += vec2 to be element-wise sum of vec2 into vec1 and not concatenation, but we already crossed that line by allowing String/Vec addition.

[oli-obk]

@japaric

Could you link to examples of this sugar?

c# event handlers: https://msdn.microsoft.com/en-us/library/aa645739%28v=vs.71%29.aspx (search for += and -=, you can add multiple handlers to an event, there's also lots of confusion about closures in event handlers: http://stackoverflow.com/questions/2226510/closures-in-c-sharp-event-handler-delegates)

[japaric]

@oli-obk thanks for the links! I'd personally prefer methods like register/unregister instead of +=/-= in that case. But, yes, if they take closures on the RHS, they would need the op_assign method to be by-value.

I guess another case where the RHS needs to be by-value is pushing elements into a Vec with vec += elem or vec += iterator. However, I'm unsure if we want to add that sugar to the stdlib.

[Gankra]

I'm pretty negative on doing collection operations with operators, personally. Not least of-which because it opens a huge coherence-asymmetry can-of-worms. Creates a huge mess of implementations to handle all the different combinations, too.

[pnkfelix]

@japaric I noticed you did not have trait AddAssign<Rhs=Self> : Add<Rhs> -- that is, it does not extend Add.

Was this considered? I didn't see it listed on the alternatives, but I also do not see any immediately obvious reason why one wouldn't have AddAssign extend Add (and likewise for all the other traits).

---

In particular, I would be curious to know if the trait could offer a default impl:

trait AddAssign<Rhs=Self> : Add<Rhs> {
    fn add_assign(&mut self, rhs: Rhs) {
        unsafe {
            ptr::write(self as *mut Self, Add::add(ptr::read(self as *const Self), rhs));
        }
    }
}

so that an unoptimized impl would just look like:

impl AddAssign for Foo { }

(though an unsafe default impl admittedly might be a really really bad idea ...)

---

Update: Obviously we do not need any default method impl to add this feature. And maybe a where clause on the method would suffice, rather than requiring all impls of AddAssign to extend Add. So ... I'm certainly not going to argue that this RFC actually be forced to adopt this idea. Nonetheless, I was still surprised that AddAssign did not extend Add, so discussion of that design detail in the RFC document itself would be appreciated.

---

Update 2: Ah, reviewing the discussion here, I see that @oli-obk already raised this point and you responded. Please do transcribe a summary of your argument into the RFC document; part of the point of the process is that the final document contains succinct discussions of matters like this. Thanks!

[pnkfelix]

@oli-obk btw you asked:

If it depends now on Add, it can never be removed (or can it?, rust doesn't have dynamic_cast).

Your first intuition was correct: If we made AddAssign depend on Add, then it could not be removed backwards-compatibly, because of functions like this, where it would be able to derive the Add bound from the presence of the AddAssign bound.

fn foo<X:Copy+AddAssign>(x: X) -> X { x + x }

[pythonesque]

Just wanted to register my support for this RFC.

I really don't see why AddAssign: Add would be a requirement... but maybe I'm missing something.

[comex]

Suggestion: allow deriving FooAssign, which would generate an implementation based on Foo, like *self = *self FOO other.

[aturon]

I'm still a little unclear on the motivation for additional traits rather that e.g. adding a method with this signature to Add, with a default implementation like the one @pnkfelix outlined. It's true that those defaults might not yield the most efficient implementation, but:

1. Having a single trait makes it easier to use += and + together in generic contexts, and
2. Overriding the default implementation seems like no more work than implementing a separate trait, as the RFC proposes.

This would also eliminate the trait inheritance questions.

Am I missing something?

[aturon]

Follow-up: I missed the relevance of some of the earlier discussion about cases where you want to overload += but not +. I'm... not convinced that's worth the extra complexity in the number of traits needed in bounds for generic code. (Thanks @gankro for pointing this out to me on IRC).

[nikomatsakis]

So I agree that the relationship to Add is the most interesting thing to get right. It seems to me that AddAssign: Add probably makes sense, personally. However, just adding a method to Add (as @aturon described) doesn't work, I don't think. You can't provide a default impl of add_assign unless we require that Add: Clone (which we don't today).

Felix's unsafe code is unsafe for a reason -- it's not exception safe. What if a panic occurs in the middle, for example? It would leave the &mut self pointer uninitialized.

If we had specialization, we could provide a default bridge impl of this form:

#[low_priority]
impl<R,T:Add<R>+Clone> AddAssign<R> for T {
        fn add_assign(&mut self, rhs: R) {
            let tmp = self.clone() + rhs;
            *self = tmp;
        }
}

In that case, if you know that T:Add<R>+Clone, you can always use +=, but you might be better off bounding by AddAssign.

[aturon]

@nikomatsakis Ah, good point -- I wasn't thinking carefully about @pnkfelix's implementation.

I agree that making Add a supertrait is reasonable. In practice, though, it's not obvious to me how often people will end up bounding by these individual traits rather than "higher level aliases" that group together several numeric operations -- the kind of numeric hierarchy we've moved away from in std for now, but which is really important for generic programming. You quickly end up wanting this to avoid writing HRTBs anyway. But once you're working with aliases grouping together traits, inheritance is much less important...

[pnkfelix]

@nikomatsakis

What if a panic occurs in the middle, for example? It would leave the &mut self pointer uninitialized.

To be precise, I used ptr::read, not ptr::read_and_zero ... so I think we'd see double drops (still bad+unsound), not uninitialized self

[pnkfelix]

Also, just to be clear: I am happy without AddAssign: Add. I just want it discussed in the RFC text.

[nikomatsakis]

@pnkfelix

To be precise, I used ptr::read, not ptr::read_and_zero ... so I think we'd see double drops (still bad+unsound), not uninitialized self

True. :)

[shepmaster]

I believe this would also be useful for the Wrapping-variant numbers in standardlib.

[nikomatsakis]

Hear ye, hear ye. This RFC is moving into final comment period.

The primary change since the RFC was first introduced is that the RHS was changed to be by-value, not by-reference.

[Aatch]

Looking forward to this. Seems pretty solid and a perfect fit for Ramp (which fits solidly into the intended use case for the feature).

[yongqli]

👍 I would find it pretty useful for vectors and matrices.

[Cigaes]

This is not a comment on the feature itself but on the documentation. Currently, [https://doc.rust-lang.org/reference.html#compound-assignment-expressions] states that “lval OP= val is equivalent to lval = lval OP val”. This is not currently true for trait-based operations.

[pnkfelix]

typo: "typecheking"

[strega-nil]

👍

[nikomatsakis]

Congratulations. The language design subteam has decided to accept this RFC. The canonical text can now be found in the text directory. Tracking issue is rust-lang/rust#28235