Auto merge of #50011 - varkor:partialord-opt-ii, r=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
rust-lang · May 15, 2018 · d711dc9 · d711dc9
2 parents 308b7b0 + 6805e5a
commit d711dc9
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 97 deletions.
diff --git a/src/etc/generate-deriving-span-tests.py b/src/etc/generate-deriving-span-tests.py
@@ -122,7 +122,7 @@ def write_file(name, string):
 
 for (trait, supers, errs) in [('Clone', [], 1),
                               ('PartialEq', [], 2),
-                              ('PartialOrd', ['PartialEq'], 5),
+                              ('PartialOrd', ['PartialEq'], 1),
                               ('Eq', ['PartialEq'], 1),
                               ('Ord', ['Eq', 'PartialOrd', 'PartialEq'], 1),
                               ('Debug', [], 1),

diff --git a/src/libsyntax_ext/deriving/cmp/partial_ord.rs b/src/libsyntax_ext/deriving/cmp/partial_ord.rs
@@ -147,34 +147,37 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<
             // as the outermost one, and the last as the innermost.
             false,
             |cx, span, old, self_f, other_fs| {
-        // match new {
-        //     Some(::std::cmp::Ordering::Equal) => old,
-        //     cmp => cmp
-        // }
+                // match new {
+                //     Some(::std::cmp::Ordering::Equal) => old,
+                //     cmp => cmp
+                // }
 
-        let new = {
-            let other_f = match (other_fs.len(), other_fs.get(0)) {
-                (1, Some(o_f)) => o_f,
-                _ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`"),
-            };
+                let new = {
+                    let other_f = match (other_fs.len(), other_fs.get(0)) {
+                        (1, Some(o_f)) => o_f,
+                                _ => {
+                                    cx.span_bug(span,
+                                        "not exactly 2 arguments in `derive(PartialOrd)`")
+                                }
+                    };
 
-            let args = vec![
-                    cx.expr_addr_of(span, self_f),
-                    cx.expr_addr_of(span, other_f.clone()),
-                ];
+                    let args = vec![
+                            cx.expr_addr_of(span, self_f),
+                            cx.expr_addr_of(span, other_f.clone()),
+                        ];
 
-            cx.expr_call_global(span, partial_cmp_path.clone(), args)
-        };
+                    cx.expr_call_global(span, partial_cmp_path.clone(), args)
+                };
 
-        let eq_arm = cx.arm(span,
-                            vec![cx.pat_some(span, cx.pat_path(span, ordering.clone()))],
-                            old);
-        let neq_arm = cx.arm(span,
-                             vec![cx.pat_ident(span, test_id)],
-                             cx.expr_ident(span, test_id));
+                let eq_arm = cx.arm(span,
+                                    vec![cx.pat_some(span, cx.pat_path(span, ordering.clone()))],
+                                    old);
+                let neq_arm = cx.arm(span,
+                                    vec![cx.pat_ident(span, test_id)],
+                                    cx.expr_ident(span, test_id));
 
-        cx.expr_match(span, new, vec![eq_arm, neq_arm])
-    },
+                cx.expr_match(span, new, vec![eq_arm, neq_arm])
+            },
             equals_expr.clone(),
             Box::new(|cx, span, (self_args, tag_tuple), _non_self_args| {
         if self_args.len() != 2 {
@@ -189,78 +192,99 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<
 }
 
 /// Strict inequality.
-fn cs_op(less: bool, equal: bool, cx: &mut ExtCtxt, span: Span, substr: &Substructure) -> P<Expr> {
-    let strict_op = if less { BinOpKind::Lt } else { BinOpKind::Gt };
-    cs_fold1(false, // need foldr,
+fn cs_op(less: bool,
+         inclusive: bool,
+         cx: &mut ExtCtxt,
+         span: Span,
+         substr: &Substructure) -> P<Expr> {
+    let ordering_path = |cx: &mut ExtCtxt, name: &str| {
+        cx.expr_path(cx.path_global(span, cx.std_path(&["cmp", "Ordering", name])))
+    };
+
+    let par_cmp = |cx: &mut ExtCtxt, span, self_f: P<Expr>, other_fs: &[P<Expr>], default| {
+        let other_f = match (other_fs.len(), other_fs.get(0)) {
+            (1, Some(o_f)) => o_f,
+            _ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`"),
+        };
+
+        // `PartialOrd::partial_cmp(self.fi, other.fi)`
+        let cmp_path = cx.expr_path(cx.path_global(span, cx.std_path(&["cmp",
+                                                                       "PartialOrd",
+                                                                       "partial_cmp"])));
+        let cmp = cx.expr_call(span,
+                               cmp_path,
+                               vec![cx.expr_addr_of(span, self_f),
+                                    cx.expr_addr_of(span, other_f.clone())]);
+
+        let default = ordering_path(cx, default);
+        // `Option::unwrap_or(_, Ordering::Equal)`
+        let unwrap_path = cx.expr_path(cx.path_global(span, cx.std_path(&["option",
+                                                                          "Option",
+                                                                          "unwrap_or"])));
+        cx.expr_call(span, unwrap_path, vec![cmp, default])
+    };
+
+    let fold = cs_fold1(false, // need foldr
         |cx, span, subexpr, self_f, other_fs| {
-            // build up a series of chain ||'s and &&'s from the inside
+            // build up a series of `partial_cmp`s from the inside
             // out (hence foldr) to get lexical ordering, i.e. for op ==
             // `ast::lt`
             //
             // ```
-            // self.f1 < other.f1 || (!(other.f1 < self.f1) &&
-            // self.f2 < other.f2
+            // Ordering::then_with(
+            //    Option::unwrap_or(
+            //        PartialOrd::partial_cmp(self.f1, other.f1), Ordering::Equal)
+            //    ),
+            //    Option::unwrap_or(
+            //        PartialOrd::partial_cmp(self.f2, other.f2), Ordering::Greater)
+            //    )
             // )
+            // == Ordering::Less
             // ```
             //
             // and for op ==
             // `ast::le`
             //
             // ```
-            // self.f1 < other.f1 || (self.f1 == other.f1 &&
-            // self.f2 <= other.f2
+            // Ordering::then_with(
+            //    Option::unwrap_or(
+            //        PartialOrd::partial_cmp(self.f1, other.f1), Ordering::Equal)
+            //    ),
+            //    Option::unwrap_or(
+            //        PartialOrd::partial_cmp(self.f2, other.f2), Ordering::Greater)
+            //    )
             // )
+            // != Ordering::Greater
             // ```
             //
             // The optimiser should remove the redundancy. We explicitly
             // get use the binops to avoid auto-deref dereferencing too many
             // layers of pointers, if the type includes pointers.
-            //
-            let other_f = match (other_fs.len(), other_fs.get(0)) {
-                (1, Some(o_f)) => o_f,
-                _ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`"),
-            };
 
-            let strict_ineq = cx.expr_binary(span, strict_op, self_f.clone(), other_f.clone());
+            // `Option::unwrap_or(PartialOrd::partial_cmp(self.fi, other.fi), Ordering::Equal)`
+            let par_cmp = par_cmp(cx, span, self_f, other_fs, "Equal");
 
-            let deleg_cmp = if !equal {
-                cx.expr_unary(span,
-                            ast::UnOp::Not,
-                            cx.expr_binary(span, strict_op, other_f.clone(), self_f))
-            } else {
-                cx.expr_binary(span, BinOpKind::Eq, self_f, other_f.clone())
-            };
-
-            let and = cx.expr_binary(span, BinOpKind::And, deleg_cmp, subexpr);
-            cx.expr_binary(span, BinOpKind::Or, strict_ineq, and)
+            // `Ordering::then_with(Option::unwrap_or(..), ..)`
+            let then_with_path = cx.expr_path(cx.path_global(span,
+                                                             cx.std_path(&["cmp",
+                                                                           "Ordering",
+                                                                           "then_with"])));
+            cx.expr_call(span, then_with_path, vec![par_cmp, cx.lambda0(span, subexpr)])
         },
         |cx, args| {
             match args {
                 Some((span, self_f, other_fs)) => {
-                    // Special-case the base case to generate cleaner code with
-                    // fewer operations (e.g. `<=` instead of `<` and `==`).
-                    let other_f = match (other_fs.len(), other_fs.get(0)) {
-                        (1, Some(o_f)) => o_f,
-                        _ => cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`"),
-                    };
-
-                    let op = match (less, equal) {
-                        (false, false) => BinOpKind::Gt,
-                        (false, true) => BinOpKind::Ge,
-                        (true, false) => BinOpKind::Lt,
-                        (true, true) => BinOpKind::Le,
-                    };
-
-                    cx.expr_binary(span, op, self_f, other_f.clone())
-                }
-                None => cx.expr_bool(span, equal)
+                    let opposite = if less { "Greater" } else { "Less" };
+                    par_cmp(cx, span, self_f, other_fs, opposite)
+                },
+                None => cx.expr_bool(span, inclusive)
             }
         },
         Box::new(|cx, span, (self_args, tag_tuple), _non_self_args| {
             if self_args.len() != 2 {
                 cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`")
             } else {
-                let op = match (less, equal) {
+                let op = match (less, inclusive) {
                     (false, false) => GtOp,
                     (false, true) => GeOp,
                     (true, false) => LtOp,
@@ -271,5 +295,16 @@ fn cs_op(less: bool, equal: bool, cx: &mut ExtCtxt, span: Span, substr: &Substru
         }),
         cx,
         span,
-        substr)
+        substr);
+
+    match *substr.fields {
+        EnumMatching(.., ref all_fields) |
+        Struct(.., ref all_fields) if !all_fields.is_empty() => {
+            let ordering = ordering_path(cx, if less ^ inclusive { "Less" } else { "Greater" });
+            let comp_op = if inclusive { BinOpKind::Ne } else { BinOpKind::Eq };
+
+            cx.expr_binary(span, comp_op, fold, ordering)
+        }
+        _ => fold
+    }
 }
diff --git a/src/test/compile-fail/derives-span-PartialOrd-enum-struct-variant.rs b/src/test/compile-fail/derives-span-PartialOrd-enum-struct-variant.rs
@@ -17,10 +17,6 @@ struct Error;
 enum Enum {
    A {
      x: Error //~ ERROR
-//~^ ERROR
-//~^^ ERROR
-//~^^^ ERROR
-//~^^^^ ERROR
    }
 }
 

diff --git a/src/test/compile-fail/derives-span-PartialOrd-enum.rs b/src/test/compile-fail/derives-span-PartialOrd-enum.rs
@@ -17,10 +17,6 @@ struct Error;
 enum Enum {
    A(
      Error //~ ERROR
-//~^ ERROR
-//~^^ ERROR
-//~^^^ ERROR
-//~^^^^ ERROR
      )
 }
 

diff --git a/src/test/compile-fail/derives-span-PartialOrd-struct.rs b/src/test/compile-fail/derives-span-PartialOrd-struct.rs
@@ -16,10 +16,6 @@ struct Error;
 #[derive(PartialOrd,PartialEq)]
 struct Struct {
     x: Error //~ ERROR
-//~^ ERROR
-//~^^ ERROR
-//~^^^ ERROR
-//~^^^^ ERROR
 }
 
 fn main() {}
diff --git a/src/test/compile-fail/derives-span-PartialOrd-tuple-struct.rs b/src/test/compile-fail/derives-span-PartialOrd-tuple-struct.rs
@@ -16,10 +16,6 @@ struct Error;
 #[derive(PartialOrd,PartialEq)]
 struct Struct(
     Error //~ ERROR
-//~^ ERROR
-//~^^ ERROR
-//~^^^ ERROR
-//~^^^^ ERROR
 );
 
 fn main() {}
diff --git a/src/test/compile-fail/range_traits-1.rs b/src/test/compile-fail/range_traits-1.rs
@@ -15,35 +15,21 @@ struct AllTheRanges {
     a: Range<usize>,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
     b: RangeTo<usize>,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
     c: RangeFrom<usize>,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
     d: RangeFull,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
     e: RangeInclusive<usize>,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
     f: RangeToInclusive<usize>,
     //~^ ERROR PartialOrd
     //~^^ ERROR Ord
-    //~^^^ ERROR binary operation `<` cannot be applied to type
-    //~^^^^ ERROR binary operation `>` cannot be applied to type
-    //~^^^^^ ERROR binary operation `<=` cannot be applied to type
-    //~^^^^^^ ERROR binary operation `>=` cannot be applied to type
 }
 
 fn main() {}