Stream: git-wasmtime

Topic: wasmtime / PR #4359 support variant, enum, and union derives

Wasmtime GitHub notifications bot (Jun 30 2022 at 19:51):

dicej opened PR #4359 from component-type-derive-more to main:

This is the second stage of implementing #4308. It adds support for deriving
variant, enum, and union impls for ComponentType, Lift, and Lower. It
also fixes derived record impls for generic structs, which I had intended to
support in my previous commit, but forgot to test.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jun 30 2022 at 20:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 20:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 20:29):

alexcrichton created PR review comment:

Is this mostly to just simplify this for now where you don't have to worry about 16-vs-8 bit loads?

Wasmtime GitHub notifications bot (Jun 30 2022 at 20:42):

jameysharp created PR review comment:

union could reasonably accept syn::Fields::Unit cases: X should just be treated like X(()). I'm thinking you could simplify a bunch of this by building a Vec<VariantCase> like in expand_variant, even for enums and unions. Only in the enum case do you need to check that all cases have ty: None.

Similarly, I think all the generated code would be correct for Lift and Lower if you used the expand_variant implementation for all three cases. expand_enum doesn't strictly need to handle non-None case types, but those branches will just never be taken.

The only thing that's actually different in an interesting way is the type-checking implementation for ComponentType. Unless I've missed something?

Wasmtime GitHub notifications bot (Jun 30 2022 at 20:42):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:08):

dicej created PR review comment:

Did you notice that both expand_variant and expand_union defer to Expander::expand_variant? So the duplication is already pretty minimal. But yeah, expand_variant and expand_union could themselves be combined.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:08):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:11):

dicej created PR review comment:

Yeah, I was just keeping it simple to start with. I was also not sure what the theoretical max was for the component model. 2^32? More?

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:11):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:12):

dicej edited PR review comment.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:16):

jameysharp created PR review comment:

I did notice that, although it took me a few read-throughs. :laughing: So yes, I'm suggesting combining Expander::expand_variant with Expander::expand_enum, except that I know you'll need to take a little care with the implementation for ComponentType. But I'm also suggesting combining the top-level expand_variant, expand_union, and expand_enum.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:20):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:20):

jameysharp created PR review comment:

The Canonical ABI defines that in the discriminant_type function, and the current maximum is 2^32.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:26):

dicej created PR review comment:

I see -- I'll give that a shot.

Wasmtime GitHub notifications bot (Jun 30 2022 at 21:26):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 22:10):

dicej updated PR #4359 from component-type-derive-more to main.

Wasmtime GitHub notifications bot (Jun 30 2022 at 22:11):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 22:11):

dicej created PR review comment:

Yeah, that slimmed it down a lot. Great suggestion!

Wasmtime GitHub notifications bot (Jun 30 2022 at 22:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 30 2022 at 23:18):

alexcrichton merged PR #4359.

Last updated: Nov 22 2024 at 16:03 UTC