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

[Eh2406]

Manually rebase of @migi #41336

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (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.

[kennytm]

The for-loop-unconstrained-element-type-i32-fallback.rs test failed (probably not essential for crater).

[00:50:26] ---- [run-pass] run-pass/for-loop-unconstrained-element-type-i32-fallback.rs stdout ----
[00:50:26] 	
[00:50:26] error: compilation failed!
[00:50:26] status: exit code: 101
[00:50:26] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/for-loop-unconstrained-element-type-i32-fallback.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/for-loop-unconstrained-element-type-i32-fallback.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/for-loop-unconstrained-element-type-i32-fallback.stage2-x86_64-unknown-linux-gnu.run-pass.libaux"
[00:50:26] stdout:
[00:50:26] ------------------------------------------
[00:50:26] 
[00:50:26] ------------------------------------------
[00:50:26] stderr:
[00:50:26] ------------------------------------------
[00:50:26] error[E0282]: type annotations needed
[00:50:26]   --> /checkout/src/test/run-pass/for-loop-unconstrained-element-type-i32-fallback.rs:17:5
[00:50:26]    |
[00:50:26] 17 |        for i in Vec::new() {
[00:50:26]    |   _____^
[00:50:26]    |  |_____|
[00:50:26]    | ||
[00:50:26] 18 | ||         sum += i;
[00:50:26] 19 | ||     }
[00:50:26]    | ||     ^
[00:50:26]    | ||_____|
[00:50:26]    | |______cannot infer type for `_`
[00:50:26]    |        consider giving `__next` a type
[00:50:26] 
[00:50:26] error: aborting due to previous error

[Eh2406]

I'm not seeing how to fix the test. It is a very fragile situation in which this test passes. I.E. this sum = sum + i; does not work on stable.

[kennytm]

The test was introduced in #42634 (comment) (cc @nikomatsakis). Maybe you could just disable the test (add a comment // ignore-test) to make the CI green for now.

[Eh2406]

Thanks for suggestion. Let's see what ci thinks.

[aturon]

@rust-lang/infra cargobomb requested!

[kennytm]

@bors try

[bors]

⌛ Trying commit 9a547fc with merge 3c27d9e...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[Mark-Simulacrum]

Started preparing cargobomb run.

[bjorn3]

Missing trailing newline

[Eh2406]

Should I fix now or wait for cargobomb?

[kennytm]

@Eh2406 Cargobomb uses the compiler already built from @bors try. You could fix it any time you like.

[Eh2406]

Done, thanks!

[Mark-Simulacrum]

Cargobomb run complete: http://cargobomb-reports.s3.amazonaws.com/pr-44287/index.html.

[Eh2406]

Thanks for running this! I am new to cargobomb, what is the best way to proceed? How can I find the original code, so I can start on Pr's?

Any iday how this Pr can lead to a test-fail regression?

[kennytm]

@Eh2406:

• regressed = Previously success, now failed. Focus on this.
• fixed = Previously failed, now success. Likely badly written tests in this case.
• unknown = Unknown.
• build-fail/test-fail = Failed in both toolchains. That means the code itself is buggy
• test-pass = success in both toolchains. That's good.

Usually regressed is the only section you need to pay attention to.

Since this change affects type inference, the "test-fail" results likely means the crate's tests are flaky, and can be safely ignored. The list of crates with flaky tests can be found in https://github.com/rust-lang-nursery/cargobomb/blob/master/blacklist.md, rust-lang/crater#126 and rust-lang/crater#127. For instance ai-0.1.0 is known to be flaky (#43963).

Sometimes even "build-fail" tests are spurious, e.g. rust-lang.cargo.98b6f6… failed due to rust-lang/crater#125, not real regression.

However, the error in generator-0.5.0 is caused by failure to build the example, but wrongly attributed as a "test-fail". In the end you need to read every log to be sure.

---

There are 12 root regressions, much more than the previous try.

Crate	Code
average	self.n[i] += d.approx().unwrap();
cov	exit_count -= self.graph
etcommon-trie	ret[end] |= vec[i].into();
finch	total_gc += kmer.kmer.iter().map(|b| {
gene-seq-intersections	tmp_count += x.into();
generator	https://github.com/Xudong-Huang/generator-rs/blob/164e28dc/examples/send.rs#L5-L15
libcantal	offset += size;
minions	sum += i;
netlink-rs	self.flags |= Flags::Multi.into();
treeflection_derive	self.qux += value.parse().unwrap();
whiteread	s += x
samsartor/pbr-demo@8cb72ed	rgb[3] *= v.parse().map_err(|_| "…")?

[kennytm]

I'd also like to note that rust-lang/rfcs#2147 may make this PR unnecessary.

[michiel-de-muynck]

@kennytm: Thanks a lot for the nice summary of the regressions. Does cargobomb test more crates than crater did? Because I find it surprising that there would be more people writing this kind of pattern in the last 3 months than in the 2 years before that.

But to answer your remark that "rust-lang/rfcs#2147 may make this PR unnecessary": I don't think that's true. Let me explain:

In non-generic code, the problems that this PR solves are indeed also solved by rust-lang/rfcs#2147. This PR helps you if you have an x which is a &i32, because you can write a += x instead of a += *x. rust-lang/rfcs#2147 does the same thing and more (small note: it looks like they forgot op-assign operators but that's probably just an oversight). That RFC also helps if you have a &&i32, or a &&&&&i32, or if you have a i32 but need a &i32. It also helps for all non-builtin numerical types like BigNum. And especially with BigNum it helps a lot, because types like BigNum are expensive to clone, so you want to use many references, leading to the eye of sauron without that RFC.

So it looks like that RFC is just a far superior version of this PR. But that's in non-generic code, where this PR only solves some very minor inconveniences. The main use of this PR is in generic code, for the use case where you want to write a generic function that has to work both with built-in numeric types (f64) and expensive-to-clone user types (BigFloat). Without this PR you cannot use op-assign operators (+=, -=, etc.) in those functions, because the builtins don't satisfy the trait bound T : AddAssign<&'a T>. It's not less ergonomic to use the op-assign operators, you simply can't. And rust-lang/rfcs#2147 doesn't help with this part at all.

So tl;dr: yes rust-lang/rfcs#2147 supersedes this PR for non-generic code, but not in generic code.

(I reworked this post a bit because it was inaccurate)

[Eh2406]

@migi Thank you for the detailed analysis! And yes, cargobomb tests more than crater did. Crater just tried to build all the crates, Cargobomb tries to run all the tests. So the the regressions in test code, like treeflection_derive, would not have been caught by Crater.

@kennytm Thank you for the explanation and for doing the triage. I will work on making PR's to fix them later today/as soon as I have time.

How can I get rustup to use the try artifact, so I can double check my fixes?

[kennytm]

@Eh2406 Not sure how to setup rustup for this, but you could download the binaries from

https://rust-lang-ci.s3.amazonaws.com/rustc-builds-try/3c27d9e8363646c0da43e8adc7cd5cd4c84aa9d1/rust-nightly-x86_64-unknown-linux-gnu.tar.xz

[Mark-Simulacrum]

https://github.com/Mark-Simulacrum/bisect-rust will download and unarchive a try build, you just need to run cargo run --bin install-sysroot --commit 3c27d9e8363646c0da43e8adc7cd5cd4c84aa9d1 with the try commit and everything should "just work" from there, I think. If not, please file an issue!

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 3357acd7de9004499804f0da3f74f4d02da5a4c7 to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

[kennytm]

@bors retry rollup #43535

[shepmaster]

@bors r-

[shepmaster]

@bors r=aturon rollup

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: First step toward implementing impl Trait in argument position #44866

[bors]

📌 Commit 2adeba6 has been approved by aturon

[bors]

☔ The latest upstream changes (presumably #44936) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

Merged in #44936.

[SimonSapin]

This broke Servo:

error[E0283]: type annotations required: cannot resolve `usize: core::ops::AddAssign<_>`
   --> /home/travis/build/servo/servo-with-rust-nightly/servo/components/script/textinput.rs:239:31
    |
239 |             |len, slice| *len += slice.chars().map(char::len_utf16).sum())
    |                               ^^
error: aborting due to previous error

(Using .sum::<usize>() fixes it, though it’s not pretty: servo/servo#18693)

[shepmaster]

Ditto fuzzy-pickles:

error[E0283]: type annotations required: cannot resolve `usize: std::ops::AddAssign<_>`
   --> src/tokenizer.rs:544:17
    |
544 |             idx += ci.take_while(|&c| UnicodeXID::is_xid_continue(c)).map(|c| c.len_utf8()).sum();
    |                 ^^

[pnkfelix]

Given #45480, should this be tagged as [breaking-change] ?

(I skimmed the comments and commits and did not see that string anywhere, thus my question. Having it in the description can be useful for release notes...)

[michiel-de-muynck]

I would be in favor of tagging this as [breaking-change], since it is breaking some amount of real-world code.

However, it should be noted that type inference is specifically excluded from the backwards compatibility guarantees of Rust. See for example in the official FAQ, under "How does Rust language versioning work?":

Rust’s language versioning follows SemVer, with backwards incompatible changes of stable APIs only allowed in minor versions if those changes fix compiler bugs, patch safety holes, or change dispatch or type inference to require additional annotation.

That FAQ further says "For additional details, read the Rust blog post “Stability as a Deliverable.”, which also says the same, under "What are the stability caveats?":

We reserve the right to fix compiler bugs, patch safety holes, and change type inference in ways that may occasionally require new type annotations.

So essentially, code relying on type inference is occasionally going to need some additional annotations. The reason is that type inference tries to do as much as it can, relying both on what impls exist and also on what impls don't exist. This is very useful, but has the downside that adding an impl may occasionally break working code, which is what happened here.

[SimonSapin]

Another important aspect of inference breakage is that it is (always?) possible to write code with more type annotations that works both in compiler with and without the change.