wasmtime / PR #4337 [do not merge] custom derive for reco... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #4337 [do not merge] custom derive for reco...

Wasmtime GitHub notifications bot (Jun 27 2022 at 20:14):

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

This is a first draft of an implementation of
https://github.com/bytecodealliance/wasmtime/issues/4308, i.e. derive macros for
ComponentType, Lift, and Lower for composite types in the component model.

It borrows heavily from the work @jameysharp did in
https://github.com/bytecodealliance/wasmtime/pull/4217. Thanks, Jamey!

Thanks also to @fibonacci1729 for pairing up on this.

This draft only covers records; I expect the other composite types will follow a
similar pattern. I'm looking to get feedback on the overall approach before I
continue.

Known (potential) issues:

Currently, the implementations for ComponentType, Lift, and Lower
duplicate a fair amount of code, so I plan to address that.

I've made ComponentTypesBuilder::record_type public for testing purposes.
Let me know if that's a problem or if there's a better way.

Also for testing: I've made Memory::wasmtime_memory and Store::inner
crate-public.

I've added a test module to crates/wasmtime/src/component/func/typed.rs for
testing the derives. Let me know if there's a better place for it.

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

Wasmtime GitHub notifications bot (Jun 27 2022 at 20:52):

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

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Could this have a .context(...) with information about the field name that failed to type-check?

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

I think it's ok to skip the #[inline] here since this is pretty beefy internally.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Would you be ok switching this to the style of use elsewhere in the crate where it's one use .. per line?

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

If you're up for it, could this be moved externally to tests/all/component_model/*.rs? I personally prefer to test the public-facing interface wherever possible because the internal ones like the builder used here are often subject to more churn.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

If possible I'd prefer to keep this hidden, but I think that will be enabled if the test above is moved to an external test.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Personally I think it's ok to inline all these at the use-site, otherwise I find myself tabbing back and forth to what exactly this error message is saying

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

I think thes can be reexported outside the derive module? (ideally we wouldn't need a derive module I think)

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Personally for patterns like this I find it convenient to have "early return" to avoid nested indentation getting too unwieldy. For example:
if attribute.path.leading_colon.is_some() || attribute.path.segments.len() != 1 {
    continue;
}

// ... everything else minus one indentation level

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Instead of cloning could this pass &T since &T is Lower if T is Lower?

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Oh this is clever! I hadn't actually thought of simply delegating to the tuple implementation. That definitely makes this easier since you don't have to do offset calculations here in the derive macro.

That being said though the main limitation of this is that it won't work for structs with > 16 fields. That doesn't seem too common though so otherwise seems like a good way to start?

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

Although note that in moving this test externally I mean doing a whole compile-a-component rigamarole and using Func::typed::<...> to run the tests that this is otherwise doing, so the test wouldn't use ComponentTypesBuilder or call typecheck at all.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:10):

alexcrichton created PR review comment:

One thing you may want to explore though is what the error messages look like if a field of the record doesn't implement the requisite trait. For example what's the compiler error message when you derive Lift for a record with a String in it?

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:17):

dicej created PR review comment:

I agree -- I only started doing that because rustfmt would refuse to format an expression that contained a long string literal. Perhaps your suggestion to reduce the indentation level will help with that.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:17):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:19):

dicej created PR review comment:

The cleverness is due to @jameysharp -- that's straight from his patch.

Wasmtime GitHub notifications bot (Jun 27 2022 at 21:19):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 27 2022 at 23:27):

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

Wasmtime GitHub notifications bot (Jun 27 2022 at 23:31):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 00:26):

jameysharp created PR review comment:

I'm certainly enjoying being called clever! But before I got sidetracked, I had intended to drop this trick. :laughing:

In #4217 I factored out next_field to make both tuple and record macro expansions relatively straightforward. While lower and lift were already pretty easy, next_field wraps up the common math for size, store, and load. So I hope it should be simple now to almost just copy impl_component_ty_for_tuples, except as a proc-macro instead of macro_rules, and with field names instead of indexes.

As I recall, this bit of cleverness is also what forced me to call .clone() in lower: I couldn't build a tuple of borrows, because the implementation I delegated to expected the borrow to be on the outside of the tuple instead. Skipping the indirection should let you remove those clones.

Wasmtime GitHub notifications bot (Jun 28 2022 at 00:26):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 14:35):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 14:35):

dicej created PR review comment:

Thanks for pointing that out, @jameysharp. I was able to change the .clone() to a borrow with no apparent ill effects -- the compiler and the test are both happy. I'll admit I haven't thought deeply about _why_ it worked, though :)

I do agree that the limit of 16 fields for structs is annoying, so I like your plan. Just to be clear: the plan is to have the proc-macro generate a custom struct for use as the ComponentType::Lower associated type for each #[derive(ComponentType)] #[component(record)] struct alongside the generated ComponentType impl, analogous to what impl_component_ty_for_tuples does for tuples -- correct?

