fantasyland / static-land

Specification for common algebraic structures in JavaScript based on Fantasy Land
MIT License
772 stars 41 forks source link

Make sure multi-value use-case doesn't hurt one-value case in runGenerator #28

Closed rpominov closed 7 years ago

rpominov commented 7 years ago

So the problem is that JS only supports mutable generators. When we call iterator.next() we move to the next yield inside generator function and we can't later continue again from the previous yield position. But for monads like List we need that functionality. For instance:

runGenerator(List, function*() {
  const x = yield [2]
  const y = yield [3, 4] // <-- we need to continue from this point twice
  return [x * y]
}) // [6, 8]

In order to support such monads we have to maintain a history of all values we have extracted from yield'ed values, and when we need to continue again we just call generator function again and feed history to the new iterator up until the point we want to continue from. This works but costs a lot of resources especially in case of big loops inside generator.

The worst part is that we do that even for monads that don't need that feature (e.g. Maybe). So we should consider to make this optional.

Although thinking a bit more about it, I found that it can be a bit tricky to understand whether we need history feature or not with a particular monad. For instance I thought that we don't need it for Task because a task represents only one value (or even 0 in case of Task.rejected()). But actually a task can be task.run() many times, and each time the same iterator will be used if we don't emulate immutable iterators, so only first task.run() will actually work correctly.

So maybe we should keep only a guaranteed correct version of runGenerator() here for sort of educational purposes, and leave implementation of more optimized versions to implementers of particular types. For instance I'm going to implement Task.do() that is specialized for Tasks and doesn't use runGenerator under the hood.

/cc @syaiful6

Avaq commented 7 years ago

You might be interested in my findings from solving the same issue for Fluture: https://github.com/Avaq/Fluture/pull/10

rpominov commented 7 years ago

Thanks, that can be useful indeed.

syaiful6 commented 7 years ago

@rpominov maybe split the runGenerator function, one if we found the type have chainRec and map, So it's still usefull if we need more specialized version.

    // for usage with chain rec
    function generatorStep(n, d, last) {
        let { next } = last
        let { done, value } = next(last.value)
        return done
            ? value.map(d)
            : value.map(function (x) { return n({ value: x, next: next }); });
    }

usage with Task:

    // wrap the generator call inside Task computation, so multiple run is supported
    static do(func) {
        return new Task((error, success) => {
          const gen = func()
          const next = (x) => gen.next(x)
          const task = Task.chainRec(generatorStep, { value: undefined, next: next })
          return task.fork(error, success)
      })
    }
rpominov commented 7 years ago

runGenerator probably will be removed from static-land package eventually. If someone wants to create a separate package with it, please do.

rpominov commented 7 years ago

Closing because I'm going to remove runGenerator from this repository.

I think this repo should contain only the specification itself and not any utilities. Utilities should live in separate repos / npm packages. I've already moved fantasy-to-static to own repo, but going to just remove runGenerator, there is not much value in it.