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

Add importize method to Resolve #1784

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

karthik2804
Copy link
Contributor

A new method importize is added to the Resolve. This is to allow to mutate the Resolve to state where it would resemble what a consuming component would expect to see during composition.

alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Sep 12, 2024
This commit extends the `Resolve::assert_valid` function with more
assertions about the structure of worlds notably to guarantee they are
"elaborated" meaning that they always list all dependencies of imports.
This is required by bindings generators and encoding. This property is
already upheld internally and is intended to reflect a preexisting
property with dynamic assertion checks.

The underlying motivation for this is to assist in the development and
fuzzing of bytecodealliance#1784.
alexcrichton added a commit to karthik2804/wasm-tools that referenced this pull request Sep 12, 2024
This commit extends the `Resolve::assert_valid` function with more
assertions about the structure of worlds notably to guarantee they are
"elaborated" meaning that they always list all dependencies of imports.
This is required by bindings generators and encoding. This property is
already upheld internally and is intended to reflect a preexisting
property with dynamic assertion checks.

The underlying motivation for this is to assist in the development and
fuzzing of bytecodealliance#1784.
@alexcrichton
Copy link
Member

I've rebased this on #1785 and #1782 to get better assertions as well as better testing utilities. With that I've added an "Updates to importize" commit which updates the method of doing this slightly and adds a few more tests as well. My hope is that it's a bit more robust to conditionally preserve imports instead of throwing them all away and recreating them. I've then also hooked this up to fuzzing.

@karthik2804 one thing I'm not certain of is that the design currently is to mutate the world in-place. I updated this to rename the world by adding -importized as a suffix, but that probably isn't great. Do you think that the name matters for the tooling integration you were envisioning? For example would it be better to create a new world and insert it into a provided package?

@karthik2804
Copy link
Contributor Author

karthik2804 commented Sep 12, 2024

I do not have strong opinions here but I think the name of the world should not matter too much.

@alexcrichton alexcrichton marked this pull request as ready for review September 12, 2024 20:40
@alexcrichton
Copy link
Member

Ok in that case let's leave this as-is and see how it works out.

Given that this is co-authored by me and @karthik2804 I'd prefer to not approve-and-merge, so @dicej I'm going to tag you in for a third set of eyes on this to make sure nothing looks too awry.

github-merge-queue bot pushed a commit that referenced this pull request Sep 12, 2024
This commit extends the `Resolve::assert_valid` function with more
assertions about the structure of worlds notably to guarantee they are
"elaborated" meaning that they always list all dependencies of imports.
This is required by bindings generators and encoding. This property is
already upheld internally and is intended to reflect a preexisting
property with dynamic assertion checks.

The underlying motivation for this is to assist in the development and
fuzzing of #1784.
karthik2804 and others added 3 commits September 12, 2024 14:13
A new method `importize` is added to the Resolve. This is to allow to
mutate the Resolve to state where it would resemble what a consuming
component would expect to see during composition.

Signed-off-by: karthik2804 <[email protected]>
* Update the CLI to have `--importize` and `--importize-world`
* Rewrite the test to use these flags and have multiple tests in one
  file, each with a smaller world.
* Update the implementation to preserve allow-listed imports instead of
  removing all imports and recreating what needs to be preserved.
@alexcrichton alexcrichton added this pull request to the merge queue Sep 12, 2024
Merged via the queue into bytecodealliance:main with commit 8f247b9 Sep 12, 2024
30 checks passed
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