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

Fix derived PartialOrd operators #81384

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Fix derived PartialOrd operators #81384

merged 2 commits into from
Feb 9, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 25, 2021 •
edited

The derived implementation of partial_cmp compares matching fields one
by one, stopping the computation when the result of a comparison is not
equal to Some(Equal).

On the other hand the derived implementation for lt, le, gt and
ge continues the computation when the result of a field comparison is
None, consequently those operators are not transitive and inconsistent
with partial_cmp.

Fix the inconsistency by using the default implementation that fall-backs
to the partial_cmp. This also avoids creating very deeply nested
closures that were quite costly to compile.

Fixes #81373.
Helps with #81278, #80118.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2021
@hameerabbasi
Copy link
Contributor

As noted in in Zulip this will require a crater run.

@tmiasko tmiasko mentioned this pull request Jan 25, 2021
Closed
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 25, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 25, 2021

⌛ Trying commit 5b3d70eaefc1e4375f955a5a94e235e60570489b with merge 69fff217252aad8de0c3b890699c7c4275062d97...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors 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 Jan 25, 2021
@tmiasko tmiasko changed the title Make derived PartialOrd operators transitive Fix derived PartialOrd operators Jan 25, 2021
@tmiasko tmiasko marked this pull request as ready for review January 25, 2021 15:43
@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 25, 2021

It will have to wait for a try build and crater run, but should be otherwise ready for review.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 25, 2021

Looks like fix for earlier build issue landed in #81380.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 25, 2021

⌛ Trying commit 96f4c7906a5f30d39eae896b974f4b86f8b90506 with merge 9d00041aa7f1137de819a10d3cbf8e8e48105d70...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jan 25, 2021

☀️ Try build successful - checks-actions
Build commit: 9d00041aa7f1137de819a10d3cbf8e8e48105d70 (9d00041aa7f1137de819a10d3cbf8e8e48105d70)

@rust-timer
Copy link
Collaborator

Queued 9d00041aa7f1137de819a10d3cbf8e8e48105d70 with parent 84864bf, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 25, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (9d00041aa7f1137de819a10d3cbf8e8e48105d70): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 25, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 9, 2021
@tmiasko
Fix derived PartialOrd operators
e4efccd 
The derived implementation of `partial_cmp` compares matching fields one
by one, stopping the computation when the result of a comparison is not
equal to `Some(Equal)`.

On the other hand the derived implementation for `lt`, `le`, `gt` and
`ge` continues the computation when the result of a field comparison is
`None`, consequently those operators are not transitive and inconsistent
with `partial_cmp`.

Fix the inconsistency by using the default implementation that fall-backs
to the `partial_cmp`. This also avoids creating very deeply nested
closures that were quite costly to compile.
@tmiasko
./x.py test --bless
62366ee
@petrochenkov
Copy link
Contributor

Some coverage tests need to be updated.

@petrochenkov petrochenkov 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 Feb 9, 2021
@tmiasko
Copy link
Contributor Author

tmiasko commented Feb 9, 2021

Updated the test output.

I will open an issue about producing a canonical implementation.

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot 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 Feb 9, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 9, 2021

📌 Commit 62366ee has been approved by petrochenkov

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2021
@bors
Copy link
Contributor

bors commented Feb 9, 2021

⌛ Testing commit 62366ee with merge c648bd5...

@tmiasko tmiasko mentioned this pull request Feb 9, 2021
Open
@bors
Copy link
Contributor

bors commented Feb 9, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing c648bd5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2021
@bors bors merged commit c648bd5 into rust-lang:master Feb 9, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 9, 2021
@tmiasko tmiasko deleted the partial-ord branch February 9, 2021 11:39
@bors bors mentioned this pull request Feb 9, 2021
Merged
@pthariensflame
Copy link
Contributor

relnotes needed AFAICT, for compatibility’s sake if nothing else.

@m-ou-se m-ou-se mentioned this pull request Jul 6, 2022
Merged
@scottmcm scottmcm mentioned this pull request Feb 17, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2023
@bors
Auto merge of rust-lang#108157 - scottmcm:tuple-gt-via-partialcmp, r=…
816f958 
…dtolnay

Use `partial_cmp` to implement tuple `lt`/`le`/`ge`/`gt`

In today's implementation, `(A, B)::gt` contains calls to *both* `A::eq` *and* `A::gt`.

That's fine for primitives, but for things like `String`s it's kinda weird -- `(String, usize)::gt` has a call to both `bcmp` and `memcmp` (<https://rust.godbolt.org/z/7jbbPMesf>) because when `bcmp` says the `String`s aren't equal, it turns around and calls `memcmp` to find out which one's bigger.

This PR changes the implementation to instead implement `(A, …, C, Z)::gt` using `A::partial_cmp`, `…::partial_cmp`, `C::partial_cmp`, and `Z::gt`.  (And analogously for `lt`, `le`, and `ge`.)  That way expensive comparisons don't need to be repeated.

Technically this is an observable change on stable, so I've marked it `needs-fcp` + `T-libs-api` and will
r? rust-lang/libs-api

I'm hoping that this will be non-controversial, however, since it's very similar to the observable changes that were made to the derives (rust-lang#81384 rust-lang#98655) -- like those, this only changes behaviour if a type overrode behaviour in a way inconsistent with the rules for the various traits involved.

(The first commit here is rust-lang#108156, adding the codegen test, which I used to make sure this doesn't regress behaviour for primitives.)

Zulip conversation about this change: <https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/.60.3E.60.20on.20Tuples/near/328392927>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.

Derived PartialOrd is incorrect when inner values are unordered
10 participants
@tmiasko @rust-highfive @hameerabbasi @rust-timer @bors @rust-log-analyzer @petrochenkov @pthariensflame @davidtwco @rustbot