reject named-field enums in SoAble derive to avoid unsound layout#45
reject named-field enums in SoAble derive to avoid unsound layout#45paradoxicalguy wants to merge 2 commits intotrynova:mainfrom
Conversation
|
hello :) and thank you for the clarification i'll take a look at implementing a correct field-name-based mapping for enums with named fields (so that fields are paired by name rather than position) and update the PR accordingly. |
|
i’ve updated the derive to implement a field-name–based SoA layout for enums with named fields, pairing fields by name rather than by position. |
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Thank you for putting in the work to generate the "correct" SoA code, and nice work!
Some issues remain: most pressing are the failing formatting / linting, and the amount of removed tests. Beyond that there were a few other issues that I commented in, mostly small stuff.
| let result = expand_data_enum(input).unwrap().to_string(); | ||
|
|
||
| // Should have unions named after fields, not positions | ||
| assert!(result.contains("union FooFieldx")); |
There was a problem hiding this comment.
issue: The field name's first character should be in upper case, so FooFieldX and FooFieldY.
|
|
||
| assert!(result.contains("union TestEnumField0")); | ||
| // For unnamed enums, unions should be named Fieldfield0, Fieldfield1, etc. | ||
| assert!(result.contains("union TestEnumFieldfield0")); |
There was a problem hiding this comment.
issue: There's an extra "Field" here. Field0, not FieldField0.
| } | ||
|
|
||
| #[test] | ||
| fn test_expand_derive_enum_explicit_discriminant() { |
There was a problem hiding this comment.
issue: It doesn't seem like a good idea to remove all of these tests.
| } | ||
|
|
||
| // Test enum with named fields | ||
| #[derive(SoAble)] |
There was a problem hiding this comment.
issue: These integration tests should not be removed.
| )); | ||
| } | ||
|
|
||
| // CHANGE: Determine if we have named fields |
There was a problem hiding this comment.
nitpick (blocking): The "CHANGE" etc comments are helpful for reviewing, but they'll be merely confusing when left in the code.
I recommend self-reviewing your PR and leaving comments in place of these comment lines in the future,
We'll need to remove the "CHANGE" / "ADD" parts at the very least.
| fn collect_all_field_names( | ||
| variants: &syn::punctuated::Punctuated<syn::Variant, syn::token::Comma>, | ||
| ) -> Vec<String> { | ||
| let mut field_names = BTreeMap::new(); |
There was a problem hiding this comment.
issue: I think we should use a Vec here instead: we do not want to sort the fields by their field name's hash value. The best sorting is probably going to be what the user provides for us, or as close to that as we can.
Effectively in this sort of case:
enum Foo {
A { x: u32, y: u32 },
B { x: u32, z: u32 },
}we'd end up with three fields for each of x, y, and z, and they'd be in that order: vec!["x", "y", "z"].
Instead of using BTreeMap insertion, just use a Vec with contains and push: it might technically be slower, but we expect the number of fields to be pretty low so it is unlikely to really matter.
| }) | ||
| .collect(); | ||
| // CHANGE: Build union fields differently for named vs unnamed | ||
| let (field_unions, all_field_identifiers): (Vec<Vec<_>>, Vec<String>) = if has_named_fields { |
There was a problem hiding this comment.
suggestion: The if and else branches here could benefit from more comments.
|
This PR will need a new title now that it does something entirely different. |
|
Closing for now; feel free to reopen this if you come in to fix the comments @paradoxicalguy |
|
yes sir |
3 participants
enums with named fields are currently laid out positionally, which can
pair unrelated fields across variants and cause unsound behavior.
this PR explicitly rejects named-field enums in the SoAble derive macro,
emitting a clear compile-time error explaining that a field-name–based layout is required.
this avoids unsoundness while leaving room for a correct implementation in the future.
fixes #43 by rejecting the problematic case.