Wasmtime GitHub notifications bot (Jun 28 2022 at 14:58):

dicej edited PR #4337 from component-type-derive to main:

This is a first draft of an implementation of
https://github.com/bytecodealliance/wasmtime/issues/4308, i.e. derive macros for
ComponentType, Lift, and Lower for composite types in the component model.

It borrows heavily from the work @jameysharp did in
https://github.com/bytecodealliance/wasmtime/pull/4217. Thanks, Jamey!

Thanks also to @fibonacci1729 for pairing up on this.

This draft only covers records; I expect the other composite types will follow a
similar pattern. I'm looking to get feedback on the overall approach before I
continue.

Known (potential) issues:

Currently, the implementations for ComponentType, Lift, and Lower
duplicate a fair amount of code, so I plan to address that.

~~I've made ComponentTypesBuilder::record_type public for testing purposes.
Let me know if that's a problem or if there's a better way.~~

~~Also for testing: I've made Memory::wasmtime_memory and Store::inner
crate-public.~~

~~I've added a test module to crates/wasmtime/src/component/func/typed.rs for
testing the derives. Let me know if there's a better place for it.~~

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 15:32):

alexcrichton created PR review comment:

Yeah that sounds right to me, generating a new type, and it could even be an "anonymous" type along the lines of:
const _: () = {
    struct Foo;
};

// doesn't have access to `Foo` here

Wasmtime GitHub notifications bot (Jun 28 2022 at 15:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:05):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:09):

dicej created PR review comment:

@alexcrichton and @jameysharp please see the commit I just pushed and let me know what you think.

Alex, I wasn't sure how to apply your "anonymous" type trick at the item level that the derive macro operates. Can you elaborate on what that would look like, i.e. how I might use it in emit_record_type?

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:09):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:23):

alexcrichton created PR review comment:

I'm not actually sure if this is possible, but if we can I think it'd be best to reuse the map_maybe_uninit! macro used internally here as well. If that doesn't work out though can you fill in the two type parameters to transmute here to clarify what's happening?

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:23):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:23):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:29):

dicej submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:29):

dicej created PR review comment:

Yeah, I started out reusing map_maybe_uninit!, but that required adding a #[macro_export] and making MaybeUninitExt public. If that sounds okay to you, I'll do that.

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

alexcrichton created PR review comment:

Could this also get prefixed with dep: to indicate to Cargo we shouldn't have a feature called wasmtime-component-macro?

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

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:32):

alexcrichton created PR review comment:

That should be fine as long as they're all marked #[doc(hidden)] to not actually show up in the public documentation

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:43):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 19:45):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 20:03):

jameysharp created PR review comment:

Random thought: for alignment, you could de-duplicate the field types, e.g. a struct with a bunch of u32s has the same alignment as one u32. I think it might marginally improve compile times overall. It's certainly not necessary to do though.

Overall, this looks good to me! It's up to Alex on final review though.

Wasmtime GitHub notifications bot (Jun 28 2022 at 20:03):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 20:10):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 20:51):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 21:54):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 21:54):

dicej edited PR #4337 from component-type-derive to main:

This is a first draft of an implementation of
https://github.com/bytecodealliance/wasmtime/issues/4308, i.e. derive macros for
ComponentType, Lift, and Lower for composite types in the component model.

It borrows heavily from the work @jameysharp did in
https://github.com/bytecodealliance/wasmtime/pull/4217. Thanks, Jamey!

Thanks also to @fibonacci1729 for pairing up on this.

This draft only covers records; I expect the other composite types will follow a
similar pattern. I'm looking to get feedback on the overall approach before I
continue.

Known (potential) issues:

~~Currently, the implementations for ComponentType, Lift, and Lower
duplicate a fair amount of code, so I plan to address that.~~

~~I've made ComponentTypesBuilder::record_type public for testing purposes.
Let me know if that's a problem or if there's a better way.~~

~~Also for testing: I've made Memory::wasmtime_memory and Store::inner
crate-public.~~

~~I've added a test module to crates/wasmtime/src/component/func/typed.rs for
testing the derives. Let me know if there's a better place for it.~~

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 22:03):

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

Wasmtime GitHub notifications bot (Jun 28 2022 at 23:10):

jameysharp created PR review comment:

Maybe one more field-count test, with too many fields? Looks like you have everything you need to do that easily.

Wasmtime GitHub notifications bot (Jun 28 2022 at 23:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 28 2022 at 23:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2022 at 01:45):

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

Wasmtime GitHub notifications bot (Jun 29 2022 at 02:15):

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

Wasmtime GitHub notifications bot (Jun 29 2022 at 02:17):

dicej has marked PR #4337 as ready for review.

Last updated: Nov 22 2024 at 16:03 UTC