Skip to content

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

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

Merged
merged 4 commits into from
May 15, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6eb4f0f
Ensure derive(PartialOrd) is no longer accidentally exponential
varkor Apr 16, 2018
6330bf2
Fix handling of None
varkor Apr 17, 2018
98e3e82
Use UFCS
varkor Apr 17, 2018
6805e5a
Update comments to UFCS style
varkor Apr 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/etc/generate-deriving-span-tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
167 changes: 101 additions & 66 deletions src/libsyntax_ext/deriving/cmp/partial_ord.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand All @@ -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
}
}
4 changes: 0 additions & 4 deletions src/test/compile-fail/derives-span-PartialOrd-enum-struct-variant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ struct Error;
enum Enum {
A {
x: Error //~ ERROR
//~^ ERROR
//~^^ ERROR
//~^^^ ERROR
//~^^^^ ERROR
}
}

Expand Down
4 changes: 0 additions & 4 deletions src/test/compile-fail/derives-span-PartialOrd-enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ struct Error;
enum Enum {
A(
Error //~ ERROR
//~^ ERROR
//~^^ ERROR
//~^^^ ERROR
//~^^^^ ERROR
)
}

Expand Down
4 changes: 0 additions & 4 deletions src/test/compile-fail/derives-span-PartialOrd-struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ struct Error;
#[derive(PartialOrd,PartialEq)]
struct Struct {
x: Error //~ ERROR
//~^ ERROR
//~^^ ERROR
//~^^^ ERROR
//~^^^^ ERROR
}

fn main() {}
4 changes: 0 additions & 4 deletions src/test/compile-fail/derives-span-PartialOrd-tuple-struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ struct Error;
#[derive(PartialOrd,PartialEq)]
struct Struct(
Error //~ ERROR
//~^ ERROR
//~^^ ERROR
//~^^^ ERROR
//~^^^^ ERROR
);

fn main() {}
14 changes: 0 additions & 14 deletions src/test/compile-fail/range_traits-1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}