Allow T op= &T for built-in numeric types T

[michiel-de-muynck]

Currently, there are 4 ways you can add two i32's together: a + b, &a + b, a + &b and &a + &b. That is, the Add trait is implemented 4 times to support both ref and non-ref arguments. The same is true for all other binary operators and all other built-in numeric types, including Wrapping. Similarly, unary operators also have a ref version and a non-ref version, so for example -7 == -&7.

However, the assignment versions of the binary operators don't allow a ref right-hand side, so for example a += &2 doesn't work. This pull request fixes this inconsistency by implementing T op= &T for all built-in numeric types (integers and floats) and also for Wrapping.

There is one final type that Rust provides compound assignment operators for: Duration. I chose not to implement Duration += &Duration (etc) for now because the non-assignment binary operators for Duration also don't support ref arguments, i.e., you can't do &Duration + &Duration either.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[arielb1]

r? @BurntSushi

[arielb1]

Thanks for the PR @migi! @BurntSushi or some other reviewer will be looking at your PR shortly.

[BurntSushi]

@aturon What do you think? I think these look fine to me, but I'd appreciate a second pair of eyes because these are insta-stable.

It also seems like adding similar impls for Duration (including impls permitting &Duration + &Duration) would be desirable as well. Perhaps in a separate PR?

[alexcrichton]

This looks great to me, but I'll tag this with waiting-on-team as we'll want to discuss this during a libs triage.

[aturon]

I'm in favor!

[alexcrichton]

Wait right we have a system for this!

@rfcbot fcp merge

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[kennytm]

Just to note that this PR closes #32094.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[alexcrichton]

@bors: r+

[bors]

📌 Commit e47c971 has been approved by alexcrichton

[bors]

⌛ Testing commit e47c971 with merge bbd636c...

[bors]

💔 Test failed - status-appveyor

[Mark-Simulacrum]

@bors retry

• Spurious failures on AppVeyor due to ar.exe errors #40546

cmd.exe /C "cd . && "C:\Program Files (x86)\CMake\bin\cmake.exe" -E remove lib\libLLVMMSP430Info.a && C:\projects\rust\mingw32\bin\ar.exe qc lib\libLLVMMSP430Info.a  lib/Target/MSP430/TargetInfo/CMakeFiles/LLVMMSP430Info.dir/MSP430TargetInfo.cpp.obj && C:\projects\rust\mingw32\bin\ranlib.exe lib\libLLVMMSP430Info.a && cd ."
C:\projects\rust\mingw32\bin\ranlib.exe: lib\libLLVMMSP430Info.a: Permission denied

[alexcrichton]

@kennytm good point, I'll close as a breakage wishlist issue.

@le-jzr it's possible, but discussing such a change is somewhat more of an RFC than in the scope of this PR.

[michiel-de-muynck]

@alexcrichton, @aturon: Sorry to bring this up again, but I wasn't there in the libs triage and I didn't find any minutes or notes online, so I don't know to what extent this PR was really discussed. There are some points in favor of this PR that I didn't discuss before, because I didn't want to make this PR into a sales talk, and I thought that the blatant inconsistency between Add and AddAssign alone was severe enough that no more reason was needed. I still think this actually (is your decision really to have f64 implement Add<&f64> but not AddAssign<&f64> forever?).

But like I said there's some other points that I didn't this elaborate in this PR before, because I didn't want to make it a sales talk. Basically, I disagree with this:

upcoming language features related to Copy and references may make this a much better experience in the future though!

I assume this is referring to this: the upcoming ergonomic improvement to "auto-deref Copy types". This would allow, for numeric types like f64, to write a += &b even if AddAssign<&f64> is not implemented, which seems like it solves the issue in this PR entirely. So what's the problem?

Well, the problem is that that is only possible if the type of a and b is known to be Copy. But aren't all built-in numeric types Copy? Yes, of course, but they might not be known to be Copy in generic code.

Consider a function that adds Ts together in some way, for some generic T. This T could be something like Matrix<f64>, so you can't have the trait bound T : Copy. You can have T : Clone at best. And to be able to add those Ts together, which trait bounds should be used for T? Well, T: AddAssign<T> works, but if performance is important (and adding numbers is definitely something that happens in inner loops), then it's not optimal. If b is reused later, then b needs to be cloned, which may be very expensive if b is a BigInteger, or a Matrix, or a Matrix<BigInteger>, or worse (Matrix<HyperDual<Complex<MultiPrecisionReal>>> is an actual type that I used in C++ code that I was porting over to Rust).

The trait bound that should be used in performance-critical generic code is (you guessed it) T : AddAssign<&T>. But, then the generic function simply doesn't work with basic types like f64. There are workarounds, like

• Using specialization. This requires some boilerplate code though for each such generic function. This boilerplate is completely unnecessary because you're not actually treating f64s differently from Matrix<f64>s.

• Defining a special trait which means the same as T: AddAssign<&T>, and implementing it for built-in numeric types and all non-user-defined types which the function should work with. If you forget to implement it for a certain library's Matrix<T>, then that Matrix<T> can't be used with your generic function without a wrapper type because of the orphan rule. Also, there's a serious hit to code readability because a += &b will look like a.add_assign_ref(&b).

• Just not using AddAssign<&T> altogether and exclusively using T : Add<&T, Output=T> and moving the result, i.e., writing a += &b as a = a + &b. Of the three solutions, this is the most reasonable one, but ironically, only as a consequence of the fact that the built-in numeric types do implement Add<&T>, as they should. Also, I imagine quite a few people would read a = a + &b and think "hey, I can simplify this to a += &b", and then be confused why it doesn't work.

I'm not trying to diminish the fact that the PR breaks type inference in some cases. This PR does break existing code. That's definitely a con of this PR, and whether that con "outweighs" the pros (or rather, the cons of the status-quo) is indeed up to the libs team. You may have discussed all of these things in that libs triage already, in which case I'm sorry for wasting your time. But I have no way to tell.

Actually, if backwards compatibility was not an issue at all, the best (but very idealistic) solution in my eyes would be if the ergonomics improvement "Allow owned values where references are expected (or: AsRef coercions)" were implemented very thoroughly, to such a degree that the compiler would understand that T : AddAssign<&T> implies T : AddAssign<T>, and accept the trait bound T : AddAssign<T> even if only T : AddAssign<&T> were implemented for T, and understand that there is no ambiguity if both a ref and an owned value are possible under the right circumstances, etc. But this is all very vague and very idealistic. And either way, even in this solution you want the numeric types to implement AddAssign<&T>.

Also, what should I do with all those pull requests that I made to the crates that would be affected? Right now, 8 of these 9 PRs have already been merged (all except pijul), but those "fixes" are pretty useless if this PR is not accepted. I think I'll just let those crates be for now, they've been bothered enough. Just next time that someone submits a PR that breaks code, don't go and tell them to submit fixes before you've decided to accept their PR, that turned out not to be the best idea I think, it kinda wastes everyone's time if the PR is eventually rejected. 😉

[aturon]

@migi Thanks for the very well-thought-out reply! I agree with essentially all of it, and given that the regressions have already been resolved, think we should go forward.

cc @rust-lang/libs wdyt?

[alexcrichton]

I'd personally still be hesitant to land this given the breakage. I definitely don't disagree with the benefits, but breaking nearly 10 crates in unique circumstances is quite a large change. Additionally I'd expect in abstract that idioms like x -= y.iter().map(...).sum(); do work, whereas this'll just require type ascription to get that to work.

I think we'd want to do another crater run before landing regardless, but I'm quite sure that if we were to land this then the breakage we've seen already is not the last we're going to hear about.

[michiel-de-muynck]

I would also expect x += y.iter().sum(); to work, just as much as I expect let x = y.iter().sum() + 1; or let x = a.iter().sum() + b.iter().sum(); to work. But these don't work either. The compiler even gives a wrong recommendation: "consider giving x a type", but giving x a type doesn't help at all. It's the generic type S in sum() that needs to be specified.

I also think that if let x = y.iter().sum() + 1; or let x = a.iter().sum() + b.iter().sum(); or let x = y.into() + 1 etc. etc. were possible, and then we'd see those happen just as often as the += variants happened here. But of course we never hear about those cases, because people fix these type inference issues before publishing their crate.

What would be ideal (though I don't know how hard this would be), is to add deprecation warnings for code that relies on these type inference cases, and then in a later release enable the new trait impls (making those warnings into hard errors). That also gives closed-source code time to adapt. This could be done with an attribute like #[upcoming] or #[future] or something, which would be like the opposite of the #[deprecated] attribute: a trait impl with that attribute doesn't exist yet, but if adding it would break your code, you get a warning. This attribute should be useful in a lot other places as well, both in the standard library and in user libraries, because adding trait implementations like this can always break type inference (right?). Or does Rust already have a different mechanism in place to "phase in" trait imps? I looked but didn't find anything.

Finally, for some of the problem cases (those involving sum() in particular), default function type parameters would solve the type inference issues as well. For example, if the generic type S in sum() were Self::Item by default, then all 3 problem cases involving sum() would be fixed, and x += y.iter().sum(); would work as well. Unfortunately, default type parameters on functions isn't allowed right now, but it sounds to me like that might change because the reason for it not being allowed is

they are basically ignored on stable Rust. On nightly Rust, meanwhile, we were considering various possible meanings, but have not yet found one that we are satisfied with.

[alexcrichton]

@migi ah yeah that's a really good point, the inference here is a bit of a minefield and there's no use in picking a few idioms as working when other similar ones don't work. With that in mind I'd be fine having another PR for this, although I'd like to run crater again to ensure that nothing new has regressed.

[Eh2406]

Hi, so just got sent here from this.

Is this still waiting on a rebase and pr?

[michiel-de-muynck]

@Eh2406 Yes, but @brson, who does the crater runs, left Rust, so I don't know if a new crater run is still a possibility.

I also don't know what the purpose is of creating a separate PR for the exact same changes.

[Eh2406]

@migi I did a rebase and submitted #44287

[kennytm]

@Eh2406 The reason this PR being closed was due to inferred type of y in x += y is changed from having a unique candidate to multiple candidates, and that makes existing code fail to compile, which breaks the stability guarantee. Rebasing is not going to fix that. The inference rule must be changed to adapt for this before the PR is going to be reconsidered. I think we need to at least wait for chalk to land first (?).

[Eh2406]

chalk Is not going to fix having 2 different valid candidates. All of the know regressions have been fixed. Inference errors of this type are exempted from the stability guarantee. And the inference around this corner of language is already arbitrary and fickle.

But most importantly @alexcrichton asked for a new PR.

[kennytm]

@Eh2406 I don't mean chalk can fix the bug, I mean any change to inference algorithm should better wait for chalk.

[Eh2406]

But this is not a change to the inference algorithm. This is just adding an impl.

[kennytm]

@Eh2406 If we still want code like #41336 (comment) to pass without type annotation after adding the impl, the inference algorithm needs to be changed.

[Eh2406]

True! But my reading of this thread is that we don't want that to pass without annotations. Specifically #41336 (comment)

[nox]

This seems to break inference for such code:

let foo: &mut usize;
*foo += iter_that_yields_usize.sum();

Is this intended?

[arielb1]

It is.

[nox]

On the ergonomics front, how is removing a * (which is what these new impls do) trump having to add type annotations in all similar cases?

[michiel-de-muynck]

The breakage is obviously not intended, but it's unfortunately unavoidable. At least until we get default type arguments for generic functions (so that the generic argument in sum() can be given a default) or something similar.

The purpose of this PR was not ergonomics. It was twofold. The first was that the lack of the impls introduced in this PR was a huge limitation for generic numerical computation. Read this (edit: or this) for a more detailed explanation.

The other reason was consistency. + supports & arguments, so why shouldn't +=? In your own example, why should

*foo += iter_that_yields_usize.sum();

work if

*foo + iter_that_yields_usize.sum()

doesn't?

[nox]

@migi Good point mentioning +, things are looking clear to me now.