Better support for `nim-result`

[emizzle]

When working inside of chronos async procs, the ? control flow doesn't seem to work as it does with normal synchronous procs. The end result is that when inside of a chronos async proc, we cannot use the ? template, and control flow suffers, being forced to check for an error condition, and return the resulting error, ie if myResult.isErr: return err myResult.error.

Consider the following:

import # vendor modules
  chronos, stew/results

type
  Asset* = object
    name*: string

  OpenSeaError* = enum
    FetchError    = "opensea: error fetching assets from opensea"

  OpenSeaResult*[T] = Result[T, OpenSeaError]

The following compiles and runs successfully:

proc queryAssets(): Future[OpenSeaResult[seq[Asset]]] {.async.} =
  return ok(@[Asset(name: "asset1")])

proc getAssets*(limit: int): Future[OpenSeaResult[seq[Asset]]] {.async.} =
  let assets = await queryAssets()
  if assets.isErr: return err assets.error
  return ok assets.get

let assets = waitFor getAssets(50)
assert assets.isOk
assert assets.get.len == 1
assert assets.get[0].name == "asset1"

However, if we switch over to the preferred control flow of nim-result by using the ? operator, like this:

proc getAssets*(limit: int): Future[OpenSeaResult[seq[Asset]]] {.async.} =
  let assets = ?(await queryAssets())
  return ok assets

then it fails to compile with the following compilation error:

vendor/nim-stew/stew/results.nim(665, 14) Error: type mismatch: got <OpenSeaResult[seq[Asset]]> but expected 'FutureBase = ref FutureBase:ObjectType'

This code can be cloned and run: https://github.com/emizzle/nim-test-env/blob/master/src/result_chronos.nim

make -j8 update && make result_chronos

[Menduist]

Same issue as https://github.com/status-im/nim-stew/issues/37

[Menduist]

A not-ideal solution I found:

template returner = # Mockup for results `?` (results would need to be updated a bit)
  result = 5
  return

proc test1(): Future[int] {.async.} =
  try:
    returner
  finally:
    return

echo waitFor(test1())

Will get transformed to

proc test1(): Future[int] {.stackTrace: off, gcsafe.} =
  iterator test1_436207620(chronosInternalRetFuture: Future[int]): FutureBase {.
      closure, gcsafe, raises: [Defect, CatchableError, Exception].} =
    {.push, warning[resultshadowed]: off.}
    var result: int
    {.pop.}
    block:
      try:
        returner
      finally:
        complete(chronosInternalRetFuture, result)
        return nil
    complete(chronosInternalRetFuture, result)

  var resultFuture = newFuture[int]("test1")
  resultFuture.closure = test1_436207620
  futureContinue(resultFuture)
  return resultFuture

Which works

We do this directly in chronos. Currently, the errors in the iterator are handled by futureContinue, but we could handle them directly in the iterator instead, and add the finally here, which would look like that:

proc test1(): Future[int] {.stackTrace: off, gcsafe.} =
  iterator test1_436207620(chronosInternalRetFuture: Future[int]): FutureBase {.
      closure, gcsafe, raises: [Defect, CatchableError, Exception].} =
    {.push, warning[resultshadowed]: off.}
    var result: int
    {.pop.}
    block:
      try:
        returner
      except CancelledError:
        chronosInternalRetFuture.cancelAndSchedule()
      except CatchableError as exc:
        chronosInternalRetFuture.fail(exc)
      finally:
        if not chronosInternalRetFuture.finished():
          complete(chronosInternalRetFuture, result)
        return nil

  var resultFuture = newFuture[int]("test1")
  resultFuture.closure = test1_436207620
  futureContinue(resultFuture)
  return resultFuture

[arnetheduck]

huh, that's an interesting find @Menduist ..

diff --git a/stew/results.nim b/stew/results.nim
index 086c8e8..f00f953 100644
--- a/stew/results.nim
+++ b/stew/results.nim
@@ -903,12 +903,15 @@ template `?`*[T, E](self: Result[T, E]): auto =
   let v = (self)
   if not v.o:
     when typeof(result) is typeof(v):
