Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update Rhs on ShlAssign to default to Self #49630

Merged
merged 1 commit into from
Apr 19, 2018
Merged

Update Rhs on ShlAssign to default to Self #49630

merged 1 commit into from
Apr 19, 2018

Conversation

npmccallum
Copy link
Contributor

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

@frewsxcv frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 3, 2018
@BurntSushi
Copy link
Member

@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?)

@BurntSushi BurntSushi added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Apr 4, 2018
@alexcrichton
Copy link
Member

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?

@alexcrichton alexcrichton added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 4, 2018
@bors
Copy link
Contributor

bors commented Apr 4, 2018

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

@bors
Copy link
Contributor

bors commented Apr 4, 2018

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

@clarfonthey
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

leoyvens commented Apr 5, 2018

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

@clarfonthey
Copy link
Contributor

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

@npmccallum
Consistently default operator Rhs/RHS to Self
587098a 
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
Copy link
Contributor Author

@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 npmccallum mentioned this pull request Apr 6, 2018
Merged
@npmccallum
Copy link
Contributor Author

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
Copy link
Member

Crater has started. ETA is ~5 days.

@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor Author

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

@alexcrichton
Copy link
Member

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

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 19, 2018

📌 Commit 587098a has been approved by alexcrichton

@bors 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
Copy link
Contributor

bors commented Apr 19, 2018

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

bors added a commit that referenced this pull request Apr 19, 2018
@bors
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
Copy link
Contributor

bors commented Apr 19, 2018

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

@bors bors merged commit 587098a into rust-lang:master Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@BurntSushi BurntSushi

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@npmccallum @BurntSushi @alexcrichton @bors @clarfonthey @leoyvens @Mark-Simulacrum @frewsxcv