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 23, 2024
1 parent 263b697 commit e099e8b
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 23 deletions.
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
137 changes: 119 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 @@ -1329,11 +1329,30 @@ 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, .. } => {
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,
);
}

// 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 @@ -1422,7 +1441,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 @@ -1463,13 +1491,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 @@ -1508,11 +1549,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 @@ -1864,6 +1913,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 @@ -1873,17 +1932,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 @@ -1936,6 +2009,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 @@ -1966,7 +2040,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 @@ -2007,7 +2090,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 @@ -2040,7 +2132,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 e099e8b

Please sign in to comment.