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

Finish closures before completing futures (fix #415) #418

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions chronos/asyncfutures2.nim
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ template complete*(future: Future[void]) =
## Completes a void ``future``.
complete(future, getSrcLocation())

proc completeWithInternalValue(future: FutureBase) =
if not(future.cancelled()):
checkFinished(future, future.internalLocation[LocationKind.Finish])
doAssert(isNil(future.internalError))
future.finish(FutureState.Completed)

proc fail(future: FutureBase, error: ref CatchableError, loc: ptr SrcLoc) =
if not(future.cancelled()):
checkFinished(future, loc)
Expand Down Expand Up @@ -317,6 +323,11 @@ proc futureContinue*(fut: FutureBase) {.raises: [], gcsafe.} =
# `await` typically) or completes / fails / is cancelled
next = fut.internalClosure(fut)
if fut.internalClosure.finished(): # Reached the end of the transformed proc

# The async macro will have filled the value and the location directly
# in the future. We just need to switch to completed state after the closure
# is _actually_ finished (ie all the `finally`s have been ran)
fut.completeWithInternalValue()
break

if next == nil:
Expand Down
122 changes: 65 additions & 57 deletions chronos/asyncmacro2.nim
Original file line number Diff line number Diff line change
Expand Up @@ -10,34 +10,13 @@
import std/[macros]

# `quote do` will ruin line numbers so we avoid it using these helpers
proc completeWithResult(fut, baseType: NimNode): NimNode {.compileTime.} =
# when `baseType` is void:
# complete(`fut`)
# else:
# complete(`fut`, result)
if baseType.eqIdent("void"):
# Shortcut if we know baseType at macro expansion time
newCall(ident "complete", fut)
else:
# `baseType` might be generic and resolve to `void`
nnkWhenStmt.newTree(
nnkElifExpr.newTree(
nnkInfix.newTree(ident "is", baseType, ident "void"),
newCall(ident "complete", fut)
),
nnkElseExpr.newTree(
newCall(ident "complete", fut, ident "result")
)
)

proc completeWithNode(fut, baseType, node: NimNode): NimNode {.compileTime.} =
proc assignOrProcessNode(node, resultNode: NimNode): NimNode {.compileTime.} =
# when typeof(`node`) is void:
# `node` # statement / explicit return
# -> completeWithResult(fut, baseType)
# else: # expression / implicit return
# complete(`fut`, `node`)
if node.kind == nnkEmpty: # shortcut when known at macro expanstion time
completeWithResult(fut, baseType)
# result = `node`
if node.kind == nnkEmpty: # shortcut when known at macro expansion time
node
else:
# Handle both expressions and statements - since the type is not know at
# macro expansion time, we delegate this choice to a later compilation stage
Expand All @@ -48,21 +27,40 @@ proc completeWithNode(fut, baseType, node: NimNode): NimNode {.compileTime.} =
ident "is", nnkTypeOfExpr.newTree(node), ident "void"),
newStmtList(
node,
completeWithResult(fut, baseType)
)
),
nnkElseExpr.newTree(
newCall(ident "complete", fut, node)
newAssignment(resultNode, node)
)
)

proc processBody(node, fut, baseType: NimNode): NimNode {.compileTime.} =
proc processBody(node, resultNode, baseType: NimNode): NimNode {.compileTime.} =
# Rewrites the returns to fill the future instead
#echo(node.treeRepr)
case node.kind
of nnkReturnStmt:
let
res = newNimNode(nnkStmtList, node)
res.add completeWithNode(fut, baseType, processBody(node[0], fut, baseType))
let res = newNimNode(nnkStmtList, node)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the comment here with generated source code.

if node[0].kind != nnkEmpty:
# transforms return to:
# when `baseType` isnot void:
# `resultNode` = processBody(node)
# else:
# {.error: "cannot return value in proc returning void".}
# return
res.add nnkWhenStmt.newTree(
nnkElifExpr.newTree(
nnkInfix.newTree(
ident "isnot", baseType, ident "void"),
newAssignment(resultNode, processBody(node[0], resultNode, baseType))
),
nnkElse.newTree(
nnkPragma.newTree(
nnkExprColonExpr.newTree(
newIdentNode("error"),
newLit("cannot return value in proc returning void"))
)
)
)
res.add newNimNode(nnkReturnStmt, node).add(newNilLit())

res
Expand All @@ -74,7 +72,7 @@ proc processBody(node, fut, baseType: NimNode): NimNode {.compileTime.} =
# We must not transform nested procedures of any form, otherwise
# `fut` will be used for all nested procedures as their own
# `retFuture`.
node[i] = processBody(node[i], fut, baseType)
node[i] = processBody(node[i], resultNode, baseType)
node

proc getName(node: NimNode): string {.compileTime.} =
Expand Down Expand Up @@ -154,14 +152,21 @@ proc asyncSingleProc(prc: NimNode): NimNode {.compileTime.} =
else: returnType
castFutureSym = nnkCast.newTree(internalFutureType, internalFutureSym)

procBody = prc.body.processBody(castFutureSym, baseType)
resultNode = nnkDotExpr.newTree(castFutureSym, ident"internalValue")
procBody = prc.body.processBody(resultNode, baseType)

# don't do anything with forward bodies (empty)
if procBody.kind != nnkEmpty:
let
# fix #13899, `defer` should not escape its original scope
procBodyBlck = nnkBlockStmt.newTree(newEmptyNode(), procBody)

# workaround https://github.com/nim-lang/Nim/issues/22645
# type InternalFutureType = `internalFutureType`
# cast[InternalFutureType](`internalFutureSym`)
internalFutureTypeSym = genSym(nskType, "InternalFutureType")
castFutureSym2 = nnkCast.newTree(internalFutureTypeSym, internalFutureSym)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expanded code comment


