implement support for resources for C and Rust generators #604

dicej · 2023-07-05T15:55:30Z

This also provides stubs for the Java, Go, and Markdown generators.

In addition to resource support, I've changed how the Rust guest generator
handles exports per a conversation with Alex. Instead of generating polymorphic
functions plus an export_{name}! to monomorphize them, we now require trait
implementations to be specified as parameters to the wit_bindgen::generate!
macro. This allows us to avoid complicated use of generics when exporting
resources.

For example:

// in foo.wit:
world foo {
    export a: func()
    export b: interface {
        c: func()
        resource d {
            e: func()
        }
    }
}

wit_bindgen::generate!({
    path: "foo.wit",
    world_exports: MyFoo,
    interface_exports: {
         "b": MyB
    }
    resource_exports: {
        "b::d": MyD
    }
});

struct MyFoo;

impl Foo for MyFoo {
    fn a() { ... }
}

struct MyB;

impl exports::b::B for MyB {
    fn c() { ... }
}

struct MyD;

impl exports::b::D for MyD {
    fn e(&self) { ... }
}

One side-effect of the above change is that the generator needs to create stub
implementations to make the codegen tests work, plus we have to be careful to
avoid name conflicts among top-level exports (hence renaming the top level
exports of lift-lower-foreign.wit to avoid clashes with issue573.wit).

Note that there are no runtime tests yet since the Wasmtime implementation of
resources is not complete as of this writing.

peterhuene · 2023-07-06T01:01:26Z

cargo-component is reliant on the generated export! macro to allow bindings to be generated into a separate crate from the component implementation; it does this because bindings generation is performed ahead-of-time by cargo-component itself from resolved (e.g. from a registry) dependencies specified in Cargo.toml.

As a result, there's no generate! macro in user code for users to supply a mapping between exported things and user-supplied type names; with today's generated bindings, the only such mapping is from a type name specified to the generated export! macro to map the ABI exports to user implementation.

This will upend the cargo-component DX, so I think this will need some careful design consideration. I'm completely open to redesigning how cargo-component does bindings, but only if we can maintain a "cargo-like" experience for dependencies (both for targeting a specific world and dependencies on other components).

dicej · 2023-07-06T13:36:20Z

Thanks for the feedback, @peterhuene. I'll see if I can bring back the export macro.

alexcrichton · 2023-07-06T16:56:29Z

Ah my bad, I forgot about that use case when I first suggested the Rust macro changes!

Brainstorming around that though, what if cargo-component generated the macro? For example cargo-component could generate:

macro_rules! export_foo {
    ($i:ident) => ($crate::wit_bindgen::generate!({
        world: "filled in by cargo-component",
        path: "filled in by cargo-component",
        world_exports: $i,
    }));
}

or something along those lines, more-or-less where the user only provides their own "stuff" which is mixed with cargo-component's "stuff" which handles all the WIT files and where they are and whatnot.

peterhuene · 2023-07-06T18:06:26Z

cargo-component does some (slightly) complicated merging of Resolves to generate bindings, as it:

