Add PartialEq and PartialOrd for NonZeroInt to Int #72405

Lucretiel · 2020-05-21T00:02:52Z

Propose adding PartialEq and PartialOrd to the NonZeroInt types to compare to their inner types. Seems like a pretty obvious change, but I'm obviously open to any feedback or suggestions.

I'd also like to implement PartialEq<NonZeroInt> for Int, but I wasn't sure what the implications were for implementing traits on primitive types like that.

Retry of #70855

rust-highfive · 2020-05-21T00:02:56Z

r? @kennytm

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

kennytm · 2020-05-21T20:33:50Z

r? @Amanieu

kennytm · 2020-05-21T20:36:32Z

IMO if impl PartialEq<uXX> for NonZeroUXX is accepted, we must also include impl PartialEq<NonZeroUXX> for uXX for avoid breaking the symmetry.

Lucretiel · 2020-05-21T21:15:48Z

I'm happy to add that; I wasn't sure if there were any especial concerns related to implementing traits on primitive types in the standard library

Lucretiel · 2020-05-21T22:00:08Z

Rebased; added symmetric operators

rust-highfive · 2020-05-21T22:05:54Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[section]Starting: Linux mingw-check
##[section]Starting: Initialize job
Agent name: 'Azure Pipelines 8'
Agent machine name: 'fv-az578'
Current agent version: '2.168.2'
##[group]Operating System
16.04.6
LTS
LTS
##[endgroup]
##[group]Virtual Environment
Environment: ubuntu-16.04
Version: 20200512.2
Included Software: https://github.com/actions/virtual-environments/blob/ubuntu16/20200512.2/images/linux/Ubuntu1604-README.md
##[endgroup]
Agent running as: 'vsts'
Prepare build directory.
Set build variables.
Download all required tasks.
Download all required tasks.
Downloading task: Bash (3.163.2)
Checking job knob settings.
   Knob: AgentToolsDirectory = /opt/hostedtoolcache Source: ${AGENT_TOOLSDIRECTORY} 
   Knob: AgentPerflog = /home/vsts/perflog Source: ${VSTS_AGENT_PERFLOG} 
Start tracking orphan processes.
##[section]Finishing: Initialize job
##[section]Starting: Configure Job Name
==============================================================================
---
========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/67352205-2924-4d46-8b55-4ab50c4abae6.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/72405/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/72405/merge:refs/remotes/pull/72405/merge
---
 ---> 3adb0605cc65
Step 6/7 : ENV RUN_CHECK_WITH_PARALLEL_QUERIES 1
 ---> Using cache
 ---> 28dbc326cb7f
Step 7/7 : ENV SCRIPT python3 ../x.py test src/tools/expand-yaml-anchors &&            python3 ../x.py check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu &&            python3 ../x.py build --stage 0 src/tools/build-manifest &&            python3 ../x.py test --stage 0 src/tools/compiletest &&            python3 ../x.py test src/tools/tidy &&            python3 ../x.py doc --stage 0 src/libstd &&            /scripts/validate-toolstate.sh
 ---> 537a01811900
Successfully built 537a01811900
Successfully tagged rust-ci:latest
Built container sha256:537a018119009dc218456238dec90b5530050db1e2a1e166550c218003f6159d
---
    Checking panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
error[E0282]: type annotations needed
    --> src/libstd/io/buffered.rs:1071:13
     |
1071 |         let prefix_amt = prefix.iter().map(|i| i.len()).sum();
     |             ^^^^^^^^^^ consider giving `prefix_amt` a type
error: aborting due to previous error

For more information about this error, try `rustc --explain E0282`.
error: could not compile `std`.
---
  local time: Thu May 21 22:05:37 UTC 2020
  network time: Thu, 21 May 2020 22:05:37 GMT
== end clock drift check ==

##[error]Bash exited with code '1'.
##[section]Finishing: Run build
##[section]Starting: Checkout rust-lang/rust@refs/pull/72405/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/72405/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3668) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

Lucretiel · 2020-05-21T22:09:17Z

Build failures seem unrelated to this PR? Unless type inference looks "through" trait implementations like this, which I thought it didn't. In any case, if that is related to this PR, then creating the int <=> nonzero impls like this might be considered a breaking change, right?

Lucretiel · 2020-05-21T22:15:29Z

Hmm. Confirmed that this change does result in that breakage, probably because the type of prefix_amt is inferred via the (previously unambiguous) comparison operators. For some reason I didn't think type inference worked at that level (that is, selecting a type that implements a trait if it's the only one that implements that trait in the given context). That would mean that in general adding new trait implementations could be breaking, because someone could be relying on a specific trait only being implemented by a single type.

crlf0710 · 2020-06-05T12:49:52Z

@Lucretiel Ping from triage! This needs to get the CI fixed. Or if you feel the breakage is acceptable, change the test itself and ask for a crater run if necessary. Thanks!

Lucretiel · 2020-06-05T16:56:41Z

Oh— I didn't think I was able to make that assessment. I'd like to ask for someone from the libraries team to determine if this breakage is acceptable.

If it is up to me, I'd say that it is acceptable, because I had the impression that operators couldn't be used for type inference in this way.

bors · 2020-06-25T21:34:09Z

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

Muirrum · 2020-07-30T20:59:12Z

@Lucretiel what's the status of this PR?

Lucretiel · 2020-07-30T21:39:29Z

The status is that a decision needs to be made about whether this constitutes a breaking change because of the apparent effect it has on type inference on integer primitives and I don't feel even remotely qualified to assess that.

