Update Rhs on ShlAssign to default to Self by npmccallum · Pull Request #49630 · rust-lang/rust

npmccallum · 2018-04-03T19:48:21Z

This matches the behavior on ShrAssign and all other *Assign operations.

BurntSushi · 2018-04-04T10:52:00Z

@rust-lang/libs This seems fine to me if it is consistent with other traits. Are there any backwards compatibility concerns to worry about here? (I don't think so?)

alexcrichton · 2018-04-04T13:48:48Z

Oh dear this definitely looks like an oversight! I can't think of any back-compat concerns but since this is a widely used trait I think we could benefit from crater

@bors: try

@rust-lang/release could a crater run be scheduled for this PR?

bors · 2018-04-04T13:48:59Z

⌛ Trying commit 37464a0a7d53cc22b4345d5b8a6773810fc0a38e with merge 4846fb5a35cbaeaa1107a3550cbfde95362b47c3...

bors · 2018-04-04T15:48:46Z

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

clarfonthey · 2018-04-04T15:51:42Z

I honestly would have suggested to default to Self for most traits, but I thought that not defaulting for shift ops was intentional. Because at least one of them defaults, I'd make sure we do fix all of them before crater runs.

npmccallum · 2018-04-05T11:59:09Z

It looks like we aren't missing any other instances of this oversight:

$ grep -R "pub trait .*Rhs" src/
src/libcore/cmp.rs:pub trait PartialEq<Rhs: ?Sized = Self> {
src/libcore/cmp.rs:pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
src/libcore/ops/arith.rs:pub trait AddAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait SubAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait MulAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait DivAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait RemAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitAndAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitOrAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitXorAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait ShlAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait ShrAssign<Rhs=Self> {

For reference, this is before my patch:

$ grep -R "pub trait .*Rhs" src/
src/libcore/cmp.rs:pub trait PartialEq<Rhs: ?Sized = Self> {
src/libcore/cmp.rs:pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
src/libcore/ops/arith.rs:pub trait AddAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait SubAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait MulAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait DivAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait RemAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitAndAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitOrAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitXorAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait ShlAssign<Rhs> {
src/libcore/ops/bit.rs:pub trait ShrAssign<Rhs=Self> {

leoyvens · 2018-04-05T12:55:41Z

I believe we currently guarantee that adding a default doesn't break anything.

clarfonthey · 2018-04-06T02:30:19Z

@npmccallum Shl and Shr don't; ShlAssign did for some reason

Operator traits seem to be inconsistent regarding the right hand side generic. Most default Rhs/RHS to Self, but a few omit this. This patch modifies them all to default to Self. This should not have any backwards compatibility issues because we are only enabling code that previously wouldn't compile.

npmccallum · 2018-04-06T13:54:25Z

@clarcharr I pushed a new version of the patch which fixes Shl and Shr in addition to ShlAssign. ShrAssign is already correct.

Please also see this related issue: rust-lang/libc#964

npmccallum · 2018-04-06T19:48:21Z

After the above patches (including libc), we get these results:

$ grep -Ri "pub trait .*Rhs" src/
src/doc/book/first-edition/src/operators-and-overloading.md:pub trait Add<RHS = Self> {
src/doc/rust-by-example/src/generics/phantom/testcase_units.md:pub trait Add<RHS = Self> {
src/libcore/cmp.rs:pub trait PartialEq<Rhs: ?Sized = Self> {
src/libcore/cmp.rs:pub trait PartialOrd<Rhs: ?Sized = Self>: PartialEq<Rhs> {
src/libcore/ops/arith.rs:pub trait Add<RHS=Self> {
src/libcore/ops/arith.rs:pub trait Sub<RHS=Self> {
src/libcore/ops/arith.rs:pub trait Mul<RHS=Self> {
src/libcore/ops/arith.rs:pub trait Div<RHS=Self> {
src/libcore/ops/arith.rs:pub trait Rem<RHS=Self> {
src/libcore/ops/arith.rs:pub trait AddAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait SubAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait MulAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait DivAssign<Rhs=Self> {
src/libcore/ops/arith.rs:pub trait RemAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitAnd<RHS=Self> {
src/libcore/ops/bit.rs:pub trait BitOr<RHS=Self> {
src/libcore/ops/bit.rs:pub trait BitXor<RHS=Self> {
src/libcore/ops/bit.rs:pub trait Shl<RHS=Self> {
src/libcore/ops/bit.rs:pub trait Shr<RHS=Self> {
src/libcore/ops/bit.rs:pub trait BitAndAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitOrAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait BitXorAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait ShlAssign<Rhs=Self> {
src/libcore/ops/bit.rs:pub trait ShrAssign<Rhs=Self> {
src/liblibc/src/dox.rs:    pub trait Div<RHS=Self> {
src/liblibc/src/dox.rs:    pub trait Shl<RHS=Self> {
src/liblibc/src/dox.rs:    pub trait Mul<RHS=Self> {
src/liblibc/src/dox.rs:    pub trait Sub<RHS=Self> {
src/liblibc/src/dox.rs:    pub trait Bitor<RHS=Self> {
src/liblibc/src/dox.rs:    pub trait Add<RHS = Self> {
src/test/compile-fail/issue-28576.rs:pub trait Foo<RHS=Self> {
src/test/run-pass/trait-inheritance-subst.rs:pub trait Add<RHS,Result> {
src/test/rustdoc/auxiliary/issue-20727.rs:pub trait Add<RHS = Self> {
src/test/rustdoc/issue-20727-2.rs:pub trait Add<RHS = Self> {

The only case we haven't handled that I can see is the one in src/test/run-pass/trait-inheritance-subst.rs, which we probably don't care about.

Mark-Simulacrum · 2018-04-13T03:33:19Z

Crater has started. ETA is ~5 days.

Mark-Simulacrum · 2018-04-18T23:54:20Z

Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49630/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

npmccallum · 2018-04-19T00:21:34Z

I'm new at this, but the failure don't appear to me to be related to this patch set.

alexcrichton · 2018-04-19T02:43:15Z

Yep those do indeed look spurious, so I think this is good to go!

@bors: r+

bors · 2018-04-19T02:43:16Z

📌 Commit 587098a has been approved by alexcrichton

bors · 2018-04-19T03:53:39Z

⌛ Testing commit 587098a with merge 883bf4b...

Update Rhs on ShlAssign to default to Self This matches the behavior on ShrAssign and all other *Assign operations.

bors · 2018-04-19T06:19:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 883bf4b to master...

rust-highfive assigned BurntSushi Apr 3, 2018

frewsxcv added the T-libs-api [DEPRECATED; DO NOT USE] label Apr 3, 2018

BurntSushi added the S-waiting-on-team label Apr 4, 2018

alexcrichton added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-team labels Apr 4, 2018

npmccallum force-pushed the shl branch from 37464a0 to 587098a Compare April 6, 2018 13:51

npmccallum mentioned this pull request Apr 6, 2018

Default RHS to Self for Div and Shl rust-lang/libc#964

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 19, 2018

bors added a commit that referenced this pull request Apr 19, 2018

Auto merge of #49630 - npmccallum:shl, r=alexcrichton

883bf4b

Update Rhs on ShlAssign to default to Self This matches the behavior on ShrAssign and all other *Assign operations.

bors merged commit 587098a into rust-lang:master Apr 19, 2018

Uh oh!

Uh oh!

Conversation

npmccallum commented Apr 3, 2018

Uh oh!

BurntSushi commented Apr 4, 2018

Uh oh!

alexcrichton commented Apr 4, 2018

Uh oh!

bors commented Apr 4, 2018

Uh oh!

bors commented Apr 4, 2018

Uh oh!

clarfonthey commented Apr 4, 2018

Uh oh!

npmccallum commented Apr 5, 2018

Uh oh!

leoyvens commented Apr 5, 2018

Uh oh!

clarfonthey commented Apr 6, 2018

Uh oh!

npmccallum commented Apr 6, 2018

Uh oh!

npmccallum commented Apr 6, 2018

Uh oh!

Mark-Simulacrum commented Apr 13, 2018

Uh oh!

Mark-Simulacrum commented Apr 18, 2018

Uh oh!

npmccallum commented Apr 19, 2018

Uh oh!

alexcrichton commented Apr 19, 2018

Uh oh!

bors commented Apr 19, 2018

Uh oh!

bors commented Apr 19, 2018

Uh oh!

bors commented Apr 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants