`=>` doesn't work with {.async.} (neither js nor c)

timotheecour commented 3 years ago

Example 1

when true:
  # nim r -b:js main
  import std/asyncjs
  import std/sugar
  let fn1 = (a:int) {.noSideEffect.} => 0 # ok
  let fn2 = (a:int) {.async.} => 0 # Error: implementation of ':anonymous' expected

Current Output: Error: implementation of ':anonymous' expected

Example 2

when true:
  # nim r main
  import std/async
  import std/sugar
  let fn1 = (a:int) {.noSideEffect.} => 0 # ok
  let fn2 = (a:int) {.async.} => 0 # Error: Expected return type of 'Future' got 'auto'

Current Output: Error: Expected return type of 'Future' got 'auto'

Proposed Solution

see https://github.com/nim-lang/Nim/issues/17254#issuecomment-865274977 which is a working POC

Additional Information

1.5.1 507873be25baa06d7da3c44b0c36d253c850625d
this follows https://github.com/nim-lang/Nim/issues/17177 which was fixed recently
when fixing this, un-disable the test disabled in https://github.com/nim-lang/Nim/pull/17189

ringabout commented 3 years ago

Why should example 1 work? Since(using int/auto)

when true:
  import std/async
  import std/sugar
  let fn3 = proc(a:int): int {.async.} = 1234

doesn't work?

At least the returned type should be specified for async procs

when true:
  import std/async
  import std/sugar
  let fn3 = proc(a:int): Future[int] {.async.} = result = 1234

timotheecour commented 3 years ago

is there a reason why the declared return type in async procs needs to be Future[T] instead of T? Seems to me that async macro could do this wrapping for you.

If it introduces ambiguities (eg cases where Future[Future[T]] would make sense), then maybe we could introduce {.async2.} that allows writing:

let fn3 = proc(a:int): int {.async2.} = 1234

and then this could work:

let fn2 = (a:int) {.async2.} => 0

(async2 can be bikeshedded)

ringabout commented 3 years ago

is there a reason why the declared return type in async procs needs to be Future[T] instead of T? Seems to me that async macro could do this wrapping for you.

I guess that's a historical issue. It should do the wrap or so.

timotheecour commented 3 years ago

so the main question is whether we can relax async to allow T and Future[T] or we need async2 that requires T (and generates Future[T])

Araq commented 3 years ago

The real issue here is once again that untyped macros do not compose. Untyped macros are simply a bad idea and Nim's builtin contructs (if/case/etc) all sem'check the subexpressions and then sem'check the construct. This is quite telling, "cannot expand this template inside an 'if' statement" is an entirely unknown problem.

timotheecour commented 3 years ago

Untyped macros are simply a bad idea

that's just too extreme. Without them many things would not be possible. Yes, they have caveats, and no they can't be removed or replaced by alternatives in many cases (at least not without some language changes), I can provide examples if needed (also, it's unclear whether you mean "immediate" macros/templates where all params are untyped, or just untyped params to macros)

That said, some macros could be changed to use typed instead of untyped, and it's quite possible that async (in async and asyncjs) could be changed to typed, TBD (implementation does look hacky and improvable, eg things like isFutureVoid)

Here's a quick and dirty POC that works and could be turned into a robust solution:

when true: # D20210621T120527
  import std/async
  import std/sugar
  import std/macros
  macro async2(a): untyped =
    result = a.copyNimTree
    let body = result[6]
    result[3][0] = quote do:
      Future[typeof(`body`)]
    result[4] = quote do: {.async.}
    result[6] = quote do: result = `body`
    echo result.repr
  let fn3 = proc(a:int): int {.async2.} = 1234
  let fn4 = (b: int) {.async2.} => 12

Araq commented 3 years ago

I can provide examples if needed

I'm interested yes. For the cases I came up with it starts to work out once we added an annotation like "in m(x) the 'x' will be a scope injected symbol name"

timotheecour commented 3 years ago

again, you need to clarify whether you mean "immediate" macros/templates where all params are untyped, or just untyped params to macros, or untyped params in general

but here are typical cases where untyped is needed:

compiler magics that need untyped, eg proc defined*(x: untyped): bool {.magic.}
things that need to preserve the AST, eg dumpToString (https://github.com/nim-lang/Nim/pull/16841, improves over the dump(a: typed)), can't use typed as explained in PR (eg because of things like const-folding); likewise, doAssert was changed to untyped (refs https://github.com/nim-lang/Nim/pull/9332, fixes excessive expansion on error refs https://github.com/nim-lang/Nim/issues/8518)
templates or macros with scope injection like {.byaddr.} (std/decls)

starts to work out once we added an annotation like "in m(x) the 'x' will be a scope injected symbol name"

even in this category of use cases, it doesn't help in cases like genAst (aka quote do with explicit capture) where the x is not explicitly listed in the parameter list, instead it's a sub-node (b = true) of the varargs[untyped] params

There are plenty other examples

things that mitigate `untyped` problems

iterable[T] introduced recently removed a whole category of use cases for untyped params
https://github.com/timotheecour/Nim/issues/630 RFC will allow overloading with untyped params (RFC: untyped(expr) for untyped params in overloaded routines)
1st class scope (https://github.com/nim-lang/Nim/pull/11754 getCurrentScope) because it allows you to resolve a sym from another scope given an ident

cases where an API uses untyped but it should used typed:

ForLoopStmt (which is an untyped tree) should be replaced by macro customFor[T](a: T, b: ForLoopStmt): untyped, refs https://github.com/nim-lang/RFCs/issues/345, allowing overloading by type
many more

dom96 commented 3 years ago

is there a reason why the declared return type in async procs needs to be Future[T] instead of T? Seems to me that async macro could do this wrapping for you.

I guess that's a historical issue. It should do the wrap or so.

Nope, it's a conscious decision. The procedure type should reflect the type that is actually returned, a Future is in fact returned so you should write it. This is the same in Hack (Awaitable<T> not T), C# (Task<T>) and likely many others. I don't think we should change it.

dom96 commented 3 years ago

Wouldn't a solution here be to simply make => aware of {.async.}? It can find this pragma and call into the async transformation.

timotheecour commented 3 years ago

make => aware of {.async.}?

this is arguably more hacky than introducing a {.async2.} that would auto-box return type as Future. => should not have to be designed with {.async.} in mind.

Araq commented 3 years ago

compiler magics that need untyped, eg proc defined*(x: untyped): bool {.magic.}

defined could instead easily be implemented differently (it could analyse an nkError node, for example).

things that need to preserve the AST, eg dumpToString (#16841, improves over the dump(a: typed)), can't use typed as explained in PR (eg because of things like const-folding); likewise, doAssert was changed to untyped (refs #9332, fixes excessive expansion on error refs #8518)

Doesn't convince me all that much. That you need to be aware of macro expansions is a price I'm willing to pay for a simplified macro system.

nim-lang / Nim