Typing issue with chained Result

[stereobutter]

Take the following two functions as an example of two (or more) functions with unique failure modes (each function may fail in their own way)

def fraction(x: float ) -> Result[float, ZeroDivisionError]:
    try: return Success(1/x)
    except ZeroDivisionError as e: return Failure(e)

def log_of_arg_minus_one(x: float) -> Result[float, ValueError]:
    try: return Success(log(1-x))
    except ValueError as e: return Failure(e)

then a computation like fraction(x).bind(log_of_arg_minus_one) has potentially three different outcomes:

1. the happy path where both functions return a Success, i.e. fraction(x) returns Success(y) where y is a float and log_of_arg_minus_one(y) returns Success(z) where z is also a float; example fraction(2).bind(log_of_arg_minus_one)
2. the failure path of fraction , i.e. Failure(ZeroDivisionError); example: fraction(0).bind(log_of_arg_minus_one)
3. the failure path of log_of_arg_minus_one, i.e. Failure(ValueError); example: fraction(1).bind(log_of_arg_minus_one)

Sadly this seems not to type correctly:

FailureCases = Union[ZeroDivisionError, ValueError]

z: Result[float, FailureCases] = fraction(1).bind(log_of_arg_minus_one)

# Argument 1 to "bind" of "Result" has incompatible type 
# "Callable[[float], Result[float, ValueError]]"; 
# expected "Callable[[float], Result[float, Union[ZeroDivisionError, ValueError]]]"

The following also doesn't work:

z: Result[float, Exception] = fraction(1).bind(log_of_arg_minus_one)

# Argument 1 to "bind" of "Result" has incompatible type 
# "Callable[[float], Result[float, ValueError]]"; 
# expected "Callable[[float], Result[float, Exception]]"

I don't know it this is an issue with bind (that might be fixable with a plugin) or just that Result[S, F] is not properly covariant with respect to F i.e. Result[X, A] is considered a subtype of Result[X, B] when A is a subtype of B

[sobolevn]

Thanks for reporting. I will look into this.

[stereobutter]

I think I found the source of this issue and a fix as well. Currently the return type for bind (in the definition of Result in result.py) is

Result[_NewValueType ,_NewErrorType]

it should be

Result[_NewValueType, Union[_ErrorType ,_NewErrorType]]

to account for the accumulation of the various failure cases.

I tried this out on my machine for the examples from above and it seems to work just as intended; excuse me for being too lazy to author a proper pull request ;)

[sobolevn]

Let's ask @astynax to review this solution. @astynax , what do you think?

[astynax]

@sobolevn, if you want to keep types sane and sound,bind shouldn't change the type of error (so even Result[_NewValueType ,_NewErrorType] is a bad signature!). But we can provide a new method, that will compose error types into the one Union.

[sobolevn]

Ok, so the plan is:

• .bind() won't change the type signature of an error
• .unify() will return Union[_ErrorType, _NewErrorType]

@astynax what about the value type?

Thanks for your suggestions, guys! New release will be available before the 20th of August.

[stereobutter]

I agree with @astynax that bind ideally shouldn't change the return type, however the accumulation of Error types is imho both very convenient (for dealing with the result of a computation that has gone wrong part way), less surprising and consistent with the runtime behavior. If you really want bind to strictly do the right thing, it should just raise an exception at runtime and issue a mypyTypeError when chaining functions with incompatible types (instead of the current behavior).

[stereobutter]

Another issue I just found is that the type of _Success is Result[_ValueType, Any] which means that Any will be accumulated if bind adopts the proposed behavior; imho the type of _Success should be Result[_ValueType, NoType] where NoType doesn't match anything. Example:

Success(0).bind(fraction).bind(log_of_arg_minus_one)
# reveal_type is 'Result[builtins.float*, Union[Any, builtins.ZeroDivisionError, builtins.ValueError*]]'

[sobolevn]

@SaschaSchlemmer it can be NoReturn, but I have not tried it yet.

[stereobutter]

Excuse me if this is bikeshedding, but I don't think it should be NoReturn either, since NoReturn is to indicate a function that always raises an Exception (which _Success doesn't do). What about

class NoType: pass

[sobolevn]

Seems legit. Thanks!

[stereobutter]

Another general issue with the typing of Result that I originally mentioned in this issue (but that turned out be unrelated) is covariance. Consider:

z: Result[float, Exception] = Success(1).bind(fraction).bind(log_of_arg_minus_one)

As it currently is (regardless of the original or the proposed behavior) this doesn't typecheck. I think it should?

[sobolevn]

Yes, I think so too. I will check either mypy is able to do that.

[stereobutter]

as far as I am aware mypy handles this just fine (when you set the TypeVars with covariant=True), however you will get warnings that you cannot use covariant type variables as parameters when you use them in methods/functions other than __new__ and __init__

[stereobutter]

@sobolevn scratch the NoType comment for _Success (and _Failure respectively). This would break something like

z: Result[float, Union[ZeroDevisionError, ValueError] = Success(1).bind(fraction).bind(log_of_arg_minus_one)

because the resulting Union[NoType, ZeroDevisionError, ValueError] would (given that Result treats its TypeVars covariantly) not typecheck against the provied Union[ZeroDevisionError, ValueError]. What one really would need (and what mypy doesn't seem to provide is an empty Union so that Union[EmptyUnion, A, B] is equal Union[A,B]).

Something curious happens with Any however, observe the the reveal_types of

res: Result[float, Union[ZeroDivisionError, ValueError]] = Success(0).bind(fraction).bind(log_of_arg_minus_one)

reveal_type(res)
# Revealed type is 
# 'returns.result.Result[builtins.float, Union[builtins.ZeroDivisionError, builtins.ValueError]]'

vs

reveal_type(Success(0).bind(fraction).bind(log_of_arg_minus_one))
# Revealed type is 
# 'returns.result.Result[builtins.float*, Union[Any, builtins.ZeroDivisionError, builtins.ValueError*]]'

[stereobutter]

According to the powers that be NoReturn (maybe with an alias that makes more sense in this situation) would be the correct type for the missing type in _Success and _Failure respectively.

I am unfortunately still struggling a bit with setting up the git repo on my end but what I found out is:

• NoReturn indeed works as return type and gets flattened by Union (compared to Any which hangs around and might cause issues down the road)
• getting Result to type check covariantly also works; this however causes mypy errors inside because mypy dislikes covariant types as function parameters (except in __init__ and __new__) such as in the definitions of Success and Failure (why are they needed anyway instead of just exposing the classes themselves?)
• when using _Success and _Failure directly instead of Success and Failure the revealed type of something like reveal_type(_Success(1)) is '_Success[builtins.int*]' which is imho fairly nice visually (compared to say Result[builtins.int*, <nothing>] where the <nothing> is actually the string representation of NoReturn or the previous Result[builtins.int*, Any]). Again, why have the Aliases Success and Failure instead of just exposing (and renaming) the classes themselves?
• changing bind to work with chained calls is possible as suggested above and basically necessitates Result to behave covariantly with respect to its TypeVars to be useful.
• these changes didn't cause any tests to fail for me

[sobolevn]

@SaschaSchlemmer awesome work! Thanks a lot for your time and effort. What problems with git setup do you have? Feel free to ask for help!

[stereobutter]

I've actually joined the gitter chat, maybe that is a more suitable channel for my issues around the setup?

[sobolevn]

@SaschaSchlemmer sorry, I don't use gitter. You can create a new github issue with your specific problem. And I will update the CONTRIBUTING.md docs based on the solution.

[stereobutter]

Sry for being silent the whole time; I was quite busy. I see from your commit that

Now bind does not change the type of an error

So that means that the use case of chained bind calls is still unresolved?

[sobolevn]

.unify is not implemented for now. Currently I create Union[ErrorOne, ErrorTwo] before.

I will consider adding .unify in the next release.

[stereobutter]

I also had an interesting discussion on the typing/gitter channel about a dual function to unify (like what rescue is to bind) that can be used to selectively handle error cases. Having both unify and it's dual function (although not possible to express using just the type system; could maybe be done with a plugin?) would be really powerful.

[sobolevn]

@SaschaSchlemmer thanks a lot for sharing this! 👍 Are you interested in giving this a try?

[stereobutter]

You mean the an implementation for fix that I mentioned in the gitter channel? I think the hard part is the plugin for getting this to typecheck.

[stereobutter]

@sobolevn I think a better name for the type aggregating bind might be chain (instead of unify)

[stereobutter]

I made some process with the plugin implementation for the proposed fix function. Apart from one edge case I think I got everything figured out. I have to try and setup the repo again before though. Last time running the tests gave me trouble. I remember I had to out-comment something in the config file for things to work at my end.