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

Remove "interface imports" from binary syntax, expand accepted strings #1262

Merged
merged 12 commits into from
Oct 28, 2023

Conversation

alexcrichton
Copy link
Member

This PR is the spiritual successor of #1146 and is an implementation of these PRs together for this repository:

This is technically a breaking change to the binary format, but the breaking change is so minor that a gradual rollout is implemented here. Previously import/export names had a discriminator byte of 0x00 or 0x01 depending on if they were a "plain name" or an "interface name". They no longer have a discriminator byte and instead always use 0x00. This means that old binaries are easily read by discarding the 0x00 or 0x01 byte. New binaries are, for now, temporarily created with the 0x01 byte still to ensure that binaries produced with this tooling is still compatible with older tooling. Once this tooling has permeated enough I plan to change the default emission to be 0x00 instead of 0x01.

The text syntax was changed in this PR as well. Previously where (interface "a:b/c") was printed only "a:b/c" is printed now. Like the binary format the text format still parses the old form with the intention of being deleted eventually.

Finally this PR additionally expands the set of accepted syntax within import and export strings. This now includes imports of implementations (locked/unlocked deps), imports of URLs (relative and not), and integrity-based imports.

@alexcrichton alexcrichton marked this pull request as draft October 24, 2023 23:48
@alexcrichton
Copy link
Member Author

alexcrichton commented Oct 24, 2023

TODO items:

  • Validate that exports can't use urls/implementations/etc, basically only imports get the new syntax
  • Parsing integrity-metadata currently does nothing but is defined by w3c. Should figure out what to do in the component model

@alexcrichton
Copy link
Member Author

Ok I've updated with parsing of what I believe is the format of integrity-metadata so this is good to go I think

This commit removes the `wasmparser::ComponentExternName` type and
updates wasmparser to handle WebAssembly/component-model#263. Names are
now always stored as a strings and are not discriminated in the binary
format. This additionally refactors the internals of `wasmparser` to
handle parsed names differently and updates tools like `wit-component`
to handle the new API changes.

Test aren't passing yet but things are compiling at least.
Also update `wasm-encoder`'s API for the new AST.
In wasm-encoder produce the "old" encoding by default rather than the
new encoding. This can get removed once enough tooling supports the
"new" encoding.
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

This looks great! I'm really happy with where we landed with the strings and relaxing the restriction on conflicting names for all externs.

Just a few feedback comments.

crates/wasm-encoder/src/component/instances.rs Outdated Show resolved Hide resolved
crates/wasmparser/src/validator/component.rs Outdated Show resolved Hide resolved
crates/wasmparser/src/validator/names.rs Outdated Show resolved Hide resolved
crates/wit-component/src/metadata.rs Outdated Show resolved Hide resolved
tests/cli/print-core-wasm-wit.wit.stdout Outdated Show resolved Hide resolved
@alexcrichton alexcrichton merged commit 112285c into bytecodealliance:main Oct 28, 2023
15 checks passed
@alexcrichton alexcrichton deleted the impl-imports-round-2 branch October 28, 2023 22:44
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Oct 30, 2023
Primarily releasing bytecodealliance#1262 to get that into the ecosystem ASAP.
alexcrichton added a commit that referenced this pull request Oct 30, 2023
Primarily releasing #1262 to get that into the ecosystem ASAP.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Oct 30, 2023
This commit updates the wasm-tools family of crate for a number of
notable updates:

* bytecodealliance/wasm-tools#1257 - wasmparser's ID-based
  infrastructure has been refactored to have more precise types for each
  ID rather than one all-purpose `TypeId`.
* bytecodealliance/wasm-tools#1262 - the implementation of
  "implementation imports" for the component model which both updates
  the binary format in addition to adding more syntactic forms of
  imports.
* bytecodealliance/wasm-tools#1260 - a new encoding scheme for component
  information for `wit-component` in objects (not used by Wasmtime but
  used by bindings generators).

Translation for components needed to be updated to account for the first
change, but otherwise this was a straightforward update.
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request Oct 30, 2023
* Update wasm-tools crates

This commit updates the wasm-tools family of crate for a number of
notable updates:

* bytecodealliance/wasm-tools#1257 - wasmparser's ID-based
  infrastructure has been refactored to have more precise types for each
  ID rather than one all-purpose `TypeId`.
* bytecodealliance/wasm-tools#1262 - the implementation of
  "implementation imports" for the component model which both updates
  the binary format in addition to adding more syntactic forms of
  imports.
* bytecodealliance/wasm-tools#1260 - a new encoding scheme for component
  information for `wit-component` in objects (not used by Wasmtime but
  used by bindings generators).

Translation for components needed to be updated to account for the first
change, but otherwise this was a straightforward update.

* Remove a TODO
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Sep 3, 2024
Starting in WebAssembly/component-model#263 the component model binary
specification was updated in a technically breaking way to encode
binaries differently. This was intended to be rolled out in a manner
that minimized breakage however so bytecodealliance#1262
implemented validation where the prefix byte which changed was actually
ignored if it was 0 or 1. The encoder at the time still emitted 1 as the
prefix byte, however. The intention was that once the validator had
percolated enough the encoder would switch to using 0.

Unfortunately this change was basically forgotten about until now with
WebAssembly/component-model#391, but now's probably "enough time passed"
so the encoder is updated to emit a 0x00 leading byte in this situation
instead of the historical 0x01 byte. This will start the transition
period to eventually removing the validator's acceptance of the 0x01
byte.
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Sep 3, 2024
Starting in WebAssembly/component-model#263 the component model binary
specification was updated in a technically breaking way to encode
binaries differently. This was intended to be rolled out in a manner
that minimized breakage however so bytecodealliance#1262
implemented validation where the prefix byte which changed was actually
ignored if it was 0 or 1. The encoder at the time still emitted 1 as the
prefix byte, however. The intention was that once the validator had
percolated enough the encoder would switch to using 0.

Unfortunately this change was basically forgotten about until now with
WebAssembly/component-model#391, but now's probably "enough time passed"
so the encoder is updated to emit a 0x00 leading byte in this situation
instead of the historical 0x01 byte. This will start the transition
period to eventually removing the validator's acceptance of the 0x01
byte.
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
* Proceed to the next step in the "remove `interface`" transition

Starting in WebAssembly/component-model#263 the component model binary
specification was updated in a technically breaking way to encode
binaries differently. This was intended to be rolled out in a manner
that minimized breakage however so #1262
implemented validation where the prefix byte which changed was actually
ignored if it was 0 or 1. The encoder at the time still emitted 1 as the
prefix byte, however. The intention was that once the validator had
percolated enough the encoder would switch to using 0.

Unfortunately this change was basically forgotten about until now with
WebAssembly/component-model#391, but now's probably "enough time passed"
so the encoder is updated to emit a 0x00 leading byte in this situation
instead of the historical 0x01 byte. This will start the transition
period to eventually removing the validator's acceptance of the 0x01
byte.

* Update test expectation
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.

2 participants