CSharpFunctionalExtensions: Changing the Result signature from Result to Result

I want to gather feedback on the change I’ve been pondering lately.

The problem

In most of my projects, I use a custom Error class (with a code and a message) to denote application errors. The problem with this approach is that it requires the use of Result<T, E> or UnitResult<E>, which looks clunky and bloated.

I would like to use the simple Result<T> and Result in such situations. These classes are currently a shortcut for Result<T, string> and Result<Unit, string>, meaning that the error type is string in both of them (Unit means “nothing” or “void”).

The solution

I propose to:

  1. Introduce a new class Error with string Code and string Message fields
  2. Change Result<T> and Result such that they use the Error class as the error type instead of string

This will allow to reduce the Result<T, E> signature to just Result<T> and UnitResult<E> to just Result.

Potential issues

  1. This is a breaking change, though the consequences can be mitigated by introducing additional conversions between the string and Error classes.
  2. I don’t know how many people use custom error types vs simple strings, this change might be disruptive to all of them
  3. I don’t know whether the signature of Error is sufficient for everyone (i.e the two string fields: Code and Message). People might need additional fields and thus won’t be able to use the built-in Error and therefore will have to use the custom Result<T, E> anyway

Personally, I feel that Error should have been introduced in the library from the very beginning. String is not a good type to represent errors anyway.

Please leave your thoughts in the comments.

cc @space-alien @hankovich @linkdotnet @JvSSD

About this issue

  • Original URL
  • State: open
  • Created 3 years ago
  • Reactions: 27
  • Comments: 46 (22 by maintainers)

Most upvoted comments

Personally, I never use Result, so I have nothing to say here.

I use Result<T> quite rarely (as a return type for methods which parse value objects, for example), for me it serves just as a replacement for Result<T, string> (for cases when I do not distinguish between different error types). But it seems to me that for most projects, plain Result<T> is enough (in many places, in the case of web applications, developers just want to return 400 Bad Request + some kind of error message). Again, the entire README focuses on Result<T> only, so it seems to me that changing the error type for Result<T> would break a lot of code.

Most projects don’t need anything other than Result<T>. They are not going to do any compensation logic in case of errors (which they might need a strongly typed error representation for).

