Mismatch in stability attributes error in wit-parser when using Unstable attribute in wit

[jsturtevant]

When a new interface is marked as @unstable (feature = somefeaturegate) and it uses a stable type from another package via use package:interface/type.{name} the resulting package can't be imported into other packages that use it. The resulting error is:

wasm-tools component wit --features tls ./wit/
error: mismatch in stability attributes

example

I've demonstrated this by adding a test: jsturtevant@4495325

As well as trying to integrate it into another world: https://github.com/WebAssembly/wasi-cli/compare/main...jsturtevant:wasi-cli:wasi-tls-example?expand=1 (note that this was just a test to see if I was getting in the a set up similiar to other imports)

I was getting this original from wit-bindgen macro: https://github.com/bytecodealliance/wit-bindgen/tree/main?tab=readme-ov-file#supported-guest-languages

workaround

for now I've patched wit-parser like jsturtevant@22cfe2e but it seems like there is probably a better way to approach?

[alexcrichton]

Thanks for the report! I'm traveling for a bit so may take me a bit to dig in to this but I plan on doing so in the not-too-distant future

[alexcrichton]

Digging into this, can you confirm that the reduced test case you have reproduces the original error you were seeing? Locally if I revert your fix on your branch and run the test suite I see:

failures:

---- "tests/ui/multi-package-features" ----
test "tests/ui/multi-package-features" failed

Caused by:
    0: failed to resolve directory while parsing WIT for path [tests/ui/multi-package-features]
    1: type `unstable-resource` not defined in interface
            --> tests/ui/multi-package-features/deps/unstable/root.wit:12:33
             |
          12 |     use wasi:dep2/[email protected].{unstable-resource};
             |                                 ^----------------


failures:
    "tests/ui/multi-package-features"

which looks different from "mismatch in stability attributes" that you were seeing with wasi-tls.

I'm definitely seeing some funny behavior that I wouldn't expect with use and feature attributes too. The test case you have can be reduced further to:

package wasmtime:test;

interface types {
  @unstable(feature = notsure)
  use wasi:dep2/stable@0.2.3.{unstable-resource};
}

package wasi:dep2@0.2.3 {
  interface stable {
    @unstable(feature = notsure)
    resource unstable-resource;
  }
}

which yields:

$ wasm-tools component wit foo.wit
error: type `unstable-resource` not defined in interface
     --> foo.wit:5:31
      |
    5 |   use wasi:dep2/[email protected].{unstable-resource};
      |                               ^----------------

and while this looks problematic it looks different from the problems you were seeing originally too

[alexcrichton]

I've fixed the issue of your reduced test case (I think) as part of #2046, and it may actually end up helping the original test case, but I'd still want to confirm before closing.

[jsturtevant]

Thanks for looking into this. I messed up when I included the unstable-resource. I was experimenting and accidentally included that in the commit.

I am still getting an error, using your simplified example as a guide, I've reduced the failure to:

package wasmtime:test;

world test{
    include wasi:foo/[email protected];
    include wasi:unstable/[email protected]; 
} 

package wasi:[email protected] {
    @unstable(feature = active)
    world imports {
        @unstable(feature = active)
        use wasi:dep2/[email protected].{stable-resource};
    }
}

package wasi:[email protected] {
    @since(version = 0.2.0)
    world imports {
        @since(version = 0.2.0)
        include wasi:dep2/[email protected];
    }
}

package wasi:[email protected] {
    @since(version = 0.2.0)
    world imports {
        @since(version = 0.2.0)
        import stable;
    }
    @since(version = 0.2.0)
    interface stable {
        resource stable-resource {
        }
    }
}

git last
commit a86afae206ad5df897c3f50a06d667d19b18bb01 

cargo run -- component wit --features active example.wit
error: mismatch in stability attributes

[jsturtevant]

I've fixed the issue of your reduced test case (I think) as part of #2046, and it may actually end up helping the original test case, but I'd still want to confirm before closing.

This might have broken general features, when I run the latest tools against the wasi-cli repo with no features I get:

git log -1 HEAD
commit a86afae206ad5df897c3f50a06d667d19b18bb01 

cargo run -- component wit ../wit/wasi-cli/wit/
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.22s
     Running `target/debug/wasm-tools component wit ../wit/wasi-cli/wit/`
error: found a reference to a type which is excluded due to its feature not being activated
     --> ../wit/wasi-cli/wit/deps/sockets/network.wit:122:42
      |
  122 |     network-error-code: func(err: borrow<error>) -> option<error-code>;

Where as if I checkout right before the change:

git log -1 HEAD
commit 823efcb9b67d36960010bafc99c73bb528bfbbbe

cargo run -- component wit ../wit/wasi-cli/wit/
// wit output successful

[alexcrichton]

Oops, good catch! Should be fixed in #2053

[jsturtevant]

Oops, good catch! Should be fixed in #2053

That's working again.

Does #1995 (comment) make sense as something that should work? This was the original issue I was running into. With a little guidance I would be happy to take a look at fixing it.

[alexcrichton]

Oh dear I apologize I completely missed that comment! I'll try to dig in to that soon.

[alexcrichton]

Ok I started to take a look at this and it's going to be tricky.

What I think is happening is something like:

• An include first includes a @since version of an interface
• A second include includes an @unstable use statement (which elaborates to an @unstable import of the interface + use)
• This leads to a conflict where the first include included a @since interface, and the second include included an @unstable version.

At the very least this should have a better error, but otherwise solving this is something I'm not quite sure how to do just yet

[jsturtevant]

That was my analysis as well. I couldn't find a great solution either so nice to hear it isn't trivial :-)

The real world use case for this is something like taking the cli world which has wasi-sockets and adding wasi-tls (see bytecodealliance/wasmtime#10249). If we aren't able to do this then we can't ever have an unstable interface use types from other worlds when building from worlds that already use it.

My thoughts are that if we are using a type from another world that is stable and that version already exists in the imports of the world then we should use the existing types stability. This assumes that feature is enabled and the type version being used is compatible version wise with what is already being used.

If I understand correctly, that approach seems to be how it works in a scenario where we use the type from two stable interfaces, the type version info is merged together and is why my temp fix works

wasm-tools/crates/wit-parser/src/resolve.rs

Lines 3819 to 3824 in 0e5b565

	fn update_stability(from: &Stability, into: &mut Stability) -> Result<()> {
	// If `from` is unknown or the two stability annotations are equal then
	// there's nothing to do here.
	if from == into || from.is_unknown() {
	return Ok(());
	}

[alexcrichton]

I agree that the shape of the fix you describes is what we should implement, and I also definitely agree it's something we want to solve! (sorry I've been quite swamped this week so I haven't sat down to think through and write up what a full fix would be, but you've already done 90% of that now!)

A caveat I might add is that instead of fixing update_stability, which is used in other places as well, I'd prefer to start out with something more localized such as unify_stability which is only used for include processing. Additionally instead of always taking the previous stability I think it might make sense to bias towards stable, so basically unify unstable/stable into just stable. That way it wouldn't matter what order things happen in, it'd always produce the same result.

Would you be up for making a PR to fix this?