JohnTitor · 2020-07-31T02:31:17Z

Let's re-assign from the libs.
r? @KodrAus

Lucretiel · 2020-07-31T02:43:35Z

I can fix the merge conflicts in the meantime; I was holding off because I was getting a vibe that this was going to end up rejected due to compatibility concerns and I wanted to see if there was a consensus there first.

JohnTitor · 2020-07-31T02:48:53Z

@Lucretiel In case, if you get no response for a while, you can ask the team to review on the Zulip stream: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs

Lucretiel · 2020-07-31T02:55:25Z

@JohnTitor excellent, thanks for the heads up

KodrAus · 2020-07-31T23:09:23Z

Hmm, let’s see if we can get an idea of how much breakage there is in the wild.

@craterbot check

craterbot · 2020-07-31T23:09:24Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

KodrAus · 2020-07-31T23:10:30Z

Guess we need to get the build green first 😄

Lucretiel · 2020-08-01T00:07:42Z

Yup I'll take care of it tonight

Lucretiel · 2020-08-03T20:21:22Z

Yup I'll take care of it tonight

Done

crlf0710 · 2020-08-21T11:17:54Z

CI is still red. Needs to fix the build. @Lucretiel

Dylan-DPC-zz · 2020-09-06T00:59:48Z

@Lucretiel any updates on this?

Lucretiel · 2020-09-06T04:34:02Z

Nope, sorry to be so periodic about this, been busy with other things. Will get to refactoring as soon as I'm able.

bors · 2020-09-07T02:21:46Z

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

crlf0710 · 2020-09-25T13:17:09Z

@Lucretiel Ping from triage, any updates?

Dylan-DPC-zz · 2020-09-25T19:52:56Z

@Lucretiel Thanks for the contribution. Sadly I have to close this due to inactivity. If you wish and feel like you have the time to contribute, you can open a new PR with the change and we can take it from there.

rust-highfive assigned kennytm May 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2020

Lucretiel mentioned this pull request May 21, 2020

Add PartialEq and PartialOrd for NonZeroInt to Int #70855

Closed

rust-highfive assigned Amanieu and unassigned kennytm May 21, 2020

kennytm added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 21, 2020

Lucretiel force-pushed the nonzero-partialcmp branch from dfe8a23 to 0ccaa56 Compare May 21, 2020 21:59

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2020

Muirrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2020

JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2020

rust-highfive assigned KodrAus and unassigned Amanieu Jul 31, 2020

Rebased Partial comparisons for NonZeroes

3948c65

Lucretiel force-pushed the nonzero-partialcmp branch from 0ccaa56 to 3948c65 Compare August 3, 2020 20:21

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2020

Dylan-DPC-zz closed this Sep 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PartialEq and PartialOrd for NonZeroInt to Int #72405

Add PartialEq and PartialOrd for NonZeroInt to Int #72405

Lucretiel commented May 21, 2020

rust-highfive commented May 21, 2020

kennytm commented May 21, 2020

kennytm commented May 21, 2020

Lucretiel commented May 21, 2020

Lucretiel commented May 21, 2020

rust-highfive commented May 21, 2020

Lucretiel commented May 21, 2020

Lucretiel commented May 21, 2020 •

edited

Loading

crlf0710 commented Jun 5, 2020

Lucretiel commented Jun 5, 2020

bors commented Jun 25, 2020

Muirrum commented Jul 30, 2020

Lucretiel commented Jul 30, 2020

JohnTitor commented Jul 31, 2020

Lucretiel commented Jul 31, 2020

JohnTitor commented Jul 31, 2020

Lucretiel commented Jul 31, 2020

KodrAus commented Jul 31, 2020

craterbot commented Jul 31, 2020

KodrAus commented Jul 31, 2020

Lucretiel commented Aug 1, 2020

Lucretiel commented Aug 3, 2020

crlf0710 commented Aug 21, 2020

Dylan-DPC-zz commented Sep 6, 2020

Lucretiel commented Sep 6, 2020

bors commented Sep 7, 2020

crlf0710 commented Sep 25, 2020

Dylan-DPC-zz commented Sep 25, 2020

Add PartialEq and PartialOrd for NonZeroInt to Int #72405

Add PartialEq and PartialOrd for NonZeroInt to Int #72405

Conversation

Lucretiel commented May 21, 2020

rust-highfive commented May 21, 2020

kennytm commented May 21, 2020

kennytm commented May 21, 2020

Lucretiel commented May 21, 2020

Lucretiel commented May 21, 2020

rust-highfive commented May 21, 2020

Lucretiel commented May 21, 2020

Lucretiel commented May 21, 2020 • edited Loading

crlf0710 commented Jun 5, 2020

Lucretiel commented Jun 5, 2020

bors commented Jun 25, 2020

Muirrum commented Jul 30, 2020

Lucretiel commented Jul 30, 2020

JohnTitor commented Jul 31, 2020

Lucretiel commented Jul 31, 2020

JohnTitor commented Jul 31, 2020

Lucretiel commented Jul 31, 2020

KodrAus commented Jul 31, 2020

craterbot commented Jul 31, 2020

KodrAus commented Jul 31, 2020

Lucretiel commented Aug 1, 2020

Lucretiel commented Aug 3, 2020

crlf0710 commented Aug 21, 2020

Dylan-DPC-zz commented Sep 6, 2020

Lucretiel commented Sep 6, 2020

bors commented Sep 7, 2020

crlf0710 commented Sep 25, 2020

Dylan-DPC-zz commented Sep 25, 2020

Lucretiel commented May 21, 2020 •

edited

Loading