migrate from anyhow to thiserror by arnaucube · Pull Request #197 · 0xPARC/pod2

arnaucube · 2025-04-16T16:12:58Z

This PR migrates from using anyhow to thiserror as agreed in #190 . Contains an initial commit that does the migration, but not as a final version to be merged.

@ax0 @ed255 feel free to update it with commits polishing the error messages. All the occurrences of the string Error::Custom(format!, are error messages that were not used more than once an it was not clear if a specific error type was needed for them. Maybe how we can approach it is that if the error message will not be used in the upper layers, it can be as a Error::Custom(String), as it was before with anyhow.

Once ready, this will resolve #190.

ed255

I've taken an initial look at the PR and left a lot of comments to discuss details :)

ed255 · 2025-04-17T10:33:37Z

src/error.rs

+    #[error("std::io::Error: {0}")]
+    IOError(#[from] std::io::Error),
+    #[error("anyhow::Error: {0}")]
+    Anyhow(#[from] anyhow::Error),


It would be interesting if we can distinguish plonky2 errors (which are anyhow::Error) from other anyhow::Errors). But perhaps that would require a lot of boilerplate in the code :/

ed255 · 2025-04-17T10:35:40Z

src/error.rs

+    #[error("{0} {1} is over the limit {2}")]
+    MaxLength(String, usize, usize),
+    #[error("{0} amount of {1} should be {1} but it's {2}")]
+    DiffAmount(String, String, usize, usize),


Did you consider naming the fields so that the error creation is more clear?
For example:

DiffAmount{object: String, unit: String, expect: usize, found: usize}

The same applies to other error cases that have several fields

src/examples/custom.rs

ed255 · 2025-04-17T10:38:49Z

src/frontend/mod.rs

            match (args).first() {
                Some(OperationArg::Statement(s)) if args.len() == 1 => Ok(s.predicate().clone()),
-                _ => Err(anyhow!("Invalid arguments to copy operation: {:?}", args)),
+                _ => Err(Error::OpInvalidArgs("copy".to_string())),


oops, the args is not captured by the error. I see that we were not doing that in some of the following errors before.

ed255 · 2025-04-17T10:41:10Z

src/error.rs

+
+    // Other
+    #[error("{0}")]
+    Custom(String),


Could we remove this Custom entry and use anyhow instead?
This way we can use the anyhow macro for convenience and it should get converted into Error::Anyhow.

src/middleware/custom.rs

src/middleware/operation.rs

src/backends/plonky2/mainpod.rs

src/backends/plonky2/primitives/signature.rs

ed255 · 2025-04-17T10:58:55Z

One major thing that I find missing from the proposal is that with this change we're losing the backtrace in the errors. anyhow includes backtraces in errors created with the anywhow! macro, so if there's an anywhow::Error displayed with the env var RUST_BACKTRACE=1, the backtrace attached to the error is also displayed.

I see in the documentation of thiserror mentions of backtraces https://docs.rs/thiserror/latest/thiserror/ but we would need to change the error type to include them and probably use a macro to create the errors so that the backtrace is generated.

arnaucube · 2025-04-17T16:45:44Z

Agree with everything, for context (@ax0 ), we had a call with @ed255 , and he is going to try adding macros for defining & returning the errors (defined by thiserror), while also returning the backtrace.

Update on the PR: got rid of the git-conflicts while rebasing the PR branch to last main branch updates.

ed255 · 2025-04-21T14:35:51Z

I'm currently working on this PR, although I'm not finished yet. Here are my goals:

Include backtraces in the errors we generate. To get this we can't just return a literal enum, because the backtrace requires a call.
Related to the previous point: I'll create methods to create errors so we can include the backtrace conveniently without changing too much the syntax. So instead of Err(Error::KeyNotFound(key)) (literal enum) it will be Err(Error::key_not_found(key)) (method call)
I think each error should be local to its scope, and each scope should only return its own error.
- The merkle tree should return TreeError and not Error
- The middleware should return MiddlewareError and not Error
With a global Error we can't easily include backend/frontend types in the error fields, so I'll declare a BackendError and a FrontendError and follow the pattern from the previous point
The Pod traits should be able to return backend errors and will be used in the frontend; for that I'll change them to use trait object Error: dyn std::error::Error

- Include backtraces in the errors we generate. To get this we can't just return a literal enum, because the backtrace requires a call. - Related to the previous point: add methods to create errors so we can include the backtrace conveniently without changing too much the syntax. So instead of `Err(Error::KeyNotFound(key))` (literal enum) it will be `Err(Error::key_not_found(key))` (method call) - Each error should be local to its scope, and each scope should only return its own error. - The merkle tree should return `TreeError` and not Error - The middleware should return `MiddlewareError` and not Error - With a global Error we can't easily include backend/frontend types in the error fields, so declare a `BackendError` and a `FrontendError` and follow the pattern from the previous point - The Pod traits should be able to return backend errors and will be used in the frontend; for that we change them to use trait object Error: `dyn std::error::Error`

ed255 · 2025-04-21T19:46:55Z

Topics to discuss:

Do you prefer types FrontendError, MiddlewareError, BackendError? or frontend::Error, middleware::Error, backend::Error?
Do you prefer type aliases FrontendResult, MiddlewareResult, BackendResult? or frontend::Result, middleware::Result, backend::Result?

ed255 · 2025-04-21T19:47:29Z

@arnaucube could you please review the new commits?

arnaucube · 2025-04-22T10:09:36Z

Regarding

Do you prefer types FrontendError, MiddlewareError, BackendError? or frontend::Error, middleware::Error, backend::Error?

Do you prefer type aliases FrontendResult, MiddlewareResult, BackendResult? or frontend::Result, middleware::Result, backend::Result?

Conceptually I prefer frontend::Error, middleware::Error, backend::Error and frontend::Result, middleware::Result, backend::Result, since each of them is the Error / Result in their context, and when you import you can always rename it like use frontend::Error as FrontendError ...

BUT (1): when using it, we will end up doing the renaming alias to use frontend::Error as FrontendError to avoid it colliding with other Error types.. so, then I think, 'why not already naming them frontend::FrontendError and we avoid having to rename them when importing them almost everytime?', also avoiding potential confusion between different Error from different submodules.

BUT (2): the case mentioned at 'BUT (1)' only happens very few times, so maybe it's fine to use frontend::Error and when needed just use an alias renaming when importing it; even we could just use it without alias and just using the Error directly as middleware::Error (eg. at mock/mainpod.rs#L248 ). Additionally, it will be shorter to use Error instead of BackendError.

So with this, unless we find other reasonings towards the other direction, I'm more inclined to use the frontend::Error naming. But it's not a strong opinion, so happy with the other way too if you find it more suitable ^^

(edited the comment to add the 'But (2)' after diving more into the last version of the branch)

arnaucube · 2025-04-22T10:19:19Z

src/backends/plonky2/primitives/merkletree_circuit.rs

    backends::plonky2::{
        basetypes::D,
        circuits::common::{CircuitBuilderPod, ValueTarget},
+        error::BackendResult,


since you moved primitives/merkletree.rs into primitives/merkletree/mod.rs, maybe we can move also this file primitives/merkeltree_circuit.rs into primitives/merkletree/circuit.rs too?

arnaucube · 2025-04-22T11:01:18Z

src/frontend/error.rs

+    #[error(transparent)]
+    Tree(#[from] crate::backends::plonky2::primitives::merkletree::error::TreeError),


is this one used? I've tried removing it and seems to not be used. I was trying this bcs I was thinking if maybe we could avoid using the TreeError since it could be 'embedded' in the backend::Error already.

ax0

LGTM!

arnaucube

LGTM!
I can not mark it as 'approve' in the GitHub UI since I opened the PR, but everything looks good from my side ^^

arnaucube changed the title ~~migrate from anyhow to thiserror (#190). pending polish error msgs~~ migrate from anyhow to thiserror Apr 16, 2025

arnaucube force-pushed the migrate-to-thiserror branch from 5b9fb26 to 046564b Compare April 16, 2025 16:21

ed255 reviewed Apr 17, 2025

View reviewed changes

arnaucube force-pushed the migrate-to-thiserror branch 2 times, most recently from e4c8b9e to d945379 Compare April 17, 2025 16:42

migrate from anyhow to thiserror (#190). pending polish error msgs

92fcdf3

arnaucube force-pushed the migrate-to-thiserror branch from d945379 to 92fcdf3 Compare April 17, 2025 16:47

ed255 force-pushed the migrate-to-thiserror branch from 0c15d87 to d0e0258 Compare April 21, 2025 19:27

Merge branch 'main' into migrate-to-thiserror

8a8f3bb

ed255 marked this pull request as ready for review April 21, 2025 19:45

ed255 requested a review from ax0 April 21, 2025 19:47

fix error

ca3a1c1

arnaucube commented Apr 22, 2025

View reviewed changes

apply suggestions from @arnaucube

84ec7a5

ax0 approved these changes Apr 22, 2025

View reviewed changes

ed255 added 4 commits April 22, 2025 13:28

rename XError and XResult to Error and Result

58e7b8b

Merge branch 'main' into migrate-to-thiserror

d999519

reorg signature

7bd7535

make frontend custom error more ergonomic

5967f0f

arnaucube commented Apr 22, 2025

View reviewed changes

ed255 added 2 commits April 22, 2025 14:08

Merge branch 'main' into migrate-to-thiserror

0718a86

remove unnecessary feature

87a6214

ed255 merged commit 29545f0 into main Apr 22, 2025
5 checks passed

		#[error(transparent)]
		Tree(#[from] crate::backends::plonky2::primitives::merkletree::error::TreeError),

Conversation

arnaucube commented Apr 16, 2025

Uh oh!

ed255 left a comment

Choose a reason for hiding this comment

Uh oh!

ed255 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

ed255 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ed255 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

ed255 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ed255 commented Apr 17, 2025

Uh oh!

arnaucube commented Apr 17, 2025

Uh oh!

ed255 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ed255 commented Apr 21, 2025

Uh oh!

ed255 commented Apr 21, 2025

Uh oh!

arnaucube commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnaucube Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

arnaucube Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

ax0 left a comment

Choose a reason for hiding this comment

Uh oh!

arnaucube left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ed255 commented Apr 21, 2025 •

edited

Loading

arnaucube commented Apr 22, 2025 •

edited

Loading