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

Investigate removing AssemblyError enum #1431

Open
bobbinth opened this issue Aug 7, 2024 · 2 comments
Open

Investigate removing AssemblyError enum #1431

bobbinth opened this issue Aug 7, 2024 · 2 comments
Assignees
Labels
assembly Related to Miden assembly
Milestone

Comments

@bobbinth
Copy link
Contributor

bobbinth commented Aug 7, 2024

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 Report everywhere instead.

@bobbinth bobbinth added the assembly Related to Miden assembly label Aug 7, 2024
@bobbinth bobbinth added this to the v0.11.0 milestone Aug 7, 2024
@bitwalker
Copy link
Contributor

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 bobbinth changed the title Investigate remove AssemblyError enum Investigate removing AssemblyError enum Aug 7, 2024
@bobbinth
Copy link
Contributor Author

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 #1466 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assembly Related to Miden assembly
Projects
None yet
Development

No branches or pull requests

3 participants