derive(Zeroable) on fieldful enums and repr(C) enums by zachs18 · Pull Request #257 · Lokathor/bytemuck

zachs18 · 2024-07-28T06:24:03Z

Closes #230.

Allows deriving Zeroable on enums where:

The enum is #[repr(Int)], #[repr(C)], or #[repr(C, Int)],
There is a variant with discriminant 0,
The variant with discriminant 0 is either fieldless, or all of its fields are Zeroable.

Also allows using the "perfect derive with additional bounds" from #196 for Zeroable on enums, where the fields for the "perfect derive" are from the variant with discriminant 0 (see the MyOption example on line 189 of derive/src/lib.rs).

Internal changes:

VariantDiscriminantIter now also gives the &'a Variant associated with the discriminant.
the comment on line 338 of derive/src/traits.rs kept getting mangled by cargo fmt, so I put it on its own line.
In derive_marker_trait_inner: let predicates = ... moved to after let fields = ..., since computing fields now requires accessing input, but predicates borrows it mutably. Does not change semantics.
generate_fields_are_trait and some other helper functions now take an Option<&Variant> to operate on if they otherwise don't support enums.
some cargo fmt
drive-by clarification/typo fixes (e.g. renaming discriminantor -> discriminant)

danielhenrymantilla

LGTM, nice! ✅

danielhenrymantilla · 2024-09-10T09:45:19Z

@@ -139,24 +142,23 @@ impl Derivable for Zeroable {
    match ty {
      Data::Struct(_) => Ok(()),
      Data::Enum(DataEnum { variants, .. }) => {


Unrelated to this PR; but this logic does not seem to fit the name of the function (check_attributes), since it is also checking the variants and whatnot for enums. The simplest fix here would be to just change the function name accordingly, fn check_input(), fn validate[_input] or whatnot.

danielhenrymantilla · 2024-09-10T09:53:45Z

+      VariantDiscriminantIterator::new(variants.iter())
+        .map(|res| {
+          let (discriminant, _variant) = res?;
+          Ok(LitInt::new(&format!("{}", discriminant), span))


Unrelated to this PR, but we might as well smuggle this tiny improvement:

Suggested change

Ok(LitInt::new(&format!("{}", discriminant), span))

Ok(LitInt::new(&discriminant.to_string(), span))

danielhenrymantilla · 2024-09-10T09:55:24Z

+    let first = &variant_discriminant_lits[0];
+    let rest = &variant_discriminant_lits[1..];


Unrelated to this PR, but it seems quite tempting to be doing the following, here:

Suggested change

let first = &variant_discriminant_lits[0];

let rest = &variant_discriminant_lits[1..];

let (first, rest) = variant_discriminant_lits.split_first().unwrap();

danielhenrymantilla · 2024-09-10T10:04:34Z

@@ -1215,7 +1248,7 @@ impl<'a, I: Iterator<Item = &'a Variant> + 'a> Iterator
      self.last_value += 1;


⚠️ Unrelated to this PR, but it seems rather important (somebody compiling proc-macros with a different choice of the overflow-checks knob may run into a panic with this logic):

Suggested change

self.last_value += 1;

self.last_value = self.last_value.wrapping_add(1);

Hmm, in the case of the (unstable) repr128 feature, I think this could cause unsoundness with derive(CheckedBitPattern) on an #[repr(i128)] enum X { A = i64::MAX as i128, B }, so to be safe, I'll make last_value an i128 and then do the wrapping_add (since the compiler will catch any overflows at the actual discriminant type).

danielhenrymantilla · 2024-09-10T10:21:35Z

+  /// If this trait has a custom meaning for "perfect derive", this function
+  /// should be overridden to return `Some`.
+  ///
+  /// The default is "the fields of a struct; unions and enums not supported".


Good and important doc 👌

🤔 I wonder if we shouldn't be moving said default logic over here (by changing the output to ::syn::Result<Fields>) to ensure actual code logic and this doc comment remain in sync

1. the enum is repr(Int), repr(C), or repr(C, Int), 2. the enum has a variant with discriminant 0, 3. and all fields of the variant with discriminant 0 are Zeroable.

…on and doctests.

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

fu5ha

Nice, LGTM. I'm deferring the in depth soundness checking of the macro to @danielhenrymantilla 's review whom I trust.

Lokathor · 2024-10-06T22:50:42Z

Sorry about the delay, bytemuck_derive-1.8.0 should be out now

Lokathor requested a review from fu5ha July 30, 2024 22:16

Lokathor self-assigned this Aug 22, 2024

Lokathor added the proc-macros I don't do proc-macros, but I accepts PRs about them. label Aug 22, 2024

danielhenrymantilla approved these changes Sep 10, 2024

View reviewed changes

zachs18 added 7 commits September 13, 2024 14:08

Add support for deriving Zeroable for fieldful enums if:

d2c1197

1. the enum is repr(Int), repr(C), or repr(C, Int), 2. the enum has a variant with discriminant 0, 3. and all fields of the variant with discriminant 0 are Zeroable.

Allow using derive(Zeroable) with explicit bounds. Update documentati…

ff316f6

…on and doctests.

doc update

c8e6fca

doc update

92f77c8

remove unused

d107b2a

Factor out get_zero_variant helper function.

be6ece8

Use i128 to track disciminants instead of i64.

b5ac207

zachs18 force-pushed the zeroable-derive-fieldful-enums branch from 653200e to b5ac207 Compare September 16, 2024 19:44

zachs18 and others added 2 commits September 16, 2024 14:46

Add doc-comment for get_fields

4dc453a

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Update derive/src/traits.rs

1b02024

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

fu5ha approved these changes Sep 21, 2024

View reviewed changes

Lokathor merged commit a637e1d into Lokathor:main Sep 24, 2024

Lokathor added the semver-minor semver minor change label Oct 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

derive(Zeroable) on fieldful enums and repr(C) enums#257

derive(Zeroable) on fieldful enums and repr(C) enums#257
Lokathor merged 9 commits into
Lokathor:mainfrom
zachs18:zeroable-derive-fieldful-enums

zachs18 commented Jul 28, 2024

Uh oh!

danielhenrymantilla left a comment

Uh oh!

danielhenrymantilla Sep 10, 2024

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Uh oh!

danielhenrymantilla Sep 10, 2024

Uh oh!

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Uh oh!

zachs18 Sep 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Uh oh!

fu5ha left a comment

Uh oh!

Lokathor commented Oct 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Ok(LitInt::new(&format!("{}", discriminant), span))
	Ok(LitInt::new(&discriminant.to_string(), span))

		let first = &variant_discriminant_lits[0];
		let rest = &variant_discriminant_lits[1..];

	let first = &variant_discriminant_lits[0];
	let rest = &variant_discriminant_lits[1..];
	let (first, rest) = variant_discriminant_lits.split_first().unwrap();

		@@ -1215,7 +1248,7 @@ impl<'a, I: Iterator<Item = &'a Variant> + 'a> Iterator
		self.last_value += 1;

	self.last_value += 1;
	self.last_value = self.last_value.wrapping_add(1);

Uh oh!

Conversation

zachs18 commented Jul 28, 2024

Uh oh!

danielhenrymantilla left a comment

Choose a reason for hiding this comment

Uh oh!

danielhenrymantilla Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

danielhenrymantilla Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

zachs18 Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielhenrymantilla Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

fu5ha left a comment

Choose a reason for hiding this comment

Uh oh!

Lokathor commented Oct 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zachs18 Sep 16, 2024 •

edited

Loading