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

merge: [email protected] #617

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Jan 14, 2025

sunfishcode and others added 14 commits December 6, 2024 15:41
wit-bindgen already knows to special-case passing strings by reference
and pass them as `&str` instead of naively passing them as `&String` and
requiring callers to have owned `Strings`. Implement this behavior for
type aliases of strings as well, so that in code like this:
```wit
type my-string = string;
foo: func(x: my-string);
```
the argument is `&str` instead of `&MyString` which is effectively
`&String`. And similar for lists.

This comes up in several functions in wasi-http; for example, in the
bindings for the `Fields::append` function, it enables this change:

```diff
@@ -5075,8 +5075,8 @@ pub mod wasi {
                 /// `field-value` are syntactically invalid.
                 pub fn append(
                     &self,
-                    name: &FieldName,
-                    value: &FieldValue,
+                    name: &str,
+                    value: &[u8],
                 ) -> Result<(), HeaderError> {
                     unsafe {
                         #[repr(align(1))]
```

where `FieldName` and `FieldValue` are defined as:
```wit
            pub type FieldKey = _rt::String;
            pub type FieldName = FieldKey;
            pub type FieldValue = _rt::Vec<u8>;
```
Signed-off-by: James Sturtevant <[email protected]>
In the C bindings, prefix exported function names with `exports_` so
that a world that imports and exports the same function can do so
without collisions.

For example, with this WIT:

```wit
package green:blue;

world my-world {
  import purple: func();
  export purple: func();
}
```wit

This fixes a wit-bindgen panic with:

```
duplicate symbols: "name `my_world_purple` already defined"
```
* Rename generators for clarity

Signed-off-by: James Sturtevant <[email protected]>

* Move interface generator to own module

Signed-off-by: James Sturtevant <[email protected]>

* Move function generator to own module

Signed-off-by: James Sturtevant <[email protected]>

* Move Ident code to own file

Signed-off-by: James Sturtevant <[email protected]>

* Move World generator code to own file

Signed-off-by: James Sturtevant <[email protected]>

* Add some comments and alittle clean up

Signed-off-by: James Sturtevant <[email protected]>

---------

Signed-off-by: James Sturtevant <[email protected]>
* Minor clean up to files

* use static list instead of regenerating each time

* Clean up

Signed-off-by: James Sturtevant <[email protected]>

---------

Signed-off-by: James Sturtevant <[email protected]>
* Fix mismatched tuple ABIs in Rust

This fixes the mistaken assumption that the tuple ABI in Rust is the
same as the component model ABI and generates different code for
lifting/lowering lists.

Closes #1112

* Attempt a java test

* Add a go test

* Fix java test

* Attempt to write C#
* Add support for async/streams/futures to Rust generator

