search

bytecodealliance / jco

JavaScript toolchain for working with WebAssembly Components
https://bytecodealliance.github.io/jco/
Apache License 2.0
642 stars 64 forks source link

feat: support semver aware exclusions #459

Closed vados-cosmonic closed 3 months ago

vados-cosmonic commented 4 months ago

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:

yoshuawuyts commented 4 months ago

@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 commented 4 months ago

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:

https://github.com/bytecodealliance/wasmtime/commit/9bdb731ab653a3845a1fa6cb6fcb20505c049ee8

yoshuawuyts commented 4 months ago

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 commented 4 months ago

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...

vados-cosmonic commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

vados-cosmonic commented 3 months ago

Thanks for review @guybedford ! I did #2, but if I'm right about https://github.com/bytecodealliance/jco/pull/459#issuecomment-2259205885 I don't think I can actually do #1 (or at least it would be a different kind of test)

guybedford commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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:

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 commented 3 months ago

@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 commented 3 months ago

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!

vados-cosmonic commented 3 months ago

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 :sweat_smile: