feat: support semver aware exclusions

[vados-cosmonic]

This PR updates jco to support the @since, @after and @unstable annotations as implemented by the upstream Rust tooling.

Currently there are a few issues left to address:

• We likely need an upstream release & alignment of the new code in wit-parser that supports the annotations:
  • bytecodealliance/wasm-tools@01bec9c
  • bytecodealliance/wasm-tools@10d2e21
• Finish implementing the test that was added (and fill out the checks for present/missing impl)

[yoshuawuyts]

@vados-cosmonic incredible; thank you for working on this!

We likely need an upstream release & alignment of the new code in wit-parser that supports the annotations

There should already be a working upstream implementation pulled in. I checked with wasmtime to make sure that a working wasm-tools version was part of the latest release, specifically so that that jco could pull it in. If that's not working somehow, maybe @guybedford will know more?

[vados-cosmonic]

Hey @yoshuawuyts thanks for chiming in, so it turns out I was using the git dep for wit-parser which was my problem!

wasmtime-environ (the last git dep) does need a release (unless I'm understanding incorrectly) -- I'm looking at the commits between main and release-22.0.0 in wasmtime/wasmtime, and in particular the commit I need (that aren't released AFAIK) is:

bytecodealliance/wasmtime@9bdb731

[yoshuawuyts]

Oh, rightt - yeah that's the "semver-aware exports" feature wasmtime was missing and only recently implemented (to my surprise that went faster than expected!). My assumption was (and this might be wrong) that both jco and wasmtime would resolve this issue in separate ways. So all we needed was a new version of the parser in wasm-tools, not a working resolver impl in wasmtime. But if jco uses wasmtime-environ for its resolution, it makes sense that we'd want to reuse it.

I'm not sure what the exact right path here will be. What I do know is that Wasmtime 0.23 will be released in two weeks - which is about a week before we're set to release WASI 0.2.1. So one option might be to have a patch ready which we can merge the moment a new wasmtime version is released, making it part of the jco release a few days later.

That does cut it a little close for my taste, so ideally we could land + test the patch well before then. I don't know what our options beyond that would be? - Floating patches? Pulling a non-release version of wasmtime? Ask wasmtime to pretty please push a patch version of wasmtime-environ with these changes? Implement the resolver logic ourselves on top? I don't know - but I think this is probably a @guybedford question.

[vados-cosmonic]

Ahhh thanks for laying this out -- this helped me understand a lot more of the high level plan -- happy to hear how best to move forward here. IIRC @ricochet is also capable of helping with the upstream release if necessary here.

Right now with the code in this branch wit-parser still doesn't seem to recognize @since, which has me a bit confused. Maybe I'm missing something even after setting Resolve's all_features...

[guybedford]

Getting the tests right seems like the most important first step here - checking the inclusion and exclusion respectively for both imports and exports across the gates. Left some suggestions as to how we might better structure that, and then we just need to ensure we have the correct test coverage.

Note the TS bindgen will also need to be implemented along with this in the ts-bindgen.rs file.

[guybedford]

The feature gating needs to be moved into the bindgen functions for:

• exports in https://github.com/bytecodealliance/jco/blob/main/crates/js-component-bindgen/src/transpile_bindgen.rs#L1740
• imports in https://github.com/bytecodealliance/jco/blob/main/crates/js-component-bindgen/src/transpile_bindgen.rs#L1057

For the TypeScript generation, it looks like you've got per-interface filtering of functions and also the filtering of exports and imports. While all interfaces may still be written, that seems like it should be roughly correct? Would be good to clearly enumerate what cases are missing.

[vados-cosmonic]

So after some discussion on Zulip it looks like the use case of trying to use @since with a version that is later than the one specified in the package is a bug -- there will be an upstream change to the tooling to make this a proper error, but we can ignore it as a test case for now, at least.

[EDIT] - NOTE: one more upstream change is needed here -- in addition to wasmparser erroring when an in-the-future @since is used, it also needs to pass through the actual feature requirement information -- right now stable w/ feature requirements are just let through without checking features.

[EDIT2] - Upstream PR

[vados-cosmonic]

So the upstream PR is still ongoing, but the tests here now reflect at least some of the intended flow.

Since it looks like feature gated items with a version newer than the package itself won't be allowed in the future, we actually don't have to test @since(version = v, feature = f) with a missing feature f, if we end up going with the features-are-just-historical sentiment.

Since wasmtime-environ has merged in changes, I'm updating that, and we can add the test mentioned above in a follow up!

[vados-cosmonic]

Ah looks like there are some other errors due to the updated code -- will look into these.

A bit odd because the primary issue seems to be saying that I missed the MemorySize case:

      Error: thread '<unnamed>' panicked at crates/js-component-bindgen/src/core.rs:557:5:
missed case MemorySize
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
RuntimeError: unreachable

But it looks covered in most of the macros...

[EDIT] - Ahhh wasmtime-environ is using 0.212.0 versions of everything...
Even with the version of wasmtime-environ on main which fixes the VisitOperator warnings errors are still present -- they're real errors.

Looks like many issues are with multiple returns being flagged now, the missing case, and some other stuff.

[guybedford]

This looks great. Can we just add two extra tests here:

1. Replacing package test:feature-gates@0.2.1 in the package with package test:feature-gates@0.2.0, and verifying that b and c are not output, even with features: ['gated'] set (which I think requires the change I've commented on in the code).
2. Running with an explicit feature enabling features: ['fancier-foo'] and verifying it turns on d (currently just features: ['all'] is tested)

It seems to me like these should be relatively straightforward to add, but would be good to verify!

[vados-cosmonic]

Thanks for review @guybedford ! I did #2, but if I'm right about #459 (comment) I don't think I can actually do #1 (or at least it would be a different kind of test)

[guybedford]

Also strictly speaking, according to the interpretation provided for since gates, we surely don't need any semver matching at all if all since gates will always be of versions less than the version we are checking?

That is, if for a package version v, all since gates are guaranteed to have version s where s < v, then we don't need to ever check s < v.

[guybedford]

Okay this is making sense now, and I've resolved the outstanding questions.

So the only capability provided by this PR is the ability to disable features in the bindgen.

Last question then - if a component was generated assuming that a feature is enabled, is it actually valid to bindgen with that same feature disabled? That is, shouldn't the features that the component was built with be something we extract from the component itself as opposed to being an input to just the bindgen?

[vados-cosmonic]

Sorry super late but forgot to properly respond to this:

Also strictly speaking, according to the interpretation provided for since gates, we surely don't need any semver matching at all if all since gates will always be of versions less than the version we are checking?

That is, if for a package version v, all since gates are guaranteed to have version s where s < v, then we don't need to ever check s < v.

This is... definitely true. The version specifier is actually strictly informational since wit-parser would have blown up before hand.

And then

So the only capability provided by this PR is the ability to disable features in the bindgen.

Yeah, basically support ensuring things marked @unstable actually don't show up.

Last question then - if a component was generated assuming that a feature is enabled, is it actually valid to bindgen with that same feature disabled? That is, shouldn't the features that the component was built with be something we extract from the component itself as opposed to being an input to just the bindgen?

Hmnnn that's an interesting question -- I'm defaulting to the user needing to supply the feature(s) on every related tool, but that's obviously not a great answer.

Would you mind laying out a scenario? This is probably also worth discussing (kind of like the packaging implications too...)

[guybedford]

Say a component was built with an imported feature enabled, then that package will be written and authored assuming it can access that feature function or interface import.

To just not then bindgen that import, would result in a Wasm core error that the import is not defined by the bindgen.

This makes me think, perhaps the right interpretation here is that imports should always be based on whatever the component itself uses? Rather than having to explicitly select features through an option? Because it feels like it will be a bad UX for imports if you have to guess what the component author intended, rather than just going with that to start.

Then maybe the feature filtering per this PR is only an exports filtering?

[guybedford]

Of course for jco types we definitely do want type generation to support feature stripping, since this is done without a specific component implementation.

[vados-cosmonic]

Ah yes, you're right -- if we can't control/don't know what the original package was built under, we have to be as permissive with the imports as possible...

Then maybe the feature filtering per this PR is only an exports filtering?

Yeah this would definitely be the simplest way to deal with it for now. I'll expand the tests to use an import to make sure it's not hidden.

Of course for jco types we definitely do want type generation to support feature stripping, since this is done without a specific component implementation.

Ah good point -- will try to make sure this works properly as well. I might add a new test or two in test/cli.js for this.

[vados-cosmonic]

@guybedford so I think this might not be a worry - I am almost done adding a bunch of a bunch of functionality to gate whether we enforce stability on imports or not, and realized that it just might not make sense:

• If a gate is marked @unstable in the upstream, we just don't get the imports in the first place (wasmparser does it's job and hides them from us)
• When we perform transpilation, we do not get feature information from the component -- we lose the feature gate information because feature gate information is source-WIT only.

After digging through the code I found this in WIT.md:

Thus, @since and @unstable gates are not part of the runtime semantics of components, just part of the source-level tooling for producing components.

So I guess this kind of solves the problem? So actually, we cannot know when we've received a WIT with feature gates if we're using a component-derived WIT.

In the case where we have the actual WIT path (i.e. the raw WIT), that's the case we do want to actually do the filtering (i.e. jco types)...

[guybedford]

@vados-cosmonic agreed, it seems like if any of these gates ever fail it's effectively an invalid component. And that this validation might belong further up the stack.

There might be a use case for when a dummy component is generated to provide features for that dummy component. Although perhaps that is something that belongs in wasm-tools again.

Overall I'm also tending towards saying this should just be a features integration for jco types, although let me know if you have any further counter arguments to share too.

[vados-cosmonic]

Definitely all for making this a jco types feature -- ts_bindgen.rs is now the only file with explicit feature gate checking

Thanks again for the patience and the reviews!

[guybedford]

Could we also add the --features flag option to the jco types command, should be relatively straightforward I think?

Then otherwise just a couple of last questions on the error reporting.

[vados-cosmonic]

Hey @guybedford thanks again for the careful review -- I made the changes and added some tests around jco types for feature use!

Finally got all the tests passing 😅

[guybedford]

Perfect, thank you for carrying this one over the line!