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

implement "part 1" of WIT Templates #5934

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Mar 5, 2023

Per WebAssembly/component-model#172, this implements "part 1" of WIT templates, allowing WIT files to define interfaces which contain a single wildcard function, which worlds may import or export.

I've taken a "minimalist" approach to this, such that the host binding generator just skips wildcards entirely, leaving it to the host to use dynamic APIs to reflect on the component and do the necessary func_wrap (for imports) and typed_func (for exports) calls directly.

This adds two new functions to the public API:

  • Component::names: provides an iterator over the names of the functions imported by the specified instance.
  • ExportInstance::funcs: provides an iterator over the names of the functions exported by this instance.

In both cases, I'm open to alternative API designs for getting that information.

Note that I've added a temporary dependency override to Cargo.toml, pointing to our fork of wasm-tools, which includes the necessary wit-parser changes. We're still iterating on that and will PR those changes separately. We also have a fork of wit-bindgen with a new "wildcards" test to verify everything works end-to-end: bytecodealliance/wit-bindgen@main...dicej:wit-templates. I'll PR that last once everything else is stable.

@dicej dicej changed the title Per WebAssembly/component-model#172, this implements "part 1" of WIT implement "part 1" of WIT Templates Mar 5, 2023
@dicej dicej force-pushed the wit-templates-minimal branch from cc5e7b0 to 6b57e8b Compare March 5, 2023 22:34
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 5, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I think the extra accessors here are fine, but reading over the test it leaves me with the impression of "why use bindgen!?" in the sense that if the *-imports are skipped then there's not actually any usage of the types in the test here. I do think though that for more complex types at least having the types generated is useful. You also mention that this is a first step, but is this intended to be iterated further on to integrate the *-imports into the codegen process?

@dicej
Copy link
Contributor Author

dicej commented Mar 6, 2023

Yes, in that particular test case, the only reason I used bindgen! was to ensure that it doesn't choke on wildcards. In a more realistic scenario, a world could contain a mix of non-wildcard interfaces and wildcard interfaces. You'd still want generated bindings for the former even if you don't need them for the latter.

Also, although WebAssembly/component-model#172 does not include "mixed" interfaces which contain both wildcard and non-wildcard functions, it would be pretty trivial to add support for that. For example:

default world wildcards {
    import imports: interface {
        *: func() -> u32
        foo: func(s: string) -> string
        bar: func() -> u32
    }
}

BTW, my initial implementation (#5925) did generate code for wildcards, but that code ended up just being wrappers around the Wasmtime API which didn't add much value vs. using the API directly. If you think there is value to be added via code generation, we can revisit that.

@alexcrichton
Copy link
Member

In some sense the whole bindgen! macro is a pretty thin wrapper around the component embedding API, but I think the main value from it is that it gets all the types right and you don't have to think about it. From another perspective any type-level bugs are caught by rustc and aren't logic bugs in your own code. Manually calling linker functions, though, you can get past rustc but accidentally deviate from the *.wit which I think is nice to fix if we can. More-or-less I do think that it's wortwhile to generate typed bindings to *-imports/exports to make it idomatic with the rest of *.wit-generated bindings and otherwise make it so you don't have to think about the types.

@dicej
Copy link
Contributor Author

dicej commented Mar 6, 2023

Fair enough. Would you mind taking a look at the diff for #5925 and provide feedback on that, then? I can close this PR and reopen that one if desired. I just want to make sure we're aligned about what the generated code should look like first.

@alexcrichton
Copy link
Member

For sure yeah, the main thing I'd consider on #5925 is sort of along the same lines of catching issues earlier rather than later. There it looks like the strategy is that *-imports bottom out into a fn call(&mut self, name: &str, ...) trait method but an alternative would be for the host to pass in some sort of configuration "I support call with these names" which then fails instantiation if enough names weren't supplied. My original idea here was to pass in a vector of named trait objects, but that may be too inefficient for the intended use cases (I'm not sure).

I don't feel super strongly about this particular point though myself. It feels a little odd to pass around the component when you're adding something to a linker but that's probably just me needing to get over my preconceptions about this. In any case I do think that #5925 is better in the sense of "everything is type-checked for you" so I think that would be a better starting point than not generating bindings for *

@dicej dicej force-pushed the wit-templates-minimal branch from f2bb16b to da17f74 Compare March 7, 2023 01:49
@dicej
Copy link
Contributor Author

dicej commented Mar 7, 2023

@alexcrichton I've implemented code generation per your feedback. Please let me know what you think.

@dicej dicej force-pushed the wit-templates-minimal branch from da17f74 to db34f8a Compare March 7, 2023 01:55
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! I'm not necessarily totally wild about the design but I can't think of alternatives that I like better either. The WildcardMatches struct feels a bit clunky and makes me sort of want a builder-style for creating imports where you can iteratively add more wildcard imports, but I don't know how that would actually work off the top of my head.

crates/wasmtime/src/component/component.rs Outdated Show resolved Hide resolved
tests/all/component_model/bindgen.rs Show resolved Hide resolved
crates/wit-bindgen/src/lib.rs Outdated Show resolved Hide resolved
crates/wit-bindgen/src/lib.rs Outdated Show resolved Hide resolved
crates/wit-bindgen/src/lib.rs Outdated Show resolved Hide resolved
tests/all/component_model/bindgen.rs Outdated Show resolved Hide resolved
@dicej
Copy link
Contributor Author

dicej commented Mar 7, 2023

@alexcrichton Thanks for all your feedback. I think I've addressed everything so far.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good to me!

(approving but we'll naturally want to wait for the wasm-tools suite of crates to be published first before actually merging)

Per WebAssembly/component-model#172, this implements "part 1" of WIT
templates, allowing WIT files to define interfaces which contain a
single wildcard function, which worlds may import or export.

I've taken a "minimalist" approach to this, such that the host binding
generator just skips wildcards entirely, leaving it to the host to use
dynamic APIs to reflect on the component and do the necessary
func_wrap (for imports) and typed_func (for exports) calls directly.

This adds two new functions to the public API:

- `Component::names`: provides an iterator over the names of the
  functions imported by the specified instance.
- `ExportInstance::funcs`: provides an iterator over the names of the
  functions exported by this instance.

In both cases, I'm open to alternative API designs for getting that
information.

Note that I've added a temporary dependency override to Cargo.toml,
pointing to our fork of wasm-tools, which includes the necessary
wit-parser changes. We're still iterating on that and will PR those
changes separately. We also have a fork of wit-bindgen with a new
"wildcards" test to verify everything works end-to-end:
bytecodealliance/wit-bindgen@main...dicej:wit-templates. I'll
PR that last once everything else is stable.

Signed-off-by: Joel Dice <[email protected]>

generate code for wildcards in `bindgen!`

This improves upon the raw `func_wrap` and `typed_func` APIs by
providing static type checking.

Signed-off-by: Joel Dice <[email protected]>

address review feedback

- Add TODO comment to `Component::function_import_names` (renamed from `names`)
- Add `Component::function_export_names` for symmetry (since an embedder may want both sets of names)
- Add a couple of codegen tests for the bindgen macro
- Use `Func` instead of `TypedFunc` for `__wildcard_matches` to avoid lifetime issues
- Update `wit-parser` and use the new `Interface::wildcard` field where applicable
- Add `name: &str` parameter to `WildcardMatch::call` trait function

Signed-off-by: Joel Dice <[email protected]>
@dicej dicej force-pushed the wit-templates-minimal branch from 9e8616d to 8ff06a7 Compare March 22, 2023 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants