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 29, 2024
1 parent 311abc7 commit d7982b3
Show file tree
Hide file tree
Showing 14 changed files with 185 additions and 37 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
178 changes: 152 additions & 26 deletions crates/wit-parser/src/resolve.rs
Original file line number Diff line number Diff line change
@@ -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")]
Expand All @@ -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.
///
Expand Down Expand Up @@ -1408,13 +1410,55 @@ impl Resolve {
}
}

fn include_stability(&self, stability: &Stability) -> bool {
match stability {
Stability::Stable { .. } | Stability::Unknown => true,
Stability::Unstable { feature, .. } => {
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,
deprecated,
} => {
// If there's a package version and a deprecation, and the deprecation version
// is already in effect, do not include the gated item
if self
.packages
.get(*pkg_id)
.and_then(|p| p.name.version.as_ref())
.is_some_and(|pkg_version| {
deprecated
.as_ref()
.is_some_and(|deprecation_version| deprecation_version <= pkg_version)
})
{
return Ok(false);
}

self.features.contains(feature) || self.all_features
}
}
})
}
}

Expand Down Expand Up @@ -1503,7 +1547,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 +1597,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 +1655,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 +2019,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 +2038,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 +2115,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 +2146,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 +2196,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 +2238,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
18 changes: 18 additions & 0 deletions tests/cli/since-on-future-package.wit
Original file line number Diff line number Diff line change
@@ -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;
}
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 gate cannot reference unreleased version 0.1.1 of package [test:invalid@0.1.0] (current version 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 d7982b3

Please sign in to comment.