Make await a template #12085

alehander92 · 2019-08-29T10:59:33Z

Adapted status-im/nim-chronos#47 for asyncdispatch with support for multisync : credit for the original impl to @yglukhov !

alehander92 · 2019-08-29T11:00:09Z

it also helps #11911 (comment) and adding stuff like waitFor check

Araq · 2019-08-29T14:25:23Z

What does that do? Enable await within templates?

alehander92 · 2019-08-29T14:36:06Z

Yes, now i can use it in templates : but it also makes await a template which is a cleaner implementation compared to checking for idents (and one can easily implement other stuff like waitFor is defined out of {.async.}, but not inside)

lib/pure/asyncmacro.nim

dom96

This is cool, but the async macro still has code which processes await. You should remove that code to make sure your template is actually being used.

lib/pure/asyncmacro.nim

dom96 · 2020-04-13T13:03:15Z

Any updates on this?

alehander92 · 2020-04-13T13:05:26Z

sorry, i left some comments but not sure if easily visible: it doesnt work for some reason with T : something like "expr has no type" errors, it really doesnt seem obvious to me why

this is for the review task to use T instead of auto

dom96 · 2020-04-13T13:12:23Z

That's fine, but there are other comments that need resolving.

alehander92 · 2020-04-13T13:25:18Z

ok, i'll try to push a fix for those

alehander92 · 2020-04-16T13:52:51Z

ok, the only remaining issue seems to be that tasyncawait_cyclebreaker.nim requires me to increase the limit from 1000 to ~150_000 which .. seems strange, have to debug how i break it

alehander92 · 2020-04-16T15:13:47Z

EDIT: ok code fixed (dont merge yet: wanted to remove some code)

otherwise Araq said https://irclogs.nim-lang.org/16-04-2020.html#13:51:11 so i'll leave the test as that (even though i think it might be leaking as it constantly deallocates less)

alehander92 · 2020-04-16T15:21:04Z

ok, now it should be ready, also used error instead of assert after i saw a subsequent @yglukhov PR to chronos status-im/nim-chronos#48

sorry, the changes comment about code processing await seems to be for outdated older changes: please review the fresh ones

lib/pure/asyncmacro.nim

tests/async/tasyncawait_cyclebreaker.nim

dom96

I like these changes a lot, but there are a couple of things that I think still need to be resolved. Sorry about the back and forth.

This time I was very thorough so this should be the last of it.

dom96 · 2020-04-16T21:06:16Z

lib/pure/asyncmacro.nim

+template await*(f: untyped): untyped =
+  when declared(retFuture):
+    static:
+      error "await expects Future[T], got " & $typeof(f)
+  elif not declared(inMultisync):
+    static:
+      error "await only available within {.async.}"
+  else:
+    f


From what I can tell this is only here to allow multisync to work. It took me a while to figure this out, can you add a comment to explain this?

Actually, now that I look at the code above, I would just remove this and keep the stripAwait logic in splitProc. Adding this inMultisync variable into the generated code is more hacky. That way you can also reuse the compiler's type mismatch error messages.

i really think this is also elegant: after all you change how this works quickly with high level code flow without any ast surgery. inMultisync should be somehow optimizeable, its needed only on compile time

A loop which modifies the AST may be more code, but it's also far more deterministic and easier to debug should something go wrong. Hygiene is important and generating variables so that you can then check for their existence is flaky. The current multisync macro does what it needs to do, and then gets out of your way.

but .. wouldn't this break templates again? after all you wouldn't be able to generate await in multisync, i think

oh, you mean if a template using await is used in a multisync proc? I think you're right. But in that case, can we resolve that in a separate PR? I just like to keep this clean.

ok, but we really shouldn't get into a situation where just one of the cases is fixed.. they are symmetric

ok, no we dont need those variables anyway, so i think this can be still one PR

lib/pure/asyncmacro.nim

dom96 · 2020-04-16T21:13:51Z

lib/pure/asyncmacro.nim

@@ -257,7 +178,7 @@ proc asyncSingleProc(prc: NimNode): NimNode {.compileTime.} =
    outerProcBody.add(prc.body[0])

  # -> var retFuture = newFuture[T]()
-  var retFutureSym = genSym(nskVar, "retFuture")
+  var retFutureSym = ident"retFuture"


Why was this changed? Without genSym here if someone declares a retFuture of their own then there will be a clash...

in order for await to be able to check for this symbol in when IIRC

Ahh. Yeah, I'm not the biggest fan of this. If someone accidentally defines a retFuture in their proc then it'll be considered async (that behaviour will be very confusing).

I wonder if we can come up with a better way to detect async procs, can you check if compiles(yield f) works? :)

interesting idea, but this would work in another iterator as well ! which we still don't want.

maybe i can generate a symbol which can't be written by users similar to :env

but this would work in another iterator as well ! which we still don't want.

IMO it's not the worst thing if it does work, it'll only work if that iterator's return type is a future and if a future is awaited, so likelihood is that it will make sense anyway.

we can do something else : we can just leave out template await and and generate just one in of async: Future[T] working and one in multisync sync: which works for every type: all other cases just fail like normal overloads!

this way we can really just use the default nim mechanisms for most of the errors as well (await just isn't defined somewhere, or it just doesnt accepts int somewhere)

and if we really want to we can still leave a global overload which errors with a more detailed message as a fallback

this works!

i'll push a new commit when i test it a bit

tests/async/tasyncawait_cyclebreaker.nim

dom96 · 2020-04-16T21:18:44Z

tests/async/tasync_traceback.nim

@@ -110,13 +110,15 @@ Async traceback:
      ## Executes pending callbacks
    asyncmacro\.nim\(\d+?\)\s+?fooNimAsyncContinue
      ## Resumes an async procedure
-    tasync_traceback\.nim\(\d+?\)\s+?fooIter
+    asyncmacro\.nim\(\d+?\)\s+?fooIter


This actually reduces the amount of signal we get from the async tracebacks. I cannot recall whether this particular line provided useful information, but if it does then that could be a blocker for these changes. Can you run this test and show us what the tracebacks are before and after your changes?

IIRC the change is similar to what is seen in the diff, basically we need to add the linedir of tasync_traceback to the result of template somehow maybe?

I want to see what line the traceback points to now.

asyncmacro.nim(261)

which seems to be (cast[type(f)](internalTmpFuture)).read()

That should be okay, I'm assuming that tasync_traceback in the old traceback doesn't point to anything interesting though.

alehander92 · 2020-04-22T12:40:27Z

ok, so now we generate await templates inline to not need those retFuture / inMultisync idents, however this require await to be mixin in templates using await (or to annotate them with {.dirty.})

this can be hinted in an error message tho

alehander92 · 2020-04-22T13:19:23Z

sorry! it seems we can just skip the global overload fallback with a custom error and then template without annotation dirty can work. not perfect, but i think it's good enough!

dom96

I love it, but please check with @Araq that this won't slow down compilation. Generating so many copies of the same template seems like it could go badly :)

dom96 · 2020-04-23T20:10:42Z

lib/pure/asyncmacro.nim

@@ -332,8 +253,24 @@ proc asyncSingleProc(prc: NimNode): NimNode {.compileTime.} =
    if returnType.kind == nnkEmpty:
      # Add Future[void]
      result.params[0] = parseExpr("owned(Future[void])")
+
+  # based on the yglukhov's patch to chronos: https://github.com/status-im/nim-chronos/pull/47


I would remove this comment, at this point this really doesn't have many parallels. I doubt it'll be useful for those exploring this code.

well, I still got the "define await as a template" overall approach from there, but I am not sure what's the best way to document that stuff

dom96 · 2020-04-23T20:15:01Z

lib/pure/asyncmacro.nim

+# overload for await as a fallback handler, based on the yglukhov's patch to chronos: https://github.com/status-im/nim-chronos/pull/47
+# template await*(f: typed): untyped =
+  # static:
+    # error "await only available within {.async.}"


You still need this, no?

oh, you removed this because you didn't want dirty. Is dirty really that bad? :)

no, but it can be a bit surprising: we need to somehow hint here in the error "if you use a template, you might want to use {.dirty.}" because otherwise many users would be just confused by why just a template/macro can't work

on the other hand just a type mismatch/undefined error is very natural, await is not found and that's it

dom96 · 2020-04-23T20:15:27Z

lib/pure/asyncmacro.nim

+    result[0][3][i][1] = splitParamType(result[0][3][i][1], async=false)
+  var multisyncAwait = quote:
+    template await(value: typed): untyped =
+      value
+
+  result[0][^1] = nnkStmtList.newTree(multisyncAwait, result[0][^1])


I love it :D

credit to @zah's idea #11911 (comment) i think i finally remembered this is also possible

alehander92 · 2020-04-23T21:07:06Z

I love it, but please check with @Araq that this won't slow down compilation. Generating so many copies of the same template seems like it could go badly :)

Ok, I was wondering about that as well, maybe the compiler should be able to somehow cache such cases (applicable for other template/macro applications)

Araq · 2020-04-24T12:36:36Z

I don't see the copies here, what do you mean?

alehander92 · 2020-04-24T13:07:15Z

Araq, it's about inserting two await templates into each async function (and one into each sync generated by multisync)

Araq · 2020-04-25T18:26:46Z

I cannot imagine these 2 little templates are noticable, merging.

The `check eventually` template is incompatible with Nim 1.2. It uses `await` in a template, which is not possible until after 1.2. nim-lang/Nim#12085 (comment)

alehander92 requested a review from dom96 August 29, 2019 11:01

juancarlospaco reviewed Aug 29, 2019

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

juancarlospaco reviewed Aug 29, 2019

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

juancarlospaco reviewed Aug 29, 2019

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

mratsim reviewed Aug 30, 2019

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

dom96 previously requested changes Sep 2, 2019

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

alehander92 force-pushed the await-template branch 3 times, most recently from ed04b24 to 6bd3a38 Compare September 28, 2019 09:19

zedeus mentioned this pull request Nov 26, 2019

Can't use await in templates #12743

Closed

alehander92 force-pushed the await-template branch 5 times, most recently from c89dc26 to 873cd78 Compare April 16, 2020 13:50

alehander92 force-pushed the await-template branch from 873cd78 to ad74a13 Compare April 16, 2020 14:17

alehander92 force-pushed the await-template branch 2 times, most recently from 937ef93 to 58ebdb9 Compare April 16, 2020 15:19

alehander92 requested a review from dom96 April 16, 2020 16:28

Araq reviewed Apr 16, 2020

View reviewed changes

lib/pure/asyncmacro.nim Outdated Show resolved Hide resolved

Araq reviewed Apr 16, 2020

View reviewed changes

tests/async/tasyncawait_cyclebreaker.nim Outdated Show resolved Hide resolved

dom96 requested changes Apr 16, 2020

View reviewed changes

alehander92 added 2 commits April 21, 2020 19:32

Make await a template

48c7751

Use typed for await

4ec022e

alehander92 force-pushed the await-template branch from 58ebdb9 to 4ec022e Compare April 21, 2020 16:39

alehander92 added 2 commits April 22, 2020 14:42

Generate await inside async/multisync

b48bdc0

Fix errors, more tests

da2c09b

Remove global handler, type error if out of async

8b016d2

dom96 approved these changes Apr 23, 2020

View reviewed changes

dom96 mentioned this pull request Apr 24, 2020

Make usage of waitFor in {.async.} compile time error #11911

Open

Araq merged commit 5658441 into nim-lang:devel Apr 25, 2020

alehander92 mentioned this pull request May 8, 2020

Nested await expressions regression: await a(await expandValue()) doesnt compile #14279

Closed

Make await a template #12085

Make await a template #12085

Conversation

alehander92 commented Aug 29, 2019 • edited Loading

alehander92 commented Aug 29, 2019

Araq commented Aug 29, 2019

alehander92 commented Aug 29, 2019

dom96 left a comment

Choose a reason for hiding this comment

dom96 commented Apr 13, 2020

alehander92 commented Apr 13, 2020 • edited Loading

dom96 commented Apr 13, 2020

alehander92 commented Apr 13, 2020

alehander92 commented Apr 16, 2020

alehander92 commented Apr 16, 2020 • edited Loading

alehander92 commented Apr 16, 2020

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alehander92 Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alehander92 commented Apr 22, 2020

alehander92 commented Apr 22, 2020 • edited Loading

dom96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alehander92 Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alehander92 commented Apr 23, 2020

Araq commented Apr 24, 2020

alehander92 commented Apr 24, 2020

Araq commented Apr 25, 2020

alehander92 commented Aug 29, 2019 •

edited

Loading

alehander92 commented Apr 13, 2020 •

edited

Loading

alehander92 commented Apr 16, 2020 •

edited

Loading

dom96 Apr 17, 2020 •

edited

Loading

alehander92 Apr 16, 2020 •

edited

Loading

alehander92 commented Apr 22, 2020 •

edited

Loading

alehander92 Apr 23, 2020 •

edited

Loading