Conversation
|
This change is |
PR SummaryMedium Risk Overview Enum
Derive and Starknet contract plugin snapshots add generic enum Reviewed by Cursor Bugbot for commit ea647df. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-plugins/src/test_data/derive line 127 at r1 (raw file):
#[derive(Drop, Hash)] enum GenericEnumMulti<T> { First: T,
Add a test with more then one generic type
derive(Hash) on a generic enum previously failed to compile: the generated impl left the payload `x: T` live across the (panicking) discriminant-hash call with no bound on `T` (E3002 "Variable not dropped"), so no generic enum could derive Hash at all. Add a per-member `Drop<T>` bound (matching the struct branch) and wrap each variant payload in `DropWith`, pinning the variant's named Drop impl so enums with multiple members of the same generic type stay unambiguous. derive(Serde) on a non-generic enum emitted a whitespace-only generics header (`<\n \n>`) via a standalone formatdoc!; switch to `impl_header`, matching every other derive.
4f51588 to
ea647df
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-plugins/src/test_data/derive line 127 at r1 (raw file):
Previously, eytan-starkware wrote…
Add a test with more then one generic type
Done.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on TomerStarkware).
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
Fixes
#[derive(Hash)]for generic enums by ensuring variant values are properly dropped after hashing. Each enum variant's value is now wrapped in aDropWithorInferDropstruct before callingupdate_state, so the value is consumed throughx.valuerather than directly. TheHashderive for enums now also requiresDropbounds on generic type parameters, addingDROP_TRAITto the list of impl generics alongsideHASH_TRAIT.Additionally, the
Serdederive for non-generic enums is simplified to useimpl_header(consistent with structs), which removes the spurious empty whitespace in the generated impl header.Type of change
Please check one:
Why is this change needed?
Deriving
Hashon a generic enum previously did not require or useDropbounds on the generic type parameters. Afterupdate_stateconsumes the variant value, the remaining matched value would not be properly dropped, causing a compilation error for generic enums. The fix wraps each variant value in aDropWith/InferDropstruct so the compiler can infer the correct drop behavior.What was the behavior or documentation before?
#[derive(Hash)]on a generic enum (e.g.,enum GenericEnum<T>) would fail to compile because the derived implementation did not includeDrop<T>bounds and did not properly drop the matched variant values.What is the behavior or documentation after?
#[derive(Hash)]on generic enums now generates correct implementations that includeDrop<T>bounds for each generic member and wrap variant values inDropWith/InferDropbefore hashing, ensuring values are properly consumed.Related issue or discussion (if any)
N/A
Additional context
Test cases for
GenericEnum<T>andGenericEnumMulti<T>have been added to the derive test data to cover single and multiple generic variant scenarios.