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

wiggle: new error configuration for generating a "trappable error" #5276

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Nov 15, 2022

Largely due to pains in wasi-common and experimentation in wit-bindgen, I have added a slightly different error handling strategy to wiggle.

When the user specifies errno => trappable Error in the macro's optional errors: {...}argument, wiggle will now generate a type named Error to be used in all positions where a function returned a witx errno in the error position.

The generated error type is an opaque struct. Internally, it contains either the errno (which wiggle generates as an enum Errno), or an anyhow::Error for trapping execution. These are constructed using either the generated impl From<Errno> for Error impl, or the Error::trap(t: anyhow::Error) -> Self constructor.

The error can be destructured using Error::downcast(self) -> Result<Errno, anyhow::Error>, inspected using Error::downcast_ref(&self) -> Option<&Errno>, and it can have context added via Error::context(self, s: Into<String>) -> Self. The context is only observable via the Debug and Display impls.

This design came about because the user defined error type wasn't really serving our needs in wasi-common. There, we had essentially 2 choices:

  1. the user maintains a thiserror style error enum that contains all of the errno values, plus a Trap(anyhow::Error) variation for trapping execution. They then write From impls for all of the foreign error types that need to be converted into their error enum, e.g. impl From<std::io::Error> for MyErrno. They then implement the (very mechanical) translation to the wiggle generated Errno cases in UserErrorConversion.
  2. They use anyhow::Error. To implement the translation to Errno or trap, the user downcasts all of the foreign error types they suspect may be thrown in their implementations.

I used both of these designs in wasi-common, in that chronological order. I found that both had real drawbacks:

  1. This error enum is repetitive to define and maintain. Additionally, it doesn't provide any way to keep additional context around on an error, which makes debugging harder.
  2. Suffers from safety problems. Since introducing this design in the 2019 wasi-common rewrite, I (unknowingly) introduced a bug where a cap_rand::Error was thrown in the random methods, but I never tried to downcast to it in UserErrorConversion. The end result is that any error in the cap_rand crate would end up trapping execution of the wasm module, rather than returning it an appropriate errno. I don't think I ever would have discovered this bug if I hadn't attempted a refactor back towards design 1.

So, my design here basically uses code generation to generate an acceptable implementation of design 1, while providing the .context("help figuring out what went wrong") method available in design 2. I was reluctant to give up the ergonomics of approach 2, but the design of wasi is evolving to give us lots of different concrete errnos in the preview 2 interfaces, so I wanted to give us as much type safety as possible when managing the complexity of the coming evolution in this crate.

start refactoring how errors are generated and configured

put a pin in this - you can now configure a generated error

but i need to go fix Names

Names is no longer a struct, rt is hardcoded to wiggle

rest of fixes to pass tests

its called a trappable error now

don't generate UserErrorConversion trait if empty

mention in macro docs
@github-actions
Copy link

Subscribe to Label Action

cc @kubkon

This issue or pull request has been labeled: "wasi"

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

  • kubkon: wasi

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

Learn more.

@pchickey pchickey marked this pull request as ready for review November 16, 2022 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants