0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
612 stars 150 forks source link

Assembler should not hide inner errors on re-exports #1297

Open hackaugusto opened 3 months ago

hackaugusto commented 3 months ago

For additional context: https://github.com/0xPolygonMiden/miden-vm/issues/1295

The assembler should not hide internal errors, if errors are to be wrapped, the inner error should be kept for additional context.

bitwalker commented 3 months ago

I think this should be largely resolved in #1277. Several of the public APIs now return a Report, which retains all of the context of the original error (depending on how it is constructed, i.e. you can obviously discard an error and construct an ad-hoc Report, but we don't generally do that anywhere that I recall). There are also now wrap_err and wrap_err_with extensions provided by the assembly::diagnostics::WrapErr trait which can be used to wrap error types we don't control with additional context, which I've taken advantage of in a few places (largely around I/O errors).

If there are still places where we hide errors/context, then I'd recommend we move those APIs to Report, rather than try to represent every possible internal error as a distinct variant in our own error types.

NOTE: All of this is based on the forks of thiserror and miette that I am maintaining that add support for no-std builds. Once the main feature I'm waiting on in Rust is stabilized, we can switch to the officially published crates for those two dependencies. The Report type and WrapErr trait are provided by miette, but are re-exported in the assembler from the diagnostics module.