Hi all,
I have submitted issue #12625 on generating func-s (closures) with mapIt, where the code crashes nimvm or behaviors wrongly in runtime.
Further checking indicates that the root cause is, for the code snippet below, env is owned by foo, so it is initialized out of for. The generated func-s in l all referring to the env by ref, so they are all referencing to the same env, which is modified in each iteration of for.
type
ft = proc (x: int): int {.noSideEffect.}
proc foo() =
let m = @[func (x: int): int = x + 100, func (x: int): int = x + 200]
var l: seq[ft] = @[]
for it in m:
# Note: the owner of env is `foo`, not `for`. `for` can't be an owner
l.add(func (x: int): int = it(x) + 1000)
let r = l.map(func (f: ft): int = f(1))
echo r[0] + r[1]
I tried to solve this by making a copy of env in genClosure.
But this modification may not work for other cases, and all checks of my pull request fails.
All suggestions are welcome.
By the way, a possible optimization: If liveness analysis of closure shows that its scope is not larger than env, then we don't need to make the copy on heap.
The generated func-s in l all referring to the env by ref, so they are all referencing to the same env, which is modified in each iteration of for.
That's actually what the spec says needs to be done (but I'm not sure it's written down somewhere in the manual. sigh). Reason: Performance, JS interop. Workaround: use system.closureScope instead.
Performance seems a nice reason, but not JS interop. Most if not all hate this JS _feature.
It is possible for the compiler to eliminate redundant copy operations. So, performance may not be a problem either.
It is possible for the compiler to eliminate redundant copy operations.
Sometimes, sure. But it already is the most complex algorithm the Nim compiler implements. To the best of my knowledge It's also on the hot path for async related code. I wouldn't mess with it tbh.
Workaround: use system.closureScope instead.
Unfortunately, this workaround does not work for this case. In the proc generated by closureScope, asgnRef is used to capture it, which is a failure. While in the example of closureScope, local loop variable is just an integer, and it works.
As system.closureScope does not work for this case, I would like to propose another macro,
macro capture(locals: openArray[typed], body: untyped): untyped =
var params = @[newEmptyNode()]
for arg in locals:
params.add(newIdentDefs(ident(arg.strVal), getTypeImpl(arg)))
result = newNimNode(nnkCall)
result.add(newProc(newEmptyNode(), params, body, nnkProcDef))
for arg in locals: result.add(arg)
Example:
type ft = proc (x: int): int
proc foo() =
let m = @[proc (x: int): int = x + 100, proc (x: int): int = x + 200]
var l: seq[ft] = @[]
for it in m:
capture([it]):
l.add(proc (x: int): int = it(x) + 1000)
let r = l.map(func (f: ft): int = f(1))
echo r[0] + r[1]
NOTE. capture only works after this pull request.
I hope that this macro can be merged into system.nim, and make closureScope deprecated. @Araq, could I create a PR for this macro? Or choose another name?
To add it into system.nim is not a good idea, macros.nim looks like a better place.
Here is the PR: https://github.com/nim-lang/Nim/pull/12712.