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

Update WIT and binary import/export syntax #198

Merged
merged 8 commits into from
Jun 15, 2023

Conversation

alexcrichton
Copy link
Collaborator

This is an update to WIT.md and a minor update to Binary.md to reflect the changes discussed in #193. Lots more discussion is in the issue, and a summary of the changes is:

  • WIT use statements are almost entirely replaced. Old pkg and self-style paths are all gone.
  • WIT documents are gone as a concept, a package is a collection of interfaces/worlds now.
  • The externname binary format production has been updated to either be a kebab-name or an "ID". The URL field has been dropped.
  • Imports and exports of interfaces in WIT worlds is now done through IDs instead of kebab-names.

This is an update to `WIT.md` and a minor update to `Binary.md` to
reflect the changes discussed in WebAssembly#193. Lots more discussion is in the
issue, and a summary of the changes is:

* WIT `use` statements are almost entirely replaced. Old `pkg` and
  `self`-style paths are all gone.
* WIT documents are gone as a concept, a package is a collection of
  interfaces/worlds now.
* The `externname` binary format production has been updated to either
  be a kebab-name or an "ID". The URL field has been dropped.
* Imports and exports of interfaces in WIT worlds is now done through
  IDs instead of kebab-names.
@alexcrichton
Copy link
Collaborator Author

I'll also note that I'm in the process of implementing these changes. Current work is:

Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some small suggestions and one interesting question at the end about the encoding of Wit into Wat.

design/mvp/Binary.md Outdated Show resolved Hide resolved
design/mvp/Binary.md Outdated Show resolved Hide resolved
design/mvp/Binary.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work. This all seems in line with previous discussions. I have two major concerns:

  1. Making package declarations required adds possible redundancy to componentization use cases.
  2. Disallowing non-package imports as being resolved by tooling bypasses some interesting usage scenarios.

Both seem relatively surface-level restrictions to me though, and I've tried to go into as much detail as I can on these.

design/mvp/WIT.md Show resolved Hide resolved
Comment on lines 35 to 37
* A namespace, for example `foo` in `foo:bar`. This namespace is intended to
disambiguate between registries, top-level organizations, etc. For example
WASI interfaces use the `wasi:` namespace.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One framing might be that if the namespace is optional, that the optional namespace and version both being omitted is then equivalent to the kebab name form. A version without a namespace would not make sense though.

Something like:

Suggested change
* A namespace, for example `foo` in `foo:bar`. This namespace is intended to
disambiguate between registries, top-level organizations, etc. For example
WASI interfaces use the `wasi:` namespace.
* An optional namespace, for example `foo` in `foo:bar`. This namespace is intended to
disambiguate between registries, top-level organizations, etc. For example
WASI interfaces use the `wasi:` namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making namespaces optional, which I think would be a half-way measure, I think these cases should just not use (interface <id>) and produce plain kebab-names instead.

design/mvp/WIT.md Show resolved Hide resolved
import clock: wasi-clock.clock
import wasi:filesystem/filesystem
import wasi:random/random
import wasi:clocks/monotonic-clock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so nice to see!

design/mvp/WIT.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alexcrichton for working through my questions today. To summarize -

The kebab names are still being supported fine with inline types which mostly handles the major use cases. Any additional use cases that may be supported in future for kebab names or instance imports can be designed on their own merits.

Finally, with regards to having good conventions for unknown package namespaces in the "optional" scenarios, I've suggested the use of local: in the examples as a possible name here for now. I'm open to other alternatives, but that would mitigate my concern of wit: being used as an example here then being taken up more widely (or some other ambiguous namespacing). Happy to bikeshed further of course!

design/mvp/WIT.md Outdated Show resolved Hide resolved

* An optional version, specified as a major and minor number. Because WIT
packages define *interfaces*, not *implementations*, there is not a patch
number, as in full semver. Both version numbers must be unsigned integers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did realize a potential use case for including an optional patch version: when you're iterating on a pre-1.0 interface, and minor version bumps can be breaking, then you might want to use patch version changes to signal non-breaking interface changes. Concretely, I could imagine that for Preview 2, we'd want to publish 0. versions of WASI interfaces (say, 0.2), and then if we make non-breaking changes post-Preview 2, we could publish them as 0.2.x versions (saving 0.3 for Preview 3).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also allow a pre-release version number and potentially a build number. I think this would more closely match the actual semantics (i.e., it's not a patch; it's a pre-release)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past we had discussed using patch versions for non-functional changes. Perhaps the only, but still important, change of this kind would be documentation changes. I think those warrant the inclusion of patch version numbers

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds reasonable to me, I went ahead and added full semver in including pre-release/build metadata.