This adds support for generating bindings which use the [Async
ABI](https://github.com/WebAssembly/component-model/blob/main/design/mvp/Async.md)
along with the [`stream`, `future`, and
`error-context`](WebAssembly/component-model#405) types.

By default, normal synchronous bindings are generated, but the user may opt-in
to async bindings for all or some of the imported and/or exported functions in
the target world and interfaces -- provided the default-enabled `async` feature
is enabled.

In addition, we generate `StreamPayload` and/or `FuturePayload` trait
implementations for any types appearing as the `T` in `stream<T>` or `future<T>`
in the WIT files, respectively.  That enables user code to call `new_stream` or
`new_future` to create `stream`s or `future`s with those payload types, then
write to them, read from them, and/or pass the readable end as a parameter to a
component import or return value of a component export.

Note that I've added new `core::abi::Instruction` enum variants to handle async
lifting and lowering, but they're currently tailored to the Rust generator and
will probably change somewhat as we add support for other languages.

This does not include any new tests; I'll add those in a follow-up commit.

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

add `async: true` case to Rust `codegen_tests`

This ensures that all the codegen test WIT files produce compile-able bindings
with `async: true` (i.e. all imports lowered and all exports lifted using the
async ABI).  That revealed some issues involving resource methods and
constructors, as well as missing stub support, which I've resolved.

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

add codegen tests for futures, streams, and error-contexts

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

remove async_support::poll_future

It was both unsafe to use and intended only for testing (and not even good for
that, it turns out).

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

add stream/future read/write cancellation support

Also, fix some issues with stream/future payload lifting/lowering which I
_thought_ I had already tested but actually hadn't.

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

support callback-less (AKA stackful) async lifts

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

revert incorrect test change in flavorful/wasm.rs

I had thoughtlessly removed test code based on a clippy warning, not realizing
it was testing (at compile time) that the generated types implemented `Debug`.

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

test `async: true` option in Rust codegen tests

I had meant to do this originally, but apparently forgot to actually use the
option.

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

add docs for new `debug` and `async` Rust macro options

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

address `cargo check` lifetime warning

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

minimize use of features based on PR feedback

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

* add comment explaining lack of recursion in `type_id_info`

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

* refactor stream and future support for Rust generator

Per a discussion with Alex, this moves most of the stream and future code into
the `wit-bindgen-rt` crate.  That's where I had intended to put it to begin
with, but ran into orphan rule issues given the trait-based approach I was
using.

The new approach uses dynamic dispatch via a vtable type.  Thus we've traded a
small (theoretical) amount of performance for much better compatibility in cases
of separately-generated bindings (e.g. passing `FutureWriter<T>` between crates
should work fine now), easier debugging, etc.

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

* break `streams_and_futures` into two modules

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

* restore previously-removed feature check in Rust macro

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

---------

Signed-off-by: Joel Dice <[email protected]>
* Update wasm-tools crates

Bringing then up-to-date with latest

* Fix compile of tests
- The generated lift/lower code for stream/future payloads was not always calculating module paths correctly when generating type names.
- Also, we were moving raw pointers into `async move` blocks and returning them without capturing the pointed-to memory.  This would have been caught by runtime tests, but we don't have those yet since the Wasmtime async PR hasn't been merged yet.  Fortunately, it was easy enough to find and fix when I updated that PR to use the latest wit-bindgen.
- The generated lift/lower code for reading and writing streams needs to return a `Box<dyn Future>` that captures the lifetimes of the parameters.

Signed-off-by: Joel Dice <[email protected]>
This commit fixes compatibility of generated code with the Rust 2024
edition coming up next month. The new edition requires that
`#[export_name]` is notably an `unsafe` attribute and is wrapped in
`#[unsafe(export_name = "...")]` so the generated code is updated to do
that. Note that this syntax is allowed on the 2021 edition as of Rust
1.82.0 but is not required, it's only required in the 2024 edition.
* enable stream/future payload lift/lower for non-Wasm platforms

Previously, we generated no code for non-Wasm platforms; which meant our codegen
tests weren't really testing much as far as streams and futures go.  Now that
the tests actually do something, they uncovered a few issues which I've fixed:

- Invalid code generation when using `duplicate_if_necessary: true`
- Invalid code generation for stream or futures whose payloads contain one or more a streams or futures

For the latter, I mimicked what we do for resources: use interior mutability to
provide `take_handle` methods for `StreamReader` and `FutureReader`.

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

* avoid lowering same values more than once in `StreamVtable::write`

Previously, we would optimistically lower all the values in the input array and
then re-lower the subset which wasn't accepted the first time.  Aside from being
inefficient, that was also incorrect since re-lowering would fail in the cases
of any resource handles, futures, or streams in the payload since we would have
already taken the handles using `take_handle`.

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

---------

Signed-off-by: Joel Dice <[email protected]>
[automatically-tag-and-release-this-commit]

Co-authored-by: Auto Release Process <[email protected]>
@rvolosatovs rvolosatovs force-pushed the merge/wit-bindgen-0-37 branch 3 times, most recently from 904db64 to 82b3472 Compare January 14, 2025 15:31
@rvolosatovs rvolosatovs force-pushed the merge/wit-bindgen-0-37 branch from 82b3472 to 579c3b2 Compare January 14, 2025 15:56
@rvolosatovs
Copy link
Member Author

@rvolosatovs rvolosatovs marked this pull request as draft January 17, 2025 13:26
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.

5 participants