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

Fix WIT sugar for resource names being own<T> #1083

Merged

Conversation

alexcrichton
Copy link
Member

This commit fixes an issue where WIT resources should not have to be
wrapped in own<T> but instead are allowed to be defined as just T.
This is a bit tricky when T is defined in a foreign package since the
"inject the own wrapper" isn't apparent during parsing but only later
once packages are merged together. This phase has been updated
accordingly to inject own wrappers as necessary.

Note: this builds on #1079 as the first commit

crates/wit-parser/src/resolve.rs Outdated Show resolved Hide resolved
This commit fixes an issue where WIT resources should not have to be
wrapped in `own<T>` but instead are allowed to be defined as just `T`.
This is a bit tricky when `T` is defined in a foreign package since the
"inject the `own` wrapper" isn't apparent during parsing but only later
once packages are merged together. This phase has been updated
accordingly to inject `own` wrappers as necessary.
@alexcrichton alexcrichton force-pushed the fix-bare-own-for-deps branch from 2afe9b1 to 4026fc2 Compare June 20, 2023 20:47
@alexcrichton alexcrichton enabled auto-merge (squash) June 20, 2023 20:47
Fuzzing the previous commit showed a number of issues around generating
too many `own<T>` handles or other transitive handles. This commit
handles this by handling deduplication during the package resolution
process to produce a `Resolve`. Additionally the package-local
resolution phase has been updated to change the structure of the AST and
rely on this elaboration phase happening a bit later.
@alexcrichton alexcrichton disabled auto-merge June 20, 2023 22:17
@alexcrichton
Copy link
Member Author

Fuzzing ended up showing a number of other issues with this commit, so I've addressed those now in a follow-up commit in the middle here. I can't add most of the fuzz test cases here yet since they require more support in wit-component, but they should all be included in a subsequent PR which fills out wit-component.

@sunfishcode could you double-check the new commits here though? They're new-enough I don't want to carry over the review of the first commit to them too

@silesmo
Copy link
Member

silesmo commented Jun 21, 2023

This looks good! I like how you changed it to handle resolving the own handles in the same way regardless of how it's used, removing the need to do it in the Name resolution as well as in other places.

@alexcrichton
Copy link
Member Author

@sunfishcode to confirm, would you be ok if I carried over #1084 (review) to here?

@sunfishcode
Copy link
Member

Yes, that's good.

@alexcrichton alexcrichton merged commit d4be472 into bytecodealliance:main Jul 5, 2023
@alexcrichton alexcrichton deleted the fix-bare-own-for-deps branch July 5, 2023 23:11
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Jul 11, 2023
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
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 added a commit that referenced this pull request Jul 11, 2023
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants