Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically catch and wrap exceptions thrown in @effect.result functions? #5

Closed
mlw214 opened this issue Nov 30, 2020 · 8 comments
Closed

Comments

@mlw214
Copy link
Contributor

mlw214 commented Nov 30, 2020

Currently, it looks like any non-Expression exceptions thrown in a effect.result decorated function is re-raised by the decorator. I'm wondering if it might be useful to have the decorator return an Error object instead, wrapping the raised Exception object?

This would reduce the amount of manual conversions from raised exceptions to Error objects, enabling easier interfacing with code/libraries that throw exceptions.

Thoughts? Happy to take a stab at implementing this. Either way, I'm digging this library!

@mlw214 mlw214 changed the title Automatically catch exceptions and wrap thrown in @effect.result functions? Automatically catch and wrap exceptions thrown in @effect.result functions? Nov 30, 2020
@dbrattli
Copy link
Owner

dbrattli commented Nov 30, 2020

Hi, this is currently by design. A Result effect is an exception free world so any called function within the effect needs to catch any exceptions and turn them into appropriate Error values. This makes the effect pure in the sense that everything is simply functions, arguments and return values.

It also aligns with FSharpx.Extras:

open FSharpx.Result

[<EntryPoint>]
let main argv =

    let xs = result {
        failwith "this happened!"
        return 42
    }

    match xs with
    | Ok value -> printfn "Got value %A" value
    | Error ex -> printfn "Got error: %A" ex

    0 // return an integer exit code

Running this program gives:

➜  results dotnet run
Unhandled exception. System.Exception: this happened!
   at Program.main(String[] argv) in /Users/dbrattli/Developer/results/Program.fs:line 10

This works the same as:

def test_result_effect_throws():
    error = CustomException("this happend!")

    @effect.result
    def fn() -> Generator[int, int, int]:
        _ = yield from Ok(42)
        raise error

    with pytest.raises(CustomException) as exc:
        fn()

    assert exc.value == error

Perhaps it's better to have another decorator for exception throwing functions that will turn the exception into an Error object instead. So instead of:

def parse(string):
    try:
        value = float(string)
        return Ok(value)
    except Exception as exn:
        return Error(exn)

We can write something like:

@result.raises(ValueError)
def parse(string):
    value = float(string)
    return Ok(value)

What do you think? It should perhaps return just value instead of Ok(value).

@mlw214
Copy link
Contributor Author

mlw214 commented Dec 1, 2020

Ah, this makes sense. I think the decorator route is the way to go based on your explanation.

What do you think? It should perhaps return just value instead of Ok(value).

Are there any downsides by returning the value instead of Ok(value) in the context of an effect.result decorated function? I'm still very new to this library, so I don't know all the ins and outs. However, since it can return an Error object, perhaps it makes more sense to have it return Ok(value) just to be consistent?

Since this is something I want for a project I'm currently working on, I'll go ahead and take a shot at implementing the decorator approach. I can then extract it and submit a PR if you think it's worth adding. Do you have any specific thoughts on where this decorator should live (e.g., a specific module)?

@dbrattli
Copy link
Owner

dbrattli commented Dec 1, 2020

I think it's ok to return the value directly and have a decorator that transforms values and exceptions to Ok and Error values. I just saw that return Ok was not necessary after writing and then added the comment.

It's safe to add it to the expession.extra.result folder, e.g misc.py, utils.py, ... Then we can move it into core later if we feel it works really well with the library. Export it in __all__ and into __init__.py of the extra module.

PS: Look at curry how it uses the functool.wraps to preserve the name and the docstring of the decorated function.

@mlw214
Copy link
Contributor Author

mlw214 commented Dec 23, 2020

@dbrattli I apologize for my slowness here. I've gotten around to this and took a stab at implementing a decorator with correct typing support. While the decorator logic is simple to implement, it unfortunately looks like getting full typing support can't quite be done (yet) - I detail the problems below. Note that I could totally be wrong, so I'd love your input. I'm currently calling the decorator catch, but I'm 100% open to renaming it as I don't think it fully captures what it does. Here's the code:

from functools import wraps
from typing import Callable, Type, TypeVar, cast, overload

from expression.core import Error, Ok, Result

E = TypeVar("E", bound=Exception)
T = TypeVar("T")


@overload
def catch(
    *, exception: Type[E]
) -> Callable[[Callable[..., T]], Callable[..., Result[T, E]]]:
    ...


@overload
def catch(f: Callable[..., T]) -> Callable[..., Result[T, Exception]]:
    ...


def catch(f: Callable[..., T] = None, *, exception=Exception):
    def decorator(fn: Callable[..., T]) -> Callable[..., Result[T, E]]:
        @wraps(fn)
        def wrapper(*args, **kwargs) -> Result[T, E]:
            try:
                out = fn(*args, **kwargs)
            except Exception as e:
                return Error(cast(E, e))
            else:
                return Ok(out)

        return wrapper

    if f is not None:
        return decorator(f)
    else:
        return decorator


@catch(exception=ZeroDivisionError)
def halved(a: float) -> float:
    return a / 2 if a > 0 else a / 0


