Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLVM derive support for enums #325

Merged
merged 11 commits into from
Dec 3, 2023
Merged

CLVM derive support for enums #325

merged 11 commits into from
Dec 3, 2023

Conversation

Rigidity
Copy link
Contributor

@Rigidity Rigidity commented Nov 29, 2023

This PR adds support for deriving ToClvm and FromClvm on enums:

  • All 3 layouts are supported, and can be set individually for each variant if desired.
  • A discriminant (the numeric value of the enum) is used as the first value in each variant, to distinguish between the values.
  • The discriminant defaults to isize and starts at 0 incrementing by 1 per variant unless overridden, similar to the defaults used by rustc.
  • You can set the integer type used for the discriminant using the repr attribute (e.g. #[repr(u16)]).
  • For a simple integer encoding, use unit variants with the tuple layout.
  • For the list and tuple enums, you can use untagged to indicate that each of the variants are to be parsed individually and with no discriminator. This is useful for matching one of a set of possible values.

You can also derive these traits on unit structs, although this just encodes the value as if it had no fields and isn't too useful.

The quality of the code was also improved, and some of the duplicated logic was refactored out of each individual macro into the helpers file.

This should have no noticeable breaking changes in the way other derives work as far as I know.

Copy link

coveralls-official bot commented Nov 30, 2023

Pull Request Test Coverage Report for Build 7071376448

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 553 of 565 (97.88%) changed or added relevant lines in 5 files are covered.
  • 80 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.3%) to 86.134%

Changes Missing Coverage Covered Lines Changed/Added Lines %
clvm-derive/src/helpers.rs 24 26 92.31%
clvm-derive/src/from_clvm.rs 247 252 98.02%
clvm-derive/src/to_clvm.rs 168 173 97.11%
Files with Coverage Reduction New Missed Lines %
clvm-derive/src/helpers.rs 1 81.03%
src/gen/conditions.rs 4 99.71%
clvm-traits/src/error.rs 6 12.5%
chia-protocol/src/program.rs 69 71.02%
Totals Coverage Status
Change from base Build 7020975872: 0.3%
Covered Lines: 10684
Relevant Lines: 12404

💛 - Coveralls

@Rigidity Rigidity requested a review from arvidn November 30, 2023 13:58
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not all the way through this review yet. But here's what I've got so far.
Generally I think it would be good with more comments explaining what's going on. Macro code is especially unintuitive sometimes.

I appreciate the refactoring of fields() and repr_macros() to handle the different kinds of types in a uniform way (although I think they could both use comments describing what they do).

As for the named "untagged", maybe it would make sense to give it a name that describes the behavior instead. i.e. that it will perform a match instead. I don't know, untagged isn't so bad either (as along as it's well documented).

The exact behavior would ideally also be demonstrated in a test. regarding match-first, match-best or ambiguity being an error.

clvm-traits/src/lib.rs Outdated Show resolved Hide resolved
clvm-traits/src/lib.rs Outdated Show resolved Hide resolved
clvm-traits/src/lib.rs Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/from_clvm.rs Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Outdated Show resolved Hide resolved
@Rigidity Rigidity requested a review from arvidn December 1, 2023 21:44
clvm-derive/src/from_clvm.rs Show resolved Hide resolved
clvm-derive/src/from_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/from_clvm.rs Outdated Show resolved Hide resolved
clvm-derive/src/from_clvm.rs Show resolved Hide resolved
clvm-derive/src/from_clvm.rs Show resolved Hide resolved
clvm-traits/src/lib.rs Show resolved Hide resolved
clvm-traits/src/lib.rs Show resolved Hide resolved
clvm-traits/docs/derive_macros.md Show resolved Hide resolved
clvm-traits/docs/derive_macros.md Outdated Show resolved Hide resolved
clvm-traits/src/lib.rs Show resolved Hide resolved
@Rigidity Rigidity requested a review from arvidn December 2, 2023 16:42
clvm-derive/src/from_clvm.rs Show resolved Hide resolved
clvm-derive/src/to_clvm.rs Show resolved Hide resolved
clvm-traits/src/lib.rs Show resolved Hide resolved
@arvidn arvidn merged commit c1926d6 into Chia-Network:main Dec 3, 2023
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants