[Issue #149] add generator-based do notation

[JLessinger]

Proposal for implementaiton using generators to simulate <- notation as in Rust (or Haskell).

[codecov-commenter]

Codecov Report

Merging #150 (a415cad) into master (35d6a5c) will increase coverage by 0.04%.
The diff coverage is 97.78%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   97.37%   97.41%   +0.04%     
==========================================
  Files           3        4       +1     
  Lines         419      464      +45     
==========================================
+ Hits          408      452      +44     
- Misses         11       12       +1     

Files	Coverage Δ
src/result/__init__.py	100.00% <ø> (ø)
tests/test_result_do.py	100.00% <100.00%> (ø)
src/result/result.py	94.68% <93.75%> (-0.09%)	⬇️

[JLessinger]

Hi @francium (+ @wbolster), I got this to pass linting, type checking and tests in 3.8-3.11.

@do is only supported in 3.10+, and this is reported to users as an exception if they're on < 3.10.

It is well-tested in 3.10-3.12. Further, it also passes strict type checking with pyright, so I'm pretty confident in the correctness.

This is a strange but convenient and well-behaved syntax for simulating do.

Added a dev section to README and updated .gitignore for good measure.

My organization is starting to use the library and this feature would make my life much more convenient. It's a great selling point for the library for FP Python users.

Please take a look, thanks.

[francium]

Thanks, I'll try to allocate some time later today to take a look through this

[francium]

We should add an easy to understand example in the README.

So something like this would be a canonical example of how to use this?

@do
def _result_sum(is_success1: bool, is_success2: bool) -> DoResult[int, str]:
    x1 = yield _get_result(is_success1, 1)
    x2 = yield _get_result(is_success2, 2)
    return Ok(x1 + x2)

Obviously, this is quite nice. One thing to think about is this forces the user to create a function for this. So it's a bit heavy weight vs an expression which you can inline anywhere -- the idea behind #112

Although maybe it would be good to have both?

[JLessinger]

@francium , sure I can add an example like that. It follows the look of the Rust or Haskell equivalent fairly closely, with <- notated here as = yield, so we can borrow any of those examples.

I didn’t think about inlined expressions, and it might be possibly with anonymous generators - I don’t know. I do think this syntax is more intuitive than ‘for…’ expressions following the final value. I also think it’s better to pick one syntax even if it means not supporting inline do.

For me, one premise of do notation is that it is convenient exactly for more complex expressions, which would usually justify a named function. Otherwise you can just manually use ‘and_then()’ if it’s just one or two calls.

I’ll look into anonymous generators a bit.

[francium]

@JLessinger anonymous generators is the basis of #112's implementation I believe.

Currently, I'm inclined towards providing both ways of doing it, a decorator for functions so you use do yield, as well as a generator based expression which you can freely embed in existing code paths. Based on my experience, it's a matter of taste whether a codebase has a lot of smaller dedicated functions or longer functions with a lot of procedural code in them.

Since the two implementations, decorator and expression, wouldn't really interfere with each other, right now I can't think of a good reason for us to omit one of them and just provide the other.

Naming would be interesting of course, we can't name both do.

[JLessinger]

@francium , ok, why don’t I rename -> named_do and we ship this. It won’t interfere with 112, so someone can push that through on top of this.

[francium]

hmm, yeah, I'm fine with that. But let's think of a better name, named_do seems like a poor choice.
Most obvious is as_do, but that's not great either.

[francium]

Can you please also look into how async functions will fit into this decorator based approach. And add test cases/readme examples for those. Thanks

[francium]

Maybe rename it to do_result for now? Naming is hard

[JLessinger]

Re: naming, how about do for this and do_block for #112?

[francium]

Let's just go with do for this. We'll figure out #112 after.

[JLessinger]

I just realized this has a big limitation: you can only contain one type. This will have a type check error:

    @do
    def f() -> DoResult[int, str]:
        x = yield Ok("hello")
        y = yield Ok(2)
        return Ok(len(x) + y)

because of the generator type, T has to evaluate to both str and int. I can basically suppress this by allowing the generator to accept and yield Any, but then you lose a lot of the type safety. I don't know how 112 addresses this kind of problem, but we should probably hold off.

[francium]

#112 is based off of what returns does I think. You can look at their implementation if it's helpful as well - https://returns.readthedocs.io/en/latest/pages/do-notation.html

[JLessinger]

@francium , I seem to have been able to get it to work with the for ... in ... syntax. This has the advantage that do expressions are anonymous, so you don't need a decorated named function.

Mypy is happy and it seems to play nicely with pyright in strict mode, which is much stricter than default mypy.

[francium]

Thanks @JLessinger, I'm gonna be a bit busy this weekend, so I may be delayed in reviewing this. I'll try to get to it as soon as I can.

[JLessinger]

Hey @francium , just to clarify, this is a fairly straight-forward specification and implementation pretty similar to the returns library's.

I agree with you and them that it's convenient to inline this kind of expression, and actually it's kind of Pythonic: it's basically a Result comprehension.

Mypy and strict pyright pass. Tests for usage with regular and async functions pass. Any changes needed to ship?

[francium]

Looks good, just a few minor changes are needed

[JLessinger]

Looks good, just a few minor changes are needed

Updated, let me know if this is good to merge and ship (I don't have write permission)

[francium]

Changes look good.

However, I've just noticed one potential issue.

When you have,

        out: Result[float, int] = do(
            Ok(len(x) + int(y) + 0.5)
            for x in resx
            for y in resy
        )

This looks fine, but if you remove the type annotation,

        out = do(
            Ok(len(x) + int(y) + 0.5)
            for x in resx
            for y in resy
        )

The out's type is inferred as Ok[float] | Err[Unknown] (aka Result[float, Unknown].

Small thing, but can you look at returns to see if their generator also suffers from this issue or if they're doing something to get around this?

From what I can tell, the generator expression is the root cause, because it's type is inferred as Generator[Ok[float], None, None], but it should really be Generator[Ok[float] | Err[int], None, None] to get proper typing here.

As in,

        out = do(
            cast(
                Generator[Result[float, int], None, None],
                (
                    Ok(len(x) + int(y) + 0.5)
                    for x in resx
                    for y in resy
                )
            )
        )

But expecting the user to do a cast is pretty terrible from a usability perspective.

---

The docs also currently don't highlight the need to have an explicit type annotation to get proper typing.

[francium]

It appears as though returns also suffers from the same issue,

import random
from returns import result as RS

def get() -> RS.Result[int, str]:
    if random.random() > 0.5:
        return RS.Success(random.randint(1, 10))
    else:
        return RS.Failure("something went wrong")

A = get() # Result[int, str] 
B = get() # Result[int, str] 

x = RS.Result.do( # Result[int, Unknown]
    a + b
    for a in A
    for b in B
)

I'm fine with leaving this in if you'd want to investigate this or think about solutions separately from this (please open an issue if you'd want to do that). Or if you want to investigate prior to merging, up to you.

But let's update the docs first to highlight this limitation and the workaround of adding an explicit type annotation on the returned value of the do call.

[JLessinger]

Good find. It might be possible to fix this, but I don't think it's at all straight-forward. We might have to do something like parametrize both Ok and Err by both T and E.

I think the convenience of do() justifies the slight annoyance of having to help type checkers with this annotation. I'll update the docs.

[JLessinger]

Good find. It might be possible to fix this, but I don't think it's at all straight-forward. We might have to do something like parametrize both Ok and Err by both T and E.

I think the convenience of do() justifies the slight annoyance of having to help type checkers with this annotation. I'll update the docs.

Updated docs

[JLessinger]

Addressed comments

[JLessinger]

fix

[francium]

I'll try to make a release as soon as I get the chance, either today or tomorrow

[francium]

Apologies for the delay. This is now released in 0.15.0.