shepmaster / snafu

Easily assign underlying errors into domain-specific errors while adding context
https://docs.rs/snafu/
Apache License 2.0
1.39k stars 60 forks source link

Add boxed_context and with_boxed_context #455

Closed ChristopherRabotin closed 2 months ago

ChristopherRabotin commented 3 months ago

Hi there,

First off, thanks for the work on snafu. I started using it a few months ago, and it's fantastic.

I've used it so thoroughly, that I'm now encountering warnings in a pretty large library I maintain where the error variant is getting quite large. As such, the compiler recommends that I return boxed errors, i.e. Result<T, Box<E>> instead of Result<T, E>.

snafu already provides a Box<dyn Error> return capability. Rightfully so, the documentation discourages its use. As such, I'm proposing the addition of boxed_context and with_boxed_context: they work as context and with_context respectively.

Let me know if this is a fringe use case, and if so, what is the recommendation for returning a boxed error via with_context.

Thanks

netlify[bot] commented 3 months ago

Deploy Preview for shepmaster-snafu ready!

Name Link
Latest commit ef871b9e548080cf33fc1d28edf4dd513ebdd8c5
Latest deploy log https://app.netlify.com/sites/shepmaster-snafu/deploys/66526afaf5d70a0008bebaa9
Deploy Preview https://deploy-preview-455--shepmaster-snafu.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

shepmaster commented 3 months ago

As such, the compiler recommends

Presumably you mean Clippy's result_large_err, right?

use snafu::prelude::*;

fn main() {
    println!("{:?}", usage());
}

fn usage() -> Result<(), Error> {
    TinySnafu.fail()
}

#[derive(Debug, Snafu)]
enum Error {
    Tiny,

    Big { source: BigSourceError },
}

#[derive(Debug, Snafu)]
struct BigSourceError {
    data: [u8; 1024],
}
warning: the `Err`-variant returned from this function is very large
  --> src/main.rs:7:15
   |