Therefore, it seems to me that such a breaking change will not benefit the library. It seems like the ideal solution to your problem would be to implement this proposal (https://github.com/dotnet/csharplang/issues/1239) so that you can write global using MyResult<T> = Result<T, MyError>. Unfortunately, we won’t see such a feature in C# 10.

I would like to add that the proposed error format is unlikely to satisfy everyone. If, nevertheless, there is a desire to make this breaking change, then I could suggest such a format of the Error type:

public class Error // not sealed to allow consumers to create their own Errors
{
    public string Message { get; }

    public IDictionary <string, object> Metadata { get; } // Possible names: [Properties, Extensions].
    // It's mutable to allow different handlers to add their own properties during processing / error handling

    // I don't really thing Code should be here, it doent's sound like it's generic enought

    public Error? InnerError { get; } // Can be handy, but I'm not 100% it's required
}

By the way, I realized that we can take a look at the ProblemDetails structure (https://datatracker.ietf.org/doc/html/rfc7807), it was also designed as an error format suitable for everyone (however, we need to exclude everything that is specific to HTTP).

Personally, I now use Result<T, TError> 99% of the time, where TError is a very tricky self-written discriminated union that covers all possible outcomes (enum is not suitable, since you cannot add additional state to enum, and for some errors it is necessary to pass additional information). And TError is different for each operation.

P.S. If I were asked to design a library from scratch, then I would probably implement Result<T> as Result<T, Error>, and not as Result<T, string>, so my single concern is compatibility.

Exceptions are not errors. Exceptions are not expected (like OutOfMemoryException), while errors are expected and documented (like validation errors). I lot of exception properties only make sense after exception was thrown. What’s the benefit of retuning exceptions? I think it’s even more counter-intuitive than returning results

Including an Http status in the error violates the Separation of Concerns principle (conflating application concerns with domain concerns), but that might be a needed concession still, this is a common scenario in a lot of projects.

Why are we trying to reinvent the wheel by implementing a new Error class when Exception is the standard way to represent errors?

public class Error : Exception { }

It would have the added benefit of allowing projects to gradually move from the traditional throw statements and flows to an FP error handling approach.

public Result<T> Foo<T>() 
{
    try 
    {
        T v = Bar<T>(); // some code that can throw
        return Result.Success<T>(v);
    } 
    catch (Exception e) 
    {
        return Result.Failue<T>(e);
    }
}

I have a question for everyone. For context, here’s the current plan for the Error class:

public class Error {

  public string Code { get; }
  public string MessageTemplate { get; }
  public object[] MessageArguments { get; }
  public string Message => string.Format(MessageTemplate, MessageArguments);
}

The question is: should Result contain just one Error or a collection or errors? Would appreciate if everyone chimes in @hankovich @seangwright @leksts @lonix1 @linkdotnet @SamuelViesselman @vbreuss

My Error type has just one message, and I have an AggregateError : Error whose message is “Aggregate of {n} errors”, and an additional property IEnumerable<string> Messages.

public class AggregateError : Error
{
  public IEnumerable<string> Messages { get; }
  private AggregateError(IEnumerable<string> messages)
    : base($"Aggregate of {messages.Count()} errors.")
  {
    Messages = messages;
  }
  
  public static Result From(IEnumerable<Error> errors) => errors.Count() switch
  {
    0 => Result.Success,
    1 => errors.Single(),
    _ => new AggregateError(errors.Select(e => e.Message))
  };
}

It’s a shame C# won’t let me implicitly convert an IEnumerable<Error> to Result with this implementation.

Thanks @LetMeSleepAlready appreciate your input.

My main issue with the error strings is differentiating hard or soft errors. “record not found” is a non-retryable error. “backend service down” however can be retried at a later time. In http status codes these would translate to either a 400 or a 500 error. Again, I don’t care about actual http mapping (application domain <> http protocol), but hard/soft error differentiation is good to have for retying.

I’d say this should be the responsibility of the application layer to map errors from the “enum” onto 400/500/404/401/etc. I wouldn’t add this directly to Error, at least not in v1.

I’m calling it an “enum” because you can define all your errors in one place, just like with an enum, and use it in a similar way, for example: https://github.com/vkhorikov/ValidationInDDD/blob/master/src/DomainModel/Error.cs#L44

Personally I am very much in favor of an Error object. This is also the way golang handles error. The difference would be that golang returns a tuple.

Maybe it would be worth to return Result<T, TError> where TError : Error and only provide a basic Error - Implementation. Then the developer can either take the Error class from the library or does its own version.

@zbarrier

Create a different repo for the solution with the Error class. It will make it easier for others to fork and modify the Error class to be what they need and prevent breaking changes in the current library.

A different repo would bring a whole new set of problems, I’d rather avoid this. The breaking changes shouldn’t be that bad, I just haven’t had time to work on this (and a couple other) feature yet.

@nabenzine I plan to work closely on this library this summer.

@vbreuss The issue with this approach is that you’d need to manually cast the library’s error to your own:

MyError error = (MyError)result.Error;

because the library doesn’t know about the MyError subclass. This is quite annoying and I’d rather avoid it. The Error class should have a structure that fits 99% of projects.

@vkhorikov : I doubt you can foresee all possible use cases in any implementation. I think it would be better to allow users to enhance the implementation to match their use cases and keep the base implementation as simple as possible. Something along the way

    public class Error
    {
        public string Message { get; }

        private Error(string message)
        {
            Message = message;
        }

        public static implicit operator Error(string message)
        {
            return new Error(message);
        }
    }

By including an implicit operator from string, you would simplify migration for existing projects.


Users of the library can then extend the Error class as they need them, e.g. in an Web API environment

    public class HttpError: Error
    {
        public int StatusCode { get; }
        public HttpError(string message, int statusCode) : base(message)
        {
            StatusCode = statusCode;
        }
    }
    public class ClientError : Error
    {
        public ClientError(string message) : base(400, message)
        {
        }
    }
    public class ServerError: Error
    {
        public ClientError(string message) : base(500, message)
        {
        }
    }

By improving the extension methods to allow filtering for specific error classes, e.g.

    public static Result OnFailure<TError>(this Result result, Action<TError> action)
    {
        if (result.IsFailure && result.Failure is TError error)
        {
            action(error);
        }
        return result;
    }

users can then react to different error cases.

Or if users need to track a list of errors, they could implement a CombinedError:

    public class CombinedError : Error
    {
        public Error[] InnerErrors { get; }

        public CombinedError(IEnumerable<Error> errors)
        : this(";", errors.ToArray())
        {
        }

        public CombinedError(string separator, IEnumerable<Error> errors)
        : this(separator, errors.ToArray())
        {
        }

        public CombinedError(params Error[] errors)
        : this(";", errors)
        {
        }

        public CombinedError(string separator, params Error[] errors)
        {
            InnerErrors = errors;
            Message = string.Join(separator, InnerErrors.Select(x => x.Message));
        }
    }

I have a question for everyone. For context, here’s the current plan for the Error class:

public class Error {

  public string Code { get; }
  public string MessageTemplate { get; }
  public object[] MessageArguments { get; }
  public string Message => string.Format(MessageTemplate, MessageArguments);
}

The question is: should Result contain just one Error or a collection or errors? Would appreciate if everyone chimes in @hankovich @seangwright @leksts @lonix1 @linkdotnet @SamuelViesselman @vbreuss

@vkhorikov The option to have a collection of errors return with Result would be useful. A project I am working on consumes a number of external API’s which often return one or more validation errors. Currently we are having to use Result<T, E> but as you mentioned it seems like a rather clunky way to handle multiple errors.

I prefer the error class. I like the idea of message templates with parameters because it plays better with logging frameworks. I am also thinking if the validation error object from data annotations nugget can be used so that the.net framework knows how to deal with it.

Because then it doesn’t matter what the default is, everyone can create their own default implementation by inheriting from Result<T, E>.

No, that wouldn’t work because the existing extensions will still return library’s Result, so you’d have to manually cast those to your custom implementation.

Maybe it would be worth to return Result<T, TError> where TError : Error and only provide a basic Error - Implementation. Then the developer can either take the Error class from the library or does its own version.

The main benefit of having a built-in Error is reducing the signature size (so essentially a syntax sugar). Result<T, TError> wouldn’t add any benefits because I would still need to specify the error type (which I already can do now). The idea is to come up with an Error class that would meet most peoples needs, so that we don’t have to use Result<T,E> most of the time.

I generally use Enums instead of strings for Error codes, so it wouldn’t help me out immediately. If the proposed Error type was introduced into the library, I would strongly consider switching over to using strings for my error codes for the sake of convivence.

Yeah, enums is a better solution from a maintenance perspective (they are more strongly typed than strings), but they aren’t extensible. We can’t hard code all possible error types in the library and allowing users to do so would require specifying the type of the enum in the Result signature, which defeats the purpose.

There’s probably a middle-ground option here: you could use string constants instead of enums. Another option is to predefine the list of possible Errors and use them instead of enums (similar to how enumerations currently work in the library, for example: https://github.com/vkhorikov/ValidationInDDD/blob/master/src/DomainModel/Error.cs#L46-L62 ).