Repository: bytecodealliance/wasm-tools
Pull request #1689 (2024-07-18 17:44:29)

fix(wit-parser): improve checking for stable feature gates
This commit introduces some extra checking (and panicking) for feature gates that are stable (i.e. `Stability::Stable`) AKA `@since` with a version and/or features specified.

There are two primary changes:
- Make referring to a *future* version of the package an error (for now)
- Ensure that if a feature is specified that it is checked before inclusion

In the past `Stability::Stable` feature gates were simply treated as `Unknown`, which hides the information necessary for downstream crates/consumers to use (and created the question around whether referring to future package versions is valid).

See also the [relevant Zulip discussion](https://bytecodealliance.zulipchat.com/#narrow/stream/223391-wasm/topic/Missing.20impl.20for.20.40since.20in.20.60Resolve.3A.3Ainclude_stability.28.29.60/near/452175637)

vados-cosmonic: Hey @alexcrichton so looking back at the WIT design doc (trying to change it):

> Together, these gates support a development flow in which new features start with an @unstable gate while the details are still being hashed out. Then, once the feature is stable (and, in a WASI context, voted upon), the @unstable gate is switched to a @since gate. To enable a smooth transition (during which producer toolchains are targeting a version earlier than the @since-specified version), the @since gate contains an optional feature field that, when present, says to enable the feature when either the target version is greator-or-equal or the feature name is explicitly enabled by the developer. Thus, c is enabled if the version is 0.2.2 or newer or the fancy-foo feature is explicitly enabled by the developer. The feature field can be removed once producer toolchains have updated their default version to enable the feature by default.

It looks like there is an expectation for `feature` *to be used* to decide when to process a `@since`... Maybe this PR should reflect that, rather than changing it and then updating the docs upstream?
alexcrichton: Ah I had missed that as well, good catch! That seems reasonable to me to implement yeah, want to do so as part of this PR?
vados-cosmonic: Done! 

I needed to add some changes (and update some tests), would appreciate some extra close eyes on [this change set](https://github.com/bytecodealliance/wasm-tools/pull/1689/commits/4b8069a553507cc3d1628b28eb5b7ad010f42e87) (and of course the rest of the PR). 

Requiring the feature be specified is making a bunch of interfaces be missing, along with the deprecated flag (which wasn't being taken into account before), just want to make sure I'm reading the test output right and it passes a human spot check that isn't mine.
vados-cosmonic: Actually, some more thinking here @alexcrichton ... The way that the note is phrased, we actually *must* allow `@since` with versions in the future right?

> Thus, c is enabled if the version is 0.2.2 or newer or the fancy-foo feature is explicitly enabled by the developer. The feature field can be removed once producer toolchains have updated their default version to enable the feature by default.

Given this sentence it assumes it is possible for `c` to be at a version that is *after* the package version -- otherwise it's just a feature check (because right now we consider future versions a bug).

I think to match this expected functionality we have to allow `@since(version = x)` where `x` is a future version.

I'm updating the code now to match
alexcrichton: Hm I think you're right on your interpretation of the text, but now I'm having second thoughts about supporting it exactly as-is. How about reverting back to:

* With `@since` the `feature` is ignored and is purely informational in the WIT itself
* The `@since` version must be <= the current package version
* Using `@since` requires the package to have a version
* Mixing `@deprecated` with `@since` has no effect on whether it's included or not

That to me feels like a more straightforward way to interpret these attributes and if you agree I can work on sending a PR upstream as well.
vados-cosmonic: Hey so after some discussion at the JS meeting with @guybedford  we actually realized another issue -- the AND vs OR combination of `version` and `feature` makes a bunch of things easier w/ respect to figuring out dependencies (i.e. if you know some world imports `a`, and `a` has `@since(version = v.v.v, feature = f)` you can intuit the required package and enabled features of the relevant package).

There's also the benefit (possibly anti-benefit?) of being able to span a feature over multiple versions (changing how a certain item works over two different versions). An example of this that might make sense is something like:

`@since(0.2.0, feature = async)`
`@since(0.3.0, feature = async)`

I'm I think there might be a bunch of surprise at the ORing of those, does it make sense to actually hammer this out/give everyone a chance to chat about it in an issue on `component-model`?

alexcrichton: I'm not sure I quite follow what you mean by AND vs OR? But regardless it seems fine to discuss some more of the finer details on the component-model repository as well.
vados-cosmonic: Ah so before I make the other issue -- basically the idea of whether `version` and `feature` should be AND or OR'd -- 

The logic right now is describe as an OR (valid version *or* feature enabled), but it might make sense/be unsurprising to have an AND -- i.e. valid version *and* feature enabled to see the import at all
alexcrichton: Ah ok I see. Personally I don't think an AND is viable given how features/since/unstable are planned to be used in WASI. I also don't think that the OR requirement as stated in WIT.md necessarily makes sense because it doesn't really make sense for `since` to list a version in the future, but if it's always <= the current package version then the OR-ness is like it's OR-ing with true which doesn't affect too much.

In any case I agree with your 3 options, and probably good to discuss upstream!
vados-cosmonic: Seems like actions/upload-artifact is having a bit of an outage, hopefully this that step can just be re-run and pass soon!

Patch:
diff --git a/crates/wit-parser/src/resolve.rs b/crates/wit-parser/src/resolve.rs
index 9a69eba4..5471a54c 100644
--- a/crates/wit-parser/src/resolve.rs
+++ b/crates/wit-parser/src/resolve.rs
@@ -1,3 +1,13 @@
+use std::collections::{BTreeMap, HashMap, HashSet};
+use std::mem;
+use std::path::{Path, PathBuf};
+
+use anyhow::{anyhow, bail, ensure, Context, Result};
+use id_arena::{Arena, Id};
+use indexmap::{IndexMap, IndexSet};
+#[cfg(feature = "serde")]
+use serde_derive::Serialize;
+
 use crate::ast::lex::Span;
 use crate::ast::{parse_use_path, ParsedUsePath};
 #[cfg(feature = "serde")]
@@ -8,14 +18,6 @@ use crate::{
     TypeOwner, UnresolvedPackage, UnresolvedPackageGroup, World, WorldId, WorldItem, WorldKey,
     WorldSpan,
 };
-use anyhow::{anyhow, bail, Context, Result};
-use id_arena::{Arena, Id};
-use indexmap::{IndexMap, IndexSet};
-#[cfg(feature = "serde")]
-use serde_derive::Serialize;
-use std::collections::{BTreeMap, HashMap, HashSet};
-use std::mem;
-use std::path::{Path, PathBuf};
 
 /// Representation of a fully resolved set of WIT packages.
 ///
@@ -1408,13 +1410,37 @@ impl Resolve {
         }
     }
 
-    fn include_stability(&self, stability: &Stability) -> bool {
-        match stability {
-            Stability::Stable { .. } | Stability::Unknown => true,
+    fn include_stability(&self, stability: &Stability, pkg_id: &PackageId) -> Result<bool> {
+        Ok(match stability {
+            Stability::Unknown => true,
+            // NOTE: deprecations are intentionally omitted -- an existing `@since` takes precedence over `@deprecated`
+            Stability::Stable { since, .. } => {
+                let Some(p) = self.packages.get(*pkg_id) else {
+                    // We can't check much without a package (possibly dealing with an item in an `UnresolvedPackage`),
+                    // @since version & deprecations can't be checked because there's no package version to compare to.
+                    //
+                    // Feature requirements on stabilized features are ignored in resolved packages, so we do the same here.
+                    return Ok(true);
+                };
+
+                // Use of feature gating with version specifiers inside a package that is not versioned is not allowed
+                let package_version = p.name.version.as_ref().with_context(|| format!("package [{}] contains a feature gate with a version specifier, so it must have a version", p.name))?;
+
+                // If the version on the feature gate is:
+                // - released, then we can include it
+                // - unreleased, then we must check the feature (if present)
+                ensure!(
+                    since <= package_version,
+                    "feature gate cannot reference unreleased version {since} of package [{}] (current version {package_version})",
+                    p.name
+                );
+
+                true
+            }
             Stability::Unstable { feature, .. } => {
                 self.features.contains(feature) || self.all_features
             }
-        }
+        })
     }
 }
 
@@ -1503,7 +1529,16 @@ impl Remap {
             .zip(&unresolved.type_spans)
             .skip(foreign_types)
         {
-            if !resolve.include_stability(&ty.stability) {
+            if !resolve
+                .include_stability(&ty.stability, &pkgid)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for type [{}] in package [{}]",
+                        ty.name.as_ref().map(String::as_str).unwrap_or("<unknown>"),
+                        resolve.packages[pkgid].name,
+                    )
+                })?
+            {
                 self.types.push(None);
                 continue;
             }
@@ -1544,13 +1579,26 @@ impl Remap {
             .zip(&unresolved.interface_spans)
             .skip(foreign_interfaces)
         {
-            if !resolve.include_stability(&iface.stability) {
+            if !resolve
+                .include_stability(&iface.stability, &pkgid)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for interface [{}] in package [{}]",
+                        iface
+                            .name
+                            .as_ref()
+                            .map(String::as_str)
+                            .unwrap_or("<unknown>"),
+                        resolve.packages[pkgid].name,
+                    )
+                })?
+            {
                 self.interfaces.push(None);
                 continue;
             }
-            self.update_interface(resolve, &mut iface, Some(span))?;
             assert!(iface.package.is_none());
             iface.package = Some(pkgid);
+            self.update_interface(resolve, &mut iface, Some(span))?;
             let new_id = resolve.interfaces.alloc(iface);
             assert_eq!(self.interfaces.len(), id.index());
             self.interfaces.push(Some(new_id));
@@ -1589,11 +1637,19 @@ impl Remap {
             .zip(unresolved.world_spans)
             .skip(foreign_worlds)
         {
-            if !resolve.include_stability(&world.stability) {
+            if !resolve
+                .include_stability(&world.stability, &pkgid)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for world [{}] in package [{}]",
+                        world.name, resolve.packages[pkgid].name,
+                    )
+                })?
+            {
                 self.worlds.push(None);
                 continue;
             }
-            self.update_world(&mut world, resolve, &span)?;
+            self.update_world(&mut world, resolve, &pkgid, &span)?;
 
             let new_id = resolve.worlds.alloc(world);
             assert_eq!(self.worlds.len(), id.index());
@@ -1945,6 +2001,16 @@ impl Remap {
         spans: Option<&InterfaceSpan>,
     ) -> Result<()> {
         iface.types.retain(|_, ty| self.types[ty.index()].is_some());
+        let iface_pkg_id = iface.package.as_ref().unwrap_or_else(|| {
+            panic!(
+                "unexpectedly missing package on interface [{}]",
+                iface
+                    .name
+                    .as_ref()
+                    .map(String::as_str)
+                    .unwrap_or("<unknown>"),
+            )
+        });
 
         // NB: note that `iface.doc` is not updated here since interfaces
         // haven't been mapped yet and that's done in a separate step.
@@ -1954,17 +2020,31 @@ impl Remap {
         if let Some(spans) = spans {
             assert_eq!(iface.functions.len(), spans.funcs.len());
         }
-        for (i, (_, func)) in iface.functions.iter_mut().enumerate() {
-            if !resolve.include_stability(&func.stability) {
+        for (i, (func_name, func)) in iface.functions.iter_mut().enumerate() {
+            if !resolve
+                .include_stability(&func.stability, iface_pkg_id)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for function [{func_name}] in package [{}]",
+                        resolve.packages[*iface_pkg_id].name,
+                    )
+                })?
+            {
                 continue;
             }
             let span = spans.map(|s| s.funcs[i]);
             self.update_function(resolve, func, span)
                 .with_context(|| format!("failed to update function `{}`", func.name))?;
         }
-        iface
-            .functions
-            .retain(|_, f| resolve.include_stability(&f.stability));
+
+        // Filter out all of the existing functions in interface which fail the
+        // `include_stability()` check, as they shouldn't be available.
+        for (name, func) in mem::take(&mut iface.functions) {
+            if resolve.include_stability(&func.stability, iface_pkg_id)? {
+                iface.functions.insert(name, func);
+            }
+        }
+
         Ok(())
     }
 
@@ -2017,6 +2097,7 @@ impl Remap {
         &mut self,
         world: &mut World,
         resolve: &mut Resolve,
+        pkg_id: &PackageId,
         spans: &WorldSpan,
     ) -> Result<()> {
         // NB: this function is more more complicated than the prior versions
@@ -2047,7 +2128,16 @@ impl Remap {
                 *id = self.map_type(*id, Some(*span))?;
             }
             let stability = item.stability(resolve);
-            if !resolve.include_stability(stability) {
+            if !resolve
+                .include_stability(stability, pkg_id)
+                .with_context(|| {
+                    format!(
+                        "failed to process imported world item type [{}] in package [{}]",
+                        resolve.name_world_key(&name),
+                        resolve.packages[*pkg_id].name,
+                    )
+                })?
+            {
                 continue;
             }
             self.update_world_key(&mut name, Some(*span))?;
@@ -2088,7 +2178,16 @@ impl Remap {
             .zip(&spans.exports)
         {
             let stability = item.stability(resolve);
-            if !resolve.include_stability(stability) {
+            if !resolve
+                .include_stability(stability, pkg_id)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for exported item [{}] in package [{}]",
+                        resolve.name_world_key(&name),
+                        resolve.packages[*pkg_id].name,
+                    )
+                })?
+            {
                 continue;
             }
             self.update_world_key(&mut name, Some(*span))?;
@@ -2121,7 +2220,16 @@ impl Remap {
             .zip(&spans.includes)
             .zip(&include_names)
         {
-            if !resolve.include_stability(&stability) {
+            if !resolve
+                .include_stability(&stability, pkg_id)
+                .with_context(|| {
+                    format!(
+                        "failed to process feature gate for included world [{}] in package [{}]",
+                        resolve.worlds[include_world].name.as_str(),
+                        resolve.packages[*pkg_id].name
+                    )
+                })?
+            {
                 continue;
             }
             self.resolve_include(world, include_world, names, *span, resolve)?;


Test patch:
diff --git a/crates/wit-component/tests/merge/success/from/a.wit b/crates/wit-component/tests/merge/success/from/a.wit
index d181c7f5..0680ab71 100644
--- a/crates/wit-component/tests/merge/success/from/a.wit
+++ b/crates/wit-component/tests/merge/success/from/a.wit
@@ -1,7 +1,7 @@
 package foo:%from;
 
 interface a {
-  use foo:foo/only-from.{r};
+  use foo:foo/only-from@1.0.0.{r};
 
   foo: func();
 }
diff --git a/crates/wit-component/tests/merge/success/from/deps/foo/shared.wit b/crates/wit-component/tests/merge/success/from/deps/foo/shared.wit
index 272c5928..c2df6de7 100644
--- a/crates/wit-component/tests/merge/success/from/deps/foo/shared.wit
+++ b/crates/wit-component/tests/merge/success/from/deps/foo/shared.wit
@@ -1,4 +1,4 @@
-package foo:foo;
+package foo:foo@1.0.0;
 
 interface shared-only-from {
   variant v {
diff --git a/crates/wit-component/tests/merge/success/into/b.wit b/crates/wit-component/tests/merge/success/into/b.wit
index ae368120..d636b6fa 100644
--- a/crates/wit-component/tests/merge/success/into/b.wit
+++ b/crates/wit-component/tests/merge/success/into/b.wit
@@ -1,7 +1,7 @@
 package foo:into;
 
 interface b {
-  use foo:foo/only-into.{r};
+  use foo:foo/only-into@1.0.0.{r};
 
   foo: func();
 }
diff --git a/crates/wit-component/tests/merge/success/into/deps/foo/shared.wit b/crates/wit-component/tests/merge/success/into/deps/foo/shared.wit
index 0adea4f0..5a97ef93 100644
--- a/crates/wit-component/tests/merge/success/into/deps/foo/shared.wit
+++ b/crates/wit-component/tests/merge/success/into/deps/foo/shared.wit
@@ -1,4 +1,4 @@
-package foo:foo;
+package foo:foo@1.0.0;
 
 interface shared-only-into {
   variant v {
diff --git a/crates/wit-component/tests/merge/success/merge/foo.wit b/crates/wit-component/tests/merge/success/merge/foo.wit
index f209dac2..c8b446a9 100644
--- a/crates/wit-component/tests/merge/success/merge/foo.wit
+++ b/crates/wit-component/tests/merge/success/merge/foo.wit
@@ -1,4 +1,4 @@
-package foo:foo;
+package foo:foo@1.0.0;
 
 interface only-into {
   record r {
diff --git a/crates/wit-component/tests/merge/success/merge/from.wit b/crates/wit-component/tests/merge/success/merge/from.wit
index a57ad141..b080c2fc 100644
--- a/crates/wit-component/tests/merge/success/merge/from.wit
+++ b/crates/wit-component/tests/merge/success/merge/from.wit
@@ -1,7 +1,7 @@
 package foo:%from;
 
 interface a {
-  use foo:foo/only-from.{r};
+  use foo:foo/only-from@1.0.0.{r};
 
   foo: func();
 }
diff --git a/crates/wit-component/tests/merge/success/merge/into.wit b/crates/wit-component/tests/merge/success/merge/into.wit
index 7448a6a2..ae40b427 100644
--- a/crates/wit-component/tests/merge/success/merge/into.wit
+++ b/crates/wit-component/tests/merge/success/merge/into.wit
@@ -1,7 +1,7 @@
 package foo:into;
 
 interface b {
-  use foo:foo/only-into.{r};
+  use foo:foo/only-into@1.0.0.{r};
 
   foo: func();
 }
diff --git a/tests/cli/since-on-future-package.wit b/tests/cli/since-on-future-package.wit
new file mode 100644
index 00000000..1c9a8eb6
--- /dev/null
+++ b/tests/cli/since-on-future-package.wit
@@ -0,0 +1,18 @@
+// FAIL: component embed --dummy %
+
+package test:invalid@0.1.0;
+
+interface foo {
+  a: func(s: string) -> string;
+
+  @since(version = 0.1.1)
+  b: func(s: string) -> string;
+
+  @since(version = 0.1.1, feature = c-please)
+  c: func(s: string) -> string;
+}
+
+world test {
+  import foo;
+  export foo;
+}
\ No newline at end of file
diff --git a/tests/cli/since-on-future-package.wit.stderr b/tests/cli/since-on-future-package.wit.stderr
new file mode 100644
index 00000000..b1c0e6e1
--- /dev/null
+++ b/tests/cli/since-on-future-package.wit.stderr
@@ -0,0 +1,4 @@
+error: failed to process feature gate for function [b] in package [test:invalid@0.1.0]
+
+Caused by:
+    0: feature gate cannot reference unreleased version 0.1.1 of package [test:invalid@0.1.0] (current version 0.1.0)
diff --git a/tests/cli/wit-stability-in-binary-format.wit b/tests/cli/wit-stability-in-binary-format.wit
index fb56e018..8ab71497 100644
--- a/tests/cli/wit-stability-in-binary-format.wit
+++ b/tests/cli/wit-stability-in-binary-format.wit
@@ -1,6 +1,6 @@
 // RUN: component wit % --wasm | component wit
 
-package a:b;
+package a:b@1.0.0;
 
 @since(version = 1.0.0)
 interface foo {
diff --git a/tests/cli/wit-stability-in-binary-format.wit.stdout b/tests/cli/wit-stability-in-binary-format.wit.stdout
index ce05750a..f6dd5adc 100644
--- a/tests/cli/wit-stability-in-binary-format.wit.stdout
+++ b/tests/cli/wit-stability-in-binary-format.wit.stdout
@@ -1,5 +1,5 @@
 /// RUN: component wit % --wasm | component wit
-package a:b;
+package a:b@1.0.0;
 
 @since(version = 1.0.0)
 interface foo {
diff --git a/tests/cli/wit-stability-inherited.wit b/tests/cli/wit-stability-inherited.wit
index 572e1c25..6dd7249f 100644
--- a/tests/cli/wit-stability-inherited.wit
+++ b/tests/cli/wit-stability-inherited.wit
@@ -1,6 +1,6 @@
 // RUN: component wit %
 
-package a:b;
+package a:b@1.0.0;
 
 interface foo {
   type t = u32;
diff --git a/tests/cli/wit-stability-inherited.wit.stdout b/tests/cli/wit-stability-inherited.wit.stdout
index 651b90e2..19b3df45 100644
--- a/tests/cli/wit-stability-inherited.wit.stdout
+++ b/tests/cli/wit-stability-inherited.wit.stdout
@@ -1,5 +1,5 @@
 /// RUN: component wit %
-package a:b;
+package a:b@1.0.0;
 
 interface foo {
   type t = u32;