Fix derived PartialOrd operators by tmiasko · Pull Request #81384 · rust-lang/rust

tmiasko · 2021-01-25T14:22:31Z

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 · 2021-01-25T14:22:35Z

r? @davidtwco

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

hameerabbasi · 2021-01-25T15:02:51Z

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

tmiasko · 2021-01-25T15:18:09Z

@bors try @rust-timer queue

rust-timer · 2021-01-25T15:18:11Z

Awaiting bors try build completion.

bors · 2021-01-25T15:18:21Z

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

tmiasko · 2021-01-25T15:44:02Z

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

tmiasko · 2021-01-25T15:52:52Z

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

@bors try @rust-timer queue

rust-timer · 2021-01-25T15:52:54Z

Awaiting bors try build completion.

bors · 2021-01-25T15:53:04Z

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

bors · 2021-01-25T16:53:35Z

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

rust-timer · 2021-01-25T16:53:37Z

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

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

rust-timer · 2021-01-25T18:49:23Z

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

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.

petrochenkov · 2021-02-09T07:47:17Z

Some coverage tests need to be updated.

tmiasko · 2021-02-09T08:40:49Z

Updated the test output.

I will open an issue about producing a canonical implementation.

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

petrochenkov · 2021-02-09T08:45:39Z

@bors r+

bors · 2021-02-09T08:45:40Z

📌 Commit 62366ee has been approved by petrochenkov

bors · 2021-02-09T08:47:43Z

⌛ Testing commit 62366ee with merge c648bd5...

bors · 2021-02-09T11:38:41Z

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

pthariensflame · 2021-04-06T08:31:28Z

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

…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>.

rust-highfive assigned davidtwco Jan 25, 2021

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

tmiasko force-pushed the partial-ord branch from 4416dea to 5b3d70e Compare January 25, 2021 14:33

tmiasko mentioned this pull request Jan 25, 2021

Can't compile huge struct #81278

Closed

This comment has been minimized.

Sign in to view

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 force-pushed the partial-ord branch from 5b3d70e to 96f4c79 Compare January 25, 2021 15:41

tmiasko changed the title ~~Make derived PartialOrd operators transitive~~ Fix derived PartialOrd operators Jan 25, 2021

tmiasko marked this pull request as ready for review January 25, 2021 15:43

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 25, 2021

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

tmiasko force-pushed the partial-ord branch from 96f4c79 to dd40a10 Compare January 25, 2021 18:52

This comment has been minimized.

Sign in to view

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 added 2 commits February 9, 2021 08:15

./x.py test --bless

62366ee

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 force-pushed the partial-ord branch from dd40a10 to 62366ee Compare February 9, 2021 08:26

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

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

tmiasko mentioned this pull request Feb 9, 2021

Create a canonical implementation of traits and use it in tuples & derive macros #81915

Open

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2021

bors merged commit c648bd5 into rust-lang:master Feb 9, 2021

rustbot added this to the 1.52.0 milestone Feb 9, 2021

tmiasko deleted the partial-ord branch February 9, 2021 11:39

bors mentioned this pull request Feb 9, 2021

Remove usages of expr_method_call in derive(Ord,PartialOrd,RustcEncode,RustcDecode) #81411

Merged

m-ou-se mentioned this pull request Jul 6, 2022

Don't derive PartialEq::ne. #98655

Merged

scottmcm mentioned this pull request Feb 17, 2023

Use partial_cmp to implement tuple lt/le/ge/gt #108157

Merged

Uh oh!

Conversation

tmiasko commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 25, 2021

Uh oh!

hameerabbasi commented Jan 25, 2021

Uh oh!

tmiasko commented Jan 25, 2021

Uh oh!

rust-timer commented Jan 25, 2021

Uh oh!

bors commented Jan 25, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

tmiasko commented Jan 25, 2021

Uh oh!

tmiasko commented Jan 25, 2021

Uh oh!

rust-timer commented Jan 25, 2021

Uh oh!

bors commented Jan 25, 2021

Uh oh!

This comment has been minimized.

bors commented Jan 25, 2021

Uh oh!

rust-timer commented Jan 25, 2021

Uh oh!

rust-timer commented Jan 25, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Feb 9, 2021

Uh oh!

tmiasko commented Feb 9, 2021

Uh oh!

petrochenkov commented Feb 9, 2021

Uh oh!

bors commented Feb 9, 2021

Uh oh!

bors commented Feb 9, 2021

Uh oh!

bors commented Feb 9, 2021

Uh oh!

pthariensflame commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tmiasko commented Jan 25, 2021 •

edited

Loading