wasmparser: Use type-specific identifiers pervasively #1257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
FWIW: 3.5 million fuzz iterations and no problems discovered thus far. |
fitzgen
force-pushed
the
different-core-vs-component-type-ids
branch
from
ed6fd19
to
370d151
Compare
This commit changes `wasmparser` from the uni-typed `TypeId` identifier to a myriad of type-specific identifiers, such as `ComponentInstanceTypeId` and `ComponentDefinedTypeId`. There are also identifiers for any kind of type within some group, such as `ComponentAnyTypeId` for any kind of component type and `AnyTypeId` for any kind of type, component or core. The `wasmparser::Types` arena can be indexed by any of these leaf type identifiers, which all implement the `TypeIdentifier` trait. The resulting indexed data is the particular type, e.g. a `ComponentFuncType`, rather than the uni-typed `wasmparser::Type`. There was a lot of ocean boiling involved here. Apologies in advance for the pain of this upgrade! That said, it should provide type safety benefits moving forward and sets the stage for further identifier refactors for core types that are necessary for Wasm GC. If you are upgrading and were using `TypeId` before, and don't want to spend time gaining type safety on your type lookups, then just use `AnyTypeId` instead of the old `TypeId`. They are morally identical. Alternatively, if you know a particular `TypeId` was always a component function, for example, and you always did `types[id].unwrap_component_func()`, then you can switch to using `ComponentFuncTypeId`. Co-Authored-By: Alex Crichton <alex@alexcrichton.com>
fitzgen
force-pushed
the
different-core-vs-component-type-ids
branch
from
370d151
to
60e1398
Compare
alexcrichton
approved these changes
Oct 19, 2023
Comment on lines
+785
to
+788
| /// Unwrap a `FuncType` or panic. | ||
| pub fn unwrap_func(&self) -> &FuncType { | ||
| self.structural_type.unwrap_func() | ||
| } |
There was a problem hiding this comment.
Should this perhaps also check that is_final is true or false? (I forget which it should check for)
There was a problem hiding this comment.
The default is final == true but there is nothing stopping one from defining a non-final function type, and I'd imagine this would still want to return the underlying function type.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit changes
wasmparserfrom the uni-typedTypeIdidentifier to a myriad of type-specific identifiers, such asComponentInstanceTypeIdandComponentDefinedTypeId. There are also identifiers for any kind of type within some group, such asComponentAnyTypeIdfor any kind of component type andAnyTypeIdfor any kind of type, component or core.The
wasmparser::Typesarena can be indexed by any of these leaf type identifiers, which all implement theTypeIdentifiertrait. The resulting indexed data is the particular type, e.g. aComponentFuncType, rather than the uni-typedwasmparser::Type.There was a lot of ocean boiling involved here. Apologies in advance for the pain of this upgrade! That said, it should provide type safety benefits moving forward and sets the stage for further identifier refactors for core types that are necessary for Wasm GC.
If you are upgrading and were using
TypeIdbefore, and don't want to spend time gaining type safety on your type lookups, then just useAnyTypeIdinstead of the oldTypeId. They are morally identical. Alternatively, if you know a particularTypeIdwas always a component function, for example, and you always didtypes[id].unwrap_component_func(), then you can switch to usingComponentFuncTypeId.