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

Better error message for wrong import implementation #7645

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

rylev
Copy link
Contributor

@rylev rylev commented Dec 6, 2023

This changes improve the error message when a component is importing/exporting something that the linker is either missing or has the wrong type.

Previously the error message for a missing function implementation would be:

Error: import `my-import` has the wrong type

Caused by:
    expected func found nothing

Now, this error will be:

Error: component imports `my-import`, but a correct implementation of `my-import` could not be found

Caused by:
    function implementation is missing

I went back and forth on whether the error message should reference the linker. For direct users of Linker this would surely help, but this error message also gets displayed to indirect users of Linker so talking at a higher level of abstraction seemed appropriate.

@rylev rylev requested a review from a team as a code owner December 6, 2023 08:37
@rylev rylev requested review from pchickey and removed request for a team December 6, 2023 08:37
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

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.

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

I think what you have is definitely trending in the right direction for an easier to understand error message.

Just a few feedback comments.

Additionally, "implementation" in some of the errors rubs me the wrong way for some reason I can't seem to articulate, nor can I think of a better word to use.

crates/wasmtime/src/component/linker.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/matching.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/component/matching.rs Outdated Show resolved Hide resolved
@rylev
Copy link
Contributor Author

rylev commented Mar 11, 2024

@peterhuene I believe I've addressed your feedback. I agree that "implementation" isn't always the right word, but I think we'll need to let this sit for a while before the right word makes itself apparent.

@rylev rylev requested a review from peterhuene March 11, 2024 10:09
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@peterhuene peterhuene added this pull request to the merge queue Mar 11, 2024
Merged via the queue into bytecodealliance:main with commit 57132f0 Mar 11, 2024
19 checks passed
@rylev rylev deleted the better-error branch March 11, 2024 20:11
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