-
Notifications
You must be signed in to change notification settings - Fork 295
Assert Resolve is valid after adding a package #2196
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
Merged
alexcrichton
merged 3 commits into
bytecodealliance:main
from
alexcrichton:fix-world-async-funcs
May 30, 2025
Merged
Assert Resolve is valid after adding a package #2196
alexcrichton
merged 3 commits into
bytecodealliance:main
from
alexcrichton:fix-world-async-funcs
May 30, 2025
Conversation
This file contains hidden or 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
dicej
approved these changes
May 29, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 29, 2025
* Assert `Resolve` is valid after adding a package In retrospect this is an obvious location to add a call to `Resolve::assert_valid` but this wasn't previously done. In #2190 it was found that many assertions were triggering once multiple packages were generated by `wit-smith`. These assertions trace back to the fact that `include` basically produces invalid worlds and this wasn't being detected before. Thus to help catch these issues this change starts off by adding a call to `assert_valid` when a new package is added to a `Resolve`. This turned out to have a large cascading effect which needed quite a lot of changes. Much of this is required now due to the fact that prior to multiple packages being generated the `assert_valid` method was simply never called. That means that by adding `assert_valid` this uncovers lots of preexisting issues which would have otherwise been revealed by preexisting tests. Some issues discovered here are: * The `WorldKey::Name` string for imports/exports did not take into account the `[async]...` mangling for imported/exported functions. Fixing this required also ensuring that importing a sync and async version of a function still failed, meaning that the `Hash` and `PartialEq` implementations of `WorldKey` have been updated to account for this. Some maps which were previously used for this are also removed as they're no longer necessary. * When printing a WIT `world` the `[async]...` prefix is stripped when printing an async function, meaning that the `world` will round-trip when it both imports and exports `async` functions. * Exponential behavior in iterating over dependencies, triggered via `assert_valid`, has been fixed with a map to prune the recursive iteration tree. * Processing of `include` has drastically changed. This is now a discrete step in adding worlds to a `Resolve` and notably properly copies types from one world to another when `include`-d. This previously did not happen and ways always creating an invalid `Resolve` where types were in a world but listed with the wrong `owner`. The preexisting `Cloner` type is used to create copies of types as they move between worlds. * Fuzzing now produces multiple packages for roundtripping WITs instead of only ever producing a single package. * Consolidate caches used when encoding WIT types This commit fixes an issue with the previous commit by introducing a new cache of `&TypeDefKind` to encoded type during the WIT encoding process. This addition of a new cache, in addition to the two previous encoding caches, necessitated a refactoring of wrapping up these caches into a single type. This change means that if the same WIT type shows up as multiple `TypeId` instances it'll only be encoded as a single type in the final wasm binary. This ensures that the text format of WIT matches the binary format of WIT and packages all round-trip correctly, especially in the face of `include`.
In retrospect this is an obvious location to add a call to `Resolve::assert_valid` but this wasn't previously done. In bytecodealliance#2190 it was found that many assertions were triggering once multiple packages were generated by `wit-smith`. These assertions trace back to the fact that `include` basically produces invalid worlds and this wasn't being detected before. Thus to help catch these issues this change starts off by adding a call to `assert_valid` when a new package is added to a `Resolve`. This turned out to have a large cascading effect which needed quite a lot of changes. Much of this is required now due to the fact that prior to multiple packages being generated the `assert_valid` method was simply never called. That means that by adding `assert_valid` this uncovers lots of preexisting issues which would have otherwise been revealed by preexisting tests. Some issues discovered here are: * The `WorldKey::Name` string for imports/exports did not take into account the `[async]...` mangling for imported/exported functions. Fixing this required also ensuring that importing a sync and async version of a function still failed, meaning that the `Hash` and `PartialEq` implementations of `WorldKey` have been updated to account for this. Some maps which were previously used for this are also removed as they're no longer necessary. * When printing a WIT `world` the `[async]...` prefix is stripped when printing an async function, meaning that the `world` will round-trip when it both imports and exports `async` functions. * Exponential behavior in iterating over dependencies, triggered via `assert_valid`, has been fixed with a map to prune the recursive iteration tree. * Processing of `include` has drastically changed. This is now a discrete step in adding worlds to a `Resolve` and notably properly copies types from one world to another when `include`-d. This previously did not happen and ways always creating an invalid `Resolve` where types were in a world but listed with the wrong `owner`. The preexisting `Cloner` type is used to create copies of types as they move between worlds. * Fuzzing now produces multiple packages for roundtripping WITs instead of only ever producing a single package.
This commit fixes an issue with the previous commit by introducing a new cache of `&TypeDefKind` to encoded type during the WIT encoding process. This addition of a new cache, in addition to the two previous encoding caches, necessitated a refactoring of wrapping up these caches into a single type. This change means that if the same WIT type shows up as multiple `TypeId` instances it'll only be encoded as a single type in the final wasm binary. This ensures that the text format of WIT matches the binary format of WIT and packages all round-trip correctly, especially in the face of `include`.
Not allowed in WIT
07ab1f5
to
cc072fb
Compare
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.
In retrospect this is an obvious location to add a call to
Resolve::assert_valid
but this wasn't previously done. In #2190 it wasfound that many assertions were triggering once multiple packages were
generated by
wit-smith
. These assertions trace back to the fact thatinclude
basically produces invalid worlds and this wasn't beingdetected before. Thus to help catch these issues this change starts off
by adding a call to
assert_valid
when a new package is added to aResolve
.This turned out to have a large cascading effect which needed quite a
lot of changes. Much of this is required now due to the fact that prior
to multiple packages being generated the
assert_valid
method wassimply never called. That means that by adding
assert_valid
thisuncovers lots of preexisting issues which would have otherwise been
revealed by preexisting tests. Some issues discovered here are:
The
WorldKey::Name
string for imports/exports did not take intoaccount the
[async]...
mangling for imported/exported functions.Fixing this required also ensuring that importing a sync and async
version of a function still failed, meaning that the
Hash
andPartialEq
implementations ofWorldKey
have been updated to accountfor this. Some maps which were previously used for this are also
removed as they're no longer necessary.
When printing a WIT
world
the[async]...
prefix is stripped whenprinting an async function, meaning that the
world
will round-tripwhen it both imports and exports
async
functions.Exponential behavior in iterating over dependencies, triggered via
assert_valid
, has been fixed with a map to prune the recursiveiteration tree.
Processing of
include
has drastically changed. This is now adiscrete step in adding worlds to a
Resolve
and notably properlycopies types from one world to another when
include
-d. Thispreviously did not happen and ways always creating an invalid
Resolve
where types were in a world but listed with the wrongowner
. The preexistingCloner
type is used to create copies oftypes as they move between worlds.
Fuzzing now produces multiple packages for roundtripping WITs instead
of only ever producing a single package.
Closes #2190