Nested interfaces in wit #1624

macovedj · 2024-06-19T18:02:17Z

This PR enables the ability to describe nested interfaces in wit via the nest keyword.

package foo:bar;

interface baz {
  nest other:pkg/qux
}

This would indicate that that the instance associated with baz would export the instance associated with interface qux from package other:pkg.

(component 
  (type instance
    (type instance 
    ... definition of "other:pkg/qux" 
    )
   (export "other:pkg/qux" type 0)
  )
  (export "foo:bar/baz" type 0)
)

As I understand it, most use cases most immediately benefit from nesting interfaces from foreign packages, so I started with that, though we could certainly extend this PR or have follow ups to support any combo of locally defined/inlined/anonymous interfaces being nested.

This issue points out that wit currently is not able to express nested instances that can be expressed in wat/binary, so wit-bindgen can't be used by language toolchains to generate that binary. This syntax can be leveraged so that the unlocked dependency syntax referenced in the linked issue can specify the exports it expects in the imports it's describing.

alexcrichton

I've done a bit of a cursory review to start off with but I think that this still has a few missing pieces. For example src/resolve.rs in the wit-parser care will need to be updated to handle this new features of interfaces in a number of locations.

At a high level though I think it would be best to propose this feature on the component-model repository first. There's a number of high-level questions about how to design this feature that I think should be thought through such as:

Is the syntax nest a:b/c; what everyone agrees on?
Should nest a; be allowed?
Should nest a { /* ... */ } be allowed?
How will code generators for guest langauges map this?

Those are at least the questions off the top of my head which I think would be best to settle first before finishing up the implementation here. A PR to the component model repository would also help flesh out the wasm-encoding of this WIT construct.

crates/wit-parser/src/ast.rs

crates/wit-parser/src/ast/resolve.rs

alexcrichton · 2024-06-21T18:39:55Z

crates/wit-parser/src/ast/resolve.rs

+                        for nest_item in i.items.iter() {
+                            if let InterfaceItem::Nest(n) = nest_item {
+                                if package_items.insert(n.name.name, n.name.span).is_some() {
+                                    bail!(Error::new(
+                                        n.name.span,
+                                        format!("duplicate item named `{}`", n.name.name),
+                                    ))
+                                }
+                                let prev = decl_list_ns.insert(n.name.name, ());
+                                assert!(prev.is_none());
+                                let prev = order.insert(n.name.name, Vec::new());
+                                assert!(prev.is_none());
+                            }
+                        }


I'm not sure if this is quite right because this seems like it would be an interface-level validation rather than a package-level validation which I think this loop is doing.

I think these changes got removed as a result of implementing some of the other comments made. While part of the logic was for validation, I think that some of the inserts here were needed to properly resolve interfaces, whereas now that's not the case. Given that these lines have been removed, I'm not sure that any validation is needed now? Unless there's a place that you'd expect to see a validation that isn't present.

crates/wit-parser/src/ast/resolve.rs

crates/wit-parser/src/lib.rs

crates/wit-component/src/printing.rs

macovedj · 2024-06-21T18:54:57Z

Sounds good! I can work on opening something up over there.

macovedj · 2024-07-17T17:35:15Z

I think the issues re: encoding seem like they've mostly been hashed out here. While there seems to be a plan for the non-foreign interface nesting, I think it's ok to start with what this PR originally scoped (only nest a:b/c)? There might still be a bit more conversation re: code generation, but I'll probably go ahead and try and address most of the comments here soon.

crates/wit-parser/src/lib.rs

crates/wit-parser/src/ast.rs

crates/wit-parser/src/ast/resolve.rs

crates/wit-parser/src/metadata.rs

crates/wit-component/src/encoding/wit/v2.rs

crates/wit-component/src/encoding/wit/v1.rs

crates/wit-parser/src/decoding.rs

alexcrichton · 2024-09-16T16:19:00Z

crates/wit-component/src/encoding/wit.rs

+    fn interface_path(&self, iface: InterfaceId) -> String {
+        let iface = &self.resolve.interfaces[iface];
+        let pkg_id = iface.package.unwrap();
+        let pkg = &self.resolve.packages[pkg_id];
+        if let Some(v) = &pkg.name.version {
+            format!(
+                "{}:{}/{}@{}",
+                pkg.name.namespace,
+                pkg.name.name,
+                iface.name.as_ref().unwrap(),
+                v.to_string()
+            )
+        } else {
+            format!("{}/{}", pkg.name.to_string(), iface.name.as_ref().unwrap())
+        }
+    }


For this I think Resolve::id_of might be what you want here? (in that I think it's equivalent to this function)

alexcrichton · 2024-09-16T16:22:38Z

crates/wit-component/src/encoding/wit.rs

+            let ty = &self.resolve.types[*id];
+            if let TypeOwner::Interface(iface_id) = ty.owner {
+                self.interface = Some(iface_id);
+            }


I think this might be best done a little differently than updating self.interface here. What I'm worried about is that this updates self.interface but doesn't restore it what it previously was.

Looking throughout this code though push_instance and pop_instance might be a good place to hook into? I think it should be ok to move most modifications of self.interface into those functions where push takes an InterfaceId and sets self.interface and then pop restores the previous value saved during push.

alexcrichton · 2024-09-16T16:23:07Z

crates/wit-parser/src/ast.rs

+        docs: Docs<'a>,
+        attributes: Vec<Attribute<'a>>,
+    ) -> Result<Self> {
+        tokens.eat(Token::Nest)?;


I think you'll want to expect here instead of eat (as eat has a boolean return)

alexcrichton · 2024-09-16T16:27:15Z

crates/wit-parser/src/decoding.rs

+                        _ => bail!("expected interface name"),
+                    }
+                }
+                _ => bail!("instance type export `{name}` is not a type, function or instance"),


I think this name needs to be updated to exp_name

alexcrichton · 2024-09-16T16:27:24Z

crates/wit-parser/src/decoding.rs

+                                stability: Default::default(),
+                            });
+                        }
+                        _ => bail!("expected interface name"),


Mind adding in the exp_name to the error message here?

alexcrichton · 2024-09-16T16:29:39Z

crates/wit-parser/src/decoding.rs

@@ -726,6 +727,40 @@ impl WitPackageDecoder<'_> {
        Ok(())
    }

+    fn register_export(


I think this function may want to get folded into register_import perhaps? (maybe coupled with a function rename). The internal structure looks quite similar and I think it's needed for importing an interface with nest as well?

alexcrichton · 2024-09-16T16:37:02Z

One thing I'd also recommend for testing this is fuzzing. For example here's a small patch to enable some fuzzing of this commit:

diff --git a/crates/wit-smith/src/generate.rs b/crates/wit-smith/src/generate.rs
index 41c5c5c1..82db5fdd 100644
--- a/crates/wit-smith/src/generate.rs
+++ b/crates/wit-smith/src/generate.rs
@@ -344,6 +344,7 @@ impl<'a> InterfaceGenerator<'a> {
             Use,
             Type,
             Function,
+            Nest,
         }

         let mut parts = Vec::new();
@@ -374,6 +375,11 @@ impl<'a> InterfaceGenerator<'a> {
                 Generate::Function => {
                     parts.push(self.gen_func(u)?);
                 }
+                Generate::Nest => {
+                    if let Some(part) = self.gen_nest(u)? {
+                        parts.push(part);
+                    }
+                }
             }
         }

@@ -882,6 +888,15 @@ impl<'a> InterfaceGenerator<'a> {
     fn gen_unique_name(&mut self, u: &mut Unstructured<'_>) -> Result<String> {
         gen_unique_name(u, &mut self.unique_names)
     }
+
+    fn gen_nest(&mut self, u: &mut Unstructured<'_>) -> Result<Option<String>> {
+        let mut path = String::new();
+        if self.gen.gen_path(u, self.file, &mut path)?.is_none() {
+            return Ok(None);
+        }
+
+        Ok(Some(format!("nest {path};")))
+    }
 }

 fn gen_unique_name(u: &mut Unstructured<'_>, set: &mut HashSet<String>) -> Result<String> {

which is executed via

FUZZER=roundtrip_wit cargo +nightly fuzz run -s none run

(you can pass --dev between run and run to run in debug mode too)

That'll help weed out issues dealing with round-tripping interfaces and such. For example that quickly finds this locally for me:

thread '<unnamed>' panicked at fuzz/src/roundtrip_wit.rs:13:61:
called `Result::unwrap()` on an `Err` value: export name `name:name1/[email protected]+1.2.0` conflicts with previous name `name:name1/[email protected]+1.2.0` (at offset 0x6a)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

which I think means two things:

Resolve validation needs to guarantee that you can't nest the same interface twice (maybe this means it should switch to IndexSet?)
The fuzz generator needs to record what was nested and not generate documents that nest the same interface twice. Either that or the error is handled in the fuzzer and the test case is discarded (but I think here the former is the better route to take)

macovedj added 4 commits June 13, 2024 14:29

init

af8879c

bulk

403d1a3

field gates, nesting nest statements, doc comments

e91ccd5

versioned nest package names and general tidying

c9ee248

alexcrichton reviewed Jun 21, 2024

View reviewed changes

macovedj mentioned this pull request Jun 24, 2024

Nested interfaces WebAssembly/component-model#372

Open

nest interface item uses UsePath

9906c2e

macovedj force-pushed the nested-interfaces branch from 3d45221 to 9906c2e Compare July 17, 2024 17:02

macovedj added 6 commits August 12, 2024 10:41

Merge branch 'main' into nested-interfaces

9cd7738

handle nested when resolving interface

e834356

better use interface ids

9d58da3

use iface ids for nested encoding

4728637

print path to interface for nest

9f33021

dedupe nested encoding logic

48c6c86

alexcrichton reviewed Sep 11, 2024

View reviewed changes

macovedj added 4 commits September 12, 2024 12:28

some comments addressed

226fa06

more comments

fc9b6bb

Merge branch 'main' into nested-interfaces

09737ba

cleaner recursion

ba550f4

alexcrichton reviewed Sep 16, 2024

View reviewed changes

alexcrichton self-assigned this Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested interfaces in wit #1624

Nested interfaces in wit #1624

macovedj commented Jun 19, 2024

alexcrichton left a comment

alexcrichton Jun 21, 2024

macovedj Sep 10, 2024

macovedj commented Jun 21, 2024

macovedj commented Jul 17, 2024

alexcrichton Sep 16, 2024

alexcrichton Sep 16, 2024

alexcrichton Sep 16, 2024

alexcrichton Sep 16, 2024

alexcrichton Sep 16, 2024

alexcrichton Sep 16, 2024

alexcrichton commented Sep 16, 2024

Nested interfaces in wit #1624

Are you sure you want to change the base?

Nested interfaces in wit #1624

Conversation

macovedj commented Jun 19, 2024

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macovedj commented Jun 21, 2024

macovedj commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Sep 16, 2024