Refactor Result implementation into concrete class with conversion operators

[aggieben]

I was just working on a repository with a method like this:

IResult<IThing,ThingMappingError> ToThing(dynamic row)
{
    var thingType = row.Type as string;
    switch (filterType)
    {
        case "ThingAmaBob":
            return Result.Ok<IThing, ThingMappingError>(new ThingAmaBob { AProperty = (int)row.Property; });
        case "ThingAmaDoodle":
            return Result.Ok<IThing,ThingMappingError>(new ThingAmaDoodle { AProperty = (int)row.Property; });
        default:
            return Result.Err<IThing,ThingMappingError>(new ThingMappingError { Exception = new Exception("Unsupported thing type"); });
}

This is....really cumbersome. I'd like to see something that allows me to do this:

IThing ToThing(dynamic row)
{
    var thingType = row.Type as string;
    switch (filterType)
    {
        case "ThingAmaBob":
            return new ThingAmaBob { AProperty = (int)row.Property; };
        case "ThingAmaDoodle":
            return new ThingAmaDoodle { AProperty = (int)row.Property; };
        default:
            throw new Exception("Unsupported type of thing");
    }
}

AnotherMethod()
{
    var things = db.Query(sql).Select(Result.Wrap(ToThing));
}

The magic being Result.Wrap, where it should return some sort of Func<dynamic, IResult<IThing, Exception>>. To start with, just using Exception as the error type would make this simple, and maybe we can explore more sophisticated type handling later on.

[chriskrycho]

🤔 So my take here is: a Wrap or similar is a good idea there. (Of might be a good name for it!) However, from a philosophical point of view, I think that's best applied to other code which may throw, rather than your own, where having it at the type level is… the whole point, as far as I see it!

Is the main pain point here that you can't just do return Result.Ok(new ThingAmaBob({ a Property = (int)row.Property; }); and have to type annotate it every time (and even more with the wrapped exception type)? Or is it having to spell out Result.Ok itself every time as well? 🤔

It's definitely verbose, in any case. (And as far as I'm concerned here, this is a place you and @chrisriesgo are better equipped to design the solution; I'm just an interested participant.)

[aggieben]

@chriskrycho Yeah, I think the verbosity is my main complaint. I can live with Result.Ok(…), but honestly, an implicit conversion there would be nice, so the ideal world would be do just do this:

Result<SuccessType, ErrorType> AMethod()
{
    return new SuccessType();
}

(note the Result as opposed to IResult)

Where Result could have the implicit conversion baked in somehow. I don't know offhand if the typesystem is flexible enough to make this work. The sticking point is ErrorType - it's not involved in any expression here at all, and yet it prevents us from doing something that would be much nicer semantically.

[chriskrycho]

So in the meantime @bmakuh and I have started the work of landing the same basic idea as a Wrap or similar function in the TS True Myth, as tryOr and tryOrElse functions. I definitely think something like that is 100% worth having.

The specific semantics around e.g. your example seem like something you'd need language-level support for, though: that's basically the kind of magic that happens with async functions in C♯, IIRC, but I have no idea if there's a way to do it with arbitrary types and returns (I would suspect not).

I think the best you'll get is TryOr and TryOrElse.

Result<Success, String> AMethod() {
    return Result.TryOrElse(e => e.ToString(), new SuccessType());
}

Some verbosity is, frankly, to be expected. This is C♯. :trollface: (I feel your pain, in all seriousness; TS can be slightly less verbose… but not a lot, and I feel the pain there as well.)

[aggieben]

@chriskrycho playing around in LinqPad last night and came up with the following, which is obviously not a full exposition of how this might work, but it builds. The semantics would be like this:

Success or failure condition would be signaled simply by returning TSuccess or TError as appropriate, and the implicit conversion operators take care of the rest. The equivalent idiomatic way to "unsafely unwrap" in C♯ is to use the explicit cast operator, like this: (TSuccess)result or (TError)result. The normal semantics of this are that it will throw an exception if the conversion is invalid. There are obviously things we can do here in terms of implementing the Select, or TryOrElse or Map methods as correspondents to the ones in TypeScript True Myth, but I wanted to think through the fundamental semantics before jumping in with both feet.

public Result<bool, string> FunctionThatSucceeds()
{
    return false;
}

public Result<bool, string> FunctionThatFails()
{
    return "error message";
}

// Define other methods and classes here
public sealed class Result<TSuccess, TError>
{
    private readonly TSuccess _value;
    private readonly TError _error;
    private readonly bool _isOk;

    private Result() { }

    private Result(TSuccess success) 
    {
        _value = success;
        _error = default(TError);
        _isOk = true;
    }

    private Result(TError error)
    {
        _value = default(TSuccess);
        _error = error;
        _isOk = false;
    }

    private Result(TSuccess success, TError error, bool isOk)
    {
        _value = success;
        _error = error;
        _isOk = isOk;
    }

    public Result<TSuccess, TError> Ok(TSuccess success)
    {
        return new Result<TSuccess, TError>(success, default(TError), true);
    }

    public Result<TSuccess, TError> Err(TError err)
    {
        return new Result<TSuccess, TError>(default(TSuccess), err, false);
    }

        // necessary because of possible ambiguity in explicit converstions when TError = TSuccess
        public TSuccess UnsafelyUnwrap() => _isOk ? _value : throw new Exception("you suck");

        public TError UnsafelyUnwrapErr() => !_isOk ? _error : throw new Exception("you suck");

    public static explicit operator TSuccess(Result<TSuccess, TError> result)
    {
        if (result._isOk)
        {
            return result._value;
        }

        throw new Exception("invalid conversion; error result cannot be used as success");
    }

    public static explicit operator TError(Result<TSuccess, TError> result)
    {
        if (!result._isOk)
        {
            return result._error;
        }

        throw new Exception("invalid conversion; success result cannot be used as error");
    }

    public static implicit operator Result<TSuccess, TError>(TSuccess success)
    {
        return new Result<TSuccess, TError>(success);
    }

    public static implicit operator Result<TSuccess, TError>(TError error)
    {
        return new Result<TSuccess, TError>(error);
    }

    public bool IsOk() => _isOk;
    public bool IsError() => !_isOk;
}

Thoughts? @chrisriesgo thoughts?

[chriskrycho]

This seems like a really sensible way of handling it natively to the language. You can then layer on the encouraged “treat it as a value” tooling and make that the happy path while making people’s normal C♯ habits do the right thing. That’s the same philosophy I applied to the TS version: you can do unsafeGet on something without checking, and it’ll throw an exception (and likewise if you construct a Just with a null and so one). Different language so different semantics, same approach. 💯 from me.

[chriskrycho]

Resolved by #18!