From 7ee44d95423a2ba4c5eff9f3bc4db05ed6a622c6 Mon Sep 17 00:00:00 2001 From: Victor Adossi Date: Fri, 19 Jul 2024 02:40:19 +0900 Subject: [PATCH] fix(wit-parser): improve checking for stable feature gates This commit introduces some extra checking for stabilized feature gates (i.e. `@since` & in-code `Stability::Stable`) with a version and/or feature specified. There are two primary changes: - Ensure that packages containing a `@since` annotation must have a version specified - Referring to a future unreleased package version in a `@since` annotation is an error 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 --- .../tests/merge/success/from/a.wit | 2 +- .../merge/success/from/deps/foo/shared.wit | 2 +- .../tests/merge/success/into/b.wit | 2 +- .../merge/success/into/deps/foo/shared.wit | 2 +- .../tests/merge/success/merge/foo.wit | 2 +- .../tests/merge/success/merge/from.wit | 2 +- .../tests/merge/success/merge/into.wit | 2 +- crates/wit-parser/src/resolve.rs | 158 +++++++++++++++--- tests/cli/since-on-future-package.wit | 18 ++ tests/cli/since-on-future-package.wit.stderr | 4 + tests/cli/wit-stability-in-binary-format.wit | 2 +- .../wit-stability-in-binary-format.wit.stdout | 2 +- tests/cli/wit-stability-inherited.wit | 2 +- tests/cli/wit-stability-inherited.wit.stdout | 2 +- 14 files changed, 166 insertions(+), 36 deletions(-) create mode 100644 tests/cli/since-on-future-package.wit create mode 100644 tests/cli/since-on-future-package.wit.stderr diff --git a/crates/wit-component/tests/merge/success/from/a.wit b/crates/wit-component/tests/merge/success/from/a.wit index d181c7f53e..0680ab711b 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 272c592880..c2df6de713 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 ae3681207b..d636b6fa75 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 0adea4f0fc..5a97ef93d9 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 f209dac2a5..c8b446a981 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 a57ad14152..b080c2fc7b 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 7448a6a228..ae40b42753 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/crates/wit-parser/src/resolve.rs b/crates/wit-parser/src/resolve.rs index 9a69eba45d..5471a54c18 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 { + 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(""), + 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(""), + 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(""), + ) + }); // 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)?; diff --git a/tests/cli/since-on-future-package.wit b/tests/cli/since-on-future-package.wit new file mode 100644 index 0000000000..1c9a8eb6db --- /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 0000000000..b1c0e6e14f --- /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 fb56e018ff..8ab7149729 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 ce05750ad4..f6dd5adc68 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 572e1c2579..6dd7249f3b 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 651b90e2f8..19b3df4505 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;