Skip to content
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

Jump to bottom

Don't use match-destructuring for derived ops on structs. #98446

Merged
merged 3 commits into from
Jul 4, 2022
Merged

Don't use match-destructuring for derived ops on structs. #98446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18f8495
Add an interesting case to the `deriving-all-codegen.rs` test.
nnethercote Jun 24, 2022
528343f
Comment fixes.
nnethercote Jun 23, 2022
ecc6e95
Don't use match-destructuring for derived ops on structs.
nnethercote Jun 24, 2022
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
127 changes: 81 additions & 46 deletions compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
//!
//! # "`cs`" functions
//!
//! The `cs_...` functions ("combine substructure) are designed to
//! The `cs_...` functions ("combine substructure") are designed to
//! make life easier by providing some pre-made recipes for common
//! threads; mostly calling the function being derived on all the
//! arguments and then combining them back together in some way (or
Expand Down Expand Up @@ -429,6 +429,7 @@ impl<'a> TraitDef<'a> {
generics,
from_scratch,
use_temporaries,
is_packed,
),
ast::ItemKind::Enum(ref enum_def, ref generics) => {
// We ignore `use_temporaries` here, because
Expand All @@ -448,6 +449,7 @@ impl<'a> TraitDef<'a> {
generics,
from_scratch,
use_temporaries,
is_packed,
)
} else {
cx.span_err(mitem.span, "this trait cannot be derived for unions");
Expand Down Expand Up @@ -729,6 +731,7 @@ impl<'a> TraitDef<'a> {
generics: &Generics,
from_scratch: bool,
use_temporaries: bool,
is_packed: bool,
) -> P<ast::Item> {
let field_tys: Vec<P<ast::Ty>> =
struct_def.fields().iter().map(|field| field.ty.clone()).collect();
Expand Down Expand Up @@ -757,6 +760,7 @@ impl<'a> TraitDef<'a> {
&self_args,
&nonself_args,
use_temporaries,
is_packed,
)
};

Expand Down Expand Up @@ -945,6 +949,7 @@ impl<'a> MethodDef<'a> {
})
}

/// The normal case uses field access.
/// ```
/// #[derive(PartialEq)]
/// # struct Dummy;
Expand All @@ -953,33 +958,21 @@ impl<'a> MethodDef<'a> {
/// // equivalent to:
/// impl PartialEq for A {
/// fn eq(&self, other: &A) -> bool {
/// match *self {
/// A {x: ref __self_0_0, y: ref __self_0_1} => {
/// match *other {
/// A {x: ref __self_1_0, y: ref __self_1_1} => {
/// __self_0_0.eq(__self_1_0) && __self_0_1.eq(__self_1_1)
/// }
/// }
/// }
/// }
/// self.x == other.x && self.y == other.y
/// }
/// }
/// ```
/// or if A is repr(packed) - note fields are matched by-value
/// instead of by-reference.
/// But if the struct is `repr(packed)`, we can't use something like
/// `&self.x` on a packed type (as required for e.g. `Debug` and `Hash`)
/// because that might cause an unaligned ref. So we use let-destructuring
/// instead.
/// ```
/// # struct A { x: i32, y: i32 }
/// impl PartialEq for A {
/// fn eq(&self, other: &A) -> bool {
/// match *self {
/// A {x: __self_0_0, y: __self_0_1} => {
/// match other {
/// A {x: __self_1_0, y: __self_1_1} => {
/// __self_0_0.eq(&__self_1_0) && __self_0_1.eq(&__self_1_1)
/// }
/// }
/// }
/// }
/// let Self { x: ref __self_0_0, y: ref __self_0_1 } = *self;
/// let Self { x: ref __self_1_0, y: ref __self_1_1 } = *other;
/// *__self_0_0 == *__self_1_0 && *__self_0_1 == *__self_1_1
/// }
/// }
/// ```
Expand All @@ -992,24 +985,33 @@ impl<'a> MethodDef<'a> {
self_args: &[P<Expr>],
nonself_args: &[P<Expr>],
use_temporaries: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure exactly when use_temporaries is needed now that is_packed seems to be making many of the relevant decisions. I tried to follow it down a bit more, but got a bit lost.

Are there any cases that strictly need temporaries when is_packed is false?

(I don't think this needs to block this PR, but might be worth exploring in future cleanup, especially if it could make callsites clearer to have an enum for the cases, rather than the bunch-of-bools that some of them seem to have today.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_temporaries is still used in create_struct_pattern, which has two call sites, so the is_packed/use_temporaries distinction is still required. I haven't changed this in any of my follow-ups, but I'll think about if it can be improved.

is_packed: bool,
) -> P<Expr> {
let mut raw_fields = Vec::new(); // Vec<[fields of self], [fields of next Self arg], [etc]>
let span = trait_.span;
let mut patterns = Vec::new();
for i in 0..self_args.len() {
// We could use `type_ident` instead of `Self`, but in the case of a type parameter
// shadowing the struct name, that causes a second, unnecessary E0578 error. #97343
let struct_path = cx.path(span, vec![Ident::new(kw::SelfUpper, type_ident.span)]);
let (pat, ident_expr) = trait_.create_struct_pattern(
cx,
struct_path,
struct_def,
&format!("__self_{}", i),
ast::Mutability::Not,
use_temporaries,
);
patterns.push(pat);
raw_fields.push(ident_expr);

for (i, self_arg) in self_args.iter().enumerate() {
let ident_exprs = if !is_packed {
trait_.create_struct_field_accesses(cx, self_arg, struct_def)
} else {
// Get the pattern for the let-destructuring.
//
// We could use `type_ident` instead of `Self`, but in the case of a type parameter
// shadowing the struct name, that causes a second, unnecessary E0578 error. #97343
let struct_path = cx.path(span, vec![Ident::new(kw::SelfUpper, type_ident.span)]);
let (pat, ident_exprs) = trait_.create_struct_pattern(
cx,
struct_path,
struct_def,
&format!("__self_{}", i),
ast::Mutability::Not,
use_temporaries,
);
patterns.push(pat);
ident_exprs
};
raw_fields.push(ident_exprs);
}

// transpose raw_fields
Expand All @@ -1036,7 +1038,6 @@ impl<'a> MethodDef<'a> {
cx.span_bug(span, "no `self` parameter for method in generic `derive`")
};

// body of the inner most destructuring match
let mut body = self.call_substructure_method(
cx,
trait_,
Expand All @@ -1045,14 +1046,18 @@ impl<'a> MethodDef<'a> {
&Struct(struct_def, fields),
);

// make a series of nested matches, to destructure the
// structs. This is actually right-to-left, but it shouldn't
// matter.
for (arg_expr, pat) in iter::zip(self_args, patterns) {
body = cx.expr_match(span, arg_expr.clone(), vec![cx.arm(span, pat.clone(), body)])
}
if !is_packed {
body.span = span;
body
} else {
// Do the let-destructuring.
let mut stmts: Vec<_> = iter::zip(self_args, patterns)
.map(|(arg_expr, pat)| cx.stmt_let_pat(span, pat, arg_expr.clone()))
.collect();
stmts.push(cx.stmt_expr(body));

body
cx.expr_block(cx.block(span, stmts))
}
}

fn expand_static_struct_method_body(
Expand Down Expand Up @@ -1522,8 +1527,6 @@ impl<'a> TraitDef<'a> {
paths.push(ident.with_span_pos(sp));
let val = cx.expr_path(cx.path_ident(sp, ident));
let val = if use_temporaries { val } else { cx.expr_deref(sp, val) };
let val = cx.expr(sp, ast::ExprKind::Paren(val));

ident_exprs.push((sp, struct_field.ident, val, &struct_field.attrs[..]));
}

Expand Down Expand Up @@ -1555,6 +1558,39 @@ impl<'a> TraitDef<'a> {
(pattern, ident_exprs)
}

fn create_struct_field_accesses(
&self,
cx: &mut ExtCtxt<'_>,
mut self_arg: &P<Expr>,
struct_def: &'a VariantData,
) -> Vec<(Span, Option<Ident>, P<Expr>, &'a [ast::Attribute])> {
let mut ident_exprs = Vec::new();
for (i, struct_field) in struct_def.fields().iter().enumerate() {
let sp = struct_field.span.with_ctxt(self.span.ctxt());

// We don't the need the deref, if there is one.
if let ast::ExprKind::Unary(ast::UnOp::Deref, inner) = &self_arg.kind {
self_arg = inner;
}

// Note: we must use `struct_field.span` rather than `span` in the
// `unwrap_or_else` case otherwise the hygiene is wrong and we get
// "field `0` of struct `Point` is private" errors on tuple
// structs.
let val = cx.expr(
sp,
ast::ExprKind::Field(
self_arg.clone(),
struct_field.ident.unwrap_or_else(|| {
Ident::from_str_and_span(&i.to_string(), struct_field.span)
}),
),
);
ident_exprs.push((sp, struct_field.ident, val, &struct_field.attrs[..]));
}
ident_exprs
}

fn create_enum_variant_pattern(
&self,
cx: &mut ExtCtxt<'_>,
Expand Down Expand Up @@ -1643,7 +1679,6 @@ where
/// fields.
/// When the `substructure` is an `EnumNonMatchingCollapsed`, the result of `enum_nonmatch_f`
/// is returned. Statics may not be folded over.
/// See `cs_op` in `partial_ord.rs` for a model example.
pub fn cs_fold1<F, B>(
use_foldl: bool,
f: F,
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_expand/src/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,19 @@ impl<'a> ExtCtxt<'a> {
ast::Stmt { id: ast::DUMMY_NODE_ID, span: expr.span, kind: ast::StmtKind::Expr(expr) }
}

pub fn stmt_let_pat(&self, sp: Span, pat: P<ast::Pat>, ex: P<ast::Expr>) -> ast::Stmt {
let local = P(ast::Local {
pat,
ty: None,
id: ast::DUMMY_NODE_ID,
kind: LocalKind::Init(ex),
span: sp,
attrs: AttrVec::new(),
tokens: None,
});
self.stmt_local(local, sp)
}

pub fn stmt_let(&self, sp: Span, mutbl: bool, ident: Ident, ex: P<ast::Expr>) -> ast::Stmt {
self.stmt_let_ty(sp, mutbl, ident, None, ex)
}
Expand Down
Loading