design/mvp/WIT.md Outdated Show resolved Hide resolved

* An optional version, specified as a major and minor number. Because WIT
packages define *interfaces*, not *implementations*, there is not a patch
number, as in full semver. Both version numbers must be unsigned integers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also allow a pre-release version number and potentially a build number. I think this would more closely match the actual semantics (i.e., it's not a patch; it's a pre-release)

Comment on lines +62 to +63
must specify a `package` identifier. Multiple files can specify a `package` and
they must all agree on what the identifier is.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to allow multiple files to contain this identifier? Why not constrain to one file and if it becomes necessary in the future loosen that requirement?

Additionally, it might be a good idea to make a note of a possible convention of having this identifier in the a main.wit file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guybedford do you have thoughts on this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be open to changing the constraint to a single file, but see no problem with the redundancy either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the concept of main doesn't quite apply to WIT currently since all WIT files are equal. Unless we introduce preferential lookup rules for default worlds and interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other tools that have this convention. For example, terraform has the convention of main files that (I believe) have no significance other than to say to the reader "look here first".

@@ -40,6 +77,8 @@ belong to an interface.
An example of an interface is:

```wit
package local:demo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: local seems like it might have some sort of specific meaning, but as far as I understand this can be anything. For illustrative purposes, perhaps you can use something that is more clearly user defined like mynamespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guybedford you might have thoughts on this? (I don't mind either way myself)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was exactly that asking users to choose an arbitrary namespace at an early stage risks exactly the naming problem namespaces are supposed to avoid in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does local actually have a meaning here or is this a convention you're trying to establish or is this purely descriptive? Either way, it would be nice to explicitly state that in this document.

design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
design/mvp/WIT.md Outdated Show resolved Hide resolved
Copy link
Member

@lukewagner lukewagner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks! Explainer.md and the text format still need to be updated, but I'm happy to do that as a quick follow-on once you merge this PR.

sunfishcode added a commit to sunfishcode/wasi-proposal-template that referenced this pull request Jun 8, 2023
This updates for the [recent syntax changes].

[recent syntax changes]: WebAssembly/component-model#198
@guybedford
Copy link
Collaborator

guybedford commented Jun 9, 2023

With regards to the ESM integration, it would be great to handle this in this PR as well since those behaviours do need to be worked out as part of this. My suggestion would be a note along the lines of the following bullet points:

  • When importing components in the ESM integration, ESM-integration-compatible components are those that do not use namespaces but instead use kebab names in the outer (exposed to the esm integration) import and export positions, or optionally URLs in the outer import position.
  • When converting components into esm-integration-compatible components, namespaced exports may be provided by convention as interfaces nested under the namespace and package interface names. In addition, if there are no conflicts against other exports, the interface being exported may be treated as a direct export of its own name in this convention.

sunfishcode added a commit to WebAssembly/wasi-proposal-template that referenced this pull request Jun 10, 2023
* Update to the latest wit-bindgen.

This updates for the [recent syntax changes].

[recent syntax changes]: WebAssembly/component-model#198

* Update the wit sources to the latest syntax, and update the generated files.
@Mossaka Mossaka mentioned this pull request Jun 14, 2023
@alexcrichton
Copy link
Collaborator Author

Ok I'm going to go ahead and merge this, if at least to have the current spec reflect what's implemented now in tooling. I talked with @lukewagner and he'll update Explainer.md shortly afterwards and I think any further minor updates can happen in subsequent PRs. Thanks all for the discussion here!

@alexcrichton alexcrichton merged commit 1bd1266 into WebAssembly:main Jun 15, 2023
@alexcrichton alexcrichton deleted the import-export-changes branch June 15, 2023 16:19
@guybedford
Copy link
Collaborator

@alexcrichton if I had known that the ESM integration upgrade wasn't going to be made in this PR I likely wouldn't have approved it. There are not "further minor updates" but fundamental details of the model that we cannot ignore.

@lukewagner
Copy link
Member

ESM-integration updates aren't "minor", but I do think this and other language integrations questions can be done as follow-ups without changing what's in the AST and binary, so it's a good idea to merge now and iterate forward. I'll be working to update some other parts of Explainer.md shortly.

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

Successfully merging this pull request may close these issues.

5 participants