x = halved(2)
reveal_type(halved)  # Revealed type is 'def (*Any, **Any) -> expression.core.result.Result[builtins.float*, builtins.ZeroDivisionError]'
reveal_type(x)  # Revealed type is 'expression.core.result.Result[builtins.float*, builtins.ZeroDivisionError]'

I see two problems on the typing front. The biggest problem is that I don't believe there's a way to preserve the type information for the arguments with mypy. Pyre-check actually does have support for this (see here). Note that this problem is directly called out in python/mypy#3157. The second problem is being able to handle functions that can throw multiple types of exceptions. Currently, they'd have to all be captured with a parent class (e.g., Exception). I believe this could be theoretically solved with something like Variadic Generics, though they aren't support currently (see python/typing#193).

I'd love to hear your thoughts/feedback on this before I make a PR.

@dbrattli
Copy link
Owner

This looks really nice and I also like the name catch. I've kind of given up on mypy so Expression only uses pyright for type checking in CI (pylance for vscode) and with pyright the return type will be inferred correctly. The code looks great but is missing an isinstance check for the error in case some other exception was thrown. Here is my version:

from functools import wraps
from typing import Any, Callable, Optional, Type, TypeVar, Union, cast, overload

from expression.core import Error, Ok, Result

TSource = TypeVar("TSource")
TError = TypeVar("TError", bound=Exception)


@overload
def catch(exception: Type[TError]) -> Callable[[Callable[..., TSource]], Callable[..., Result[TSource, TError]]]:
    ...


@overload
def catch(f: Callable[..., TSource], *, exception: Type[TError]) -> Callable[..., Result[TSource, TError]]:
    ...


def catch(
    f: Optional[Callable[..., TSource]] = None, *, exception: Type[TError]
) -> Callable[[Callable[..., TSource]], Union[Callable[..., Result[TSource, TError]], Result[TSource, TError]]]:
    def decorator(fn: Callable[..., TSource]) -> Callable[..., Result[TSource, TError]]:
        @wraps(fn)
        def wrapper(*args: Any, **kwargs: Any) -> Result[TSource, TError]:
            try:
                out = fn(*args, **kwargs)
            except Exception as exn:
                if isinstance(exn, exception):
                    return Error(cast("TError", exn))
                raise
            else:
                ret: Result[TSource, TError] = Ok(out)
                return ret

        return wrapper

    if f is not None:
        return decorator(f)

    return decorator


@catch(exception=ZeroDivisionError)
def halved(a: float) -> float:
    return a / 2 if a > 0 else a / 0

x = halved(2)
reveal_type(halved)
reveal_type(x)

@dbrattli
Copy link
Owner

A way to allow multiple exception types could be handled if we allowed the decorators to be chained e.g:

from functools import wraps
from typing import Any, Callable, Optional, Type, TypeVar, Union, cast, overload

from expression.core import Error, Ok, Result

TSource = TypeVar("TSource")
TError = TypeVar("TError", bound=Exception)
TError_ = TypeVar("TError_", bound=Exception)


@overload
def catch(
    exception: Type[TError_],
) -> Callable[
    [Callable[..., Union[TSource, Result[TSource, TError]]]], Callable[..., Result[TSource, Union[TError, TError_]]]
]:
    ...


@overload
def catch(f: Callable[..., TSource], *, exception: Type[TError]) -> Callable[..., Result[TSource, TError]]:
    ...


def catch(
    f: Optional[Callable[..., TSource]] = None, *, exception: Type[TError]
) -> Callable[[Callable[..., TSource]], Union[Callable[..., Result[TSource, TError]], Result[TSource, TError]]]:
    def decorator(fn: Callable[..., TSource]) -> Callable[..., Result[TSource, TError]]:
        @wraps(fn)
        def wrapper(*args: Any, **kwargs: Any) -> Result[TSource, TError]:
            try:
                out = fn(*args, **kwargs)
            except Exception as exn:
                if isinstance(exn, exception):
                    return Error(cast("TError", exn))
                raise
            else:
                if isinstance(out, Result):
                    return cast(Result[TSource, TError], out)

                return Ok(out)

        return wrapper

    if f is not None:
        return decorator(f)

    return decorator


@catch(exception=ArithmeticError)
@catch(exception=ZeroDivisionError)
def halved(a: float) -> float:
    return a / 2 if a > 0 else a / 0


reveal_type(halved)
for x in halved(0):
    reveal_type(x)
    print(x)

@mlw214
Copy link
Contributor Author

mlw214 commented Dec 30, 2020

This is great! Thanks for your corrections/input.

One thing I noticed is that the decorator doesn't appear to work well with the @curried decorator. Personally, I'd like to get them to play nicely together, if possible. I'll play around with @catch to see if I can make that happen. Regardless, I hope to have a PR for you this weekend.

@mrahtz
Copy link

mrahtz commented Feb 20, 2021

@mlw214 I noticed you referenced python/typing#193 on variadic generics in this thread. Heads up that we've been working on a draft of a PEP for this in PEP 646. If this is something you still care about, take a read and let us know any feedback in this thread in typing-sig. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants