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 99bbd65
Show file tree
Hide file tree
Showing 15 changed files with 225 additions and 82 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
190 changes: 164 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, 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,67 @@ 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,
Stability::Stable {
since,
feature,
deprecated,
} => {
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 version has been deprecated already, we should exclude it
if deprecated.as_ref().is_some_and(|v| v <= package_version) {
return Ok(false);
}

// If the version on the feature gate is already released, we know we can include regardless of features
if since <= package_version {
return Ok(true);
}

// If the package version is in the future, but the feature is enabled, we can include
if feature.as_ref().is_some_and(|f| self.features.contains(f)) {
return Ok(true);
}

// At this point the since version must be in the future, with no way to pull it into the present (i.e. via feature)
return Ok(false);
} else {
// If we don't have a package_version to compare, we can't say much about the item.
// Since version & deprecations can't be checked because there's no package version to compare to.
//
// We *can* check features though.
return Ok(feature.is_none()
|| self.all_features
|| feature.as_ref().is_some_and(|f| self.features.contains(f)));
}
}
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 +1559,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 +1609,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 +1667,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 +2031,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 +2050,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 +2127,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 +2158,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 +2208,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 +2250,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
Loading

0 comments on commit 99bbd65

Please sign in to comment.