TartanLlama/expected

Implicit error conversions from E1 to E2 are not prevented on calls to .and_then()

Waffl3x opened this issue · 0 comments

example: https://godbolt.org/z/xnxzGs11x
To be honest, I thought this behavior was intentional until I ran into some nasty edge cases. With modifications I think it would be the ideal mode of operation.

I also want to use this moment to soapbox on how I think this should work and implications for the future.
The link above should sufficiently demonstrate the observed problem, the following is what I learned and my thoughts after experimenting, reading papers, and discussing the matter with others. I would greatly appreciate anyone who takes the time to read any or all of it, however I do not expect it.

In a scenario like the following, I believe that E1 implicitly converting to E2 (as tl::expected currently erroneously behaves) is incorrect, I do not advocate for it. However, I believe that E2 implicitly converting to E1 would make sense, and be of utility. As it is right now, there is no way to immediately map/transform an error E2 to E1 after a call to .and_then(). This would allow gracefully utilizing separate libraries that use expected based error handling just by using a wrapped error type that has error mapping built into it.

expected<T, E1> expected1 = foo();
expected1
    .and_then([](auto&& arg) -> expected<U, E2> {return bar(arg);});

As for the aforementioned edge cases, there are situations where you (I) want to call a function that returns an expected<U, E2> through member function and_then() on an object of type expected<T, E1>, and E2 is completely unrelated to E1. Even in a standard implementation, the solution to this (aside from another that I will note after) is to map the error E1 to E2 and then back to E1 again. However, since E1 is unrelated to E2, the only solution I can think of to facilitate this without potentially losing error information is to map E1 to values outside of the valid values of E2. This is legal for enum and enum class, but obviously very messy, this is also ignoring the possibility of error types that are not simply enum` but this isn't a real solution in my eyes so I won't bother going down that road. Converting back to E1 would then consist of checking the out of range values, and if none match, then doing a regular mapping from the values of E2 to E1. Not very nice.

This case is what motivates me to suggest allowing implicit conversions from E2 to E1, or having overloads of .or_else() that take a callable as the second parameter to immediately perform an error map/transform. I don't believe it is desirable to allow implicit conversions from E1 to E2 because of the confusion it causes in what error type gets propagated. I believe that changing the type of error that gets propagated should remain as a very explicit map_error/transform_error.

For transparencies sake I would like to note that I effectively pitched this idea to jwakely and he (respectfully) disagreed with my stance on the matter.

As I hinted at, there is an easy enough workaround to this (perceived) issue with .and_then(). Instead of calling an invocable directly through and_then(), wrapping it in a higher order function that calls the invocable and transforms the error type immediately makes error propagation easy. I would still prefer a solution that doesn't require this construct, especially as you are required to explicitly state E1 as .and_then() only exposes the value_type, not the error_type to the called invocable. This will cause mild pains when refactoring and a bit of repetition, but it's nothing major, just enough to make me a little sad.

The aforementioned wrapper:

constexpr auto make_transformer(auto&& f, auto&& transform)
{
    return [f = std::forward<decltype(f)>(f), transform = std::forward<decltype(transform)>(transform)](auto&& arg){
        return std::invoke(f, std::forward<decltype(arg)>(arg))
            .map_error(transform);
    };
}

I think that allowing conversions from E2 to E1 after calls to and_then() would facilitate mixing use of generic libraries that use std::expected for error handling. As std::expected becomes more prevalent I believe that there will be an increased interest in this. There is still a strong argument to be made that implicit conversions, even from E2 to E1, is bug prone and unclear. I have to say that I somewhat agree after spending lots of time trying to unravel the issues I've been having with my silly design. Another fair argument is that this functionality just doesn't belong on and_then(), and I don't know if I agree with that (which might just be because of my lack of formal math/functional backround) but I would take it on a separate member function.