feat(wit): add map type support #2356
Conversation
|
Thanks for this! Are you looking to get this merged ASAP? Or is this more of a proof-of-concept to support the development of the PR on the spec itself? If you're looking to get this merged sooner-rather-than-later, do you have plans to work on other parts of the stack such as wit-bindgen or Wasmtime? |
|
@alexcrichton no rush at all in my end. I want to learn how to do the whole thing if possible if that is ok with you |
yordis
force-pushed
the
yordis/add-map-support
branch
3 times, most recently
from
8a0b8c4 to
cd02439
Compare
Signed-off-by: Yordis Prieto <[email protected]>
yordis
force-pushed
the
yordis/add-map-support
branch
from
cd02439 to
2487714
Compare
|
Note for self: |
| TypeDefKind::Enum(_) => Self::empty(), | ||
| TypeDefKind::List(t) => Self::for_type(resolve, t) | Self::LIST, | ||
| TypeDefKind::Map(k, v) => { | ||
| Self::for_type(resolve, k) | Self::for_type(resolve, v) | Self::LIST |
There was a problem hiding this comment.
| Self::for_type(resolve, k) | Self::for_type(resolve, v) | Self::LIST | |
| Self::for_type(resolve, k) | Self::for_type(resolve, v) |
Should this be Self::MAP or just removed?
There was a problem hiding this comment.
If you're up for it renaming LIST to something like NEEDS_MEMORY or something like that would be appropriate. Otherwise though this looks correct where LIST is mostly a proxy for "needs a memory canon option" which is always true for map
| | TypeDefKind::FixedSizeList(ty, _) | ||
| | TypeDefKind::Option(ty) => find_in_type(types, *ty), | ||
| TypeDefKind::Map(k, v) => { | ||
| find_in_type(types, *k).or_else(|| find_in_type(types, *v)) |
There was a problem hiding this comment.
This is fine yeah, and close enough to an approximation.
alexcrichton
requested review from
alexcrichton
and removed request for
pchickey
There was a problem hiding this comment.
Ok looked over things, and thanks for this!
In terms of implementing this, adding a new type is a relatively major endeavor that takes awhile as there are a lot of integration points. Given that we'll want to make sure that this work is properly "gated" from external users to avoid, in theory, accidentally hitting unimplemented parts of the type system. The primary way this is done is via WasmFeatures in the wasmparser crate. What I'd recommend is adding a new cm_map feature similar to cm_fixed_size_lists. Effectively you'll want to add a bunch of tests/checks/etc in the same manner as fixed-size-lists, also an unstable feature at this time.
Additionally you'll want to add tests in tests/cli/component-model/*.wast for this new type as well. You should be able to find tests for fixed-size-lists around there and you can structure tests in a similar way.
Other than that though I think this would be reasonable to land. After landing here would come integration work into wasmtime/wit-bindgen/etc.
|
|
||
| ComponentDefinedType::List(ty) | ComponentDefinedType::FixedSizeList(ty, _) => { | ||
| ComponentDefinedType::List(ty) | ||
| | ComponentDefinedType::Map(ty, _) |
There was a problem hiding this comment.
For this I think it would be best for Map to be routed to some sort of "not implemented" error and you can defer the work to someone fleshing out GC support in the future
| TypeDefKind::Enum(_) => Self::empty(), | ||
| TypeDefKind::List(t) => Self::for_type(resolve, t) | Self::LIST, | ||
| TypeDefKind::Map(k, v) => { | ||
| Self::for_type(resolve, k) | Self::for_type(resolve, v) | Self::LIST |
There was a problem hiding this comment.
If you're up for it renaming LIST to something like NEEDS_MEMORY or something like that would be appropriate. Otherwise though this looks correct where LIST is mostly a proxy for "needs a memory canon option" which is always true for map
| pub struct wit_map { | ||
| pub interface: *const ::std::os::raw::c_char, | ||
| pub name: *const ::std::os::raw::c_char, | ||
| pub key_ty: wit_type_t, | ||
| pub value_ty: wit_type_t, | ||
| } | ||
| pub type wit_map_t = wit_map; | ||
| #[repr(C)] | ||
| #[derive(Debug, Copy, Clone)] |
There was a problem hiding this comment.
For this I'd recommend skipping wit-dylib entirely. I ended up skipping fixed-size-lists mostly for example and figure it's fine to come back later and add support as needed.
There was a problem hiding this comment.
Although, if you're specifically interested in getting this working mind splitting it out to a separate follow-up PR?
| | TypeDefKind::FixedSizeList(ty, _) | ||
| | TypeDefKind::Option(ty) => find_in_type(types, *ty), | ||
| TypeDefKind::Map(k, v) => { | ||
| find_in_type(types, *k).or_else(|| find_in_type(types, *v)) |
There was a problem hiding this comment.
This is fine yeah, and close enough to an approximation.
2 participants
Related Add a 'map<K,V>' type