Skip to content

Commit

Permalink
wrap fields iterations in if true scope [backport] (#24343)
Browse files Browse the repository at this point in the history
fixes #24338

When unrolling each iteration of a `fields` iterator, the compiler only
opens a new scope for semchecking, but doesn't generate a node that
signals to the codegen that a new scope should be created. This causes
issues for reused template instantiations that reuse variable symbols
between each iteration, which causes the codegen to generate multiple
declarations for them in the same scope (regardless of `inject` or
`gensym`). To fix this, we wrap the unrolled iterations in an `if true:
body` node, which both opens a new scope and doesn't interfere with
`break`.
  • Loading branch information
metagn authored Oct 22, 2024
1 parent 6744247 commit ca5df9a
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 2 deletions.
15 changes: 14 additions & 1 deletion compiler/semfields.nim
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ type
replaceByFieldName: bool
c: PContext

proc wrapNewScope(c: PContext, n: PNode): PNode {.inline.} =
# use `if true` to not interfere with `break`
# just opening scope via `openScope(c)` isn't enough,
# a scope has to be opened in the codegen as well for reused
# template instantiations
let trueLit = newIntLit(c.graph, n.info, 1)
trueLit.typ() = getSysType(c.graph, n.info, tyBool)
result = newTreeI(nkIfStmt, n.info, newTreeI(nkElifBranch, n.info, trueLit, n))

proc instFieldLoopBody(c: TFieldInstCtx, n: PNode, forLoop: PNode): PNode =
if c.field != nil and isEmptyType(c.field.typ):
result = newNode(nkEmpty)
Expand Down Expand Up @@ -72,7 +81,9 @@ proc semForObjectFields(c: TFieldsCtx, typ, forLoop, father: PNode) =
)
openScope(c.c)
inc c.c.inUnrolledContext
let body = instFieldLoopBody(fc, lastSon(forLoop), forLoop)
var body = instFieldLoopBody(fc, lastSon(forLoop), forLoop)
# new scope for each field that codegen should know about:
body = wrapNewScope(c.c, body)
father.add(semStmt(c.c, body, {}))
dec c.c.inUnrolledContext
closeScope(c.c)
Expand Down Expand Up @@ -148,6 +159,8 @@ proc semForFields(c: PContext, n: PNode, m: TMagic): PNode =
replaceByFieldName: m == mFieldPairs
)
var body = instFieldLoopBody(fc, loopBody, n)
# new scope for each field that codegen should know about:
body = wrapNewScope(c, body)
inc c.inUnrolledContext
stmts.add(semStmt(c, body, {}))
dec c.inUnrolledContext
Expand Down
46 changes: 45 additions & 1 deletion tests/system/tfields.nim
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,48 @@ block timplicit_with_partial:
echo x
echo x

foo(FooTask())
foo(FooTask())

block: # issue #24338
var innerCount = 0
var outerCount = 0
template c(w: int): int =
let q = w
inc innerCount
0

template t(r: (int, int); x: int) =
for _ in r.fields:
let w = x
doAssert w == 0
dec outerCount

proc k() =
t((0, 0), c(0))

k()
doAssert innerCount == 2
doAssert outerCount == -2

block: # issue #24338 with object
type Foo = object
x, y: int
var innerCount = 0
var outerCount = 0
template c(w: int): int =
let q = w
inc innerCount
0

template t(r: Foo; x: int) =
for _ in r.fields:
let w = x
doAssert w == 0
dec outerCount

proc k() =
t(Foo(x: 0, y: 0), c(0))

k()
doAssert innerCount == 2
doAssert outerCount == -2

0 comments on commit ca5df9a

Please sign in to comment.