Skip to content

Commit

Permalink
fix(wit-parser): improve checking for stable feature gates
Browse files Browse the repository at this point in the history
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).

Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
  • Loading branch information
vados-cosmonic committed Jul 24, 2024
1 parent 368184c commit fe67930
Show file tree
Hide file tree
Showing 14 changed files with 156 additions and 29 deletions.
2 changes: 1 addition & 1 deletion crates/wit-component/tests/merge/success/from/a.wit
Original file line number Diff line number Diff line change
@@ -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();
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package foo:foo;
package foo:foo@1.0.0;

interface shared-only-from {
variant v {
Expand Down
2 changes: 1 addition & 1 deletion crates/wit-component/tests/merge/success/into/b.wit
Original file line number Diff line number Diff line change
@@ -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();
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package foo:foo;
package foo:foo@1.0.0;

interface shared-only-into {
variant v {
Expand Down
2 changes: 1 addition & 1 deletion crates/wit-component/tests/merge/success/merge/foo.wit
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package foo:foo;
package foo:foo@1.0.0;

interface only-into {
record r {
Expand Down
2 changes: 1 addition & 1 deletion crates/wit-component/tests/merge/success/merge/from.wit
Original file line number Diff line number Diff line change
@@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion crates/wit-component/tests/merge/success/merge/into.wit
Original file line number Diff line number Diff line change
@@ -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();
}
Expand Down
144 changes: 126 additions & 18 deletions crates/wit-parser/src/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::{
TypeOwner, UnresolvedPackage, UnresolvedPackageGroup, World, WorldId, WorldItem, WorldKey,
WorldSpan,
};
use anyhow::{anyhow, bail, Context, Result};
use anyhow::{anyhow, bail, ensure, Context, Result};
use id_arena::{Arena, Id};
use indexmap::{IndexMap, IndexSet};
#[cfg(feature = "serde")]
Expand Down Expand Up @@ -1408,13 +1408,39 @@ 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,
Stability::Stable { since, feature, .. } => {
if let Some(p) = self.packages.get(*pkg_id) {
// 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 `@since` version specified on this stability is ahead of the version on the
// package that it belongs to, we throw an explicit error as that is not allowed
ensure!(
since <= package_version,
"feature gates cannot refer to an unreleased (future) package version [{}] in package [{}]",
since,
p.name,
);
}

// If a feature was specified, use it to determine whether to include or not
// at this point if the package has a version that *can* be checked against
// the feature gate version, it has been.
if let Some(f) = feature {
return Ok(self.features.contains(f));
}

// For all other cases, and specifically if `@since` is present and the version is valid (in the past),
// ignore which features were required to be enabled (such features only gate unstable APIs)
true
}
Stability::Unstable { feature, .. } => {
self.features.contains(feature) || self.all_features
}
}
})
}
}

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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.
Expand All @@ -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(())
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))?;
Expand Down Expand Up @@ -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))?;
Expand Down Expand Up @@ -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)?;
Expand Down
15 changes: 15 additions & 0 deletions tests/cli/since-on-future-package.wit
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// FAIL: component embed --dummy --wat %

package test:invalid@0.1.0;

interface foo {
a: func(s: string) -> string;

@since(version = 0.1.1)
b: func(s: string) -> string;
}

world test {
import foo;
export foo;
}
4 changes: 4 additions & 0 deletions tests/cli/since-on-future-package.wit.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: failed to process feature gate for function [b] in package [test:invalid@0.1.0]

Caused by:
0: feature gates cannot refer to an unreleased (future) package version [0.1.1] in package [test:invalid@0.1.0]
2 changes: 1 addition & 1 deletion tests/cli/wit-stability-in-binary-format.wit
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tests/cli/wit-stability-in-binary-format.wit.stdout
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion tests/cli/wit-stability-inherited.wit
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: component wit %

package a:b;
package a:b@1.0.0;

interface foo {
type t = u32;
Expand Down
2 changes: 1 addition & 1 deletion tests/cli/wit-stability-inherited.wit.stdout
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/// RUN: component wit %
package a:b;
package a:b@1.0.0;

interface foo {
type t = u32;
Expand Down

0 comments on commit fe67930

Please sign in to comment.