rylev requested pchickey for a review on PR #7645.
rylev opened PR #7645 from rylev:better-error
to bytecodealliance:main
:
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 ofLinker
so talking at a higher level of abstraction seemed appropriate.
rylev requested wasmtime-core-reviewers for a review on PR #7645.
rylev updated PR #7645.
github-actions[bot] commented on PR #7645:
Subscribe to Label Action
cc @peterhuene
<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
peterhuene submitted PR review:
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.
peterhuene submitted PR review:
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.
peterhuene created PR review comment:
format!("component imports {desc} `{name}`, but a matching implementation was not found", desc = ty.desc())
With an additional implementation of
TypeDef::desc
to say what it is, e.g. "instance", "type", "function", etc.
peterhuene created PR review comment:
Unfortunately
TypeDef::Interface
is misnamed, it's actually representing a type so the message should indicate a type.If it's not too invasive, I think we should probably change
Interface
toType
too.
peterhuene created PR review comment:
None => bail!("module implementation is missing"),
peterhuene submitted PR review.
peterhuene created PR review comment:
Actually, probably not worth taking a breaking change to rename the enum variant (I think the only external consumer of it is
jco
, but there could be others); let's leave itInterface
and just change the error message here back to indicate a "type".
rylev updated PR #7645.
@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 requested peterhuene for a review on PR #7645.
rylev updated PR #7645.
peterhuene submitted PR review.
peterhuene merged PR #7645.
Last updated: Dec 23 2024 at 12:05 UTC