Skip to content

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

Conversation

alexcrichton
Copy link
Member

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.

Closes #2190

@alexcrichton alexcrichton requested review from a team and dicej and removed request for a team May 29, 2025 16:54
@alexcrichton alexcrichton added this pull request to the merge queue 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`.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 29, 2025
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`.
@alexcrichton alexcrichton force-pushed the fix-world-async-funcs branch from 07ab1f5 to cc072fb Compare May 30, 2025 21:48
@alexcrichton alexcrichton requested a review from a team as a code owner May 30, 2025 21:48
@alexcrichton alexcrichton requested review from abrown and removed request for a team May 30, 2025 21:48
@alexcrichton alexcrichton enabled auto-merge May 30, 2025 21:49
@alexcrichton alexcrichton added this pull request to the merge queue May 30, 2025
Merged via the queue into bytecodealliance:main with commit b2b60ff May 30, 2025
32 checks passed
@alexcrichton alexcrichton deleted the fix-world-async-funcs branch May 30, 2025 22:00
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.

wit-smith doesn't generate multiple packages
2 participants