std: Remove addition on vectors for now

[alexcrichton]

Ideally this trait implementation would be unstable, requiring crates to opt-in
if they would like the functionality, but that's not currently how stability
works so the implementation needs to be removed entirely.

This may come back at a future date, but for now the conservative option is to
remove it.

[breaking-change]

[rust-highfive]

r? @gankro

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

r? @aturon

cc #25006, @bluss, @bstrie, @llogiq

[Manishearth]

r=me if you want

Might be worth a taskcluster run before sending it to the beta branch.

[tshepang]

Why remove this?

[llogiq]

Because overloading '+' for concatenation is considered a misfeature. '+' is usually reserved for a commutative Operation, which vec concatenation clearly isn't. Removing this undos the wrong precedent and allows us to freely explore the design space.

There were a few suggestions for a concatenating operator, so expect this to come back in different form in a future version.

[aturon]

For better or worse, there's a clear consensus that it's simply too late to make this change (which would need to be made to String as well). I'm going to close this PR. Thanks @alexcrichton.

[bluss]

We said we were going to look at the results of the taskcluster / regression report with this PR. Let's stick to the plan?

[aturon]

Sure, it seems there was a misunderstanding on my part about the consensus here. Reopening.

[brson]

Regression report: https://gist.github.com/brson/3455344f4a856d2d21ab

[tshepang]

Is it not a bug to have a Stable method use an Unstable method internally? In this case, I see that push_all is used in the add method.

[Gankra]

No, it's not a bug. Implementation details are implementation details. We're free to use unstable APIs to better implement the stable ones.

[aturon]

So, I'm not sure what conclusions to draw from the regression report. This seems like a fairly small amount of breakage, but I still feel uneasy making breaking changes at this stage. @cmr @sfackler @bluss @alexcrichton, do any of you have strong opinions here?

[alexcrichton]

With at least 8 crates directly breaking as a result of this change (and possibly more that are hidden in non-root failures) I'm also quite worried about this, and I am much less motivated to push this through.

[bluss]

I think it looks viable to remove it, just from the test. Of course that only considers Rust-stable crates. The non-stable crates breaking might not be so happy though.

[sfackler]

I say we go for it.

[alexcrichton]

We discussed this at triage today and concluded that although this is close to 1.0 it's best to take the conservative route for now, so we're going to move forward on this.

@bors: r=aturon 8d3f84e p=10

[aturon]

In particular, the breakage seems limited enough, and we have enough other minor last-minute breakage being back-ported to beta, that on balance this seems OK. Thanks @bluss for helping push this through.

[bluss]

Ok, exciting. Looking forward to a good discussion about concatenation when the dust after 1.0 settles.

[llogiq]

@bluss: Me too. Thanks @aturon and @alexcrichton for going through with this.

[bors]

⌛ Testing commit 8d3f84e with merge 5ae026e...

[tshepang]

@bluss is push too clunky?

[bluss]

@tshepang Push is just one element, for multiple we have .push_all or .extend which are suboptimal right now. There's an rfc for discussing improvements to extend already.

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt