lann / wasm-wave

Web Assembly Value Encoding
Apache License 2.0
38 stars 6 forks source link

Update wasmtime dependency from 18 to 21 #34

Closed FrankReh closed 6 months ago

FrankReh commented 6 months ago

Update the wasmtime dependency version from 18 to 21.

Also add type checks for the wasmtime/component.

Removed the ty method from the WasmValue trait.

Also add type checks for the value::{Value, Type} as the ty method is no longer available for building up a tree of types from a tree of values.

FrankReh commented 6 months ago

Well, in addition to wondering if the direction of the last commit suits you. I was wondering if you wanted to revisit the Value::make_* functions in general. They are in src/value/mod.rs. I can't find that they are used except from tests, with the exception of three: Value::make_list, Value::make_option, and Value::make_result, used in src/value/convert.rs, in supporting From traits. So arrays, vectors, options, and results, can be converted into Value types.

But they all generate their Type information automatically, stored as a local variable ty, which is then passed to the make_* function. So the type isn't going to be compared to anything that has come from a WIT definition.

And what would trigger creation of Value records, variants, enums or flags.

Also wondering if the ty field is needed in structs in src/value/mod.rs, unless maybe a value of this type could be created with an empty vec and then operations were planned that would allow adding to the list later?

#[derive(Debug, Clone, PartialEq)]
#[doc(hidden)]
pub struct List {
    ty: Arc<ListType>,
    elements: Vec<Value>,
}

I know for sure I don't see all the ways this wasm-wave library was planned to be used in the future, so its current form is just a stepping stone.

Thanks.

FrankReh commented 6 months ago

@lann What should we do? Want me to drop this line?

lann commented 6 months ago

Ah sorry, I lost track of your comment over the weekend.

wondering if the direction of the last commit suits you

Yes, that looks good to me.

I was wondering if you wanted to revisit the Value::make_* functions in general.

These are used here, which is how the parser makes typed values: https://github.com/lann/wasm-wave/blob/e69b66e10383295debd16ce4bd031620fc5e1c22/src/ast.rs#L207-L236

FrankReh commented 6 months ago

Still regarding the value module.

I understand that for the unit tests, the types represented by the ty variable passed through the call chain would come from the unit test functions themselves. But that isn't helping me understand where they would come from when this is used as a library. If a WIT file or files were being used, wouldn't the library be able to use the wasmtime core or component types?

I don't yet see the need for duplicating the logic behind building variables and checking types for the concrete wasmtime core and component branches, with the library proprietary value::{Value, Type} variables and types.

You can always take this over if you think that is easier than getting me to see this. Thanks.

lann commented 6 months ago

wouldn't the library be able to use the wasmtime core or component types

That may be possible with the wasmtime 20 val type (it wasn't before), but there are some use cases that don't otherwise need to use wasmtime and it would represent a very heavyweight dependency just to get these value types. If these types could be moved into a 3rd party crate to be a shared dependency I think this would be feasible, but the details of resource types make that difficult, currently.

You can always take this over if you think that is easier than getting me to see this.

I'm happy to if you'd prefer. What is the status of this PR as-is? Is it complete enough to merge?

FrankReh commented 6 months ago

Your comment reminded me that the wit-parser and wasmtime dependencies are optional.

FrankReh commented 6 months ago

The last commit has the Tuple checking arm fleshed out. There are a lot of types being checked.

FrankReh commented 6 months ago

@lann I think now it's ready.

Down the road, perhaps the check_type and check_type2 that are called in value/mod.rs will be called at a higher spot so they are only called once for any Node that is constructed. As it stands right now, the pieces are built up from leaf nodes and their types are also checked and then as higher order nodes are built, their types are checked again, going all the way down to the leaves. It seems unnecessary but that can wait.

Also, it seems the Record type in the value module isn't as flexible as the one in the wasmtime component module. The field names are not stored so there is no chance to provide the fields in a different order and no chance to omit optional fields. But maybe that's intentional.