Victor Adossi (Jul 01 2024 at 14:17): Victor Adossi (Jul 01 2024 at 14:17):Hey all, been working on support for the new WIT feature gating functionality -- it's required a bunch of updates (even further than what was added just last week), and I'm trying to add a test, etc.
Would love anyone with opinions/comments to take a look at the draft PR:
https://github.com/bytecodealliance/jco/pull/459
Guy Bedford (Jul 02 2024 at 17:16):Thanks for working on this! I've left some comments in the PR.
Victor Adossi (Jul 09 2024 at 15:35):Hey all -- made some progress here, but I think I'm stuck.
I have two tests that do what they're supposed to, and one that doesn't -- in particular @since(0.2.2) is not being removed from a package that is at @0.2.1 -- and the Stability values I'm getting from the functions are all actually Unknown when I'd expect them to be marked with versions
Victor Adossi (Jul 09 2024 at 15:39):The PR is updated (it seems to be steadily growing...) but the relevant wit is:
package test:feature-gates@0.2.1;
interface foo {
a: func();
@since(version = 0.2.1)
b: func();
@since(version = 0.2.2)
c: func();
@unstable(feature = fancier-foo)
d: func();
}
world import-and-export {
import foo;
export foo;
}
I've also added printlns so if you comment out the tests that aren't witTest you'll see output coming from ts_bindgen.rs that looks like this:
│> @bytecodealliance/jco@1.3.1 test │
│> node --stack-trace-limit=100 node_modules/mocha/bin/mocha.js -u tdd test/test.js --timeout 30000 │
│ │
│ │
│ │
│ WIT │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│ │
│ │
│[TEST] interfaces file content? export namespace TestFeatureGatesFoo { │
│ export function a(): void; │
│ export function b(): void; │
│ export function c(): void; │
│} │
│ │
│ 1) feature-gates: since should exclude future versions │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [d] (stability: Unknown) │
│ ✔ feature-gates: unstable should be present with all features enabled │
│[TS_BINDGEN] generating TS func: [a] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [b] (stability: Unknown) │
│[TS_BINDGEN] generating TS func: [c] (stability: Unknown) │
│ ✔ feature-gates: unstable should not be present without all features enabled
Just want to make sure I'm not misunderstanding the feature, but I'd expect to not see c() because it's @since(0.2.2) and the package is @0.2.1.
I also tried with 0.3.0 just to be sure I wasn't running into a semver compat smoothing thing.
Weirdly enough, unstable works just fine w/ all features specified (those are the passing tests)
Guy Bedford (Jul 09 2024 at 18:06):@Victor Adossi this looks amazing. Given you're seeing information being lost, I'm wondering if the issue here is just lower-level then? There seems to have been a bit of movement in this code in wasm-tools. Perhaps this is an issue in the Wasmtime machinery itself? I wonder if the unreleased wasm-tools code here will fix it?
Victor Adossi (Jul 10 2024 at 01:52):Appreciate all the help so far :bow:
I think you're right, I'm going to take a look at the lower level wasm-tools again -- I'm already on 0.212.0, but it really feels like they shouldn't all be Unknown, there may have been some movement there I haven't pulled down... Looking at the upstream code I'm also fairly certain I shouldn't even see the import on the to begin with (like how unstable worked)...
Christof Petig (Jul 10 2024 at 05:06):Yesterday I encountered a panic in wit-parser because Wasi-cli's since isn't merged, yet. This was due to a mismatching since and unknown on the same interface.
Perhaps upgrading cli to Joel's version already fixes things.
Victor Adossi (Jul 10 2024 at 13:03):mismatching since & unknown on the same interface is a use case I don't even have in the test yet -- I should add it.
Would you mind sharing a snippet of the WIT, @Christof Petig ?
Christof Petig (Jul 10 2024 at 13:23):I will need to gather the details, just combining Wasi-cli main with main on the other Wasi dependencies (io, fs, clocks) using wit-deps created the problem for us.
Victor Adossi (Jul 10 2024 at 15:29):Ahh thanks, let me take a look and see those changes, maybe no need to waste cycles gathering the details just yet, though I'm going to add that test
Christof Petig (Jul 10 2024 at 20:27)::thinking: it was far more difficult to reproduce than I thought. Here it is: take this wit/deps.toml and wit/test.wit combo, run wit-deps update and then wit-bindgen markdown wit.
It crashes with
thread 'main' panicked at …/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wit-parser-0.212.0/src/resolve.rs:2350:21:
assertion `left == right` failed
left: Interface { id: Id { idx: 1 }, stability: Unknown }
right: Interface { id: Id { idx: 1 }, stability: Stable { since: Version { major: 0, minor: 2, patch: 0 }, feature: None } }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
If you replace either clocks with v0.2.0 or cli with the version by JoelYoshua the crash is gone.
wasi-problem.tgz
Christof Petig (Jul 10 2024 at 20:29):Version by Yoshua at https://github.com/WebAssembly/wasi-cli/pull/43
Christof Petig (Jul 10 2024 at 20:32):But to be honest I haven't yet figured out which interface is referenced both with and without since.
Christof Petig (Jul 10 2024 at 20:35):Here is the full version (no wit-deps update required to retrieve the deps)
wasi-problem.full.tgz
Victor Adossi (Jul 11 2024 at 08:08):Thanks for providing the tarballs -- I'm not sure I'm completely right (maybe you could confirm) but the line that is being pointed to seems to be this one:
https://github.com/bytecodealliance/wasm-tools/blob/v1.212.0/crates/wit-parser/src/resolve.rs#L2350
Victor Adossi (Jul 11 2024 at 08:09):It looks like wit parser expects stabilities to never change, and we're getting the interface in there as Unknown at one poitn then the newer version actually has it specified?
Victor Adossi (Jul 11 2024 at 08:10):Wonder if that assert_eq! should be a bail! with a more descriptive error message
Christof Petig (Jul 11 2024 at 11:06):Ideally it would output the name of the interface which got redefined, but due to rearranging of the structure during the merge this is a far from easy to get information. Another solution would be to ignore unknown in the comparison.
Christof Petig (Jul 11 2024 at 11:07):And yes this assert_eq is exactly the panicking line.
Christof Petig (Jul 11 2024 at 11:12):I feel filling a bug report on wit-parser would be adequate.
Victor Adossi (Jul 12 2024 at 14:15):Filed an issue:
https://github.com/bytecodealliance/wasm-tools/issues/1666
@Yoshua Wuyts would you mind taking a look if you find some time? The thing is that the check makes sense -- the same item shouldn't have it's Stability essentially discovered twice (and be different)... so the assert makes sense, and however we're getting the Unknown in there in the first place is probably what's jamming me up on thejco impl
Victor Adossi (Jul 17 2024 at 16:38):Hmnnn, so I've updated to 0.214.0 and relevant packages but still see the issue -- all stabilities are still coming in as Unknown.
Still looking into why this could be, maybe the right annotation is ending up in the wrong place possibly
Victor Adossi (Jul 17 2024 at 18:52):OK so I think I've found where the issue might be -- it seems that update_stability is not called when moving worlds, and in particular moving interfaces...
Victor Adossi (Jul 17 2024 at 18:53):I'm wondering if there's not enough copying/moving done there -- when I print out the stabilities for the items as they pass through, they're correct (non-Unknown)
Victor Adossi (Jul 18 2024 at 14:44):Ah some interesting discussion on how the feature gating should work in the first place:
Found the bit that was missing -- but maybe it should be missing
Last updated: Oct 23 2024 at 20:03 UTC