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

Add a wit-encoder tool #1580

Merged
merged 40 commits into from
Jun 14, 2024
Merged

Add a wit-encoder tool #1580

merged 40 commits into from
Jun 14, 2024

Conversation

MendyBerger
Copy link
Contributor

This PR is a collaboration between me and @yoshuawuyts.

Adds a new tool to allow building up wit with code.

It's not perfect, but I think that it's better than writing wit by hand.

@yoshuawuyts
Copy link
Member

To add some additional context here: this PR is a continuation of #1549. Both Mendy and I found that independently of each other we had been encoding WIT documents by hand. And it seemed useful to have a WIT counterpart to the wasm-encoder crate. Because correctly encoding rich, structured formats by hand is hard - and so this crate aims to make that a little easier. Because of that, it also handles some of the trickier bits of WIT, like automatically escaping reserved keywords.

Example

Say we wanted to encode the following document:

package mendyberger:encoder-example;

interface error-reporter {}

world %world {
    /// inline interface
    export example: interface {
        /// func docs
        do-nothing: func();
    }
    
    /// scan stuff
    export scan: func() -> list<u8>;
    import error-reporter;
    import print: func(s: string);
}

Using wit-encoder the code for that would look something like this (adapted from this test):

use wit_encoder::{
    Interface,
    Package,
    PackageName,
    StandaloneFunction,
    Type,
    World,
};

let mut package = Package::new(PackageName::new("foo", "functions", None));
package.interface(Interface::new("error-reporter"));
package.world({
    let mut world = World::new("world");
    world.inline_interface_export({
        let mut interface = Interface::new("example");
        interface.docs(Some("inline interface"));
        interface.function({
            let mut func = StandaloneFunction::new("do-nothing");
            func.docs(Some("func docs"));
            func
        });
        interface
    });
    world.function_export({
        let mut func = StandaloneFunction::new("scan");
        func.results(Type::list(Type::U8));
        func.docs(Some("scan stuff"));
        func
    });
    world.named_interface_import("error-reporter");
    world.function_import({
        let mut func = StandaloneFunction::new("print");
        func.params(("s", Type::String));
        func
    });
    world
});

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Some minor nits around naming and docs. But overall this looks really useful - thank you for finishing this up! Being able to use this would very useful for my work on wasm-http-tools. This definitely seems like a big step up from having to hand-code things.

For other folks reviewing this code: checking that the right things have the right names would be especially helpful here I think. I've looked over Mendy's code and it looks largely right to me - but at least my knowledge of WIT is not developed enough that I can validate all the terminology used is correct.


#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize))]
pub struct StandaloneFunction {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this to Function? That matches the term used in wit-encoder too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have StandaloneFunction and ResourceFunc, whereas wit-parser has a single Function type that takes a kind argument.
I made separate types so that the type system enforces where each function kind can be. I.e. that standalone functions are not part of a resource, and method/static/constructor are not without a resource.

(also, should probably rename StandaloneFunction to StandaloneFunc)

Copy link
Contributor Author

@MendyBerger MendyBerger May 31, 2024

Choose a reason for hiding this comment

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

I renamed StandaloneFunction to StandaloneFunc

use std::fmt;

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct RenderOpts {
Copy link
Member

Choose a reason for hiding this comment

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

Some docs explaining what this does would probably be helpful. It is not immediately clear why there are RenderOpts, why we have a Render trait rather than use Display, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Display is implemented for package.
Render/RenderOpts is mostly for internal use, but figured I'll make it public as we might wanna add in some options later.
I can make it private if you think it's confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yoshuawuyts think I should make the render stuff private?

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.

Thanks for continuing to push on this! All looks good to me 👍

Does it make sense to leave in the serde bits here as well given that the serde bits are also present in wit-parser? (e.g. reducing the amount of json formats in play in theory)

@MendyBerger
Copy link
Contributor Author

@alexcrichton are you suggesting that I remove serde from wit-bindgen? Sure, I'm cool with that.

@alexcrichton
Copy link
Member

Oh sorry no to clarify I mean removing the serde bits from this new crate wit-encoder

@MendyBerger
Copy link
Contributor Author

🤦
I meant wit-encoder. No idea why I wrote wit-bindgen. 😅

@MendyBerger
Copy link
Contributor Author

MendyBerger commented May 31, 2024

I removed serde from wit-encoder

@yoshuawuyts yoshuawuyts mentioned this pull request Jun 9, 2024
@MendyBerger MendyBerger requested a review from alexcrichton June 14, 2024 14:27
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.

Thanks again for this!

@alexcrichton alexcrichton added this pull request to the merge queue Jun 14, 2024
Merged via the queue into bytecodealliance:main with commit 999fc16 Jun 14, 2024
27 checks passed
@MendyBerger MendyBerger deleted the wit-encoder branch September 5, 2024 17:45
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.

4 participants