Working with Result from a library authors' perspective


#1

I am working on a library for learning purposes and needed to report success or failure from a function. Here is a piece of code that illustrates the essentials.

// Version 1

enum WorkResult {
    Ok { a: i32 },
    Problem1 { a: i32 },
    Problem2,
}

fn work(input: i32) -> WorkResult {
    match input {
        0 => WorkResult::Ok { a: 1 },
        1 => WorkResult::Problem1 { a: 1 },
        _ => WorkResult::Problem2,
    }
}

While this works, it does not leverage the functionality that the Result type gives you. So, after some changes I ended up with the following.

// Version 2

// The WorkResult enum is split into the success (WorkOk) and error (WorkErr)
// types. This must be done to properly define WorkResult based on Result.

// This is a wrapper around an int to make the meaning of its value clear. I
// realize I could also simple use i32 instead but decided not to for the
// previous reason and consistency with a similar function of which the success
// type contains two i32 fields.
struct WorkOk {
    a: i32,
}

enum WorkErr {
    Problem1 { a: i32 },
    Problem2,
}

type WorkResult = Result<WorkOk, WorkErr>;

fn work(input: i32) -> WorkResult {
    match input {
        // Writing ResultOk(WorkOk ...) is simple and explicit, but it would be
        // nice if I could get away with just WorkOk ...
        0 => Result::Ok(WorkOk { a: 1 }),
        1 => Result::Err(WorkErr::Problem1 { a: 1 }),
        _ => Result::Err(WorkErr::Problem2),
    }
}

As mentioned in the comments, writing Result::Err(WorkErr ...) feels kind of double so I wondered if there was a better approach. Then I discovered the From trait.

// Version 3

struct WorkOk {
    a: i32,
}

enum WorkErr {
    Problem1 { a: i32 },
    Problem2,
}

type WorkResult = Result<WorkOk, WorkErr>;

// This allows me to use .into() to coerce a WorkOk ... into a Result::Ok(WorkOk
// ...). Unfortunately this requires this boilerplate code. A macro would help
// me now but make the implementation harder to understand for other developers
// including Future Me.
impl From<WorkOk> for WorkResult {
    fn from(ok: WorkOk) -> WorkResult {
        Result::Ok(ok)
    }
}

impl From<WorkErr> for WorkResult {
    fn from(err: WorkErr) -> WorkResult {
        Result::Err(err)
    }
}

fn work(input: i32) -> WorkResult {
    match input {
        // Initially I didn't like it but perhaps writing .into() is not too
        // bad. Makes it clear one type is being transformed into another.
        0 => WorkOk { a: 1 }.into(),
        1 => WorkErr::Problem1 { a: 1 }.into(),
        _ => WorkErr::Problem2.into(),
    }
}

Using From has its up and downsides.

I was wondering if there is 1) another approach to this and 2) what the preferred way of going about this is and why.


#2

You might find http://blog.burntsushi.net/rust-error-handling/ useful, if you’ve not read it yet.

Personally, I’d skip the WorkOk wrapper. I see your rationale, but Result<i32, WorkErr> seems to be sufficient to me to convey the meaning. Once the caller deals with your WorkResult and takes the i32 out, I can’t quite see why it’s still important to know that it came from a WorkOk. That would, I think, avoid your feeling of writing double.

I’d also punt on version 3 with the From impls. The following looks good to me:

[code]
type WorkResult = Result<i32, WorkErr>;

fn work(input: i32) -> WorkResult {
match input {
0 => Ok(1),
1 => Err(WorkErr::Problem1{ a: 1 }),
_ => Err(WorkErr::Problem2),
}
}[/code]
Then impl Error for WorkErr and you’re good to go.


#3

First off, thank you for your reply. I finally found time to read the article you link to and it is a great read. Especially the in-depth information on the Error trait and the try! macro were useful to me.

I do not intend to start a discussion you about the WorkOk wrapper but I do want to shed some light on the case I am considering to apply it to. Partly because one sentence of your reply signaled that the case was not completely clear.

There are two functions encode and decode which could both read from an array of bytes and write to another one. On success, they could both return the following struct:

struct EncodeDecodeOk {
    /** The number of bytes read from the input byte array */
    bytes_consumed: usize,
    /** The number of bytes written to the output byte array */
    bytes_produced: usize,
}

