Box<dyn Error>

[kolektiv]

It's quite a common pattern (I think!) in throwaway scripts and example code at least (and I use it quite often just to play around with ideas) for a top-level main to return something like Result<(), Box<dyn Error>> - any normal errors can be converted to that, and it means you can have all kinds of different error-producing code called in your top-level main without having to worry about all of the errors converging to a concrete return type. However, that doesn't seem possible at the moment with exn - Exn<T> where T: Error doesn't have that conversion built in, and there's no simple way to fix that as a library user because of the orphan rule.

Would it be possible to address this in some way? I get that the general approach is to treat each module as an error boundary, but when you've got some experimental code calling all kinds of different parts of a codebase, having to .or_raise(...)? every function which was just ? in previous approaches is a bit of a pain for simple but rapidly changing code.

[SteelAlloy]

I second that, even in production code !
I use libraries (async-graphql) that return Result<_,Box<dyn std::error::Error + Send + Sync>> and I'm quite stuck.
I would really like to be able to call .or_raise on it.

the method `or_raise` exists for enum `Result<OpaqueCursor<_>, Box<dyn Error + Send + Sync>>`, but its trait bounds were not satisfied
the following trait bounds were not satisfied:
`std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>: exn::Error`
which is required by `std::result::Result<async_graphql::connection::OpaqueCursor<_>, std::boxed::Box<dyn std::error::Error + std::marker::Send + std::marker::Sync>>: exn::ResultExt`

[Byron]

Seeing no maintainer interactions here with any of the two issues or single PR makes me think the best course of action is to vendor the crate, and turn it into what one needs specifically.
That's also the great thing about exn - ~300SLOC of straightforward code are easy to maintain and adapt.

[Byron]

This is a patch that makes Exn compatible to thiserror and probably anyhow. The problem here is that this only makes interop possible, it doesn't make it convenient.

diff --git a/exn/src/debug.rs b/exn/src/debug.rs
index e45ea7e..f3449a3 100644
--- a/exn/src/debug.rs
+++ b/exn/src/debug.rs
@@ -19,7 +19,7 @@ use crate::Error;
 use crate::Exn;
 use crate::Frame;
 
-impl<E: Error> fmt::Debug for Exn<E> {
+impl<E: Error + Send + Sync + 'static> fmt::Debug for Exn<E> {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         write_exn(f, self.as_frame(), 0, "")
     }
diff --git a/exn/src/display.rs b/exn/src/display.rs
index 7b839d6..f896f77 100644
--- a/exn/src/display.rs
+++ b/exn/src/display.rs
@@ -17,7 +17,7 @@ use std::fmt;
 use crate::Error;
 use crate::Exn;
 
-impl<E: Error> fmt::Display for Exn<E> {
+impl<E: Error + Send + Sync + 'static> fmt::Display for Exn<E> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(f, "{}", self.as_error())
     }
diff --git a/exn/src/impls.rs b/exn/src/impls.rs
index fadf1fb..56ea0e5 100644
--- a/exn/src/impls.rs
+++ b/exn/src/impls.rs
@@ -19,20 +19,20 @@ use std::panic::Location;
 use crate::Error;
 
 /// An exception type that can hold an error tree and additional context.
-pub struct Exn<E: Error> {
+pub struct Exn<E: Error + Send + Sync + 'static> {
     // trade one more indirection for less stack size
     frame: Box<Frame>,
     phantom: PhantomData<E>,
 }
 
-impl<E: Error> From<E> for Exn<E> {
+impl<E: Error + Send + Sync + 'static> From<E> for Exn<E> {
     #[track_caller]
     fn from(error: E) -> Self {
         Exn::new(error)
     }
 }
 
