Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wit: propose to propogate World's name to inline Interfaces' names #1805

Open
Mossaka opened this issue Sep 19, 2024 · 9 comments
Open

wit: propose to propogate World's name to inline Interfaces' names #1805

Mossaka opened this issue Sep 19, 2024 · 9 comments

Comments

@Mossaka
Copy link
Member

Mossaka commented Sep 19, 2024

Consider the following WIT file

package example:name;

interface exports {
  foo: func();
}

world only-exports {
  export exports;
  export exports: interface {
      foo: func();
  }
}

The Resolve representation of the above World will have two interfaces, one with name: Some("exports") and the other with name: None, but it left some ambiguity on how to interpret the name of the second "annonymous" interface. For example, in wit-bindgen, the interpretation of the name of the seond interface is the World's key name, namely "exports".

I am proposing to change the type of Interface::name from Option<String> to String, and should use the correct naming mechanism so that tools like wit-bindgen or wit-bindgen-go do not have to guess what interface names are in case when they are inlinedly defined.

The first export's name should be "example:name/exports", and the second export's name should be "exports", so that there is no colliding. It also matches the definition in the component model spec.

For reference, this is the wat representation of the above WIT

(component
  (type (;0;)
    (component
      (type (;0;)
        (instance
          (type (;0;) (func))
          (export (;0;) "foo" (func (type 0)))
        )
      )
      (export (;0;) "example:name/exports" (instance (type 0)))
    )
  )
  (export (;1;) "exports" (type 0))
  (type (;2;)
    (component
      (type (;0;)
        (component
          (type (;0;)
            (instance
              (type (;0;) (func))
              (export (;0;) "foo" (func (type 0)))
            )
          )
          (export (;0;) "example:name/exports" (instance (type 0)))
          (type (;1;)
            (instance
              (type (;0;) (func))
              (export (;0;) "foo" (func (type 0)))
            )
          )
          (export (;1;) "exports" (instance (type 1)))
        )
      )
      (export (;0;) "example:name/only-exports" (component (type 0)))
    )
  )
  (export (;3;) "only-exports" (type 2))
  (@custom "package-docs" "\00{}")
  (@producers
    (processed-by "wit-component" "0.216.0")
  )
)

Discussion

In addition to the proposal, I found that it's a bit strange that I can't reference the first export using a fully qualified name like the one below:

world only-exports {
  export example:name/exports;
  export exports: interface {
      foo: func();
  }
}

wasm-tools will throw an error saying

error: package depends on itself
     --> testdata/issues/issue170.wit:8:10
      |
    8 |   export example:name/exports;
      |          ^-----------

I realized this is, by design, prohibited, but I have to admit that it striked me as a surprise.

CC @alexcrichton

@alexcrichton
Copy link
Member

The original intention was that codegen based on a world would use the WorldKey structure to name exports, not the interface names and that way they'd get the two names that you're expecting here. There's definitely a lot of subtelty in writing a robust bindings generator for WIT for sure, but would that sort of refactoring help the wit-bindgen-go case? That's how wit-bindgen is structured right now (IIRC)

For the package-depending-on-itself I think that's a bug that shouldn't be too hard to fix. Want to open a dedicated issue for that?

@Mossaka
Copy link
Member Author

Mossaka commented Sep 19, 2024

Thanks for the explanation. I understand the design better now. I guess we can push the logic of "guessing" interface names from the WorldKey struct in consumers of Resolve structure, namely, the WIT bindgen projects, to the Resolve structure itself, right? After #1809 is resolved, interface name can technically be fully qualified when parsed and resolved by wasm-tools component wit. So the example WIT file would be output as

package example:name;

interface exports {
  foo: func();
}

world only-exports {
  export example:name/exports;
  export exports: interface {
      foo: func();
  }
}

Then, there is no ambiguity over the question "what's the name of the interface A, B, C..."

@ydnar
Copy link
Contributor

ydnar commented Sep 19, 2024

