Repository: bytecodealliance/wasm-tools
Pull request #2046 (2025-02-10 17:18:13)

Fix handling stability of `use` of "foreign" types
This commit addresses an issue where stability attributes on a `use` didn't quite work as expected when a type to another package was referred to. This fix was to update the "merging" process to skip types being processed in one more location which involved threading some more contexts around. Additionally `use` items, when elaborated, now contain their stability instead of the default stability to ensure that's propagated correctly as well.

cc #1995 but doesn't fix it


Patch:
diff --git a/crates/wit-parser/src/ast.rs b/crates/wit-parser/src/ast.rs
index 16c8b147..f7091fdf 100644
--- a/crates/wit-parser/src/ast.rs
+++ b/crates/wit-parser/src/ast.rs
@@ -142,6 +142,7 @@ impl<'a> DeclList<'a> {
         &'b self,
         f: &mut dyn FnMut(
             Option<&'b Id<'a>>,
+            &'b [Attribute<'a>],
             &'b UsePath<'a>,
             Option<&'b [UseName<'a>]>,
             WorldOrInterface,
@@ -158,42 +159,58 @@ impl<'a> DeclList<'a> {
                     let mut exports = Vec::new();
                     for item in world.items.iter() {
                         match item {
-                            WorldItem::Use(u) => {
-                                f(None, &u.from, Some(&u.names), WorldOrInterface::Interface)?
-                            }
-                            WorldItem::Include(i) => {
-                                f(Some(&world.name), &i.from, None, WorldOrInterface::World)?
-                            }
+                            WorldItem::Use(u) => f(
+                                None,
+                                &u.attributes,
+                                &u.from,
+                                Some(&u.names),
+                                WorldOrInterface::Interface,
+                            )?,
+                            WorldItem::Include(i) => f(
+                                Some(&world.name),
+                                &i.attributes,
+                                &i.from,
+                                None,
+                                WorldOrInterface::World,
+                            )?,
                             WorldItem::Type(_) => {}
-                            WorldItem::Import(Import { kind, .. }) => imports.push(kind),
-                            WorldItem::Export(Export { kind, .. }) => exports.push(kind),
+                            WorldItem::Import(Import {
+                                kind, attributes, ..
+                            }) => imports.push((kind, attributes)),
+                            WorldItem::Export(Export {
+                                kind, attributes, ..
+                            }) => exports.push((kind, attributes)),
                         }
                     }
 
-                    let mut visit_kind = |kind: &'b ExternKind<'a>| match kind {
-                        ExternKind::Interface(_, items) => {
-                            for item in items {
-                                match item {
-                                    InterfaceItem::Use(u) => f(
-                                        None,
-                                        &u.from,
-                                        Some(&u.names),
-                                        WorldOrInterface::Interface,
-                                    )?,
-                                    _ => {}
+                    let mut visit_kind =
+                        |kind: &'b ExternKind<'a>, attrs: &'b [Attribute<'a>]| match kind {
+                            ExternKind::Interface(_, items) => {
+                                for item in items {
+                                    match item {
+                                        InterfaceItem::Use(u) => f(
+                                            None,
+                                            &u.attributes,
+                                            &u.from,
+                                            Some(&u.names),
+                                            WorldOrInterface::Interface,
+                                        )?,
+                                        _ => {}
+                                    }
                                 }
+                                Ok(())
                             }
-                            Ok(())
-                        }
-                        ExternKind::Path(path) => f(None, path, None, WorldOrInterface::Interface),
-                        ExternKind::Func(..) => Ok(()),
-                    };
+                            ExternKind::Path(path) => {
+                                f(None, attrs, path, None, WorldOrInterface::Interface)
+                            }
+                            ExternKind::Func(..) => Ok(()),
+                        };
 
-                    for kind in imports {
-                        visit_kind(kind)?;
+                    for (kind, attrs) in imports {
+                        visit_kind(kind, attrs)?;
                     }
-                    for kind in exports {
-                        visit_kind(kind)?;
+                    for (kind, attrs) in exports {
+                        visit_kind(kind, attrs)?;
                     }
                 }
                 AstItem::Interface(i) => {
@@ -201,6 +218,7 @@ impl<'a> DeclList<'a> {
                         match item {
                             InterfaceItem::Use(u) => f(
                                 Some(&i.name),
+                                &u.attributes,
                                 &u.from,
                                 Some(&u.names),
                                 WorldOrInterface::Interface,
@@ -212,7 +230,13 @@ impl<'a> DeclList<'a> {
                 AstItem::Use(u) => {
                     // At the top-level, we don't know if this is a world or an interface
                     // It is up to the resolver to decides how to handle this ambiguity.
-                    f(None, &u.item, None, WorldOrInterface::Unknown)?;
+                    f(
+                        None,
+                        &u.attributes,
+                        &u.item,
+                        None,
+                        WorldOrInterface::Unknown,
+                    )?;
                 }
 
                 AstItem::Package(pkg) => pkg.decl_list.for_each_path(f)?,
diff --git a/crates/wit-parser/src/ast/resolve.rs b/crates/wit-parser/src/ast/resolve.rs
index 4ddca1b9..a1392d6a 100644
--- a/crates/wit-parser/src/ast/resolve.rs
+++ b/crates/wit-parser/src/ast/resolve.rs
@@ -257,7 +257,7 @@ impl<'a> Resolver<'a> {
         let mut foreign_worlds = mem::take(&mut self.foreign_worlds);
         for decl_list in decl_lists {
             decl_list
-                .for_each_path(&mut |_, path, _names, world_or_iface| {
+                .for_each_path(&mut |_, _attrs, path, _names, world_or_iface| {
                     let (id, name) = match path {
                         ast::UsePath::Package { id, name } => (id, name),
                         _ => return Ok(()),
@@ -433,7 +433,7 @@ impl<'a> Resolver<'a> {
 
             // With this file's namespace information look at all `use` paths
             // and record dependencies between interfaces.
-            decl_list.for_each_path(&mut |iface, path, _names, _| {
+            decl_list.for_each_path(&mut |iface, _attrs, path, _names, _| {
                 // If this import isn't contained within an interface then it's
                 // in a world and it doesn't need to participate in our
                 // topo-sort.
@@ -561,11 +561,12 @@ impl<'a> Resolver<'a> {
     fn populate_foreign_types(&mut self, decl_lists: &[ast::DeclList<'a>]) -> Result<()> {
         for (i, decl_list) in decl_lists.iter().enumerate() {
             self.cur_ast_index = i;
-            decl_list.for_each_path(&mut |_, path, names, _| {
+            decl_list.for_each_path(&mut |_, attrs, path, names, _| {
                 let names = match names {
                     Some(names) => names,
                     None => return Ok(()),
                 };
+                let stability = self.stability(attrs)?;
                 let (item, name, span) = self.resolve_ast_item_path(path)?;
                 let iface = self.extract_iface_from_item(&item, &name, span)?;
                 if !self.foreign_interfaces.contains(&iface) {
@@ -582,7 +583,7 @@ impl<'a> Resolver<'a> {
                     }
                     let id = self.types.alloc(TypeDef {
                         docs: Docs::default(),
-                        stability: Default::default(),
+                        stability: stability.clone(),
                         kind: TypeDefKind::Unknown,
                         name: Some(name.name.name.to_string()),
                         owner: TypeOwner::Interface(iface),
diff --git a/crates/wit-parser/src/resolve.rs b/crates/wit-parser/src/resolve.rs
index 0d1bc178..060dfb11 100644
--- a/crates/wit-parser/src/resolve.rs
+++ b/crates/wit-parser/src/resolve.rs
@@ -1848,6 +1848,19 @@ package {name} is defined in two different locations:\n\
         })
     }
 
+    /// Convenience wrapper around `include_stability` specialized for types
+    /// with a more targeted error message.
+    fn include_type(&self, ty: &TypeDef, pkgid: PackageId, span: Span) -> Result<bool> {
+        self.include_stability(&ty.stability, &pkgid, Some(span))
+            .with_context(|| {
+                format!(
+                    "failed to process feature gate for type [{}] in package [{}]",
+                    ty.name.as_ref().map(String::as_str).unwrap_or("<unknown>"),
+                    self.packages[pkgid].name,
+                )
+            })
+    }
+
     /// Performs the "elaboration process" necessary for the `world_id`
     /// specified to ensure that all of its transitive imports are listed.
     ///
@@ -2625,12 +2638,6 @@ impl Remap {
         resolve: &mut Resolve,
         unresolved: UnresolvedPackage,
     ) -> Result<PackageId> {
-        self.process_foreign_deps(resolve, &unresolved)?;
-
-        let foreign_types = self.types.len();
-        let foreign_interfaces = self.interfaces.len();
-        let foreign_worlds = self.worlds.len();
-
         let pkgid = resolve.packages.alloc(Package {
             name: unresolved.name.clone(),
             docs: unresolved.docs.clone(),
@@ -2640,6 +2647,12 @@ impl Remap {
         let prev = resolve.package_names.insert(unresolved.name.clone(), pkgid);
         assert!(prev.is_none());
 
+        self.process_foreign_deps(resolve, pkgid, &unresolved)?;
+
+        let foreign_types = self.types.len();
+        let foreign_interfaces = self.interfaces.len();
+        let foreign_worlds = self.worlds.len();
+
         // Copy over all types first, updating any intra-type references. Note
         // that types are sorted topologically which means this iteration
         // order should be sufficient. Also note though that the interface
@@ -2652,16 +2665,7 @@ impl Remap {
             .zip(&unresolved.type_spans)
             .skip(foreign_types)
         {
-            if !resolve
-                .include_stability(&ty.stability, &pkgid, Some(*span))
-                .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,
-                    )
-                })?
-            {
+            if !resolve.include_type(&ty, pkgid, *span)? {
                 self.types.push(None);
                 continue;
             }
@@ -2838,6 +2842,7 @@ impl Remap {
     fn process_foreign_deps(
         &mut self,
         resolve: &mut Resolve,
+        pkgid: PackageId,
         unresolved: &UnresolvedPackage,
     ) -> Result<()> {
         // Invert the `foreign_deps` map to be keyed by world id to get
@@ -2877,7 +2882,7 @@ impl Remap {
 
         // Finally, iterate over all foreign-defined types and determine
         // what they map to.
-        self.process_foreign_types(unresolved, resolve)?;
+        self.process_foreign_types(unresolved, pkgid, resolve)?;
 
         for (id, span) in unresolved.required_resource_types.iter() {
             let mut id = self.map_type(*id, Some(*span))?;
@@ -2988,9 +2993,12 @@ impl Remap {
     fn process_foreign_types(
         &mut self,
         unresolved: &UnresolvedPackage,
+        pkgid: PackageId,
         resolve: &mut Resolve,
     ) -> Result<(), anyhow::Error> {
-        for (unresolved_type_id, unresolved_ty) in unresolved.types.iter() {
+        for ((unresolved_type_id, unresolved_ty), span) in
+            unresolved.types.iter().zip(&unresolved.type_spans)
+        {
             // All "Unknown" types should appear first so once we're no longer
             // in unknown territory it's package-defined types so break out of
             // this loop.
@@ -2998,6 +3006,12 @@ impl Remap {
                 TypeDefKind::Unknown => {}
                 _ => break,
             }
+
+            if !resolve.include_type(unresolved_ty, pkgid, *span)? {
+                self.types.push(None);
+                continue;
+            }
+
             let unresolved_iface_id = match unresolved_ty.owner {
                 TypeOwner::Interface(id) => id,
                 _ => unreachable!(),


Test patch:
diff --git a/crates/wit-parser/tests/ui/gated-use.wit b/crates/wit-parser/tests/ui/gated-use.wit
new file mode 100644
index 00000000..04e28647
--- /dev/null
+++ b/crates/wit-parser/tests/ui/gated-use.wit
@@ -0,0 +1,13 @@
+package wasmtime:test;
+
+interface types {
+  @unstable(feature = inactive)
+  use wasi:dep2/stable@0.2.3.{unstable-resource};
+}
+
+package wasi:dep2@0.2.3 {
+  interface stable {
+    @unstable(feature = inactive)
+    resource unstable-resource;
+  }
+}
diff --git a/crates/wit-parser/tests/ui/gated-use.wit.json b/crates/wit-parser/tests/ui/gated-use.wit.json
new file mode 100644
index 00000000..7a823b83
--- /dev/null
+++ b/crates/wit-parser/tests/ui/gated-use.wit.json
@@ -0,0 +1,34 @@
+{
+  "worlds": [],
+  "interfaces": [
+    {
+      "name": "stable",
+      "types": {},
+      "functions": {},
+      "package": 0
+    },
+    {
+      "name": "types",
+      "types": {},
+      "functions": {},
+      "package": 1
+    }
+  ],
+  "types": [],
+  "packages": [
+    {
+      "name": "wasi:dep2@0.2.3",
+      "interfaces": {
+        "stable": 0
+      },
+      "worlds": {}
+    },
+    {
+      "name": "wasmtime:test",
+      "interfaces": {
+        "types": 1
+      },
+      "worlds": {}
+    }
+  ]
+}
\ No newline at end of file
diff --git a/crates/wit-parser/tests/ui/parse-fail/bad-pkg6.wit.result b/crates/wit-parser/tests/ui/parse-fail/bad-pkg6.wit.result
index 5ca6c2d7..0adea11b 100644
--- a/crates/wit-parser/tests/ui/parse-fail/bad-pkg6.wit.result
+++ b/crates/wit-parser/tests/ui/parse-fail/bad-pkg6.wit.result
@@ -1,5 +1,6 @@
 failed to resolve directory while parsing WIT for path [tests/ui/parse-fail/bad-pkg6]: package 'foo:bar' not found. known packages:
     foo:baz
+    foo:foo
 
      --> tests/ui/parse-fail/bad-pkg6/root.wit:3:7
       |
diff --git a/crates/wit-parser/tests/ui/parse-fail/unresolved-interface4.wit.result b/crates/wit-parser/tests/ui/parse-fail/unresolved-interface4.wit.result
index e2417d69..2c9268f9 100644
--- a/crates/wit-parser/tests/ui/parse-fail/unresolved-interface4.wit.result
+++ b/crates/wit-parser/tests/ui/parse-fail/unresolved-interface4.wit.result
@@ -1,4 +1,6 @@
-package 'some:dependency' not found. no known packages.
+package 'some:dependency' not found. known packages:
+    foo:foo
+
      --> tests/ui/parse-fail/unresolved-interface4.wit:6:10
       |
     6 |   import some:dependency/iface;