The number of bytes produced is always important, unless an error occurs, for both encoding and decoding.

For decoding, the number of bytes consumed is important because it is the index at which the next decoding operation should start.

For encoding however, the number of bytes consumed is always the entire input byte array, unless an error occurs. If an error occurs the number of bytes consumed are not useful.

For consistency, which I value as a library consumer, I am tempted to define the result types as follows (showing essentials only):

struct EncodeOk {
    produced: usize,
}

struct DecodeOk {
    consumed: usize,
    produced: usize,
}

Once the caller deals with your WorkResult and takes the i32 out, I can’t quite see why it’s still important to know that it came from a WorkOk

It is not the type that is important, it is the name of the field in combination with the return type for the reverse operation that is important.

Thanks again, I will take some time to absorb and work with the information from the article.


#4

It sounds like the question here is more about the type of the return value, and less about errors. In other words, even if your operations were infallible, you still have the decision to make on how to structure the return values.

Your DecodeOk type makes sense (I’d use a different name, but that’s beside the point) since it groups two related usize values. Since you’re returning multiple values, putting them into a type is reasonable - an alternative is returning a tuple, but then it’s easy for caller to mix up the two values in the tuple (especially since they’re of the same type); giving the two usizes meaningful names is also user friendly.

The EncodeOk, though, I’m less certain of. I understand the desire for symmetry. But, wrapping a single value with a type that doesn’t elevate the abstraction level nor provide any added type safety seems like a net negative. If you take this line of thinking to the extreme, as an exercise, then one could wrap every single value you return in a custom type just to give it a different name. I’m sure we’ll agree that’s not sensible and the library users won’t be happy.


#5

In this topic I request a pattern that works well in practice for defining and returing custom errors in libraries. I realized at the time of writing someone might dwell on the WorkOk wrapper and considered taking it out because it is not the subject of the question I wanted an answer to. For people who are interested in the trade-off, I will provide my point of view.

I agree that returning a tuple is not the way to go because consumed and produced do not have a strong natural ordering.

The wrapper is actually doing the opposite. It gives information on how to interpret the usize inside of it. It is taking a measurement and assigning the unit. Documentation can do that too, but I prefer understandable consistent code about without too many exceptions.

These are arguments in favor of keeping EncodeOk. Everyone values them differently and that is fine with me.

  1. Symmetry between the fields in the return types of encode and decode. Fields with the same name convey the same information.
  2. If encode were to return a usize, it could contain either consumed or produced. Reading the documentation is necessary when writing code but more importantly, also when reviewing or maintaining code, depending on surrounding comments and variable naming.
  3. Consistency; when destructuring an EncodeOk, big chance you’ll bind a variable called produced consistently everywhere you call encode.
  4. Better version stability; adding a field later on to EncodeOk is possible while changing the return type from usize to a struct is not.

From these arguments it should be clear that I value code maintainability highly. I am also a minimalist so I can see the arguments in favor of the simplicity of a usize. I need practical experience in order to get a better understanding of this trade-off, not more theoretical arguments.

As with everything there is a balance. There are many good things that turn bad when you take them to extremes. Perhaps you find this an interesting read: reductio ad absurum.


#6

I could maybe buy that argument if you were returning a struct BytesConsumed(usize) newtype :slight_smile:. As it stands, you’re just assigning a label (i.e. field name) to a single value, which a variable does as well.[quote=“mickvangelderen, post:5, topic:11262”]
Symmetry between the fields in the return types of encode and decode. Fields with the same name convey the same information.
If encode were to return a usize, it could contain either consumed or produced. Reading the documentation is necessary when writing code but more importantly, also when reviewing or maintaining code, depending on surrounding comments and variable naming.
Consistency; when destructuring an EncodeOk, big chance you’ll bind a variable called produced consistently everywhere you call encode.
[/quote]

Similarly, if you newtype wrapped the values that conveys their meaning (bytes produced, bytes consumed), that would achieve your intent better I think. Some would still consider this overengineering.[quote=“mickvangelderen, post:5, topic:11262”]
Better version stability; adding a field later on to EncodeOk is possible while changing the return type from usize to a struct is not.
[/quote]

At the source level, yes. Not at the binary/ABI level though.

There’s more than one way to skin this cat, so as long as you’re aware of the tradeoffs of the approaches, make the decision that you feel is best.