supports targeting a world via a reference to a binary WIT package in a registry (e.g. wasi:http/proxy@1.0.0) or local file
supports targeting a world via a local WIT package in text format, but resolving foreign dependencies via local or registry references (i.e. it doesn't use a deps folder).
merges in imports to the target world for dependencies on other components (via local binary components or component packages from a registry).

So it probably doesn't make sense to wrap the generate! macro or move what it's doing into the generate! macro itself.

However, I think this can be solved with a proc macro that's specific to cargo-component which builds the Resolve based on whatever cargo-component resolved dependencies to. That way we keep the dependency resolution out of the proc macro (and thus the network access out of rustc).

@dicej let's move forward on your implementation as is and I will start to implement a proc macro replacement for the bindings crate in cargo-component.

dicej · 2023-07-06T18:15:59Z

Sorry for the communication lag -- I'm just finishing up a rewrite that brings back the export macro. It has the virtue of touching less code, and I'm hoping it helps address the Windows CI failure, so I'm inclined to follow through on it.

alexcrichton · 2023-07-06T18:17:23Z

I do think it would be good though to avoid duplicating too much, and one thing I've wanted to do for the longest time is to support wasm-encoded WIT packages to the macro, and perhaps that would work here? cargo-component would emit a wasm-encoded Resolve with all the necessary information and that'd then get decoded in the proc-macro invocation? That way cargo-component would prepare everything and then users would only need to fill in Rust-specific options to wit-bindgen like what they called various structs, what to assign for resource types, etc.

alexcrichton · 2023-07-06T18:18:11Z

Ah ok, if the export macro sticks around I'll take a look at that when it's ready.

dicej · 2023-07-06T18:44:31Z

I've pushed the update. Here's an example of how to use the macro with resources: https://github.com/dicej/wit-bindgen/blob/56a36d79be8aaed9dbd1bf965497fdc0fa442b4b/crates/rust/tests/codegen.rs#L230-L269

alexcrichton · 2023-07-06T19:25:32Z

I think I may be missing something, but I'm also not following how this works with exported resource types? Given this input:

package my:resources


interface e1 {
  resource x

  record foo { x: x }

  a: func(f: foo) -> list<x>
}

interface e2 {
  use e1.{x, foo as bar}

  record foo { x: x }

  a: func(f: foo, g: bar) -> list<x>
}

world resources {
  export e1
  export e2
}

I think the output Rust code doesn't compile, and I'm seeing things like:

There's lots of references to OwnX but it's not defined anywhere
The type parameter of <Self> seems odd as it shows up as:

                pub struct Foo {
                    pub x: OwnX<Self>,
                }
// ...
                pub trait E1 {
                    fn a(f: Foo) -> wit_bindgen::rt::vec::Vec<OwnX<Self>>;
                }
// ...
                pub struct Foo {
                    pub x: OwnX<Self>,
                }

So I think I'm basically lost in how a user-defined type gets wired up into the bindings generated for these types (e.g. some MyX I define myself). I'm probably naive, but I'm assuming that at generation time the macro needs to understand what Rust type an exported resource type maps to which is what we were providing as input earlier. Projections from a trait I thought we concluded won't work because when the module is generated with type information it doesn't know what to project out of. We could plumb a generic everywhere but that's where I was thinking it's not worth it when everything is concrete anyway

dicej · 2023-07-06T20:04:11Z

I think I just haven't been imaginative enough to consider all the ways resources can be used, especially across interfaces. I'll study your example and see if I can make it work properly. BTW, I suspected just appending <Self> was not going to work in general, and you confirmed it.

dicej · 2023-07-06T20:55:57Z

So all we need to do here is patch Rust to support polymorphic modules. Problem solved!

More seriously: I finally understand what @alexcrichton means about how pervasive the generics would need to be to support the export macro. Sorry for the churn, @peterhuene, but is it okay if we go back to the original (macro-less) implementation?

peterhuene · 2023-07-06T20:56:59Z

Yep, totally fine. I'll take care of things on the cargo-component side.

dicej · 2023-07-06T22:13:40Z

Even with the macro-less implementation, I need to do some work to get resource aliases working per Alex's latest example. I'll push an update when that's ready.

alexcrichton · 2023-07-07T18:45:11Z

We were talking a bit earlier about this, but here's some codegen tests which I think are failing currently:

world foo {
  resource x

  export foo: func() -> x
}

interface x {
  resource x

  foo: func() -> x
}

world foo {
  export x // or `import`
}

alexcrichton · 2023-07-07T19:46:19Z

To clarify from earlier though, I definitely don't want to create undue work for cargo-component and/or you @peterhuene, so I want to confirm that you think that's still the case? For example I'd imagine that ideally cargo component-generated macros would be relatively thin layers around the underlying wit-bindgen stuff to ensure that docs and such could be shared. If cargo component otherwise is generating lots of unique stuff then it seems like we should redesign wit-bindgen to offload the work from there.

dicej · 2023-07-07T19:53:26Z

@alexcrichton regarding your second WIT example above: it seems wit-bindgen already has trouble when you name a type the same as the interface it is contained in -- the generated trait ends up having the same name as the type, which causes a compiler error. I.e. if you change the resource to a record in your example, it still blows up.

Any opinions on how to differentiate the names to avoid conflicts? One option would be to define the trait one level up in the module hierarchy and refer to it as super::{name}. I don't think just adding a prefix or suffix to the interface name would be robust, since it would still conflict if the WIT file also happened to use that prefix or suffix in a type name.

alexcrichton · 2023-07-07T19:56:07Z

Ah true! In that case I'll file an issue and that can be handled separately.

I'd agree though that the best solution is probably to lift it up one level.

peterhuene · 2023-07-07T20:20:14Z

@alexcrichton I don't think the cargo-component changes will be that bad.

I envision it implemented with:

Removal of the existing bindings crate generation entirely.
Instead, cargo-component will encode the built-up Resolve previously used for generating bindings to a known location in the target directory; this happens only when the output file is missing or resolved dependencies change.
Implement a cargo-component proc-macro that is, effectively, a small shim around generate! that locates the encoded Resolve and feeds it into the code generation machinery; if the encoded Resolve can't be found, it generates a compilation error instructing users to build with cargo component build and maybe cargo-component installation instructions (an improvement over today's UX when accidentally building with cargo build).

Today, a component looks like this for cargo-component:

use bindings::{Example, export};

struct Component;

impl Example for Component {
    // ...
}

export!(Component);

and after these changes:

struct Component;

impl Example for Component {
    // ...
}

// Crate/name of macro TBD
cargo_component_bindings::generate!(Component);

Where it is expecting all exported interfaces to be impl on the same type (unless there's a compelling reason for having multiple implementation types for the interface exports?).

I'd imagine it to use a similar syntax for specifying the resource types:

cargo_component_bindings::generate!({
    implementor: Component,
    resources: {
        a::b: MyB,
        // ...
    }
});

Maybe the macro could have some default expected type names, like Component for the world/interface exports impl type and a naming convention for resource types (assuming no conflicting names); thus enabling a simple cargo_component_bindings::generate!() in the default project template. At any rate, such a DX improvement can come later.

alexcrichton · 2023-07-07T20:31:59Z

Ok cool that all sounds great to me and I feel better confirming that this won't require cargo-component to reimplement parsing or anything like that.

The DX bits around generate!() vs implementor: Foo vs implementors: [Foo, Bar, Baz] or something like that I think can be a responsibility of wit-bindgen rather than cargo-component as well if you'd like to offload that, and I like the idea of making it better here for sure.

dicej · 2023-07-07T21:51:05Z

FYI, I'm nearing the end of my day and will be out on vacation all next week, but I'm planning to add more test cases when I return on the 17th.

cpetig · 2023-07-09T20:00:48Z

While the Rust code looks really nice the generated C code for resource-drop-own and -borrow seems to have a wrong module name (own uses C style with underscores and borrow seems to use the world name).

Update: cpetig@18ca99c contains a potential fix.

crates/rust-macro/src/lib.rs

dicej · 2023-07-19T14:20:32Z

I think I've addressed all the feedback so far, so I've marked this "ready for review". @peterhuene I'm happy to make any additional macro changes you think are appropriate.

alexcrichton

Thanks again for helping to push this all forward! I've left some comments mainly on the C generator for now and from my read the Rust generator is similarly shaped so they're probably similarly applicable there, but curious what you think about those before diving much more into the Rust generator

tests/codegen/lift-lower-foreign.wit

src/bin/wit-bindgen.rs

crates/rust/tests/codegen.rs

crates/rust-macro/src/lib.rs

crates/c/src/lib.rs

crates/rust/src/lib.rs

alexcrichton · 2023-07-19T17:12:13Z

crates/rust/src/lib.rs

+                    self.src,
+                    r#"
+                        #[derive(Debug)]
+                        pub struct {camel} {{


This is sort of similar to the C generator I think, but I'd ideally expect this to be present during the type declaration of the resource type itself. Will that not work out though?

Note that we need to generate the trait for the resource, which requires looping over the functions (constructor, methods, static functions) in the resource, and import_interface is where we have access to those, not type_resource, AFAICT. Am I missing something?

Ah sorry yeah those bits will need to happen at the end, but type declarations I'd hope could happen during the type_foo functions

I could certainly do that. The other reason I did it this way was to keep the type declaration for each resource close to its trait declaration, but I guess it would be nice to have all the types declared together.

Yeah it's not critical really, although I think it's more important for C where items must be lexically "sorted" rather than the Rust style "dear rustc please figure it out"

It already dawned on me that lexical sorting might be the only reasonable order for C++ to define types from more complex wit structures (I need to generate both host and guest side).

The combination of a method receiving/returning a (typedef to a) record and a record containing a typedef and a resource is quite common, so I didn't see any natural ordering for these header parts.

Hm I'm not sure I quite understand, although @cpetig it sounds like you're saying that some patterns are going to be difficult to implement in C++? Could you sketch out a *.wit and hypothetical header with bindings "generated" to help me better understand the issue?

Sorry, I tried to support your assessment that lexical sorting will be needed by a C or C++ code generator and might have replied a bit too terse. I am currently developing above mentioned C++ generators in https://github.com/cpetig/wit-bindgen/tree/resources/autosar and that directory contains two wit files which show this problem:

ErrorDomain (resource) depends on ErrorCodeType (typedef), ErrorCode (record) depends on the first resource and is aliased a lot (typedef), e.g. any of the future-X resources depend on the ErrorCode alias in radar.

[I am perfectly aware that these generators need re-work as I created them as I was exploring the code base]

Cargo.toml

dicej · 2023-07-26T13:47:57Z

I realized last night that the C binding generator isn't generating separate types when importing and exporting the same interface containing a resource, unlike the Rust generator. It only generates the import version, not the export one.

One way to address that is to prefix all export names with export_, analogous to how the Rust generator puts them all in an exports module. Does that sound reasonable?

@cpetig

This also provides stubs for the Java, Go, and Markdown generators. In addition to resource support, I've changed how the Rust guest generator handles exports per a conversation with Alex. Instead of generating polymorphic functions plus an `export_{name}!` to monomorphize them, we now require trait implementations to be specified as parameters to the `wit_bindgen::generate!` macro. This allows us to avoid complicated use of generics when exporting resources. For example: ``` // in foo.wit: world foo { export a: func() export b: interface { c: func() resource d { e: func() } } } ``` ``` wit_bindgen::generate!({ path: "foo.wit", world_exports: MyFoo, interface_exports: { "b": MyB } resource_exports: { "b::d": MyD } }); struct MyFoo; impl Foo for MyFoo { fn a() { ... } } struct MyB; impl exports::b::B for MyB { fn c() { ... } } struct MyD; impl exports::b::D for MyD { fn e(&self) { ... } } ``` One side-effect of the above change is that the generator needs to create stub implementations to make the codegen tests work, plus we have to be careful to avoid name conflicts among top-level exports (hence renaming the top level exports of lift-lower-foreign.wit to avoid clashes with issue573.wit). Note that there are no runtime tests yet since the Wasmtime implementation of resources is not complete as of this writing. Signed-off-by: Joel Dice <joel.dice@fermyon.com> replace hard-coded interface names with values from WIT Signed-off-by: Joel Dice <joel.dice@fermyon.com> add resource alias test and make it work Signed-off-by: Joel Dice <joel.dice@fermyon.com> update dependencies Signed-off-by: Joel Dice <joel.dice@fermyon.com> replace imports with panicking stubs on non-Wasm targets This should address the Windows CI failures. Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix handling of resources imported at world level Signed-off-by: Joel Dice <joel.dice@fermyon.com> add missing cfg attribute Signed-off-by: Joel Dice <joel.dice@fermyon.com> parse export types as `syn::Path` This allows them to refer to types external to the current module. Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix import module names in C generator Thanks to @cpetig for the patch: cpetig@18ca99c Signed-off-by: Joel Dice <joel.dice@fermyon.com> add more resources tests; fix issues found Signed-off-by: Joel Dice <joel.dice@fermyon.com> report missing export parameters gracefully The Rust generator requires parameters when exporting functions or interfaces, so `export_func` and `export_interface` now return a `Result<()>`. Previously, it panicked, which made for an alarming user experience. Signed-off-by: Joel Dice <joel.dice@fermyon.com> change macro syntax per review feedback Signed-off-by: Joel Dice <joel.dice@fermyon.com> improve `--exports` docs and error reporting Signed-off-by: Joel Dice <joel.dice@fermyon.com> fix resource alias handling in C generator Signed-off-by: Joel Dice <joel.dice@fermyon.com>

alexcrichton · 2023-07-26T14:44:13Z

Yeah I think that'd work well 👍

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

alexcrichton · 2023-07-26T18:35:28Z

This is all looking good to me, thanks again for working on this @dicej!

I'm happy to merge whenever at this point basically and while it's probably not bug-free wit-bindgen was not bug-free beforehand anyway so I think it'd be good to start plumbing this change around and it's ok to have follow-up bug fixes

dicej · 2023-07-26T18:41:25Z

@alexcrichton sounds good. I'm working on making the C generator generate distinct types for resources which are both imported and exported since the current behavior of completely ignoring the export is not acceptable.

The only other thing on my to-do list is to eliminate the dealias function as you suggested, but I can open an issue to tackle that later if that sounds okay to you.

alexcrichton · 2023-07-26T18:45:59Z

Yeah I think that's ok to do as a follow-up as it'll change the bindings a bit at most I think

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

dicej · 2023-07-26T19:31:54Z

Alex and I had a quick out-of-band conversation about importing and exporting the same resource in the C generator, and we decided to punt on it for now since it will require a significant refactor. For the time being, I've added a todo! and a comment so it will at least fail loudly and informatively.

With that, I think it's ready to merge from my perspective. I'll open issues for the above and the dealias thing.

alexcrichton · 2023-07-26T19:56:06Z

👍

I had meant to include this in bytecodealliance#604 but forgot to `git add` it. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

I had meant to include this in #604 but forgot to `git add` it. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

dicej force-pushed the resources branch 2 times, most recently from 81233ac to a8400d6 Compare July 5, 2023 17:30

dicej force-pushed the resources branch from 2224683 to 888109f Compare July 6, 2023 18:22

dicej force-pushed the resources branch from 56a36d7 to c81935a Compare July 6, 2023 22:53

Mossaka mentioned this pull request Jul 12, 2023

Go: implement resource types #615

Closed

peterhuene mentioned this pull request Jul 13, 2023

Notice: upcoming breaking change bytecodealliance/cargo-component#97

Closed

peterhuene reviewed Jul 13, 2023

View reviewed changes

crates/rust-macro/src/lib.rs Outdated Show resolved Hide resolved

peterhuene reviewed Jul 13, 2023

View reviewed changes

crates/rust-macro/src/lib.rs Outdated Show resolved Hide resolved

cpetig mentioned this pull request Jul 13, 2023

wit-parser: todo!() in Generator::call for option<resource> bytecodealliance/wasm-tools#1120

Closed

dicej force-pushed the resources branch 2 times, most recently from 813a9fc to f452d89 Compare July 18, 2023 15:54

dicej marked this pull request as ready for review July 19, 2023 14:19

alexcrichton reviewed Jul 19, 2023

View reviewed changes

cpetig reviewed Jul 22, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

fix aliases of exported resources in Rust generator

4be372f

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

dicej force-pushed the resources branch from 1cf832f to 4be372f Compare July 26, 2023 15:36

add lifetime generic for structural types with borrow fields

0afb7c7

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

indicate importing and exporting same resource not yet supported for C

bd8de71

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

This was referenced Jul 26, 2023

Support importing and exporting the same resource in the C generator #629

Closed

Remove dealias function #630

Closed

alexcrichton merged commit 62f3f64 into bytecodealliance:main Jul 26, 2023
9 checks passed

dicej added a commit to dicej/wit-bindgen that referenced this pull request Jul 26, 2023

add resource-borrow-in-record-export codegen test

026d313

I had meant to include this in bytecodealliance#604 but forgot to `git add` it. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

dicej mentioned this pull request Jul 26, 2023

add resource-borrow-in-record-export codegen test #631

Merged

alexcrichton pushed a commit that referenced this pull request Jul 26, 2023

add resource-borrow-in-record-export codegen test (#631)

c94c8bb

I had meant to include this in #604 but forgot to `git add` it. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

juntyr mentioned this pull request Jul 29, 2023

Issues with resources split across the WASM boundary #634

Closed

cpetig mentioned this pull request Aug 6, 2023

Rust: exporting a resource results in exports:: prefix in exported name #635

Closed

LLFourn mentioned this pull request Aug 22, 2023

Example in readme does not work #649

Closed

implement support for resources for C and Rust generators #604

implement support for resources for C and Rust generators #604

Conversation

dicej commented Jul 5, 2023 • edited

peterhuene commented Jul 6, 2023

dicej commented Jul 6, 2023

alexcrichton commented Jul 6, 2023

peterhuene commented Jul 6, 2023

dicej commented Jul 6, 2023

alexcrichton commented Jul 6, 2023

alexcrichton commented Jul 6, 2023

dicej commented Jul 6, 2023

alexcrichton commented Jul 6, 2023

dicej commented Jul 6, 2023

dicej commented Jul 6, 2023 • edited

peterhuene commented Jul 6, 2023

dicej commented Jul 6, 2023

alexcrichton commented Jul 7, 2023

alexcrichton commented Jul 7, 2023

dicej commented Jul 7, 2023

alexcrichton commented Jul 7, 2023

peterhuene commented Jul 7, 2023 • edited

alexcrichton commented Jul 7, 2023

dicej commented Jul 7, 2023

cpetig commented Jul 9, 2023 • edited

dicej commented Jul 19, 2023

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Jul 19, 2023

Choose a reason for hiding this comment

dicej Jul 19, 2023

Choose a reason for hiding this comment

alexcrichton Jul 19, 2023

Choose a reason for hiding this comment

dicej Jul 19, 2023

Choose a reason for hiding this comment

alexcrichton Jul 19, 2023

Choose a reason for hiding this comment

cpetig Jul 20, 2023 • edited

Choose a reason for hiding this comment

alexcrichton Jul 20, 2023

Choose a reason for hiding this comment

cpetig Jul 20, 2023

Choose a reason for hiding this comment

dicej commented Jul 26, 2023

alexcrichton commented Jul 26, 2023

alexcrichton commented Jul 26, 2023

dicej commented Jul 26, 2023

alexcrichton commented Jul 26, 2023

dicej commented Jul 26, 2023

alexcrichton commented Jul 26, 2023

dicej commented Jul 5, 2023 •

edited

dicej commented Jul 6, 2023 •

edited

peterhuene commented Jul 7, 2023 •

edited

cpetig commented Jul 9, 2023 •

edited

cpetig Jul 20, 2023 •

edited