Ensure derive(PartialOrd) is no longer accidentally exponential #50011

varkor · 2018-04-16T22:35:28Z

Previously, two comparison operations would be generated for each field, each of which could delegate to another derived PartialOrd. Now we use ordering and optional chaining to ensure each pair of fields is only compared once, addressing #49650 (comment).

Closes #49505.

r? @Manishearth (sorry for changing it again so soon!)

Close #50755

Manishearth

@bors r+

Manishearth · 2018-04-16T23:24:51Z

@bors r+

bors · 2018-04-16T23:24:52Z

📌 Commit 3ecaeba has been approved by Manishearth

varkor · 2018-04-17T00:23:22Z

Argh, messed up the default there, sorry.

Manishearth · 2018-04-17T00:24:58Z

@bors r+

Please test this on various combinations of greater than/less than just in case 😄

bors · 2018-04-17T00:24:59Z

📌 Commit d0e6f70 has been approved by Manishearth

rust-highfive · 2018-04-17T01:26:18Z

Your PR failed on Travis (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.

Resolving deltas: 100% (616285/616285), completed with 4913 local objects.
---
[00:00:43] configure: rust.quiet-tests     := True
---
[00:48:03] .................................................................................i..................
[00:48:09] ........................i...........................................................................
---
[00:48:52] .......................i...........................................................................i
[00:48:58] ....................................................................................................
[00:49:05] .............ii.....................................................................................
---
[00:49:52] .............................................i......................................................
[00:50:12] ....................................................................................................
[00:50:29] ....................................................................................................
[00:50:45] ...............................................................F....................................
---
[00:53:55] ..............................i.....................................................................
[00:54:10] ...............................................................i....................................
[00:54:25] .................................................i..................................................
[00:54:46] ....................................................................................................
[00:55:07] ....................................................................................................
[00:55:29] ....................................................................................................
[00:55:55] .......i............................................................................................
[00:56:22] ...i.............................................................................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:56:32] ...................
[00:57:05] ....................................................................................................
[00:57:37] .....................................................................ii.............................
[00:58:28] ................................i.................................................test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:58:35] ...i.ii...........
[00:59:19] .............................................................................................iiiiiii
[00:59:49] ....................................................................................................
[01:00:18] ....................................................................................................
[01:00:42] ....................................................................................................
---------------
[01:01:00]
[01:01:00] thread '[run-pass] run-pass/derive-partialord-correctness.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2919:9
---
[01:01:00] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm"Tue, 17 Apr 2018 01:26:13 GMT

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 @TimNN. (Feature Requests)

nox · 2018-04-17T08:18:41Z

src/libsyntax_ext/deriving/cmp/partial_ord.rs

+        // `self.fi.partial_cmp(other.fi)`
+        let cmp = cx.expr_method_call(span,
+                                      cx.expr_addr_of(span, self_f),
+                                      Ident::from_str("partial_cmp"),


How does this avoid using an inherent partial_cmp method if the underlying type doesn't actually implement PartialOrd?

Should be PartialOrd::partial_cmp(...) - like Clone::clone deriving (IIRC).

nox · 2018-04-17T08:21:35Z

src/libsyntax_ext/deriving/cmp/partial_ord.rs

+
+        let default = ordering_path(cx, "Equal");
+        // `_.unwrap_or(Ordering::Equal)`
+        cx.expr_method_call(span, cmp, Ident::from_str("unwrap_or"), vec![default])


I'm also not sure about what happens if there is a trait implemented by Option<Ordering> in scope with an unwrap_or method.

SimonSapin · 2018-04-17T08:26:29Z

@Manishearth did you mean to r+, if this needs additional tests?

Additionally for nox’s concern (which I think suggest to use UFCS, like other derives seem to do), and for tests failing on Travis, let’s remove this from the queue for now:

@bors r-

Manishearth · 2018-04-17T18:12:52Z

Nah, I was asking for this to be manually tested and we can land the tests later if we wish. But yeah we should probably have the tests part of this before landing itself.

I'll defer to @nox though

@bors delegate=nox

bors · 2018-04-17T18:12:53Z

✌️ @nox can now approve this pull request

bors · 2018-04-17T18:13:16Z

✌️ @nox can now approve this pull request

varkor · 2018-04-17T19:00:51Z

I think there's actually a good range of tests for this already — that's why the previous commits were caught. I just... might not... have been running all the tests before pushing... 😳

I think everything's passing now, but let's wait for Travis to confirm. PartialOrd is a tricky one.

kennytm · 2018-04-18T10:57:19Z

(Travis passed)

pietroalbini · 2018-04-23T10:41:01Z

Ping from triage @nox! You are now delegated to review this PR!

nox · 2018-04-23T11:08:25Z

src/libsyntax_ext/deriving/cmp/partial_ord.rs

-            // )
+            // self.f1.partial_cmp(other.f1).unwrap_or(Ordering::Equal)
+            //     .then_with(|| self.f2.partial_cmp(other.f2).unwrap_or(Ordering::Equal))
+            // != Ordering::Greater


IMO those should just emit nested match expressions that return false directly.

Would there be any direct advantage to doing so? The main reason I used functions was because of the existing issues with match codegen efficiency, but I don't see that it makes a huge difference either way.

nox · 2018-04-23T11:08:37Z

src/libsyntax_ext/deriving/cmp/partial_ord.rs


-            let strict_ineq = cx.expr_binary(span, strict_op, self_f.clone(), other_f.clone());
+            // `self.fi.partial_cmp(other.fi).unwrap_or(Ordering::Equal)`


This comment seems outdated, given we now use UFCS.

nox · 2018-04-23T11:08:41Z

src/libsyntax_ext/deriving/cmp/partial_ord.rs

-
-            let and = cx.expr_binary(span, BinOpKind::And, deleg_cmp, subexpr);
-            cx.expr_binary(span, BinOpKind::Or, strict_ineq, and)
+            // `self.fi.partial_cmp(other.fi).unwrap_or(Ordering::Equal).then_with(...)`


This comment seems outdated, given we now use UFCS.

bors · 2018-04-25T04:25:47Z

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

Previously, two comparison operations would be generated for each field, each of which could delegate to another derived PartialOrd. Now we use ordering and optional chaining to ensure each pair of fields is only compared once.

shepmaster · 2018-05-06T22:03:31Z

Ping from triage, @nox! Looks like this is ready to review again.

shepmaster · 2018-05-06T22:03:54Z

Nit: AFAIK it's not called UFCS anymore, but Fully-Qualified Syntax

nox · 2018-05-09T09:23:05Z

I still don't like that this is not using proper match expressions. I'll defer to @Manishearth for the final review (not sure why he delegated me to review it anyway).

pietroalbini · 2018-05-14T10:21:31Z

Ping from triage @Manishearth! This PR needs your review.

Manishearth · 2018-05-14T13:53:38Z

I don't have time to review this rn, sorry.

Manishearth · 2018-05-14T14:11:35Z

oh, the logic has already been reviewed for this

code looks good.

@bors r+

bors · 2018-05-14T14:11:36Z

📌 Commit 6805e5a has been approved by Manishearth

kennytm · 2018-05-14T14:48:31Z

@bors p=0

bors · 2018-05-15T03:14:53Z

⌛ Testing commit 6805e5a with merge d711dc9...

@Manishearth

Ensure derive(PartialOrd) is no longer accidentally exponential Previously, two comparison operations would be generated for each field, each of which could delegate to another derived PartialOrd. Now we use ordering and optional chaining to ensure each pair of fields is only compared once, addressing #49650 (comment). Closes #49505. r? @Manishearth (sorry for changing it again so soon!) Close #50755

bors · 2018-05-15T05:39:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing d711dc9 to master...

rust-highfive assigned Manishearth Apr 16, 2018

rust-highfive added the S-waiting-on-review label Apr 16, 2018

varkor mentioned this pull request Apr 16, 2018

Error in derived PartialOrd for structs based on types with incomparable elements. #49650

Closed

Manishearth approved these changes Apr 16, 2018

View reviewed changes

bors added S-waiting-on-bors and removed S-waiting-on-review labels Apr 16, 2018

rust-lang deleted a comment from rust-highfive Apr 17, 2018

nox reviewed Apr 17, 2018

View reviewed changes

bors added S-waiting-on-author and removed S-waiting-on-bors labels Apr 17, 2018

pietroalbini assigned nox Apr 23, 2018

nox suggested changes Apr 23, 2018

View reviewed changes

varkor force-pushed the partialord-opt-ii branch 2 times, most recently from 211ca31 to 228228c Compare April 24, 2018 21:02

varkor added 3 commits April 25, 2018 11:26

Fix handling of None

6330bf2

Use UFCS

98e3e82

Update comments to UFCS style

6805e5a

varkor force-pushed the partialord-opt-ii branch from 228228c to 6805e5a Compare April 25, 2018 10:26

varkor added S-waiting-on-review and removed S-waiting-on-author labels Apr 27, 2018

bors added S-waiting-on-bors and removed S-waiting-on-review labels May 14, 2018

ishitatsuyuki mentioned this pull request May 15, 2018

Latest nightly rustc deadlocks/hangs indefinitely when trying to compile otherwise correct code (nightly-x86_64-unknown-linux-gnu 9fae15374 2018-05-13) #50755

Closed

bors merged commit 6805e5a into rust-lang:master May 15, 2018

varkor deleted the partialord-opt-ii branch May 15, 2018 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure derive(PartialOrd) is no longer accidentally exponential #50011

Ensure derive(PartialOrd) is no longer accidentally exponential #50011

varkor commented Apr 16, 2018 •

edited by ishitatsuyuki

Loading

Manishearth left a comment

Manishearth commented Apr 16, 2018

bors commented Apr 16, 2018

varkor commented Apr 17, 2018

Manishearth commented Apr 17, 2018

bors commented Apr 17, 2018

rust-highfive commented Apr 17, 2018

nox Apr 17, 2018

eddyb Apr 17, 2018

nox Apr 17, 2018

SimonSapin commented Apr 17, 2018

Manishearth commented Apr 17, 2018 •

edited

Loading

bors commented Apr 17, 2018

bors commented Apr 17, 2018

varkor commented Apr 17, 2018 •

edited

Loading

kennytm commented Apr 18, 2018

pietroalbini commented Apr 23, 2018

nox Apr 23, 2018

varkor Apr 24, 2018

nox Apr 23, 2018

nox Apr 23, 2018

bors commented Apr 25, 2018

shepmaster commented May 6, 2018

shepmaster commented May 6, 2018

nox commented May 9, 2018

pietroalbini commented May 14, 2018

Manishearth commented May 14, 2018

Manishearth commented May 14, 2018

bors commented May 14, 2018

kennytm commented May 14, 2018 •

edited

Loading

bors commented May 15, 2018

bors commented May 15, 2018


		let strict_ineq = cx.expr_binary(span, strict_op, self_f.clone(), other_f.clone());
		// `self.fi.partial_cmp(other.fi).unwrap_or(Ordering::Equal)`

Ensure derive(PartialOrd) is no longer accidentally exponential #50011

Ensure derive(PartialOrd) is no longer accidentally exponential #50011

Conversation

varkor commented Apr 16, 2018 • edited by ishitatsuyuki Loading

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth commented Apr 16, 2018

bors commented Apr 16, 2018

varkor commented Apr 17, 2018

Manishearth commented Apr 17, 2018

bors commented Apr 17, 2018

rust-highfive commented Apr 17, 2018

nox Apr 17, 2018

Choose a reason for hiding this comment

eddyb Apr 17, 2018

Choose a reason for hiding this comment

nox Apr 17, 2018

Choose a reason for hiding this comment

SimonSapin commented Apr 17, 2018

Manishearth commented Apr 17, 2018 • edited Loading

bors commented Apr 17, 2018

bors commented Apr 17, 2018

varkor commented Apr 17, 2018 • edited Loading

kennytm commented Apr 18, 2018

pietroalbini commented Apr 23, 2018

nox Apr 23, 2018

Choose a reason for hiding this comment

varkor Apr 24, 2018

Choose a reason for hiding this comment

nox Apr 23, 2018

Choose a reason for hiding this comment

nox Apr 23, 2018

Choose a reason for hiding this comment

bors commented Apr 25, 2018

shepmaster commented May 6, 2018

shepmaster commented May 6, 2018

nox commented May 9, 2018

pietroalbini commented May 14, 2018

Manishearth commented May 14, 2018

Manishearth commented May 14, 2018

bors commented May 14, 2018

kennytm commented May 14, 2018 • edited Loading

bors commented May 15, 2018

bors commented May 15, 2018

varkor commented Apr 16, 2018 •

edited by ishitatsuyuki

Loading

Manishearth commented Apr 17, 2018 •

edited

Loading

varkor commented Apr 17, 2018 •

edited

Loading

kennytm commented May 14, 2018 •

edited

Loading