Uncaught Exceptions on ResultAsync

[Chuckytuh]

The new ResultAsync constructs are pretty cool and an improvement over the chain function however, as it is implemented, it opens the door for Uncaught Exceptions to lurk easily!

Take the following example:

const foo = okAsync(1);
try {
    foo.map(() => {
        throw new Error("this is uncaught");
    }).mapErr(() => {
        console.error("this mapErr is ignored");
    });
} catch (error) {
    console.error("this is also ignored");
}

The error ("this is uncaught") thrown from the map is uncaught-able and the reason for it is because an async function is being passed into the Promise.then and no error handling is being made there.

https://github.com/supermacro/neverthrow/blob/master/src/result-async.ts#L29-L34

The same happens for mapErr.

This might be by design or effectively a bug on the implementation.

Would wrapping the transform function f be acceptable?

  map<A>(f: (t: T) => A | Promise<A>): ResultAsync<A, E> {
    return new ResultAsync(
      this._promise.then(async (res: Result<T, E>) => {
        if (res.isErr()) {
          return new Err<A, E>(res.error)
        }
        try {
          return new Ok<A, E>(await f(res.value))
        } catch(error) {
          return new Err<A, E>(error);
        }
      }),
    )
  }

The problems I see with this approach is that the thrown error is not necessarily of type E so we're potentially introducing runtime errors.

[paduc]

Hello @Chuckytuh, thanks for this remark.

You are right that these methods can include uncaught errors, however they are by design.
Neverthrow aims at replacing the general exception mechanism in JavaScript with a typed error system. That means that everywhere there can be an exception, the developper wraps that piece of code and decides which type of error should be used. I don’t think we would want it to be fully automatic.

In the case of map and mapErr, the provided transform methods should remain very simple and not throw. If you want to transform the result value with a function that can throw, then you should either wrap it in a try/catch to always return a value, or isolate that code inside a method that handles the potentiel exception and returns a Result type. For the latter case, you would chain it with andThen instead of map.

Does this make sense to you ? If you have any suggestions to make Result/ResultAsync safer, feel free to share them !

[Chuckytuh]

Yes, it makes absolute sense, and it is known and disclaimed, that neverthrow isn't a solution that eliminates exceptions being thrown!

If you have any suggestions to make Result/ResultAsync safer, feel free to share them !

The only that I can think of, and also the easiest one, would be to have this, or a similar example, on the documentation for ResultAsync so that users be aware that this can happen and your words seem very fitting for documentation, specifically:

In the case of map and mapErr, the provided transform methods should remain very simple and not throw. If you want to transform the result value with a function that can throw, then you should either wrap it in a try/catch to always return a value, or isolate that code inside a method that handles the potentiel exception and returns a Result type. For the latter case, you would chain it with andThen instead of map.

[supermacro]

Although not mentioned explicitly in this project, the idea behind a Result or ResultAsync is that their operations are pure - meaning that the types define the set of things that can occur with those operations.

There shouldn't be any side effects. And yes, throwing exceptions is a side effect.

If I have a ResultAsync<Number, string> then I should expect that this function only has 2 outcomes. There shouldn't be a third outcome that I need to account for (i.e. promise is rejected, or for synchronous Result - that there is a thrown exception).

If you have some exception or failed promise that you did not account for then I personally (in my own work) treat those as critical errors that shouldn't be recovered. I would much rather prefer my program to crash and stop running than to continue running in some weird indeterminate state. Then review logs and see what the error was and then encode that error into your type system so as to handle it smoothly if it ever occurs again.

With all that said, I don't think there's anything to be done here. It's just a matter of making the distinction between expected errors and unexpected / irrecoverable errors.

[supermacro]

Going to close this issue. But feel free to open a PR to improve the documentation!