bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Allow more deriving on packed structs.

Browse source 
Currently, deriving on packed structs has some non-trivial limitations,
related to the fact that taking references on unaligned fields is UB.

The current approach to field accesses in derived code:
- Normal case: `&self.0`
- In a packed struct that derives `Copy`: `&{self.0}`
- In a packed struct that doesn't derive `Copy`: `&self.0`

Plus, we disallow deriving any builtin traits other than `Default` for any
packed generic type, because it's possible that there might be
misaligned fields. This is a fairly broad restriction.

Plus, we disallow deriving any builtin traits other than `Default` for most
packed types that don't derive `Copy`. (The exceptions are those where the
alignments inherently satisfy the packing, e.g. in a type with
`repr(packed(N))` where all the fields have alignments of `N` or less
anyway. Such types are pretty strange, because the `packed` attribute is
not having any effect.)

This commit introduces a new, simpler approach to field accesses:
- Normal case: `&self.0`
- In a packed struct: `&{self.0}`

In the latter case, this requires that all fields impl `Copy`, which is
a new restriction. This means that the following example compiles under
the old approach and doesn't compile under the new approach.
```
 #[derive(Debug)]
 struct NonCopy(u8);

 #[derive(Debug)
 #[repr(packed)]
 struct MyType(NonCopy);
```
(Note that the old approach's support for cases like this was brittle.
Changing the `u8` to a `u16` would be enough to stop it working. So not
much capability is lost here.)

However, the other constraints from the old rules are removed. We can now
derive builtin traits for packed generic structs like this:
```
 trait Trait { type A; }

 #[derive(Hash)]
 #[repr(packed)]
 pub struct Foo<T: Trait>(T, T::A);
```
To allow this, we add a `T: Copy` bound in the derived impl and a `T::A:
Copy` bound in where clauses. So `T` and `T::A` must impl `Copy`.

We can now also derive builtin traits for packed structs that don't derive
`Copy`, so long as the fields impl `Copy`:
```
 #[derive(Hash)]
 #[repr(packed)]
 pub struct Foo(u32);
```
This includes types that hand-impl `Copy` rather than deriving it, such as the
following, that show up in winapi-0.2:
```
 #[derive(Clone)]
 #[repr(packed)]
 struct MyType(i32);

 impl Copy for MyType {}
```
The new approach is simpler to understand and implement, and it avoids
the need for the `unsafe_derive_on_repr_packed` check.

One exception is required for backwards-compatibility: we allow `[u8]`
fields for now. There is a new lint for this,
`byte_slice_in_packed_struct_with_derive`.
This commit is contained in:
Nicholas Nethercote 2022-11-21 14:40:32 +11:00
parent c7bf469fec
commit 2e93f2c92f
25 changed files with 874 additions and 349 deletions
Download patch file Download diff file Expand all files Collapse all files

1
compiler/rustc_builtin_macros/src/deriving/bounds.rs
View file

@ -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(),

1
compiler/rustc_builtin_macros/src/deriving/clone.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/cmp/ord.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs
View file

@ -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,

1
compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs
View file

@ -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],

1
compiler/rustc_builtin_macros/src/deriving/debug.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/decodable.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/default.rs
View file

@ -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 {

1
compiler/rustc_builtin_macros/src/deriving/encodable.rs
View file

@ -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 {

135
compiler/rustc_builtin_macros/src/deriving/generic/mod.rs
View file

@ -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)
})

1
compiler/rustc_builtin_macros/src/deriving/hash.rs
View file

@ -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 {