Ensure all errors implement at least `Debug`, `Clone`, `PartialEq` and `Eq` traits

[polydez]

This is needed for better API

[bobbinth]

Is this something we need immediately? If so, we should make this PR against main and then I'll need to make a v0.9.1 release of the relevant crates. If not, we can keep it against next - but I would merge it after #1277 and we can propagate it to miden-base with v0.10.0 release (probably in several weeks).

[polydez]

@bobbinth it blocks me because I need to connect these error types with other types in miden-node which are forced to have these traits implemented. And it will help us to improve API. We should require at least these traits for every error enum.

[polydez]

@Overcastan

Just wondering, why should we change the order of traits from alphabetical order to Debug, Clone, PartialEq, Eq? Is this a convention?

Thank you for noticing! There is no convention, rustfmt team says that they don't even want to add such feature to their tool because changing ordering of derives could change the behavior of compiled code, since each derive can see only following derives. I just made ordering by my preference and made it the same for all structs. But if some of us prefer to order them alphabetically, I can change this ordering. My only wish is to have consistent ordering.

[bitwalker]

I want to push back on this. We should not impose this restriction on error types anywhere (except for Debug - that feels like it should always be implemented for all error types), or rather, we can't in general - so we are going to have to fix the underlying issue here a different way anyway, especially once 1277 is merged.

The fundamental problem is that many error types do not implement any of these traits, and contorting our error types to do so is going to make them less useful. A few points:

• We do not need Eq or PartialEq for testing - we can either use assert_matches! or assert!(matches!(...)), rather than assert_eq!, or we can render to a string and assert a match against the error message, or we can return errors from test functions and use #[should_panic(...)] to assert a specific type of failure.
• I would like to know more about why Clone is needed. If we have a special case where we need to store a cloneable error value, then we should box up the error type, and move it into an Arc, effectively making uncloneable errors, cloneable, on a case-by-case basis.
• The Report type introduced by 1277 (and more generally, things like anyhow::Error) do not implement any of these traits except Debug, and can't in general
• Many standard library errors (of particular note, std::io::Error), do not implement any of these except `Debug

[bobbinth]

@bitwalker - i think your points make sense. I do think these changes are OK for v0.9 release as we do these things for many error types already, I believe. So, I'm still inclined to make this release and then do a more comprehensive refactoring once we migrate to the next version of Miden VM (v0.10) which would include 1277.

[bitwalker]

@bobbinth The problem is it will now be a breaking change in the next release (when we roll these changes back). It also sounds like we intend to write code with the assumption that all of our error types implement these traits, which will then be broken as a result. I'll leave it up to you all, but I think this is going to be more pain than it is worth.

[polydez]

@bitwalker now I agree with you, we shouldn't force to implement all of these traits. I'd like to have at least PartialEq until assert_matches! is stabilized, because assert!(matches!(...)) doesn't print actual value of the expression. Can't remember, where we use Clone trait, but I have seen it today. You're right, we can get rid of clones of errors.

[bobbinth]

Based on offline discussions, I've published v0.9.1 as is. We should revisit our error handling strategy for v0.10 release. Specifically, we should migrate to thiserror (as discussed in https://github.com/0xPolygonMiden/miden-vm/issues/1261) for all error types and I think this would also lead to removing extra trait impls from these errors.

[bitwalker]

@bitwalker now I agree with you, we shouldn't force to implement all of these traits. I'd like to have at least PartialEq until assert_matches! is stabilized, because assert!(matches!(...)) doesn't print actual value of the expression.

@polydez If you pull in miden-core, it exports an assert_matches! macro that provides equivalent behavior as the unstable nightly macro without needing nightly :). EDIT: Once 1277 is merged I mean, but we could probably split it out into a separate PR if it is needed before then

Also, with assert!(matches!(...), "{lhs:?} doesn't match {rhs:?}"); you can accomplish something similar, though the output might not be quite as nice, it still gives you useful output.

[polydez]

@bitwalker oh, I didn't know that, thanks a lot! Don't need to extract it to separate PR, will wait for #1277. :)