search

0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
630 stars 160 forks source link

Investigate removing `AssemblyError` enum #1431

Open bobbinth opened 3 months ago

bobbinth commented 3 months ago

Currently, public API of the assembler returns Report errors. However, internally in the assembler we generate AssemblyError in quite a few places. We should investigate whether we can get rid of AssemblyError entirely and just use Reporteverywhere instead.

bitwalker commented 3 months ago

Definitely - this is an easy one to do, but as a pre-requisite I'd like to add a macro for creating ad-hoc Reports (basically miette::miette!, but our own, so that we don't have to export miette from our public API). After that, all of the existing places where we return AssemblyError can be converted to use that.

We might still want a concrete error type that implements Diagnostic, for errors that are used in multiple places, so that we don't have to duplicate the diagnostic information multiple times, but I think there are still several AssemblyError variants which are used in basically one place, but I'd have to double check.

bobbinth commented 1 month ago

This is an example of an error where using Report and ad-hoc diagnostics works better than a concrete error type like AssemblyError. This error sets the stage for what is wrong, but is sort of incomplete from the user perspective. Ideally, you'd have the primary label span the procedure body, with a secondary label spanning the first decorator, with a message like '<opcode>' is a decorator, which cannot appear in a procedure body without at least one non-decorator instruction, with help text attached to the diagnostic like try adding a 'nop' instruction at the end of the procedure body to anchor the decorators.

By using AssemblyError for things like this, we make it difficult (if not impossible) to construct these kind of helpful diagnostics, which is why most APIs are designed to return Report rather than AssemblyError.

Your call on whether you want to make the change, but we should try to get in the habit of thinking about how these errors will be seen and acted on by users, particularly those that don't have as strong a familiarity with Miden Assembly, and may not understand the difference between things like decorators and instructions, since they appear the same way in the source code.

_Originally posted by @bitwalker in https://github.com/0xPolygonMiden/miden-vm/pull/1466#discussion_r1755153155_