-      return v
+      result = v
+      return
     else:
       when E is void:
-        return err(typeof(result))
+        result = err(typeof(result))
+        return
       else:
-        return err(typeof(result), v.e)
+        result = err(typeof(result), v.e)
+        return
 
   when not(T is void):
     v.v

this patch to nim-result makes @emizzle 's code snippet work actually - next up: figuring out why, and what the implications / downsides are

[Menduist]

Patching only results works.. Until there is an error :)

Basically, we rely on the fact that

proc test: int =
  result = 5
  return

Works, and async transformation tries to replicate that. However, currently it doesn't work in this case:

template rr = return
proc test: Future[int] {.async.} =
  result = 5
  rr # this return will not be transformed, and will instead
     # return `nil` (shadowed result) in the iterator, causing crash

My fix to chronos would ensure that if the iterator returns nil, we complete the future with the "fake" result

As far as implications.. At least a few more variables copy. And handing this case may hide cases where we rely on result returning, but that's already the case with current async transformation

[emizzle]

Wow that's definitely an interesting workaround, great find @Menduist!

@michaelsbradleyjr and I came up with a workaround for ? that works with chronos. The first ? template handles async cases for Results -- future is unwrapped, and then used to complete the chronos future in the async context. The second ? template allows ? to be used in both a sync or async context and makes a choice to use Results when not in an async context.

import chronos
import stew/results except `?`

export results

template `?`*[T, E](self: Future[Result[T, E]]): auto =
  # can only be used within the body of a nim-chronos {.async.} proc
  # h/t @emizzle 🎉
  assert declared(chronosInternalRetFuture)
  let v = (self)
  var rv = await v
  if rv.isErr:
    when typeof(result) is typeof(rv):
      chronosInternalRetFuture.complete(rv)
    else:
      when E is void:
        chronosInternalRetFuture.complete(err(typeof(rv)))
      else:
        chronosInternalRetFuture.complete(err(typeof(rv), rv.error))
    return chronosInternalRetFuture
  when not(T is void):
    rv.get

template `?`*[T, E](self: Result[T, E]): auto =
  let v = (self)
  when declared(chronosInternalRetFuture):
    let av = proc(): Future[Result[T, E]] {.async.} = return v
    ? av()
  else:
    results.`?` v

The usage changes a little bit in that await is no longer needed, as it is done inside ?:

proc getAssets*(limit: int): Future[OpenSeaResult[seq[Asset]]] {.async.} =
  let assets = ? queryAssets()
  return ok assets

I don't think this belongs in chronos nor stew...

[emizzle]

Possible solution in https://github.com/status-im/nim-stew/pull/134.

https://github.com/status-im/nim-chronos/pull/297 as well.

[arnetheduck]

Which works

@Menduist can you make that PR? I wouldn't mind comparing vs #297 - want to test the interaction with #349 as well

[Menduist]

Can try, but that requires a bit of refacto (since we currently handle the errors in futureContinue instead of in the closure), so won't be able to tackle that in the next few days

[arnetheduck]

iterator xxx(): int {.closure.} =
  try:
    yield 42
  finally:
    return

let v = xxx

for a in v():
  echo a

It looks like the return-in-finally trick doesn't really work if there's a yield (aka await):

Error: unhandled exception: closureiters.nim(855, 11) `ctx.nearestFinally != 0`  [AssertionDefect]

The closure transformation won't accept it

[Menduist]

@arnetheduck accidently, this PR: https://github.com/status-im/nim-chronos/pull/418 offers a workaround for that

import ./chronos
template returner = # Mockup for results `?` (results would need to be updated a bit)
  result = 5
  return

proc test1(): Future[int] {.async.} =
  returner

echo waitFor(test1())

works properly on that branch

[arnetheduck]

fixed in https://github.com/arnetheduck/nim-results/pull/37 + master