fix mismatch in stability attributes error in wit-parser

[jsturtevant]

fixes: #1995

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 should be able to be imported into other packages that use it. The use case for this is adding a unstable interface such as wasi-tls to wasi-cli. Once we get to resolve_include's processing we've already verified that everything checks out from a feature/version perspective for those features. i.e. we can't be trying to consolidate an interface type with a feature that hasn't already been resolved.

[alexcrichton]

Hm ok the CI failure is kind of worrisome since I think this looks like we're getting nondeterministic output? I forget what affects the order things are fed in to the resolve (maybe filesystem ordering?) but it may mean that we still want some sort of "unifying" here but with a defined ordering rather than that update_stability method

[jsturtevant]

I've been looking into it and found that it is because I am not preserving the versioning that was previously in update_stability, specifically:

// Otherwise if `into` is unknown then inherit the stability listed in
    // `from`.
    if into.is_unknown() {
        *into = from.clone();
        return Ok(());
    }

but I found that there might be a bug prior to this due to ordering in https://github.com/jsturtevant/wasm-tools/tree/fix-mismatch-attributes-on-include/crates/wit-component/tests/merge/success/merge.

For instance the final merge preserves the @since for the into but not the from:

world shared-world-with-versions-and-include {
  import shared-version-on-from;
  @since(version = 1.0.0)
  import shared-version-on-into;
}

And it does the opposite with the since when I merge into onto from (so ordering matters here):

world shared-world-with-versions-and-include {
  @since(version = 1.0.0)
  import shared-version-on-from;
  import shared-version-on-into;
}

Where I would expect to see:

world shared-world-with-versions-and-include {
  @since(version = 1.0.0)
  import shared-version-on-from;
  @since(version = 1.0.0)
  import shared-version-on-into;
}

My thought is replace the update_stability with something that copies those appropriately, but might need to sort through some changes with unstable attribute which isn't covered by that set of tests currently.

[alexcrichton]

My thought is replace the update_stability with something that copies those appropriately, but might need to sort through some changes with unstable attribute which isn't covered by that set of tests currently.

That sounds reasonable to me, sort of a "define an Ord for stability and preserve the max" strategy of sorts

[jsturtevant]

For instance the final merge preserves the @since for the into but not the from:

This ended up being a separate issue. I'd be happy to split this out into a separate change and do a little refactoring to cleanup the change which I sorta shove into the world merge section.

That sounds reasonable to me, sort of a "define an Ord for stability and preserve the max" strategy of sorts

I added derive ord to the enum, the order ends up being the order in which it is in the enum so stable is less than unstable which I find a little awkward. I am not sure if it's OK that way or if we should reorder the enum (would that be a breaking change?)

I'll clean up some of the comments tomorrow too

[alexcrichton]

This ended up being a separate issue. I'd be happy to split this out into a separate change and do a little refactoring to cleanup the change which I sorta shove into the world merge section.

Ah ok good catch! I'm happy to leave that in here, too, but if you'd be up for refactoring to deduplicate a bit I think that'd be good 👍

I am not sure if it's OK that way or if we should reorder the enum (would that be a breaking change?)

Definitely ok to reorder an enum, so how about a swap of Unknown and Stable so that way > should work? Also, mind leaving a comment on the enum that the derive(Ord) ordering is significant?

[jsturtevant]

I've dropped the changes for the world merge section. It was requiring a bit more work to catch the various edge cases and wanted to get this moving forward. I'll open a separate PR for that.

[ydnar]

@alexcrichton found an issue with this patch: when WIT is recreated from the resolved JSON, it has versioned feature gates on the unversioned wasmtime:test package:

package wasmtime:test;

world test {
	@since(version = 0.2.0)
	import wasi:dep2/stable@0.2.3;
	@unstable(feature = active)
	use wasi:dep2/stable@0.2.3.{stable-resource};
	@unstable(feature = active)
	import wasi:dep-unversioned/unversioned;
	@unstable(feature = active)
	use wasi:dep-unversioned/unversioned.{unversioned-resource};
	@unstable(feature = active)
	import wasi:dep-unstable/unstable;
	@unstable(feature = active)
	use wasi:dep-unstable/unstable.{unstable-resource};
}

world test-ordered {
	@since(version = 0.2.0)
	import wasi:dep2/stable@0.2.3;
	@unstable(feature = active)
	import wasi:dep-unversioned/unversioned;
	@unstable(feature = active)
	import wasi:dep-unstable/unstable;
	@unstable(feature = active)
	use wasi:dep2/stable@0.2.3.{stable-resource};
	@unstable(feature = active)
	use wasi:dep-unversioned/unversioned.{unversioned-resource};
	@unstable(feature = active)
	use wasi:dep-unstable/unstable.{unstable-resource};
}

package wasi:foo@0.2.3 {
	@since(version = 0.2.0)
	world imports {
		@since(version = 0.2.0)
		import wasi:dep2/[email protected];
		import wasi:dep-unversioned/unversioned;
		@unstable(feature = active)
		import wasi:dep-unstable/unstable;
	}
}

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

package wasi:dep-unstable {
	@unstable(feature = active)
	interface unstable {
		@unstable(feature = active)
		resource unstable-resource;
	}

	@unstable(feature = active)
	world imports {
		@unstable(feature = active)
		import unstable;
	}
}

package wasi:dep-unversioned {
	interface unversioned {
		resource unversioned-resource;
	}

	world imports {
		import unversioned;
	}
}

package wasi:dep2@0.2.3 {
	@since(version = 0.2.1)
	interface stable {
		resource stable-resource;
	}

	@since(version = 0.2.0)
	world imports {
		@since(version = 0.2.0)
		import stable;
	}
}

[ydnar]

This should be feature gate, not a version gate. The package that world test belongs to (wasmtime:test) is unversioned, so this stability is impossible.

[ydnar]

During resolution, when a world includes another world from a different package, should version gates be resolved then, and stripped out entirely?

[alexcrichton]

I think that everything new coming from include should probably forcibly get whatever feature gate is on the include itself, but otherwise I do agree that whatever the source is should likely be discarded entirely as features/versions aren't guaranteed to make sense in the destination world

[jsturtevant]

@ydnar calls out in bytecodealliance/go-modules#325 the test suite there does WIT → JSON → WIT. This seems like a good addition that would have caught this. I will open a new issue and see if can get this resolved.