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

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

Open
timotheecour opened this issue Mar 4, 2021 · 12 comments
Open

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

timotheecour opened this issue Mar 4, 2021 · 12 comments
Labels
Async Everything related to Nim's async Feature Standard Library

Comments

@timotheecour
Copy link
Member

timotheecour commented Mar 4, 2021

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 #17254 (comment) which is a working POC

Additional Information

@ringabout
Copy link
Member

ringabout commented Jun 21, 2021

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
Copy link
Member Author

timotheecour commented Jun 21, 2021

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
Copy link
Member

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 timotheecour added the Async Everything related to Nim's async label Jun 21, 2021
@timotheecour
Copy link
Member Author

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
Copy link
Member

Araq commented Jun 21, 2021

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
Copy link
Member Author

timotheecour commented Jun 21, 2021

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
Copy link
Member

Araq commented Jun 21, 2021

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
Copy link
Member Author

timotheecour commented Jun 21, 2021

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:

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

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

@dom96
Copy link
Contributor

dom96 commented Jun 21, 2021

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
Copy link
Contributor

dom96 commented Jun 21, 2021

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

@timotheecour
Copy link
Member Author

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
Copy link
Member

Araq commented Jun 22, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Async Everything related to Nim's async Feature Standard Library
Projects
None yet
Development

No branches or pull requests

4 participants