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.

Sign up for GitHub

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

Jump to bottom

Update accessors on TypesRef #1094

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Update accessors on TypesRef #1094

merged 1 commit into from
Jul 6, 2023

Conversation

alexcrichton
Copy link
Member

Previously accessors of type information would return the actual information itself, and this commit updates them to instead return the TypeId. A TypeId can be turned into the concrete type information and otherwise upcoming resources support in Wasmtime needs more aggressive use of TypeId so I found it easier to have those returned from these methods since it's not possible to go from &FuncType to TypeId.

Additionally this commit adds accessors for the size of all the index spaces in the component model and core wasm binary format. This enables witnessing intermediate state while validating a component for example which is also used in the upcoming support for resources in Wasmtime.

@alexcrichton
Update accessors on TypesRef
ac7403b 
Previously accessors of type information would return the actual
information itself, and this commit updates them to instead return the
`TypeId`. A `TypeId` can be turned into the concrete type information
and otherwise upcoming resources support in Wasmtime needs more
aggressive use of `TypeId` so I found it easier to have those returned
from these methods since it's not possible to go from `&FuncType` to
`TypeId`.

Additionally this commit adds accessors for the size of all the index
spaces in the component model and core wasm binary format. This enables
witnessing intermediate state while validating a component for example
which is also used in the upcoming support for resources in Wasmtime.
sunfishcode
sunfishcode approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

