Auto merge of #104429 - nnethercote:more-deriving-on-packed-structs, r=RalfJung

More deriving on packed structs

See [here](#104429 (comment)) for the t-lang nomination summary, and [here](#104429 (comment)) for the approval.

r? `@RalfJung`

Author: bors

compiler/rustc_builtin_macros/src/deriving/bounds.rs

@@ -17,6 +17,7 @@ pub fn expand_deriving_copy(
         span,
         path: path_std!(marker::Copy),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: false,
         additional_bounds: Vec::new(),
         supports_unions: true,
         methods: Vec::new(),

compiler/rustc_builtin_macros/src/deriving/clone.rs

@@ -73,6 +73,7 @@ pub fn expand_deriving_clone(
         span,
         path: path_std!(clone::Clone),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: bounds,
         supports_unions: true,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs

@@ -27,6 +27,7 @@ pub fn expand_deriving_eq(
         span,
         path: path_std!(cmp::Eq),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: true,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/cmp/ord.rs

@@ -20,6 +20,7 @@ pub fn expand_deriving_ord(
         span,
         path: path_std!(cmp::Ord),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

@@ -84,6 +84,7 @@ pub fn expand_deriving_partial_eq(
         span,
         path: path_std!(cmp::PartialEq),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods,

compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs

@@ -59,6 +59,7 @@ pub fn expand_deriving_partial_ord(
         span,
         path: path_std!(cmp::PartialOrd),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: vec![],
         supports_unions: false,
         methods: vec![partial_cmp_def],

compiler/rustc_builtin_macros/src/deriving/debug.rs

@@ -23,6 +23,7 @@ pub fn expand_deriving_debug(
         span,
         path: path_std!(fmt::Debug),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/decodable.rs

@@ -25,6 +25,7 @@ pub fn expand_deriving_rustc_decodable(
         span,
         path: Path::new_(vec![krate, sym::Decodable], vec![], PathKind::Global),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/default.rs

@@ -25,6 +25,7 @@ pub fn expand_deriving_default(
         span,
         path: Path::new(vec![kw::Default, sym::Default]),
         skip_path_as_bound: has_a_default_variant(item),
+        needs_copy_as_bound_if_packed: false,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/encodable.rs

@@ -109,6 +109,7 @@ pub fn expand_deriving_rustc_encodable(
         span,
         path: Path::new_(vec![krate, sym::Encodable], vec![], PathKind::Global),
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

@@ -165,11 +165,12 @@ pub use SubstructureFields::*;
 use crate::deriving;
 use rustc_ast::ptr::P;
 use rustc_ast::{
-    self as ast, BindingAnnotation, ByRef, EnumDef, Expr, Generics, Mutability, PatKind,
+    self as ast, BindingAnnotation, ByRef, EnumDef, Expr, GenericArg, GenericParamKind, Generics,
+    Mutability, PatKind, TyKind, VariantData,
 };
-use rustc_ast::{GenericArg, GenericParamKind, VariantData};
 use rustc_attr as attr;
 use rustc_expand::base::{Annotatable, ExtCtxt};
+use rustc_session::lint::builtin::BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{Span, DUMMY_SP};
 use std::cell::RefCell;
@@ -191,6 +192,9 @@ pub struct TraitDef<'a> {
     /// Whether to skip adding the current trait as a bound to the type parameters of the type.
     pub skip_path_as_bound: bool,
 
+    /// Whether `Copy` is needed as an additional bound on type parameters in a packed struct.
+    pub needs_copy_as_bound_if_packed: bool,
+
     /// Additional bounds required of any type parameters of the type,
     /// other than the current trait
     pub additional_bounds: Vec<Ty>,
@@ -455,18 +459,6 @@ impl<'a> TraitDef<'a> {
                     }
                     false
                 });
-                let has_no_type_params = match &item.kind {
-                    ast::ItemKind::Struct(_, generics)
-                    | ast::ItemKind::Enum(_, generics)
-                    | ast::ItemKind::Union(_, generics) => !generics
-                        .params
-                        .iter()
-                        .any(|param| matches!(param.kind, ast::GenericParamKind::Type { .. })),
-                    _ => unreachable!(),
-                };
-                let container_id = cx.current_expansion.id.expn_data().parent.expect_local();
-                let copy_fields =
-                    is_packed && has_no_type_params && cx.resolver.has_derive_copy(container_id);
 
                 let newitem = match &item.kind {
                     ast::ItemKind::Struct(struct_def, generics) => self.expand_struct_def(
@@ -475,7 +467,7 @@ impl<'a> TraitDef<'a> {
                         item.ident,
                         generics,
                         from_scratch,
-                        copy_fields,
+                        is_packed,
                     ),
                     ast::ItemKind::Enum(enum_def, generics) => {
                         // We ignore `is_packed` here, because `repr(packed)`
@@ -493,7 +485,7 @@ impl<'a> TraitDef<'a> {
                                 item.ident,
                                 generics,
                                 from_scratch,
-                                copy_fields,
+                                is_packed,
                             )
                         } else {
                             cx.span_err(mitem.span, "this trait cannot be derived for unions");
@@ -565,6 +557,7 @@ impl<'a> TraitDef<'a> {
         generics: &Generics,
         field_tys: Vec<P<ast::Ty>>,
         methods: Vec<P<ast::AssocItem>>,
+        is_packed: bool,
     ) -> P<ast::Item> {
         let trait_path = self.path.to_path(cx, self.span, type_ident, generics);
 
@@ -607,20 +600,32 @@ impl<'a> TraitDef<'a> {
             .map(|param| match &param.kind {
                 GenericParamKind::Lifetime { .. } => param.clone(),
                 GenericParamKind::Type { .. } => {
-                    // I don't think this can be moved out of the loop, since
-                    // a GenericBound requires an ast id
-                    let bounds: Vec<_> =
-                    // extra restrictions on the generics parameters to the
-                    // type being derived upon
-                    self.additional_bounds.iter().map(|p| {
-                        cx.trait_bound(p.to_path(cx, self.span, type_ident, generics))
-                    }).chain(
-                        // require the current trait
-                        self.skip_path_as_bound.not().then(|| cx.trait_bound(trait_path.clone()))
-                    ).chain(
-                        // also add in any bounds from the declaration
-                        param.bounds.iter().cloned()
-                    ).collect();
+                    // Extra restrictions on the generics parameters to the
+                    // type being derived upon.
+                    let bounds: Vec<_> = self
+                        .additional_bounds
+                        .iter()
+                        .map(|p| cx.trait_bound(p.to_path(cx, self.span, type_ident, generics)))
+                        .chain(
+                            // Add a bound for the current trait.
+                            self.skip_path_as_bound
+                                .not()
+                                .then(|| cx.trait_bound(trait_path.clone())),
+                        )
+                        .chain({
+                            // Add a `Copy` bound if required.
+                            if is_packed && self.needs_copy_as_bound_if_packed {
+                                let p = deriving::path_std!(marker::Copy);
+                                Some(cx.trait_bound(p.to_path(cx, self.span, type_ident, generics)))
+                            } else {
+                                None
+                            }
+                        })
+                        .chain(
+                            // Also add in any bounds from the declaration.
+                            param.bounds.iter().cloned(),
+                        )
+                        .collect();
 
                     cx.typaram(param.ident.span.with_ctxt(ctxt), param.ident, bounds, None)
                 }
@@ -692,9 +697,17 @@ impl<'a> TraitDef<'a> {
                             .map(|p| cx.trait_bound(p.to_path(cx, self.span, type_ident, generics)))
                             .collect();
 
-                        // require the current trait
+                        // Require the current trait.
                         bounds.push(cx.trait_bound(trait_path.clone()));
 
+                        // Add a `Copy` bound if required.
+                        if is_packed && self.needs_copy_as_bound_if_packed {
+                            let p = deriving::path_std!(marker::Copy);
+                            bounds.push(
+                                cx.trait_bound(p.to_path(cx, self.span, type_ident, generics)),
+                            );
+                        }
+
                         let predicate = ast::WhereBoundPredicate {
                             span: self.span,
                             bound_generic_params: field_ty_param.bound_generic_params,
@@ -762,7 +775,7 @@ impl<'a> TraitDef<'a> {
         type_ident: Ident,
         generics: &Generics,
         from_scratch: bool,
-        copy_fields: bool,
+        is_packed: bool,
     ) -> P<ast::Item> {
         let field_tys: Vec<P<ast::Ty>> =
             struct_def.fields().iter().map(|field| field.ty.clone()).collect();
@@ -790,7 +803,7 @@ impl<'a> TraitDef<'a> {
                         type_ident,
                         &selflike_args,
                         &nonselflike_args,
-                        copy_fields,
+                        is_packed,
                     )
                 };
 
@@ -806,7 +819,7 @@ impl<'a> TraitDef<'a> {
             })
             .collect();
 
-        self.create_derived_impl(cx, type_ident, generics, field_tys, methods)
+        self.create_derived_impl(cx, type_ident, generics, field_tys, methods, is_packed)
     }
 
     fn expand_enum_def(
@@ -861,7 +874,8 @@ impl<'a> TraitDef<'a> {
             })
             .collect();
 
-        self.create_derived_impl(cx, type_ident, generics, field_tys, methods)
+        let is_packed = false; // enums are never packed
+        self.create_derived_impl(cx, type_ident, generics, field_tys, methods, is_packed)
     }
 }
 
@@ -1011,8 +1025,8 @@ impl<'a> MethodDef<'a> {
     /// ```
     /// But if the struct is `repr(packed)`, we can't use something like
     /// `&self.x` because that might cause an unaligned ref. So for any trait
-    /// method that takes a reference, if the struct impls `Copy` then we use a
-    /// local block to force a copy:
+    /// method that takes a reference, we use a local block to force a copy.
+    /// This requires that the field impl `Copy`.
     /// ```
     /// # struct A { x: u8, y: u8 }
     /// impl PartialEq for A {
@@ -1027,10 +1041,6 @@ impl<'a> MethodDef<'a> {
     ///         ::core::hash::Hash::hash(&{ self.y }, state)
     ///     }
     /// }
-    /// ```
-    /// If the struct doesn't impl `Copy`, we use the normal `&self.x`. This
-    /// only works if the fields match the alignment required by the
-    /// `packed(N)` attribute. (We'll get errors later on if not.)
     fn expand_struct_method_body<'b>(
         &self,
         cx: &mut ExtCtxt<'_>,
@@ -1039,12 +1049,12 @@ impl<'a> MethodDef<'a> {
         type_ident: Ident,
         selflike_args: &[P<Expr>],
         nonselflike_args: &[P<Expr>],
-        copy_fields: bool,
+        is_packed: bool,
     ) -> BlockOrExpr {
         assert!(selflike_args.len() == 1 || selflike_args.len() == 2);
 
         let selflike_fields =
-            trait_.create_struct_field_access_fields(cx, selflike_args, struct_def, copy_fields);
+            trait_.create_struct_field_access_fields(cx, selflike_args, struct_def, is_packed);
         self.call_substructure_method(
             cx,
             trait_,
@@ -1514,7 +1524,7 @@ impl<'a> TraitDef<'a> {
         cx: &mut ExtCtxt<'_>,
         selflike_args: &[P<Expr>],
         struct_def: &'a VariantData,
-        copy_fields: bool,
+        is_packed: bool,
     ) -> Vec<FieldInfo> {
         self.create_fields(struct_def, |i, struct_field, sp| {
             selflike_args
@@ -1533,10 +1543,39 @@ impl<'a> TraitDef<'a> {
                             }),
                         ),
                     );
-                    if copy_fields {
-                        field_expr = cx.expr_block(
-                            cx.block(struct_field.span, vec![cx.stmt_expr(field_expr)]),
-                        );
+                    // In general, fields in packed structs are copied via a
+                    // block, e.g. `&{self.0}`. The one exception is `[u8]`
+                    // fields, which cannot be copied and also never cause
+                    // unaligned references. This exception is allowed to
+                    // handle the `FlexZeroSlice` type in the `zerovec` crate
+                    // within `icu4x-0.9.0`.
+                    //
+                    // Once use of `icu4x-0.9.0` has dropped sufficiently, this
+                    // exception should be removed.
+                    let is_u8_slice = if let TyKind::Slice(ty) = &struct_field.ty.kind &&
+                        let TyKind::Path(None, rustc_ast::Path { segments, .. }) = &ty.kind &&
+                        let [seg] = segments.as_slice() &&
+                        seg.ident.name == sym::u8 && seg.args.is_none()
+                    {
+                        true
+                    } else {
+                        false
+                    };
+                    if is_packed {
+                        if is_u8_slice {
+                            cx.sess.parse_sess.buffer_lint_with_diagnostic(
+                                BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE,
+                                sp,
+                                ast::CRATE_NODE_ID,
+                                "byte slice in a packed struct that derives a built-in trait",
+                                rustc_lint_defs::BuiltinLintDiagnostics::ByteSliceInPackedStructWithDerive
+                            );
+                        } else {
+                            // Wrap the expression in `{...}`, causing a copy.
+                            field_expr = cx.expr_block(
+                                cx.block(struct_field.span, vec![cx.stmt_expr(field_expr)]),
+                            );
+                        }
                     }
                     cx.expr_addr_of(sp, field_expr)
                 })

compiler/rustc_builtin_macros/src/deriving/hash.rs

@@ -24,6 +24,7 @@ pub fn expand_deriving_hash(
         span,
         path,
         skip_path_as_bound: false,
+        needs_copy_as_bound_if_packed: true,
         additional_bounds: Vec::new(),
         supports_unions: false,
         methods: vec![MethodDef {

compiler/rustc_lint/src/context.rs

@@ -882,6 +882,9 @@ pub trait LintContext: Sized {
                         );
                     }
                 }
+                BuiltinLintDiagnostics::ByteSliceInPackedStructWithDerive => {
+                    db.help("consider implementing the trait by hand, or remove the `packed` attribute");
+                }
             }
             // Rewrap `db`, and pass control to the user.
             decorate(db)