Skip to content

Commit

Permalink
Auto merge of #81384 - tmiasko:partial-ord, r=petrochenkov
Browse files Browse the repository at this point in the history 
Fix derived PartialOrd operators

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.
  • Loading branch information
@bors
bors committed Feb 9, 2021
2 parents f4008fe + 62366ee commit c648bd5
Show file tree
Hide file tree
Showing 26 changed files with 83 additions and 1,718 deletions.
197 changes: 7 additions & 190 deletions compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
pub use OrderingOp::*;

use crate::deriving::generic::ty::*;
use crate::deriving::generic::*;
use crate::deriving::{path_local, path_std, pathvec_std};
use crate::deriving::{path_std, pathvec_std};

use rustc_ast::ptr::P;
use rustc_ast::{self as ast, BinOpKind, Expr, MetaItem};
use rustc_ast::{Expr, MetaItem};
use rustc_expand::base::{Annotatable, ExtCtxt};
use rustc_span::symbol::{sym, Ident, Symbol};
use rustc_span::symbol::{sym, Ident};
use rustc_span::Span;

pub fn expand_deriving_partial_ord(
Expand All @@ -17,26 +15,6 @@ pub fn expand_deriving_partial_ord(
item: &Annotatable,
push: &mut dyn FnMut(Annotatable),
) {
macro_rules! md {
($name:expr, $op:expr, $equal:expr) => {{
let inline = cx.meta_word(span, sym::inline);
let attrs = vec![cx.attribute(inline)];
MethodDef {
name: $name,
generics: Bounds::empty(),
explicit_self: borrowed_explicit_self(),
args: vec![(borrowed_self(), sym::other)],
ret_ty: Literal(path_local!(bool)),
attributes: attrs,
is_unsafe: false,
unify_fieldless_variants: true,
combine_substructure: combine_substructure(Box::new(|cx, span, substr| {
cs_op($op, $equal, cx, span, substr)
})),
}
}};
}

let ordering_ty = Literal(path_std!(cmp::Ordering));
let ret_ty = Literal(Path::new_(
pathvec_std!(option::Option),
Expand All @@ -62,21 +40,6 @@ pub fn expand_deriving_partial_ord(
})),
};

// avoid defining extra methods if we can
// c-like enums, enums without any fields and structs without fields
// can safely define only `partial_cmp`.
let methods = if is_type_without_fields(item) {
vec![partial_cmp_def]
} else {
vec![
partial_cmp_def,
md!(sym::lt, true, false),
md!(sym::le, true, true),
md!(sym::gt, false, false),
md!(sym::ge, false, true),
]
};

let trait_def = TraitDef {
span,
attributes: vec![],
Expand All @@ -85,39 +48,12 @@ pub fn expand_deriving_partial_ord(
generics: Bounds::empty(),
is_unsafe: false,
supports_unions: false,
methods,
methods: vec![partial_cmp_def],
associated_types: Vec::new(),
};
trait_def.expand(cx, mitem, item, push)
}

#[derive(Copy, Clone)]
pub enum OrderingOp {
PartialCmpOp,
LtOp,
LeOp,
GtOp,
GeOp,
}

pub fn some_ordering_collapsed(
cx: &mut ExtCtxt<'_>,
span: Span,
op: OrderingOp,
self_arg_tags: &[Ident],
) -> P<ast::Expr> {
let lft = cx.expr_ident(span, self_arg_tags[0]);
let rgt = cx.expr_addr_of(span, cx.expr_ident(span, self_arg_tags[1]));
let op_sym = match op {
PartialCmpOp => sym::partial_cmp,
LtOp => sym::lt,
LeOp => sym::le,
GtOp => sym::gt,
GeOp => sym::ge,
};
cx.expr_method_call(span, lft, Ident::new(op_sym, span), vec![rgt])
}

pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
let test_id = Ident::new(sym::cmp, span);
let ordering = cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::Equal]));
Expand Down Expand Up @@ -171,132 +107,13 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
if self_args.len() != 2 {
cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`")
} else {
some_ordering_collapsed(cx, span, PartialCmpOp, tag_tuple)
let lft = cx.expr_ident(span, tag_tuple[0]);
let rgt = cx.expr_addr_of(span, cx.expr_ident(span, tag_tuple[1]));
cx.expr_method_call(span, lft, Ident::new(sym::partial_cmp, span), vec![rgt])
}
}),
cx,
span,
substr,
)
}

/// Strict inequality.
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(&[sym::cmp, sym::Ordering, Symbol::intern(name)])),
)
};

let par_cmp = |cx: &mut ExtCtxt<'_>, span, self_f: P<Expr>, other_fs: &[P<Expr>], default| {
let other_f = match other_fs {
[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(&[sym::cmp, sym::PartialOrd, sym::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(&[sym::option, sym::Option, sym::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 `partial_cmp`s from the inside
// out (hence foldr) to get lexical ordering, i.e., for op ==
// `ast::lt`
//
// ```
// 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`
//
// ```
// 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.

// `Option::unwrap_or(PartialOrd::partial_cmp(self.fi, other.fi), Ordering::Equal)`
let par_cmp = par_cmp(cx, span, self_f, other_fs, "Equal");

// `Ordering::then_with(Option::unwrap_or(..), ..)`
let then_with_path = cx.expr_path(
cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::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)) => {
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, inclusive) {
(false, false) => GtOp,
(false, true) => GeOp,
(true, false) => LtOp,
(true, true) => LeOp,
};
some_ordering_collapsed(cx, span, op, tag_tuple)
}
}),
cx,
span,
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,
}
}
82 changes: 0 additions & 82 deletions ...rtial_eq/partial_eq.{impl#2}-ge-{closure#0}-{closure#0}.-------.InstrumentCoverage.0.html
View file

This file was deleted.

Loading

0 comments on commit c648bd5

Please sign in to comment.