Skip to content

Reduce verbosity in wasmparser Types API#1106

Merged
sunfishcode merged 2 commits into
bytecodealliance:mainfrom
alexcrichton:refactor-types
Jul 10, 2023
Merged

Reduce verbosity in wasmparser Types API#1106
sunfishcode merged 2 commits into
bytecodealliance:mainfrom
alexcrichton:refactor-types

Conversation

@alexcrichton

@alexcrichton alexcrichton commented Jul 7, 2023

Copy link
Copy Markdown
Member

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.

@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
sunfishcode reviewed Jul 7, 2023
View reviewed changes

@sunfishcode sunfishcode left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

Comment thread crates/wasmparser/src/validator/types.rs Outdated
Comment thread crates/wit-component/src/validation.rs
match resolve.worlds[world].imports.get(&world_key(resolve, name)) {
Some(WorldItem::Function(func)) => {
let ty = types.func_type_at(*ty).unwrap();
let ty = types[types.core_type_at(*ty)].unwrap_func();

@sunfishcode sunfishcode Jul 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this code know it has a core type index, rather than a component type index?

@alexcrichton alexcrichton Jul 10, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's contextually known since the funcs map comes from parsing a core wasm module (the input to wasm-tools component new which is also where the types type information comes from.

Comment thread crates/wasmparser/src/validator/types.rs Outdated
Comment thread crates/wasmparser/src/validator/types.rs Outdated
Comment thread crates/wasmparser/src/validator/types.rs Outdated
@alexcrichton alexcrichton requested a review from sunfishcode July 10, 2023 20:01
@sunfishcode sunfishcode merged commit b3990e8 into bytecodealliance:main Jul 10, 2023
@alexcrichton alexcrichton deleted the refactor-types branch July 10, 2023 20:29
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.

2 participants

@alexcrichton @sunfishcode