Add Fork derive macro for enum forking and unzipping#184
Add Fork derive macro for enum forking and unzipping#184kishore08-07 wants to merge 3 commits intoopen-rmf:mainfrom
Conversation
Signed-off-by: kishore08-07 <kishorebsm8@gmail.com>
mxgrey
left a comment
There was a problem hiding this comment.
Some broad thoughts on this: This PR does a good job of capturing what jotted out in #144 regarding a Fork macro that implements the Unzippable trait.
However this doesn't quite cover the original intent of allowing any enum to support the semantics that we have for Result and Option. I think this PR is a good first step in that direction, but I'll probably need to follow up this PR with a more concrete requirements outline for what the Fork or Forkable trait would look like.
One important distinction I expect between Unzippable and Forkable is that Unzippable provides the Outputs as an anonymous sequence, whereas Forkable should refer to specific names. This is especially relevant for Diagrams where users will want to refer to enum variants by name instead of by index numbers, which is what unzip currently uses.
Maybe the best long-term approach is to remove Unzippable entirely and just have a Forkable trait which uses Identifier to label each of its outputs. We would then phase out the unzip operation from diagrams in favor of a more general fork operation that can be used for tuples, Results, Options, all enums, and even structs (by splitting the fields).
I think most of this is beyond the scope of this PR, but I wanted to lay out my thoughts more clearly so we understand the opportunities available for follow-up work.
The one thing that I do think should be changed before merging this is to not allow named fields in a variant of an enum that we're forking. See my inline comment for more detail.
src/chain/unzip.rs
Outdated
| .connect(scope.terminate); | ||
| builder | ||
| .chain(named) | ||
| .map_block(|(left, right)| left - right) |
There was a problem hiding this comment.
I'm reluctant to turn named fields of a variant into an anonymous tuple. One reason that named fields get used in a variant is because the names of the fields are important, not the ordering of the fields. If a user changes the order of the named fields in their variant but keep the names the same, then they will expect that nothing is negatively impacted. However if we pack the field values into a tuple like this then changing the ordering of the field will change the meaning of this tuple.
I would rather the Fork macro emit a compilation error if it detects that one of the variants in the enum has more than one named field. If it's just a single named field then I don't mind treating it like the single value variant.
There was a problem hiding this comment.
@mxgrey I’ve addressed the inline comment by emitting a compile-time error for variants with multiple named fields, while allowing single-field named variants.Let me know if any further changes are needed.
…ate tests Signed-off-by: kishore08-07 <kishorebsm8@gmail.com>
Thanks for the detailed thoughts , this really helps clarify the bigger picture. I agree that a Forkable approach with named outputs would be a better long-term direction. For now, I’ve kept this PR scoped as a first step with Unzippable, and I’ve addressed the review comment. Looking forward to the follow-up discussion on shaping the Forkable design. |
New feature implementation
Implemented feature
This PR implements #144 , It introduces a procedural macro that enables forking and unzipping for any user-defined enum type, extending the existing support for Result and Option.
Implementation description
GenAI Use
We follow OSRA's policy on GenAI tools
Generated-by:
GPT-4.1