From 72fcda1b35452795a8c2e7ad01708c2cda410f41 Mon Sep 17 00:00:00 2001
From: metagn <metagngn@gmail.com>
Date: Tue, 22 Oct 2024 20:56:37 +0300
Subject: [PATCH] wrap `fields` iterations in `if true` scope [backport]
 (#24343)

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`.

(cherry picked from commit ca5df9ab25f1444025f9b8928cf21893a160489b)
---
 compiler/semfields.nim   | 15 ++++++++++++-
 tests/system/tfields.nim | 46 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/compiler/semfields.nim b/compiler/semfields.nim
index 5f3172f81607..8e7d8b64a4cd 100644
--- a/compiler/semfields.nim
+++ b/compiler/semfields.nim
@@ -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)
@@ -70,7 +79,9 @@ proc semForObjectFields(c: TFieldsCtx, typ, forLoop, father: PNode) =
     fc.replaceByFieldName = c.m == mFieldPairs
     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)
@@ -145,6 +156,8 @@ proc semForFields(c: PContext, n: PNode, m: TMagic): PNode =
       fc.c = c
       fc.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
diff --git a/tests/system/tfields.nim b/tests/system/tfields.nim
index 0bf3a4e1adf0..8e7533baacdf 100644
--- a/tests/system/tfields.nim
+++ b/tests/system/tfields.nim
@@ -105,4 +105,48 @@ block timplicit_with_partial:
       echo x
     echo x
 
-  foo(FooTask())
\ No newline at end of file
+  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