-impl<E: Error> Exn<E> {
+impl<E: Error + Send + Sync + 'static> Exn<E> {
     /// Create a new exception with the given error.
     ///
     /// This will automatically walk the [source chain of the error] and add them as children
@@ -118,6 +118,16 @@ impl<E: Error> Exn<E> {
             .expect("error type must match")
     }
 
+    /// Discard all error context and return the underlying error.
+    ///
+    /// This may be needed to obtain something that once again implements `std::error::Error`.
+    pub fn into_box(self) -> Box<E> {
+        match Box::<dyn std::any::Any + Send + Sync + 'static>::downcast(self.frame.error) {
+            Ok(err) => err,
+            Err(_) => unreachable!("The type in the frame is always the type of this instance"),
+        }
+    }
+
     /// Return the underlying exception frame.
     pub fn as_frame(&self) -> &Frame {
         &self.frame
@@ -127,7 +137,7 @@ impl<E: Error> Exn<E> {
 /// A frame in the exception tree.
 pub struct Frame {
     /// The error that occurred at this frame.
-    error: Box<dyn Error>,
+    error: Box<dyn Error + Send + Sync + 'static>,
     /// The source code location where this exception frame was created.
     location: &'static Location<'static>,
     /// Child exception frames that provide additional context or source errors.
diff --git a/exn/src/lib.rs b/exn/src/lib.rs
index c3cc71b..ad5bb04 100644
--- a/exn/src/lib.rs
+++ b/exn/src/lib.rs
@@ -119,6 +119,6 @@ impl<T> Error for T where T: std::error::Error + std::any::Any + Send + Sync + '
 ///    |         consider giving this pattern the explicit type `std::result::Result<i32, E>`, where the type parameter `E` is specified
 /// ```
 #[expect(non_snake_case)]
-pub fn Ok<T, E: Error>(value: T) -> Result<T, E> {
+pub fn Ok<T, E: Error + Send + Sync + 'static>(value: T) -> Result<T, E> {
     Result::Ok(value)
 }
diff --git a/exn/src/result.rs b/exn/src/result.rs
index 8d58548..95b58f1 100644
--- a/exn/src/result.rs
+++ b/exn/src/result.rs
@@ -37,7 +37,7 @@ pub trait ResultExt {
 
 impl<T, E> ResultExt for std::result::Result<T, E>
 where
-    E: Error,
+    E: Error + Send + Sync + 'static,
 {
     type Success = T;
     type Error = E;
@@ -57,7 +57,7 @@ where
 
 impl<T, E> ResultExt for std::result::Result<T, Exn<E>>
 where
-    E: Error,
+    E: Error + Send + Sync + 'static,
 {
     type Success = T;
     type Error = E;

So it seems that at the boundary of the crate, if one would use anyhow for instance, one would have to convert every error by hand like my_exn_function().map_error(Exn::into_box)? if the surrounding function is fn foo() -> anyhow::Result.

And the reason one cannot just implement std::error::Error for Exn<E> is that it results in conflicting ResultExt implementations. One of these is for impl std::error::Error and the other one has to be for Exn<E: Error>.
So it looks like this is an unsurmountable issue at least with the current compiler.
But I will see if merely removing the conflicting impl still works correctly, despite the problem that we would get Box<Box<Box<Exn<E>>> quite quickly if enough nested crates are using it.

Maybe one could have some sort of RootExn which implements std::error::Error, and that would work even if it would only be exposed by the top-level crate in a set of plumbing crates. In this case, gix would use that, and would then would be disallowed to ever add context when calling its own functions if these already return a TopLevel error.

From my own experience, it's definitely required to return error types that actually implement std::error::Error, particularly so when such errors are returned by a library.

In theory, at least thiserror could do auto-conversions if source: Box<E>, but From<Exn<E>> for Box<E> also can't be implemented due to Rust rules.

[Byron]

Experimenting with Error for Exn<E> more, I noticed a side-effect of Exn<E> boxing itself. The type gets more and more nested, which also means that Exn<Exn<E>> isn't the same as Exn<E> anymore, even though it is conceptually. A failing test in the exn crate made that very clear.

Thus, a hypothetical TopExn would have to erase all type information of the boxed inner error. This shouldn't even impede the functioning of examples like this one were we search the error tree for information, the killer feature and motivation for me to switch.

From all this so far I can extract a usage pattern like this:

• If no error is tracked in a plumbing function, let it return std::result::Result<_, PlainError>.
• If an error is tracked in a plumbing function, let it return exn::Result<_, PlainError>
  • plumbing crates are all in the exn ecosystem, so they will have no trouble to conveniently interop.
  • users who opt to use plumbing crates directly have to deal with exn or not exn, depending, and must live with manual map_error(Exn::into_box) conversions.
• All functions in the top-level crate (that most users will consume) return the RootExn (naming is hard) error which has no type information whatsoever, but a bunch of utilities to help find data in the error tree. Top-level crates can wrap it to expose their own custom features, like err.is_retryable().
  • The top-level crate as to be careful to not chain multiple RootExn probably only for performance reasons, and instead try to call specialised conversion functions.

All that sounds quite reasonable

[Skgland]

Seeing no maintainer interactions here with any of the two issues or single PR

The oldest issue/PR is 3 days old, which isn't a long time in my opinion.
The last commit is 5 days old so it doesn't look unmaintained to me.

[Byron]

I never said it's unmaintained, and shared my own experience.

[Byron]

This might be my last message here as I am closing in on vendoring a gitoxide specific version of exn, and I think it will be what I looked for since its inception.

What I wanted to share here is that I finally found out why exn stores error trees, instead of just a list - after all a callstack is nothing more.

The gist is that it supports explicit instantiation to pass more than one child. That would allow the collection of related errors into a tree, which is pretty cool particularly for async applications. gitoxide is probably too linear to be able to use it, but I can now be looking out for opportunities.

#[test]
fn error_tree() {
    let e1 = Error("E1").raise();
    let e3 = e1.raise(Error("E3"));

    let e9 = Error("E9").raise();
    let e10 = e9.raise(Error("E10"));

    let e11 = Error("E11").raise();
    let e12 = e11.raise(Error("E12"));

    let e5 = Exn::from_iter([e3, e10, e12], Error("E5"));

    let e2 = Error("E2").raise();
    let e4 = e2.raise(Error("E4"));

    let e7 = Error("E7").raise();
    let e8 = e7.raise(Error("E8"));

    let e6 = Exn::from_iter([e5, e4, e8], Error("E6"));
    insta::assert_debug_snapshot!(e6, @r"
    E6, at gix-error/tests/error/exn.rs:70:14
    |
    |-> E5, at gix-error/tests/error/exn.rs:62:14
    |   |
    |   |-> E3, at gix-error/tests/error/exn.rs:54:17
    |   |   |
    |   |   |-> E1, at gix-error/tests/error/exn.rs:53:26
    |   |
    |   |-> E10, at gix-error/tests/error/exn.rs:57:18
    |   |   |
    |   |   |-> E9, at gix-error/tests/error/exn.rs:56:26
    |   |
    |   |-> E12, at gix-error/tests/error/exn.rs:60:19
    |       |
    |       |-> E11, at gix-error/tests/error/exn.rs:59:28
    |
    |-> E4, at gix-error/tests/error/exn.rs:65:17
    |   |
    |   |-> E2, at gix-error/tests/error/exn.rs:64:26
    |
    |-> E8, at gix-error/tests/error/exn.rs:68:17
        |
        |-> E7, at gix-error/tests/error/exn.rs:67:26
    ");
}

[kolektiv]

I've been pondering this whole design as well. I think the conceptual foundation - keeping a tree of causal errors - is a really good one. Where I think it gets tricky is the split between Error and Exn - which means you now have things which are errors, and things which are error-like but not errors, which causes subtle but awkward issues further down (and also starts to encourage things like specific types/functions for Result/Ok which also start to cause difficulties when mixing them in with more complex code).

The problem fundamentally seems to be that we've got a container for errors, not an error which can also contain errors. I started to look at the latter as an approach, but I've not given it enough thought yet. There are definitely some obvious challenges - with a container type, you can control things like the Debug/Display impls easily, but with an Error-containing Error, you'd need to introduce some new kind of "error level" Display equivalent which would get called for leaves in the error tree but not otherwise. The current approach also erases quite a bit of information - the errors in the tree get converted to an erased string form, potentially losing other attached data (and in future if the Error trait grows, or becomes augmented, that will implicitly erase more and more). I'm not sure what the answer is, and I don't mean this is criticism of exn at all, it's the best thinking I've seen around an Error approach so far, but I think there's further we need to go!

[Byron]

Thanks for sharing, much appreciated!

The current approach also erases quite a bit of information - the errors in the tree get converted to an erased string form, potentially losing other attached data

I don't think this is correct. There is type erasure only for the 'linked' types, the ones that get stored in child frames, but one still has access to the dyn Error trait so the context API will be usable through that.
Of course, one will also be able to downcast these errors to well-known types, and this is how I plan to use it for now, just like in the examples.

I am working on this here if you want to take a look: GitoxideLabs/gitoxide#2352.

[kolektiv]

I don't think this is correct. There is type erasure only for the 'linked' types, the ones that get stored in child frames, but one still has access to the dyn Error trait...

Ah, yes, I think you might be right there, I think I was looking at this line

exn/exn/src/impls.rs

Line 65 in bb4d8ea

error: Box::new(SourceError(source.to_string())),

as being more applicable than it is. I'll take a look at what you're doing as well!

[Byron]

Before you try to browse through the diff, let me share real quick what my main changes are:

• make it dyn std::error::Error + Send + Sync + 'static
• add Exn::into_box to be able to interact with places that can't have Exn. This is good for now, but probably won't be used later anymore.
• Rename Error to ErrorExt and add Error as top-level that is either a boxed std::Error, or a Frame. Full type erasure, but it keeps enough types to be able to walk the tree to probe for well-known errors that contain information.

The idea is to have standard errors in gix-errors and reuse them so Error can provide utilities to extract information from the error tree. The Error is then used as standard, unified error in gix, while all plumbing uses plain errors or Exn<E>. Note that I make Exn explicit, no hiding in a custom Result.

And that, I believe, will do the trick, while allowing for future improvements and ensuring the error type can be stable.

[Eddio0141]

Having this feature itself on ext would be nice, but I just settled with a new type ErrorWrap

#[derive(Debug)]
struct ErrorWrap(Box<dyn exn::Error>);

impl ErrorWrap {
    fn new<T>(err: T) -> Self
    where
        T: exn::Error,
    {
        Self(Box::new(err))
    }
}

impl std::fmt::Display for ErrorWrap {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "{}", self.0)
    }
}

impl std::error::Error for ErrorWrap {}

Edit: Nevermind, I see the problem if it's being returned by an external crate. I haven't ran into that problem yet, is someone going to make a PR for this?

[Byron]

Did anyone figure out why exn::Error is needed? It also inherits from any instead of using the built-in downcast facilities that std::error::Error provides that it also inherits from, and from all I can tell, it's not needed while possibly making exn::Ok necessary.

[tisonkun]

Did anyone figure out why exn::Error is needed? It also inherits from any instead of using the built-in downcast facilities that std::error::Error provides that it also inherits from, and from all I can tell, it's not needed while possibly making exn::Ok necessary.

@Byron

@andylokandy tried to remove it in #23 but I notice that we need still Send + Sync in several places in my use case.

In your patch above you suggest to inline the trait bound everywhere and that can be a choice.

Let me consider it and perhaps release a 0.3.0-rc.2 for testing.

[tisonkun]

Seeing no maintainer interactions here

As you know, I'm maintaining several OSS projects ¹. So feel free to ping me here, or even if I watch the project, I can miss some notifications ².

Footnotes

1. I generally read recent repos weekly or monthly. Sometimes I totally forget some of them. I'm glad to be ping-ed to keep me remembering what I have exported to the public. ↩

2. See also https://github.com/EmbarkStudios/cargo-deny/pull/805#issuecomment-3566634178. ↩