search

0xPolygonMiden / miden-vm

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

MASM errors should track origin #1298

Open hackaugusto opened 3 months ago

hackaugusto commented 3 months ago

Context: https://github.com/0xPolygonMiden/miden-vm/issues/1295

When errors are generated, they should track their source location as in source file, line, and column, to make it easier to find the root cause.

bobbinth commented 3 months ago

I think compilation errors will have that as a part of #1277 (but @bitwalker can correct if I'm off here). Proving this info for runtime errors will take more work though.

bitwalker commented 3 months ago

Yep! During my refactoring, I made most source-related errors associated with a source span, so that those errors render a source code snippet when displayed. There are still errors that lack spans, but these can be addressed one-by-one after 1277 is merged.

hackaugusto commented 3 months ago

Note: This should include advice instructions, for example:

---- tests::test_note::test_get_inputs stdout ----
thread 'tests::test_note::test_get_inputs' panicked at miden-lib/src/tests/test_note.rs:303:41:
called `Result::unwrap()` on an `Err` value: AdviceStackReadFailed(3771)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Is not a useful error at all.

hackaugusto commented 2 months ago

are we planning on making a new vm release? this is still a pain point, specially for error results that don't have error codes, like NotBinaryValue.

Edit: Btw, question, it seems none of the ExecutionError variants have any context. Maybe these were not handled by #1277 ?

bobbinth commented 2 months ago

are we planning on making a new vm release? this is still a pain point, specially for error results that don't have error codes, like NotBinaryValue.

We are - but first we need to migrate to MAST-based program serialization. I think we should try to get this done (and the new version of the VM released) but early/mid June.

Edit: Btw, question, it seems none of the ExecutionError variants have any context. Maybe these were not handled by #1277 ?

1277 improved error reporting during the assembly phase only. Improving error reporting (i.e., adding source locations) during program execution is still outstanding and will probably be a separate task (which will happen after the next release).