I think the current design moves some of the heavy lifting onto wit-bindgen and wit-bindgen-go, perhaps unnecessarily so.

  • Interface names are one example, and I did implement a somewhat ugly workaround to discover the name of an anonymous interface. If WorldKey should be the source of generated names, perhaps remove the name field from the Interface struct altogether?
    • Edit: keys like interface-0 are not useful, and AFAIK shouldn’t appear in generated import or export identifiers.
  • Another case is that interfaces, types, and functions can be imported, exported, or both, and the representation in the Resolve struct (and JSON representation) fuses both into a single instance. Implementing this adds substantial complexity to wit-bindgen-go. proposal: wit-parser: create distinct imported and exported TypeDefs, Functions, and Interfaces in a Resolve #1497

@alexcrichton
Copy link
Member

In general I'm always eager to push more complexity into wit-parser so it doesn't reside externally in bindings generators, I agree it's best to centralize complexity where possible. For the name field on struct Interface though I'm not sure what best to do about that. The name field is an accurate reflection of what's in the WIT itself which is that it's what you write down when you say interface foo { ... } for example. For interface-0 I think you're running into a possibly sub-par encoding of WorldKey into a JSON string, and that should probably be improved. For example $interface-0 or something to clearly indicate it's not a WorldKey::Name or kebab-case string.

For fusing things into a single instance that's definitely a well-known complexity of wit-parser. It's not just causing headaches for wit-bindgen-go but all other bindings generators too. That though I think is orthogonal from this issue?

@ydnar
Copy link
Contributor

ydnar commented Sep 20, 2024

Agreed the problems are orthogonal!

Can name field be consistent with world key in all cases?

@Mossaka
Copy link
Member Author

Mossaka commented Sep 20, 2024

My original proposal was to define the name field of a interface to reflect their fully qualified names, like "example:name/exports" for interfaces defined like interface exports { ... } and "exports" for anonymous interface defined in a World like export exports: interface { ... }.

This actually alignes nicely with the generated Rust namespaces. One is exports::example::name::exports and the second one is exports::exports

Do you think there are any corner cases where this does not cover?

@ydnar
Copy link
Contributor

ydnar commented Sep 20, 2024

wit-bindgen-go uses the WIT package + world or interface name as extension to map to Go package paths, e.g.:

  • wasi:cli/command -> wasi/cli/command
  • wasi:filesystem/types -> wasi/filesystem/types

For this WIT, what would the fully-qualified WIT identifier be for the inline exported exports interface?

package example:name;

interface exports {
  foo: func();
}

world only-exports {
  export example:name/exports;
  export exports: interface {
      foo: func();
  }
}
  • example:name/exports is already taken
  • Should it be example:name/only-exports/exports?

@alexcrichton
Copy link
Member

Can name field be consistent with world key in all cases?

I think for WIT as-of today this can be done, but I'm less certain about the WIT-of-tomorrow. One thing we've talked about is having, for example, just a bare interface { ... } working in a *.wit file or in various contexts. AFAIK the design isn't fully fleshed out nor is there a prototype implementation.

Mostly I'm hesitant to couple this too much together. Is it possible for the Go generator to thread the WorldKey through to where bindings are generated? That would remove the need to look at the name on the interface? Or perhaps the Go generator could postprocess the deserialized world to update the name field given bindings for a particular world?

Another alternative though would be to introduce a second key in Interface along the lines of world_name: String or something like that. That would be the "fully qualified" name (a:b/c) for named interfaces and just the bare name (foo) for import foo: interface { .. }

Do you think there are any corner cases where this does not cover?

For future-compat reasons above I'm hesitant to repurpose name, but also mentioned above having a new key I think might make sense for this?

For this WIT, what would the fully-qualified WIT identifier be for the inline exported exports interface?

In Rust and C a top-level interface or function starts at the root of the bindgen namespace itself. For example in C that would generate void exports_exports_foo(void); and in Rust it would be exports::exports::foo(). The first exports level is a WIT-specific "here's a prefix so imports/exports don't clash". The second exports is the name of the interface, in this case exports, and the foo is the function name.

I'm not familiar enough with Go to know if this translates well, but does that help answer the question perhaps? The intention is that top-level anonymous keys definitely don't get namespaced under the package name, for example.

@ydnar
Copy link
Contributor

ydnar commented Sep 21, 2024

I implemented a fix for this here: https://github.com/bytecodealliance/wasm-tools-go/pull/176

It nests named interfaces imported into or exported from a world under a Go package path for that world.

This allows multiple WIT packages/worlds/interfaces to share a source tree without clobbering each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants