RFC - Human friendly HostImports' builders

[andreaTP]

Since code is worth a thousand words, I started coding a bunch of HostImports' Builders to see how far I could get with a small corpus of examples.

Notes:

• I avoided the usage of reflection
• This implementation is mostly bound to the specialized functional interfaces it will be a bit of code to map everything, but I think it's something manageable
• The focus has been solely on DX(e.g. let's keep the discussion around performance to a later iteration)

Happy to hear comments!

[electrum]

Typo "filed"?

[electrum]

I find this style of nested builder to be strange, especially since only one method can be called. For nested builders, I've found that having a Consumer argument is a good approach, but that doesn't seem useful here. The only thing we are saving by having withNewImport() is the two name arguments.

All of the with methods on Builder replace the entire collection. For example, withGlobals() replaces all of the globals, whereas addGlobal() adds additional globals. So the name withNewImport() seems inconsistent.

Instead, what if we put new methods directly on Builder:

.addGlobal(moduleName, fieldName, Value.i32(1))
.addGlobal(moduleName, fieldName, MutabilityType.Var, Value.i32(1))
.addMutableGlobal(moduleName, fieldName, Value.i32(1))
.addMemory(moduleName, fieldName)
.addMemory(moduleName, fieldName, new MemoryLimits(1))
.addMemory(moduleName, fieldName, 1) // I'd drop these as the MemoryLimits version seems more clear
.addMemory(moduleName, fieldName, 1, 2)
.addTable(moduleName, fieldName)
.addTable(moduleName, fieldName, ValueType.ExternRef)
.addTable(moduleName, fieldName, new Limits(1))
.addTable(moduleName, fieldName, 1) // same, I'd drop these and use the Limits version
.addTable(moduleName, fieldName, 1, 2)
// does type inference work out if we name all of these "addFunction"?
.addProcedure(moduleName, fieldName, () -> System.out.println("hello world"))
.addProcedure(moduleName, fieldName, (Instance inst) -> () -> System.out.println("hello world"))
.addSupplier(moduleName, fieldName, () -> 1)

[andreaTP]

Thanks for your input @electrum !
A few different subjects:

• add instead of with: agree will change accordingly, thanks for noticing!
• two name arguments: personally, I feel it awkward to have to type those "names" along with the implementation, the nested builder was an attempt to fix it.
• new methods directly on Builder: this works for sure, I'm afraid is an improvement less impactful than I originally thought

[electrum]

I feel it awkward to have to type those "names" along with the implementation

This test might look worse due to repeating the same variables. In actual usage, the field names will need to be unique (and the builder should validate that)? So if we change it to use constants, does it look better?

.addGlobal("test", "g1", Value.i32(1))
.addGlobal("test", "g2", MutabilityType.Var, Value.i32(1))
.addMutableGlobal("test", "gmut", Value.i32(1))
.addMemory("test", "m1")
.addMemory("test", "mlim", new MemoryLimits(1))
.addTable("test", "t1")
.addTable("test", "tref", ValueType.ExternRef)
.addTable("test", "t3", new Limits(1))

Another problem is that the code formatter forces each chained call to be on a separate line. If we were formatting the code by hand, it would look better:

.add("test", "g1").global(Value.i32(1))
.add("test", "g2").global(MutabilityType.Var, Value.i32(1))
.add("test", "gmut").mutableGlobal(Value.i32(1))
.add("test", "m1").memory()
.add("test", "mlim").memory(new MemoryLimits(1))
.add("test", "t1").table()
.add("test", "tref").table(ValueType.ExternRef)
.add("test", "t3").table(new Limits(1))

Which I agree looks slightly cleaner, but we can't format like that...

[andreaTP]

I love this!

.add("test", "g1").global(Value.i32(1))

I believe that we should not be limited by our own formatter, and we can always disable it when necessary 😏

[andreaTP]

The more I look at this approach, the less I'm convinced this is the way to go.

public Builder withSupplier(Function<Instance, IntSupplier> consumer)

and

public Builder withSupplier(Function<Instance, LongSupplier> consumer)

are going to be not distinguishable after type erasure.

Currently, I think that leaving only:

public Builder withFunction(Function<Instance, Function<Value[], Value[]>> consumer)

and generating the binding out of real modules with something like: https://github.com/andreaTP/chicory-bindgen-poc is the best bet.

[bhelx]

Something I wanted to chat about in regards to #441 is that building a imports are really just another module that you link to. It feels like we could only have one way to build a module internally. Then we could maybe have some helpers to quickly build these when you just want a couple host functions. I'm just not sure there needs to be a HostImports object at all.

[andreaTP]

@bhelx thanks for sharing! Super interesting POV indeed.
A couple of things on top of my head:

• other than "using the Parser" we don't have a "great" API to build Modules internally, this seems a prerequisite to your proposal
• we should make it extremely convenient to define Host Functions

I'm happy to see further exploration of the idea, thought! Seems appealing!

[andreaTP]

A couple of additional cents:

building a imports are really just another module that you link to

correct, at the moment all the HostXXX are implementing the FromHost interface, here we are basically adding the 2 module and field names.
One option is to make those fields optional on the Memory, Global, etc. classes, or there is a better option?

[bhelx]

The way i see it, host imports are just a type of Module whose instance is not a wasm instance, but lives on the host. But it could / should have the same API as any other module you might want to link (e.g. other wasm modules which must be instantiated as wasm). You can run into situations where you might need to link up a mix of both host and wasm modules to instantiate a module.

I need some time to study how others runtimes do it, but ideally there could be a low-level, imperative API for building up an instance, and a high level API for when you just want to throw some host functions at a module.

We'd need some things like:

1. A builder API for building a Module (host or wasm)
2. An api for instantiating these

• we mostly have this, my only point in bringing it up is cases where we need to instantiate a linked wasm module

3. Some kind of Linker which can dynamically link any set of wasm (or host) modules

• The linker could handle the complexity of lining up all the imports / exports, validating, and instantiating the modules

For the higher level, shorthand API, where you maybe just want to pass some host functions. We can support passing functions to the instantiation process (kind of like how we do now). You wouldn't need the linker you could just use the normal module builder and instantiator.

[bhelx]

This is a lower-level reference doc, but here is the API for the wasmtime linker https://docs.wasmtime.dev/api/wasmtime/struct.Linker.html

[andreaTP]

I see, at the moment cross-linking modules is possible but challenging for sure.
Do you have/want to craft real-world use cases where this functionality is needed?

I'm a bit afraid that it might be a bit "too early" to "future-proof" something we don't entirely grasp.

[evacchi]

I'm starting to feel like HostImports (now "ExternalValues") could be subclassed(*) to HostModule (#482) and this builder would be the "low-level" builder for a HostModule. Then #496 would be the higher-level, user-friendly version of it.

(*) well not necessarily, we can just let the HostModule have a toExternalValues() method.

[andreaTP]

This is too outdated, let's close it and move on, the Store helps a lot in smoothening the experience and the upcoming code-gens are going to become the standard way to define ExternalValues.