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

Remove the type-driven ability for duplicates in a Linker #2789

Merged

Conversation

alexcrichton
Copy link
Member

When Linker was first created it was attempted to be created with the
ability to instantiate any wasm modules, including those with duplicate
import strings of different types. In an effort to support this a
Linker supports defining the same names twice so long as they're
defined with differently-typed values.

This ended up causing wast testsuite failures module linking is enabled,
however, because the wrong error message is returned. While it would be
possible to fix this there's already the possibility for confusing error
messages today due to the Linker trying to take on this type-level
complexity. In a way this is yet-another type checker for wasm imports,
but sort of a bad one because it only supports things like
globals/functions, and otherwise you can only define one Memory, for
example, with a particular name.

This commit completely removes this feature from Linker to simplify
the implementation and make error messages more straightforward. This
means that any error message coming from a Linker is purely "this
thing wasn't defined" rather than a hybrid of "maybe the types didn't
match?". I think this also better aligns with the direction that we see
conventional wasm modules going which is that duplicate imports are not
ever present.

When `Linker` was first created it was attempted to be created with the
ability to instantiate any wasm modules, including those with duplicate
import strings of different types. In an effort to support this a
`Linker` supports defining the same names twice so long as they're
defined with differently-typed values.

This ended up causing wast testsuite failures module linking is enabled,
however, because the wrong error message is returned. While it would be
possible to fix this there's already the possibility for confusing error
messages today due to the `Linker` trying to take on this type-level
complexity. In a way this is yet-another type checker for wasm imports,
but sort of a bad one because it only supports things like
globals/functions, and otherwise you can only define one `Memory`, for
example, with a particular name.

This commit completely removes this feature from `Linker` to simplify
the implementation and make error messages more straightforward. This
means that any error message coming from a `Linker` is purely "this
thing wasn't defined" rather than a hybrid of "maybe the types didn't
match?". I think this also better aligns with the direction that we see
conventional wasm modules going which is that duplicate imports are not
ever present.
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Mar 29, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@sunfishcode
Copy link
Member

This looks reasonable to me. My understanding of the duplicate-imports discussion is that there are core-wasm use cases where linking based on the name identifiers alone isn't sufficient, however it also seems like core-wasm types and signatures also aren't sufficient for those use cases, because they're too low-level in general, so they'll need to do something else anyway.

@alexcrichton
Copy link
Member Author

Oh that reminds me of another reason to do this. Currently Linker is sort of a weird hybrid of trying to do name-based resolution but also doing some amount of positional-based resolution. Instead though I think it should lean one of the two directions and not try to bridge the gap which in the limit seems un-bridge-able.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Good point. Another consideration here is that exports that differ only in type represent interfaces which can't be implemented by wasm modules (without custom linking), which makes them difficult to virtualize.

@alexcrichton alexcrichton merged commit a301202 into bytecodealliance:main Mar 29, 2021
@alexcrichton alexcrichton deleted the better-linker-errors branch March 29, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants