Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename HostImports to ExternalValues #527

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

evacchi
Copy link
Collaborator

@evacchi evacchi commented Sep 16, 2024

a.k.a. The Big Rename™️. This follows an offline discussion with @andreaTP. The idea would be to finalize naming before the future 1.0 release.

The class HostImports and the related dependencies HostFunction, HostTable, HostGlobal, HostMemory are currently being used with two different purposes:

  1. allowing a host, that is, the application that embeds the Wasm runtime, to define such values programmatically using the host language (especially true for functions)
  2. allowing the runtime to link together two different module instances, connecting their "import" requirements to the "exports" of other instances.

However, name "Host" makes most sense for the first case, while it feels slightly off and ambiguous in the second case: are those Host-defined values, or are those just values belonging to another instance?

The spec in this regard is not ambiguous, and generally refers to External Types and External Values. Specifically:

  • External Types classify imports and external values. Notice we are already using the term ExternalType under package com.dylibso.chicory.wasm.types.
  • I think External Values identifies more correctly what we are currently calling HostImports; i.e. instances of the respective types (instances of functions, of globals, of memories or of tables), capturing the fact that these values are just "externally defined", not necessarily by the host, but possibly by an instance of another module.

Thus, with this change, I propose the following renames:

  • FromHost -> ExternalValue
  • FromHost.FromHostType -> ExternalValue.Type notice that we could also reuse com.dylibso.chicory.wasm.types. ExternalType, but this would require exposing to the end users package com.dylibso.chicory.wasm.types from package com.dylibso.chicory.runtime
  • HostImports -> ExternalValues
  • HostGlobal -> ExternalGlobal
  • HostTable -> ExternalTable
  • HostMemory -> ExternalMemory
  • HostFunction still exists, as a subtype of ExternalFunction: this allows to keep a distinction that might be useful in the future; in fact, the constructor to ExternalFunction could be even package-local.

Notice that, I have not re-defined Host{Global,Table,Memory} as subtypes of External{Global,Table,Memory}, because I feel like there is no meaningful distinction:

  • the implementation for the "host" version does not conceptually differs from the "wasm" version
  • on the other hand, HostFunctions implement logic using the host language, while ExternalFunctions are otherwise just the exported image of an instance of a Function defined in a Wasm module. So, even though their interface might be the same, they are different concepts.

Even if we decided to drop the concrete type HostFunction altogether, it might still make sense to expose a factory ExternalFunction.createHostFunction() for an ExternalFunction` because the term "host function" is generally known to refer to a host-defined function.

It might be now feel slightly more awkward to import host functions directly through an Instance.Builder; e.g.:

var wasi = new WasiPreview1(logger, wasiOpts);
var hostFunctions = new ExternalValues(wasi.toHostFunctions());
var inst = Instance.builder(Parser.parse(new File("greet-wasi.wasm")))
              .withExternalValues(hostFunctions).build();

but consider that, on the other hand, the Instance.Builder interface is a lower-level interface, so it might be acceptable for it to expose "lower-level" naming; in the future, I would expect users to interact with the runtime using higher level interfaces, such as the Store. In this case, users would write:

var wasi = new WasiPreview1(logger, wasiOpts);
var store = new Store();
store.addFunctions(wasi.toHostFunctions());
var inst = store.instantiate("main", Parser.parse(new File("greet-wasi.wasm")));

Notes

Will follow-up with another, conceptually unrelated rename, but with the same goal of improved clarity; i.e.:

  • fieldName() -> symbolName()

reason being fieldName() to me sounds too much like "name of a property", that is, it makes me think "Global" but we also use it for "Function"s. "Symbol" is more generic and should avoid ambiguity.

Signed-off-by: Edoardo Vacchi [email protected]

@evacchi evacchi requested review from andreaTP, electrum and bhelx and removed request for andreaTP and electrum September 16, 2024 09:53
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal, I like it overall!

Copy link
Contributor

@bhelx bhelx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. Although it might create some dissonance with our use of "host" in other places (like the docs). Actually you might want to do a quick search for "host" to make sure you've covered all the docs as well. Maybe all of them don't need to change and they make sense in context, but some might.

pom.xml Outdated Show resolved Hide resolved
@evacchi
Copy link
Collaborator Author

evacchi commented Sep 17, 2024

@bhelx I think the docs look good, from what I can see the only place where we still used "host" "improperly" (so to speak) was a line of the WASI readme:

e774e26#diff-2a0f73c29b5710acc7ebe0330361345e74a05b4e51a027012ccbf9862ed6779a

@evacchi evacchi changed the title Rename HostImports to ExternalValues. Rename HostImports to ExternalValues Sep 17, 2024
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi evacchi marked this pull request as ready for review September 17, 2024 11:59
@evacchi
Copy link
Collaborator Author

evacchi commented Sep 17, 2024

FYI I have pushed the other refactor I have mentioned in the top post ExternalValue#fieldName() -> ExternalValue#symbolName() to this branch evacchi@60026a0

Using fieldName might cause confusion now (a "field" in a module to me sounds like a "global" value, not a function) and in the future; e.g. fields are the fields in a Structure (see WasmGC);

I can create another PR once this gets merged or just push it here. The changes are related, but they don't need to be shipped together; but if we agree on this rename too, then it makes sense to ship them in the same release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants