Cannot create a recursive error type about snafu HOT 15 CLOSED

In the code for mdBook, they had some similar code to box the error to save space (not sure why, I haven't dug into it yet).

Yeah I do the same sometimes if benchmarking guides me there. For example: https://github.com/BurntSushi/rust-csv/blob/366c07528af626318c1cc8c172e3242b7fe12570/src/error.rs#L28

Basically the idea is that boxing an error is very inexpensive in most scenarios, since in most scenarios, errors rarely occur. But the error type itself can become quite large, which in turn makes Result<T, Error> large, which in turn results in more time spent in memcpy'ing that stuff around. Using a box can drastically reduce that size and improve overall performance.

from snafu.

I'm not likely to be able to push out a new version in the next week or so, but I do see an (ugly) workaround to enable you to move forward.

If you implement Borrow yourself and use foo().map_err(Box::new).context(Selector), you can use the created functions. The autogenerated implementation of Borrow will look something like:

impl/*<...>*/ std::borrow::Borrow<std::error::Error> for std::boxed::Box<Error>
where
    Self: std::error::Error + 'static,
    /* ... */
{
    fn borrow(&self) -> &(std::error::Error + 'static) {
        self
    }
}

from snafu.

N.B. even if you get past this issue by implementing Borrow on Box<Error>, the context selector stuff doesn't work, presumably because you have an Error but this formulation wants a Box<Error>.

I'm on mobile, but I can provide a code sample tomorrow when I'm on a machine if the above isn't clear.

(Also apologies if you consider this a separate bug.)

from snafu.

This problem is caused because we need to use Borrow in our implementation of source / cause in order to support boxed trait objects:

use std::{borrow::Borrow, error, fmt};

#[derive(Debug)]
pub enum Error {
    Recursive { source: Box<Error> },
    TraitObject { source: Box<dyn error::Error> },
}

impl fmt::Display for Error {
    fn fmt(&self, _f: &mut fmt::Formatter) -> fmt::Result {
        Ok(())
    }
}

impl std::error::Error for Error
where
    Self: fmt::Debug + fmt::Display,
{
    fn description(&self) -> &str {
        ""
    }

    fn source(&self) -> Option<&(error::Error + 'static)> {
        match *self {
            // Must not have `Borrow`
            Error::Recursive { ref source, .. } => Some(source),
            
            // Requires `Borrow`
            Error::TraitObject { ref source, .. } => Some(Borrow::borrow(source)),
        }
    }
}

If there was some way of avoiding the usage of Borrow, I'd be a fan.

from snafu.

you have an Error but this formulation wants a Box<Error>

In the code for mdBook, they had some similar code to box the error to save space (not sure why, I haven't dug into it yet).

One thought I've had for that that might help with that is to allow some sort of customization. Possibly something like:

#[derive(Snafu)]
enum Error {
    // Just let them transform the source?
    #[snafu(transform_source(|source| Box::new(source)))]
    IdeaOne { source: Box<Error> },

    // Let the user write it and do what they want; ultimate flexibility
    #[snafu(from(disable))]
    IdeaTwo { source: Box<Error> },

    // Maybe it's only ever a `Box`? If idea one allows `Box::new`, this is pretty weak.
    #[snafu(from(box))]
    IdeaThree { source: Box<Error> },
}

impl From<Context<IdeaTwo<A, B, C>>> for Error {
    fn from(...) -> Self {
        Error::IdeaTwo { source: Box::new(context.source), ... }
    }
}

from snafu.

From a bit of playing around, the attribute idea would have to look like

#[snafu(whatever(StartingType, expression-converting-from-start-to-expected))]

So maybe looking like

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

from snafu.

I have a local branch with this working well enough; it looks like this:

#[derive(Debug, Snafu)]
enum Error {
    Variable {
        name: String,
    },
    Operation {
        #[snafu(source(from(Error, Box::new)))]
        source: Box<Error>,
    },
}

When you use snafu(source(from(...))), that replaces the default From implementation. In this example, it will only convert from Error, not Box<Error>.

@BurntSushi do you think that (coupled with the Borrow implementation) would suit your case?

from snafu.

Please take a look at #67 if you'd like to try it out. It contains a test specifically for recursive types.

from snafu.

One thing that was confusing for me is that you do need to .map_err(Box::new) yourself, before the context() call, as @shepmaster mentioned above. I had assumed that specifying source(from(Error, Box::new)) would make it box the recursive error for you.

Maybe it would be helpful if the example in https://docs.rs/snafu/0.4.1/snafu/guide/attributes/index.html#transforming-the-source were more explicit about that? I'm guessing that's where people will look to figure out how to set up recursive errors, since I don't see an example anywhere else.

from snafu.

Can you try source(from(Error, |e| Box::new(e)))? These are subtly different to Rust.

from snafu.

@shepmaster Thanks for your response! Unfortunately that doesn't seem to make a difference. I've created a minimal example I'm using to test; without the map_err uncommented, neither source(from) form avoids the error in the included comment.

https://gist.github.com/tjkirch/8917dd863bc284db20384fb303651ea6

from snafu.

Hmm. We have tests which seem superficially the same…

from snafu.

Oh! Yes, I realllllly need to fix this. The attribute is in the wrong spot:

Current

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

Correct

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

from snafu.

I’ve created an issue for that: #105

from snafu.

Aha! Thanks! Sorry I missed the difference. It's working great now.

from snafu.