-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Wasmtime component bindgen: opt-in trappable error types #5397
Conversation
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
struct MyImports {} | ||
|
||
impl imports::Imports for MyImports { | ||
fn empty_error(&mut self, a: f64) -> Result<Result<f64, ()>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I thought the Error<T>
was going to work which would avoid this result-of-result, but could you explain a bit more why it didn't work out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't have any tests that actually use the functionality in yet. I wanted to get this "trivial" behavior working with the new test scheme before I put in the more complicated stuff.
We can't use Result<f64, Error<()>>
in this case because Error requires T impl std::error::Error. Additionally, all of this PR only supports substituting named types, and doesn't support substituting primitive types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok makes sense. I'm worried about the ergonomics of this, though. Are you thinking we'd special-case -> result<T, rich-error>
in WIT to have a Result<T, E>
return value here? For degenerate cases like -> result<T>
with no error or primitive errors I'm fine with doing whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the tests in below that show we can special case a interface i { ... result<t, e> }
to Result<T, TrappableE>
when the user invokes the macro with trappable_error_types: { i::e: TrappableE, ... }
crates/wit-bindgen/src/lib.rs
Outdated
@@ -63,6 +68,18 @@ impl Opts { | |||
} | |||
} | |||
|
|||
#[cfg(feature = "clap")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh all this clap stuff is a holdover from copying over from the wit-bindgen repository, I think all of it can be removed since clap
isn't even a dependency in this repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I didn't even think to look. Deleted.
these are not used - clap isnt even an optional dep here - but were a holdover from the old home
This was gotten working over in wit-bindgen, and I ported it to the wasmtime repo since the generator moved.
My earlier failed experiment of a "HostError" has been deleted, and bindgen macro users can now opt in to a special
trappable_error_type
which should work exactly how wiggle's trappable errors work.When the user provides the macro argument
the code generator looks in wit interfaces named
i
for a wit type namede
, it will generate a typestruct TrappableE
whichimpl Error
andimpl From<E> for TrappableE
. In any functions that returnresult<t, e>
, it generates the return typeResult<T, TrappableE>
, rather than the much less idiomaticResult<Result<T, E>, anyhow::Error>
.I introduced new tests in
tests/all/component-model/bindgen/results.rs
that show this functionality in use.