Refactor wasmtime::Func to "unsplat" arguments like in #5065

[pchickey]

Feature

PR #5065 got rid of a trait that "splatted" a type-level tuple of arguments into individual arguments to a closure. @alexcrichton and I feel this is a design choice which isn't worth its weight, and advocate for getting rid of the equivalent functionality for core wasm wasmtime::Funcs.

Benefit

The current Func::wrap signature is hard to read, and the set of functions Func::wrap{n}_async is just unsightly. This should bring us down to a much simpler Func::wrap and a single Func::wrap_async constructor.

Unfortunately, the benefits are tempered by this being a sorta annoying breaking change for downstream users. It won't break anyones use case, but all downstream users of Func::wrap will have to make some small syntactic adjustments to their code for it to build.

Implementation

You'll need to make the changes in wasmtime, then chase down the consequences in wiggle, the test suite, and any other places in the repository that invoke Func::wrap.

We haven't dug deep into the exact way this will be implemented, but it should end up looking like #5065 if you squint. If that approach doesn't look like it will work, lets discuss why here and figure out an alternative.

[jwcesign]

Can I take this? /assign

[pchickey]

Yes, please assign me as the reviewer for any PRs.

[ssnover]

@pchickey Is this issue still valid or is the API of wasmtime intended to be more static at this point? I have something working in the wasmtime crate, but it causes a lot of breaking API changes that look like this:

Before:

linker.func_wrap(
    "spectest",
    "print_f64_f64",
    move |f1: f64, f2: f64| {
        if !suppress {
            println!("{}: f64", f1);
            println!("{}: f64", f2);
        }
    },
)?;

After:

linker.func_wrap(
    "spectest",
    "print_f64_f64",
    move |_, (f1, f2): (f64, f64)| {
        if !suppress {
            println!("{}: f64", f1);
            println!("{}: f64", f2);
        }
    },
)?;

Before I go through and modify examples, tests, and implementations of proc-macros like from_witx, I wanted to check that this refactor to the arguments is still desired.

[pchickey]

Honestly, I’m glad you asked because I’m less convinced than I was 18 months ago when I filed the issue. Getting rid of all of the trait stuff that lets the faux-variadic functions work is a nice cleanup for wasmtime, but it seems like a needless breaking change for our users, who haven’t complained about it ever afaik, and would be rightfully annoyed if it changed with no benefit to them. I’m glad we changed how it worked in wasmtime::component before it was stable enough to use, but I now feel like changing one of the oldest parts of the wasmtime crates API ought to have a bigger payoff than this does.

@alexcrichton what do you think?

[alexcrichton]

I think that's a good point yeah that it's probably not worth breaking lots of code today. That being said there might still be something to be done here perhaps. Depending on how all the trait impls and such work out I think this would still be a great internal refactoring to have. For example implementing a trait-per-tuple and have the "meat" be a simple F, P, R generics type parameters would avoid having tons of code in macros as-is today.

Ideally, if the traits work out in terms of coherence and overlapping, it'd be great to support both today's version of Func::wrap in addition to a "tupled approach", basically enabling both to work. I think it would be great to remove all the func_wrapN_async methods in favor of a single wrap_async. That'd break consumers calling wrapN_async but I think that's much rarer (outside of bindings generators) than the blocking versions.

[ssnover]

Sounds good. I can back out some of the changes very easily in the public API. I'll go through some of the existing examples and tests and swap some of the calls to the new method so there is coverage on both. Should code generation in from_witx migrate to the new method or remain on the existing one?

For the new method on on Func do you all have any preferences? Is Func::wrap_tupled too on the nose?

[alexcrichton]

I think it's reasonable to change from_witx yeah, but could the tuple-based impls also go through Func::wrap? That's got some weird trait bounds and everything is pretty carefully aligned there so I'm not sure if it'd work, but I'm not sure it's worth adding a new *_tupled method per se

[ssnover]

Yeah, based on my tinkering that's totally possible; I think that means there won't be any changes required for sync APIs generated by from_witx, just async in order to do away with the *N_async methods.

[alexcrichton]

Sounds great to me 👍, thanks again for helping to push on this!

[ssnover]

I've been doing more investigation here and I can't figure out a way to make the two styles of parameters work together nicely. I either get down a path that requires a lot of code duplication or I run into conflicting trait implementations in IntoFunc.

[alexcrichton]

Want to open a PR with your work-in-progress or want to share a link here? Might be able to try to help sort this out and/or confirm it may not be possible

[ssnover]

I actually managed to get something that seemed reasonable after some time away from the code, PR above ^

[alexcrichton]

I believe that the work in https://github.com/bytecodealliance/wasmtime/pull/8732 effectively completes this, so I'm going to close it. Thanks again @ssnover!