search

0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
615 stars 152 forks source link

Change errors to use thiserror #1261

Open hackaugusto opened 5 months ago

hackaugusto commented 5 months ago

related: https://github.com/0xPolygonMiden/miden-node/issues/62

hackaugusto commented 5 months ago

thiserror doesn't support no-std

bitwalker commented 5 months ago

That's going to change when the error_in_core feature stabilizes.

In the meantime, we should use this fork. I've introduced it in my assembler changes (hope to have those pushed tonight/tomorrow).

bobbinth commented 5 months ago

That's going to change when the error_in_core feature stabilizes.

Hopefully, this will happen soon - but also I think the timing of this is rather unpredictable.

In the meantime, we should use this fork. I've introduced it in my assembler changes (hope to have those pushed tonight/tomorrow).

Seems like this crate was published just yesterday? I'd be hesitant to use such a new dependency (we try to minimize the number of dependencies in general, and when required use more stable and well-known dependencies). How critical is this to the assembler changes?

bitwalker commented 5 months ago

I mean, we can fork thiserror ourselves and make the same changes - with code gen of the Error trait disabled for no_std, so this isn't a question of it being a new crate or something, we're talking about the same crate in practical terms. I would have actually done that myself, but someone helpfully did it already, hence why I'm recommending we use it in the meantime.

I'm conservative about dependencies, more so than most I think, typically preferring to write something myself unless there are well established options that meet my other picky criteria, so I don't generally recommend a crate unless I believe it meets the bar for inclusion by a fair margin.

In any case, I didn't think the question was whether we should start using thiserror or not, but when. There isn't a good reason to wait for stabilization of error_in_core, dtolnay is doing so because it is easier for them to maintain the crate, i.e. they don't want to publish a temporary workaround like the linked fork. We don't have the same constraints, so it's really a matter of whether we can benefit from it before that happens, and that seems like a clear yes to me.

That said, I'd suggest waiting to see what my changes look like, and evaluate in that context, since we may not have any other stuff in the works that needs it just yet and this will provide some useful comparison.

bobbinth commented 5 months ago

with code gen of the Error trait disabled for no_std, so this isn't a question of it being a new crate or something, we're talking about the same crate in practical terms.

Right - this is more of a general principal which I think is important specifically for crypto-related projects where supply-chain attacks are a very real risk. So, I try to keep the number of external dependencies to an absolute minimum and only use very well-known dependencies.

we can fork thiserror ourselves and make the same changes

We we do want to use this error, I would probably prefer this route. No need to do anything different for the PR you are working on, but we should probably follow it up with our own fork and then we can use it in all other repos as well.

bobbinth commented 2 weeks ago

Currently, we have our own forks of thiserror and miette dependencies which support no-std. Once relevant features in core get stabilized and these crates are updates to make use of them, we should move the dependencies to the canonical crates.

bitwalker commented 1 week ago

The 1.81 release looks like it will contain the stabilized error_in_core trait, and there is a draft PR in thiserror that implements the necessary changes there, but I'm not sure exactly how soon after 1.81 releases we can expect the thiserror release to drop, but probably pretty quickly afterwards, we can certainly update our fork to be the same changes so we're aligned with upstream.

For miette, we'll need to maintain a fork for awhile. I'm planning to spend some time reworking miden-diagnostics to basically take over the role of that dependency (by exporting a mostly-equivalent API, with the source management stuff integrated more natively into it, so we can move that out of miden-core). While miette is great for what it does, there are some aspects of it I don't like, and it isn't designed to be no-std compatible (I had to hack it in to our fork). The parts I do mostly like, are the rendering of diagnostics, the IntoDiagnostic, Diagnostic, and WrapErr traits, Report, and some of the conveniences like the diagnostic! and miette! macros - but I don't like any of the source management stuff it does, which is not well-suited to an environment where multiple sources are being worked on simultaneously (there is no way to tell the Report printer how to map a source span to a specific source file, it expects that you bind the relevant source file to the Report, but you can have diagnostics with spans in multiple files, and it is extremely awkward to handle this (I hacked together the RelatedLabel type to make this work, but it is not convenient to use).

Anyway, brain dumping this here for when we get to this, there isn't too much required to implement what I described above, so when I have some time to get to it, I'll do so.