The sequence of function_at, unwrap, type_from_id, unwrap, as_func_type, unwraphappens three times; would it make sense to factor it out into a utility function? Perhaps by renaming thisfunction_attofunc_type_id_atand addding afunc_type_at` which does the full sequence?

But it's not that big a deal, so this is ok either way.

@alexcrichton
Copy link
Member Author

Nah yeah I definitely agree, and this is something I've wanted to fix for awhile. I'd like to add accessors along the lines of:

types[id].unwrap_func_type()

since most of the time it's unwrap()'d anyway so the indexing communicates panicking along with the unwrap_ in the name. That should tighten up a lot of the code here I think, but I'll leave that for a future PR.

@alexcrichton alexcrichton merged commit c4e501c into bytecodealliance:main Jul 6, 2023
14 checks passed
@alexcrichton alexcrichton deleted the index-accessors branch July 6, 2023 15:30
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 6, 2023
@alexcrichton
Fix a merge conflict
90b9c96 
Currently the `main` branch doesn't build after merging bytecodealliance#1094 due to
changes happening in the interim, and this fixes the compile.
@alexcrichton alexcrichton mentioned this pull request Jul 6, 2023
Merged
alexcrichton added a commit that referenced this pull request Jul 6, 2023
@alexcrichton
Fix a merge conflict (#1101)
4f86eaf 
Currently the `main` branch doesn't build after merging #1094 due to
changes happening in the interim, and this fixes the compile.
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 7, 2023
@alexcrichton
Reduce verbosity in wasmparser Types API
eb276e6 
This additionally applies to `TypesRef` and this PR applies a few
changes:

* Going from a `TypeId` to a `Type` is now done with `&types[id]` or
  `types.get(id)` instead of `type_from_id(id).unwrap()`.
* All index-based accessors return a `T` instead of `Option<T>`.
  Fallibility is possible through the `*_count()` methods for embedders
  to test if necessary, but otherwise almost all accesses were followed
  by `.unwrap()` anyway.
* Conversions from `Type` to a sub-component which were previously
  `as_foo_type(&self) -> Option<&Foo>` are now expressed as
  `unwrap_foo(&self) -> &Foo`. This renames `as` to `unwrap` to
  highlight the panicking behavior and additionally drops the `_type`
  suffix since that's implied on everything anyway.
* The `type_at` function and its `core: bool` argument are removed in
  favor of `core_type_at` and `component_type_at` functions.
* The `func_type_at` function was removed as it can be achieved through
  combining other functions.
* Some methods which returned `&T` changed to return a `TypeId` which
  represents `T` for consistency.

This is inspired by comments on bytecodealliance#1094 and is something I've wanted to
address for awhile and will ideally make the `Types` API a bit more
ergonomic to use with fewer conversions and less `.unwrap()`
boilerplate without any loss of functionality.
@alexcrichton alexcrichton mentioned this pull request Jul 7, 2023
Merged
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 7, 2023
@alexcrichton
Reduce verbosity in wasmparser Types API
677a922 
This additionally applies to `TypesRef` and this PR applies a few
changes:

* Going from a `TypeId` to a `Type` is now done with `&types[id]` or
  `types.get(id)` instead of `type_from_id(id).unwrap()`.
* All index-based accessors return a `T` instead of `Option<T>`.
  Fallibility is possible through the `*_count()` methods for embedders
  to test if necessary, but otherwise almost all accesses were followed
  by `.unwrap()` anyway.
* Conversions from `Type` to a sub-component which were previously
  `as_foo_type(&self) -> Option<&Foo>` are now expressed as
  `unwrap_foo(&self) -> &Foo`. This renames `as` to `unwrap` to
  highlight the panicking behavior and additionally drops the `_type`
  suffix since that's implied on everything anyway.
* The `type_at` function and its `core: bool` argument are removed in
  favor of `core_type_at` and `component_type_at` functions.
* The `func_type_at` function was removed as it can be achieved through
  combining other functions.
* Some methods which returned `&T` changed to return a `TypeId` which
  represents `T` for consistency.

This is inspired by comments on bytecodealliance#1094 and is something I've wanted to
address for awhile and will ideally make the `Types` API a bit more
ergonomic to use with fewer conversions and less `.unwrap()`
boilerplate without any loss of functionality.
sunfishcode pushed a commit that referenced this pull request Jul 10, 2023
@alexcrichton
Reduce verbosity in wasmparser Types API (#1106)
b3990e8 
* Reduce verbosity in wasmparser `Types` API

This additionally applies to `TypesRef` and this PR applies a few
changes:

* Going from a `TypeId` to a `Type` is now done with `&types[id]` or
  `types.get(id)` instead of `type_from_id(id).unwrap()`.
* All index-based accessors return a `T` instead of `Option<T>`.
  Fallibility is possible through the `*_count()` methods for embedders
  to test if necessary, but otherwise almost all accesses were followed
  by `.unwrap()` anyway.
* Conversions from `Type` to a sub-component which were previously
  `as_foo_type(&self) -> Option<&Foo>` are now expressed as
  `unwrap_foo(&self) -> &Foo`. This renames `as` to `unwrap` to
  highlight the panicking behavior and additionally drops the `_type`
  suffix since that's implied on everything anyway.
* The `type_at` function and its `core: bool` argument are removed in
  favor of `core_type_at` and `component_type_at` functions.
* The `func_type_at` function was removed as it can be achieved through
  combining other functions.
* Some methods which returned `&T` changed to return a `TypeId` which
  represents `T` for consistency.

This is inspired by comments on #1094 and is something I've wanted to
address for awhile and will ideally make the `Types` API a bit more
ergonomic to use with fewer conversions and less `.unwrap()`
boilerplate without any loss of functionality.

* Review feedback
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
@alexcrichton
Bump crate versions
b8b1cf3 
Releases recent changes such as:

* More support for the gc proposal. bytecodealliance#1045 bytecodealliance#1059
* Support for resources throughout most tooling. bytecodealliance#1053 bytecodealliance#1068 bytecodealliance#1070 bytecodealliance#1082
  bytecodealliance#1079 bytecodealliance#1083 bytecodealliance#1084 bytecodealliance#1105 bytecodealliance#1113 bytecodealliance#1116
* Support for `include` in WIT files. bytecodealliance#1054 bytecodealliance#1085 bytecodealliance#1088
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously. bytecodealliance#1081 bytecodealliance#1107
* Support for registry metadata in `wasm-tools metadata`. bytecodealliance#1060
* A new subcommand `wasm-tools component targets`. bytecodealliance#1089
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package. bytecodealliance#1108
* Miscellaneous bug fixes and improvements. bytecodealliance#1061 bytecodealliance#1065 bytecodealliance#1074 bytecodealliance#1073 bytecodealliance#1078 bytecodealliance#1077
  bytecodealliance#1072 bytecodealliance#1086 bytecodealliance#1091 bytecodealliance#1094 bytecodealliance#1114 bytecodealliance#1106
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
@alexcrichton
Bump crate versions
6297d32 
Releases recent changes such as:

* More support for the gc proposal.
  [bytecodealliance#1045](bytecodealliance#1045)
  [bytecodealliance#1059](bytecodealliance#1059)
* Support for resources throughout most tooling.
  [bytecodealliance#1053](bytecodealliance#1053)
  [bytecodealliance#1068](bytecodealliance#1068)
  [bytecodealliance#1070](bytecodealliance#1070)
  [bytecodealliance#1082](bytecodealliance#1082)
  [bytecodealliance#1079](bytecodealliance#1079)
  [bytecodealliance#1083](bytecodealliance#1083)
  [bytecodealliance#1084](bytecodealliance#1084)
  [bytecodealliance#1105](bytecodealliance#1105)
  [bytecodealliance#1113](bytecodealliance#1113)
  [bytecodealliance#1116](bytecodealliance#1116)
* Support for `include` in WIT files.
  [bytecodealliance#1054](bytecodealliance#1054)
  [bytecodealliance#1085](bytecodealliance#1085)
  [bytecodealliance#1088](bytecodealliance#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [bytecodealliance#1081](bytecodealliance#1081)
  [bytecodealliance#1107](bytecodealliance#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [bytecodealliance#1060](bytecodealliance#1060)
* A new subcommand `wasm-tools component targets`.
  [bytecodealliance#1089](bytecodealliance#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [bytecodealliance#1108](bytecodealliance#1108)
* Miscellaneous bug fixes and improvements.
  [bytecodealliance#1061](bytecodealliance#1061)
  [bytecodealliance#1065](bytecodealliance#1065)
  [bytecodealliance#1074](bytecodealliance#1074)
  [bytecodealliance#1073](bytecodealliance#1073)
  [bytecodealliance#1078](bytecodealliance#1078)
  [bytecodealliance#1077](bytecodealliance#1077)
  [bytecodealliance#1072](bytecodealliance#1072)
  [bytecodealliance#1086](bytecodealliance#1086)
  [bytecodealliance#1091](bytecodealliance#1091)
  [bytecodealliance#1094](bytecodealliance#1094)
  [bytecodealliance#1114](bytecodealliance#1114)
  [bytecodealliance#1106](bytecodealliance#1106)
@alexcrichton alexcrichton mentioned this pull request Jul 11, 2023
Merged
alexcrichton added a commit that referenced this pull request Jul 11, 2023
@alexcrichton
Bump crate versions (#1118)
620b4ab 
Releases recent changes such as:

* More support for the gc proposal.
  [#1045](#1045)
  [#1059](#1059)
* Support for resources throughout most tooling.
  [#1053](#1053)
  [#1068](#1068)
  [#1070](#1070)
  [#1082](#1082)
  [#1079](#1079)
  [#1083](#1083)
  [#1084](#1084)
  [#1105](#1105)
  [#1113](#1113)
  [#1116](#1116)
* Support for `include` in WIT files.
  [#1054](#1054)
  [#1085](#1085)
  [#1088](#1088)
* WIT worlds may now be rejected if the same interface can be "reached"
  as both an import and an export simultaneously.
  [#1081](#1081)
  [#1107](#1107)
* Support for registry metadata in `wasm-tools metadata`.
  [#1060](#1060)
* A new subcommand `wasm-tools component targets`.
  [#1089](#1089)
* An `--out-dir` argument is supported on `wasm-tools component wit` to
  print the entire `Resolve` instead of just one package.
  [#1108](#1108)
* Miscellaneous bug fixes and improvements.
  [#1061](#1061)
  [#1065](#1065)
  [#1074](#1074)
  [#1073](#1073)
  [#1078](#1078)
  [#1077](#1077)
  [#1072](#1072)
  [#1086](#1086)
  [#1091](#1091)
  [#1094](#1094)
  [#1114](#1114)
  [#1106](#1106)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunfishcode sunfishcode sunfishcode approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexcrichton @sunfishcode