resultDecl = nnkWhenStmt.newTree(
# when `baseType` is void:
nnkElifExpr.newTree(
Expand All @@ -173,34 +178,26 @@ proc asyncSingleProc(prc: NimNode): NimNode {.compileTime.} =
),
# else:
nnkElseExpr.newTree(
newStmtList(
quote do: {.push warning[resultshadowed]: off.},
# var result {.used.}: `baseType`
# In the proc body, result may or may not end up being used
# depending on how the body is written - with implicit returns /
# expressions in particular, it is likely but not guaranteed that
# it is not used. Ideally, we would avoid emitting it in this
# case to avoid the default initializaiton. {.used.} typically
# works better than {.push.} which has a tendency to leak out of
# scope.
# TODO figure out if there's a way to detect `result` usage in
# the proc body _after_ template exapnsion, and therefore
# avoid creating this variable - one option is to create an
# addtional when branch witha fake `result` and check
# `compiles(procBody)` - this is not without cost though
nnkVarSection.newTree(nnkIdentDefs.newTree(
nnkPragmaExpr.newTree(
ident "result",
nnkPragma.newTree(ident "used")),
baseType, newEmptyNode())
),
quote do: {.pop.},
nnkStmtList.newTree(
# type `internalFutureTypeSym` = `internalFutureType` (workaround Nim#22645)
nnkTypeSection.newTree(nnkTypeDef.newTree(
internalFutureTypeSym, newEmptyNode(), internalFutureType
)),
# template result(): untyped {.used.} = `castFutureSym2`.internalValue
nnkTemplateDef.newTree(
ident"result",
newEmptyNode(),
newEmptyNode(),
nnkFormalParams.newTree(ident"untyped"),
nnkPragma.newTree(ident"used"),
newEmptyNode(),
nnkDotExpr.newTree(castFutureSym2, ident"internalValue")
)
)
)
)

completeDecl = completeWithNode(castFutureSym, baseType, procBodyBlck)

completeDecl = assignOrProcessNode(procBodyBlck, resultNode)
closureBody = newStmtList(resultDecl, completeDecl)

internalFutureParameter = nnkIdentDefs.newTree(
Expand Down Expand Up @@ -265,6 +262,17 @@ proc asyncSingleProc(prc: NimNode): NimNode {.compileTime.} =
iteratorNameSym)
)

# -> resultFuture.internalLocation[LocationKind.Finish] = getSrcLocation("", -1)
outerProcBody.add(
newAssignment(
nnkBracketExpr.newTree(
newDotExpr(retFutureSym, newIdentNode("internalLocation")),
newDotExpr(newIdentNode("LocationKind"), newIdentNode("Finish")),
),
newCall("getSrcLocation", newLit("\"\""), newLit(-1))
)
)

# -> futureContinue(resultFuture))
outerProcBody.add(
newCall(newIdentNode("futureContinue"), retFutureSym)
Expand Down
6 changes: 3 additions & 3 deletions chronos/srcloc.nim
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,6 @@ proc srcLocImpl(procedure: static string,
)
return addr(loc)

template getSrcLocation*(procedure: static string = ""): ptr SrcLoc =
srcLocImpl(procedure,
instantiationInfo(-2).filename, instantiationInfo(-2).line)
template getSrcLocation*(procedure: static string = "", level: static int = -2): ptr SrcLoc =
const instInfo = instantiationInfo(level)
srcLocImpl(procedure, instInfo.filename, instInfo.line)
6 changes: 3 additions & 3 deletions tests/testfut.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1218,8 +1218,8 @@ suite "Future[T] behavior test suite":
test "location test":
# WARNING: This test is very sensitive to line numbers and module name.

proc macroFuture() {.async.} = # LINE POSITION 1
let someVar {.used.} = 5 # LINE POSITION 2
proc macroFuture() {.async.} = # LINE POSITION 1 & 2
let someVar {.used.} = 5
let someOtherVar {.used.} = 4
if true:
let otherVar {.used.} = 3
Expand Down Expand Up @@ -1256,7 +1256,7 @@ suite "Future[T] behavior test suite":

check:
chk(loc10, "testfut.nim", 1221, "macroFuture")
chk(loc11, "testfut.nim", 1222, "")
chk(loc11, "testfut.nim", 1221, "")
chk(loc20, "testfut.nim", 1234, "template")
chk(loc21, "testfut.nim", 1237, "")
chk(loc30, "testfut.nim", 1231, "procedure")
Expand Down
45 changes: 45 additions & 0 deletions tests/testmacro.nim
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,51 @@ suite "Macro transformations test suite":
check:
waitFor(gen(int)) == default(int)

test "Nested return":
proc nr: Future[int] {.async.} =
return
if 1 == 1:
return 42
else:
33

check waitFor(nr()) == 42

test "Issue #415 (run closure to completion on return)":
var x = 0
proc test415 {.async.} =
try:
return
finally:
await stepsAsync(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to see more than one await in finally. and also i want to see how it works in exception handler, e.g. more than one await. Also we need to test defer functionality.

x = 5
waitFor(test415())
check: x == 5

x = 0
proc testDefer {.async.} =
defer:
await stepsAsync(1)
x = 5
return
waitFor(testDefer())
check: x == 5

x = 0
proc testExceptionHandling {.async.} =
try:
return
finally:
try:
await stepsAsync(1)
raise newException(ValueError, "")
except ValueError:
await stepsAsync(1)
await stepsAsync(1)
x = 5
waitFor(testExceptionHandling())
check: x == 5

test "Implicit return":
proc implicit(): Future[int] {.async.} =
42
Expand Down