7  | fn usage() -> Result<(), Error> {
   |               ^^^^^^^^^^^^^^^^^
...
15 |     Big { source: BigSourceError },
   |     ------------------------------ the largest variant contains at least 1024 bytes
   |
   = help: try reducing the size of `Error`, for example by boxing large elements or replacing it with `Box<Error>`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
   = note: `#[warn(clippy::result_large_err)]` on by default

In these cases, I usually transform the source to box it:

diff --git a/src/main.rs b/src/main.rs
index f0fd409..ff73c54 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -12,7 +12,10 @@ fn usage() -> Result<(), Error> {
 enum Error {
     Tiny,

-    Big { source: BigSourceError },
+    Big {
+        #[snafu(source(from(BigSourceError, Box::new)))]
+        source: Box<BigSourceError>,
+    },
 }

 #[derive(Debug, Snafu)]

This has the nice benefit that any existing .context calls will automatically perform the boxing. Do you think that would work in your case?

ChristopherRabotin commented 3 months ago

Excellent, yes, I'm sure this will indeed solve my use case, thank you!

On Tue, Jun 4, 2024, 13:47 Jake Goulding @.***> wrote:

As such, the compiler recommends

Presumably you mean Clippy's result_large_err https://rust-lang.github.io/rust-clippy/stable/index.html#/result_large_err, right?

use snafu::prelude::*; fn main() { println!("{:?}", usage());} fn usage() -> Result<(), Error> { TinySnafu.fail()}

[derive(Debug, Snafu)]enum Error {

Tiny,

Big { source: BigSourceError },}

[derive(Debug, Snafu)]struct BigSourceError {

data: [u8; 1024],}
warning: the Err-variant returned from this function is very large --> src/main.rs:7:15 7 fn usage() -> Result<(), Error> { ^^^^^^^^^^^^^^^^^ ... 15 Big { source: BigSourceError }, ------------------------------ the largest variant contains at least 1024 bytes

= help: try reducing the size of Error, for example by boxing large elements or replacing it with Box<Error> = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err = note: #[warn(clippy::result_large_err)] on by default

In these cases, I usually transform the source https://docs.rs/snafu/latest/snafu/derive.Snafu.html#transforming-the-source to box it:

diff --git a/src/main.rs b/src/main.rs index f0fd409..ff73c54 100644--- a/src/main.rs+++ b/src/main.rs@@ -12,7 +12,10 @@ fn usage() -> Result<(), Error> { enum Error { Tiny,

  • Big { source: BigSourceError },+ Big {+ #[snafu(source(from(BigSourceError, Box::new)))]+ source: Box,+ }, }

    [derive(Debug, Snafu)]

This has the nice benefit that any existing .context calls will automatically perform the boxing. Do you think that would work in your case?

— Reply to this email directly, view it on GitHub https://github.com/shepmaster/snafu/pull/455#issuecomment-2148299557, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEZV2CLYYNXNCF2WUTYYODZFYKTPAVCNFSM6AAAAABIJHV5BKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBYGI4TSNJVG4 . You are receiving this because you authored the thread.Message ID: @.***>

ChristopherRabotin commented 3 months ago

I'm only trying this out now, and I'm seemingly not using it correctly, but I can't seem to figure out what I'm doing wrong.

Here is what I did:

--- a/src/od/mod.rs
+++ b/src/od/mod.rs
@@ -158,11 +158,21 @@ where
     }
 }

+#[derive(Debug, Snafu)]
+#[snafu(visibility(pub(crate)))]
+pub enum Error {
+    SourceNeedsToBeBoxed {
+        #[snafu(source(from(TrajError, Box::new)))]
+        source: Box<TrajError>,
+    },
+}
+
 #[derive(Debug, PartialEq, Snafu)]
 #[snafu(visibility(pub(crate)))]
 pub enum ODError {
-    #[snafu(display("during an orbit determination, encountered {source}"))]
-    ODPropError { source: PropagationError },
+    // #[snafu(display("during an orbit determination, encountered {source}"))]
+    #[snafu(source(from(PropagationError, Box::new)))]
+    ODPropError { source: Box<PropagationError> },
     #[snafu(display("during an orbit determination, encountered {source}"))]
     ODDynamicsError { source: DynamicsError },
     #[snafu(display("at least {need} measurements required for {action}"

And here is the error I'm getting:

error: `source` attribute is only valid on enum variant or struct fields with a name, not on an enum variant
   --> src/od/mod.rs:174:13
    |
174 |     #[snafu(source(from(PropagationError, Box::new)))]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Note that the error only happens in the ODError enum, not in the Error, which is from the documentation you linked to.

Further, I haven't figured out how to add a display to a Boxed error. It seems to also cause the same error.

What am I misunderstanding here?

Thanks

shepmaster commented 3 months ago

error: source attribute is only valid on enum variant or struct fields with a name, not on an enum variant

I don't know how to say it better than this message, so I'm really hoping you can help me reword it to be clearer!

You have

#[snafu(source(from(PropagationError, Box::new)))]
ODPropError { source: Box<PropagationError> },

You need

ODPropError { 
    #[snafu(source(from(PropagationError, Box::new)))]
    source: Box<PropagationError>,
},
Stargateur commented 2 months ago

@shepmaster there is a problem with the error message "error: source attribute is only valid on enum variant or struct fields with a name, not on an enum variant"

"only valid on enum variant" "not on an enum variant"

XD The "or" is quite confusing here, I think you want to write "is only valid on enum variant field" or something like that. I think the best would be to write "error: source attribute is only valid for fields"

ChristopherRabotin commented 2 months ago

I was confused by the error because, as Stargateur pointed out, it said that I need to place it on the enum variant ... which is what I was doing. I don't know how I didn't see the difference between the demo code and what I coded, but it's obvious now, and it works out of the box.

Thank you!

alerque commented 2 months ago

error: source attribute is only valid on enum variant or struct fields with a name, not on an enum variant

I don't know how to say it better than this message, so I'm really hoping you can help me reword it to be clearer!

Depending on what exactly you mean (I'm not 100% clear myself) perhaps something like one of these: