Expand hoisted default params in sem (#15270)

* Expand hoisted default params in sem
Introduce ast.newTree{I,IT}
Add test for default params in procs

* Cleanup

* Simplify hoist transformation and expand test

12 files changed, 178 insertions, 123 deletions

diff --git a/compiler/ast.nim b/compiler/ast.nim
index 23b564af3..b1a3a4a7b 100644
--- a/compiler/ast.nim
+++ b/compiler/ast.nim
@@ -312,10 +312,6 @@ const
     # the compiler will avoid printing such names
     # in user messages.
 
-  sfHoisted* = sfForward
-    # an expression was hoised to an anonymous variable.
-    # the flag is applied to the var/let symbol
-
   sfNoForward* = sfRegister
     # forward declarations are not required (per module)
   sfReorder* = sfForward
@@ -1121,12 +1117,49 @@ proc newNode*(kind: TNodeKind): PNode =
       writeStackTrace()
     inc gNodeId
 
+proc newNodeI*(kind: TNodeKind, info: TLineInfo): PNode =
+  result = PNode(kind: kind, info: info)
+  when defined(useNodeIds):
+    result.id = gNodeId
+    if result.id == nodeIdToDebug:
+      echo "KIND ", result.kind
+      writeStackTrace()
+    inc gNodeId
+
+proc newNodeI*(kind: TNodeKind, info: TLineInfo, children: int): PNode =
+  result = PNode(kind: kind, info: info)
+  if children > 0:
+    newSeq(result.sons, children)
+  when defined(useNodeIds):
+    result.id = gNodeId
+    if result.id == nodeIdToDebug:
+      echo "KIND ", result.kind
+      writeStackTrace()
+    inc gNodeId
+
+proc newNodeIT*(kind: TNodeKind, info: TLineInfo, typ: PType): PNode =
+  result = newNode(kind)
+  result.info = info
+  result.typ = typ
+
 proc newTree*(kind: TNodeKind; children: varargs[PNode]): PNode =
   result = newNode(kind)
   if children.len > 0:
     result.info = children[0].info
   result.sons = @children
 
+proc newTreeI*(kind: TNodeKind; info: TLineInfo; children: varargs[PNode]): PNode =
+  result = newNodeI(kind, info)
+  if children.len > 0:
+    result.info = children[0].info
+  result.sons = @children
+
+proc newTreeIT*(kind: TNodeKind; info: TLineInfo; typ: PType; children: varargs[PNode]): PNode =
+  result = newNodeIT(kind, info, typ)
+  if children.len > 0:
+    result.info = children[0].info
+  result.sons = @children
+
 template previouslyInferred*(t: PType): PType =
   if t.sons.len > 1: t.lastSon else: nil
 
@@ -1227,43 +1260,6 @@ proc newSymNode*(sym: PSym, info: TLineInfo): PNode =
   result.typ = sym.typ
   result.info = info
 
-proc newNodeI*(kind: TNodeKind, info: TLineInfo): PNode =
-  result = PNode(kind: kind, info: info)
-  when defined(useNodeIds):
-    result.id = gNodeId
-    if result.id == nodeIdToDebug:
-      echo "KIND ", result.kind
-      writeStackTrace()
-    inc gNodeId
-
-proc newNodeI*(kind: TNodeKind, info: TLineInfo, children: int): PNode =
-  result = PNode(kind: kind, info: info)
-  if children > 0:
-    newSeq(result.sons, children)
-  when defined(useNodeIds):
-    result.id = gNodeId
-    if result.id == nodeIdToDebug:
-      echo "KIND ", result.kind
-      writeStackTrace()
-    inc gNodeId
-
-proc newNode*(kind: TNodeKind, info: TLineInfo, sons: TNodeSeq = @[],
-              typ: PType = nil): PNode =
-  # XXX use shallowCopy here for ownership transfer:
-  result = PNode(kind: kind, info: info, typ: typ)
-  result.sons = sons
-  when defined(useNodeIds):
-    result.id = gNodeId
-    if result.id == nodeIdToDebug:
-      echo "KIND ", result.kind
-      writeStackTrace()
-    inc gNodeId
-
-proc newNodeIT*(kind: TNodeKind, info: TLineInfo, typ: PType): PNode =
-  result = newNode(kind)
-  result.info = info
-  result.typ = typ
-
 proc newIntNode*(kind: TNodeKind, intVal: BiggestInt): PNode =
   result = newNode(kind)
   result.intVal = intVal
diff --git a/compiler/docgen.nim b/compiler/docgen.nim
index 1e0756d64..ff3a540be 100644
--- a/compiler/docgen.nim
+++ b/compiler/docgen.nim
@@ -992,8 +992,8 @@ proc documentEffect(cache: IdentCache; n, x: PNode, effectType: TSpecialWord, id
       # set the type so that the following analysis doesn't screw up:
       effects[i].typ = real[i].typ
 
-    result = newNode(nkExprColonExpr, n.info, @[
-      newIdentNode(getIdent(cache, specialWords[effectType]), n.info), effects])
+    result = newTreeI(nkExprColonExpr, n.info, 
+      newIdentNode(getIdent(cache, specialWords[effectType]), n.info), effects)
 
 proc documentWriteEffect(cache: IdentCache; n: PNode; flag: TSymFlag; pragmaName: string): PNode =
   let s = n[namePos].sym
@@ -1005,8 +1005,8 @@ proc documentWriteEffect(cache: IdentCache; n: PNode; flag: TSymFlag; pragmaName
       effects.add params[i]
 
   if effects.len > 0:
-    result = newNode(nkExprColonExpr, n.info, @[
-      newIdentNode(getIdent(cache, pragmaName), n.info), effects])
+    result = newTreeI(nkExprColonExpr, n.info, 
+      newIdentNode(getIdent(cache, pragmaName), n.info), effects)
 
 proc documentRaises*(cache: IdentCache; n: PNode) =
   if n[namePos].kind != nkSym: return
diff --git a/compiler/lowerings.nim b/compiler/lowerings.nim
index 5e75c36de..f6ca98196 100644
--- a/compiler/lowerings.nim
+++ b/compiler/lowerings.nim
@@ -369,14 +369,3 @@ proc genLen*(g: ModuleGraph; n: PNode): PNode =
     result[0] = newSymNode(getSysMagic(g, n.info, "len", mLengthSeq))
     result[1] = n
 
-proc hoistExpr*(varSection, expr: PNode, name: PIdent, owner: PSym): PSym =
-  result = newSym(skLet, name, owner, varSection.info, owner.options)
-  result.flags.incl sfHoisted
-  result.typ = expr.typ
-
-  var varDef = newNodeI(nkIdentDefs, varSection.info, 3)
-  varDef[0] = newSymNode(result)
-  varDef[1] = newNodeI(nkEmpty, varSection.info)
-  varDef[2] = expr
-
-  varSection.add varDef
diff --git a/compiler/parser.nim b/compiler/parser.nim
index e7db8d8b1..0fa860c99 100644
--- a/compiler/parser.nim
+++ b/compiler/parser.nim
@@ -1444,7 +1444,7 @@ proc parseExprStmt(p: var Parser): PNode =
         result.add(commandParam(p, isFirstParam, pmNormal))
         if p.tok.tokType != tkComma: break
     elif p.tok.indent < 0 and isExprStart(p):
-      result = newNode(nkCommand, a.info, @[a])
+      result = newTreeI(nkCommand, a.info, a)
       while true:
         result.add(commandParam(p, isFirstParam, pmNormal))
         if p.tok.tokType != tkComma: break
diff --git a/compiler/pragmas.nim b/compiler/pragmas.nim
index b2529a5b8..71bd6f81f 100644
--- a/compiler/pragmas.nim
+++ b/compiler/pragmas.nim
@@ -532,7 +532,7 @@ proc processLink(c: PContext, n: PNode) =
 proc semAsmOrEmit*(con: PContext, n: PNode, marker: char): PNode =
   case n[1].kind
   of nkStrLit, nkRStrLit, nkTripleStrLit:
-    result = newNode(if n.kind == nkAsmStmt: nkAsmStmt else: nkArgList, n.info)
+    result = newNodeI(if n.kind == nkAsmStmt: nkAsmStmt else: nkArgList, n.info)
     var str = n[1].strVal
     if str == "":
       localError(con.config, n.info, "empty 'asm' statement")
@@ -563,7 +563,7 @@ proc semAsmOrEmit*(con: PContext, n: PNode, marker: char): PNode =
       a = c + 1
   else:
     illFormedAstLocal(n, con.config)
-    result = newNode(nkAsmStmt, n.info)
+    result = newNodeI(nkAsmStmt, n.info)
 
 proc pragmaEmit(c: PContext, n: PNode) =
   if n.kind notin nkPragmaCallKinds or n.len != 2:
@@ -626,7 +626,7 @@ proc processPragma(c: PContext, n: PNode, i: int) =
     invalidPragma(c, n)
 
   var userPragma = newSym(skTemplate, it[1].ident, nil, it.info, c.config.options)
-  userPragma.ast = newNode(nkPragma, n.info, n.sons[i+1..^1])
+  userPragma.ast = newTreeI(nkPragma, n.info, n.sons[i+1..^1])
   strTableAdd(c.userPragmas, userPragma)
 
 proc pragmaRaisesOrTags(c: PContext, n: PNode) =
diff --git a/compiler/semdata.nim b/compiler/semdata.nim
index b99ddcba3..b15f65967 100644
--- a/compiler/semdata.nim
+++ b/compiler/semdata.nim
@@ -381,8 +381,7 @@ proc makeNotType*(c: PContext, t1: PType): PType =
   result.flags.incl tfHasMeta
 
 proc nMinusOne(c: PContext; n: PNode): PNode =
-  result = newNode(nkCall, n.info, @[
-    newSymNode(getSysMagic(c.graph, n.info, "pred", mPred)), n])
+  result = newTreeI(nkCall, n.info, newSymNode(getSysMagic(c.graph, n.info, "pred", mPred)), n)
 
 # Remember to fix the procs below this one when you make changes!
 proc makeRangeWithStaticExpr*(c: PContext, n: PNode): PType =
@@ -391,9 +390,8 @@ proc makeRangeWithStaticExpr*(c: PContext, n: PNode): PType =
   result.sons = @[intType]
   if n.typ != nil and n.typ.n == nil:
     result.flags.incl tfUnresolved
-  result.n = newNode(nkRange, n.info, @[
-    newIntTypeNode(0, intType),
-    makeStaticExpr(c, nMinusOne(c, n))])
+  result.n = newTreeI(nkRange, n.info, newIntTypeNode(0, intType),
+    makeStaticExpr(c, nMinusOne(c, n)))
 
 template rangeHasUnresolvedStatic*(t: PType): bool =
   tfUnresolved in t.flags
diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim
index 575d0b774..0e1c5e9d3 100644
--- a/compiler/semexprs.nim
+++ b/compiler/semexprs.nim
@@ -1556,10 +1556,9 @@ proc propertyWriteAccess(c: PContext, n, nOrig, a: PNode): PNode =
   # this is ugly. XXX Semantic checking should use the ``nfSem`` flag for
   # nodes?
   let aOrig = nOrig[0]
-  result = newNode(nkCall, n.info, sons = @[setterId, a[0],
-                                            semExprWithType(c, n[1])])
+  result = newTreeI(nkCall, n.info, setterId, a[0], semExprWithType(c, n[1]))
   result.flags.incl nfDotSetter
-  let orig = newNode(nkCall, n.info, sons = @[setterId, aOrig[0], nOrig[1]])
+  let orig = newTreeI(nkCall, n.info, setterId, aOrig[0], nOrig[1])
   result = semOverloadedCallAnalyseEffects(c, result, orig, {})
 
   if result != nil:
@@ -2041,7 +2040,7 @@ proc semQuoteAst(c: PContext, n: PNode): PNode =
     dummyTemplate[paramsPos].add getSysSym(c.graph, n.info, "untyped").newSymNode # return type
     ids.add getSysSym(c.graph, n.info, "untyped").newSymNode # params type
     ids.add c.graph.emptyNode # no default value
-    dummyTemplate[paramsPos].add newNode(nkIdentDefs, n.info, ids)
+    dummyTemplate[paramsPos].add newTreeI(nkIdentDefs, n.info, ids)
 
   var tmpl = semTemplateDef(c, dummyTemplate)
   quotes[0] = tmpl[namePos]
@@ -2053,10 +2052,10 @@ proc semQuoteAst(c: PContext, n: PNode): PNode =
                     newIdentNode(getIdent(c.cache, "newIdentNode"), n.info)
                   else:
                     identNodeSym.newSymNode
-  quotes[1] = newNode(nkCall, n.info, @[identNode, newStrNode(nkStrLit, "result")])
-  result = newNode(nkCall, n.info, @[
+  quotes[1] = newTreeI(nkCall, n.info, identNode, newStrNode(nkStrLit, "result"))
+  result = newTreeI(nkCall, n.info, 
      createMagic(c.graph, "getAst", mExpandToAst).newSymNode,
-    newNode(nkCall, n.info, quotes)])
+     newTreeI(nkCall, n.info, quotes))
   result = semExpandToAst(c, result)
 
 proc tryExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode =
@@ -2579,6 +2578,43 @@ proc shouldBeBracketExpr(n: PNode): bool =
           n[0] = be
           return true
 
+proc hoistParamsUsedInDefault(c: PContext, call, letSection, defExpr: var PNode) =
+  # This takes care of complicated signatures such as:
+  # proc foo(a: int, b = a)
+  # proc bar(a: int, b: int, c = a + b)
+  #
+  # The recursion may confuse you. It performs two duties:
+  #
+  # 1) extracting all referenced params from default expressions
+  #    into a let section preceding the call
+  #
+  # 2) replacing the "references" within the default expression
+  #    with these extracted skLet symbols.
+  #
+  # The first duty is carried out directly in the code here, while the second
+  # duty is activated by returning a non-nil value. The caller is responsible
+  # for replacing the input to the function with the returned non-nil value.
+  # (which is the hoisted symbol)
+  if defExpr.kind == nkSym and defExpr.sym.kind == skParam and defExpr.sym.owner == call[0].sym:
+    let paramPos = defExpr.sym.position + 1
+
+    if call[paramPos].kind != nkSym:
+      let hoistedVarSym = newSym(skLet, getIdent(c.graph.cache, genPrefix), c.p.owner, letSection.info, c.p.owner.options)
+      hoistedVarSym.typ = call[paramPos].typ
+
+      letSection.add newTreeI(nkIdentDefs, letSection.info,
+        newSymNode(hoistedVarSym),
+        newNodeI(nkEmpty, letSection.info),
+        call[paramPos])
+
+      call[paramPos] = newSymNode(hoistedVarSym) # Refer the original arg to its hoisted sym
+
+    # arg we refer to is a sym, wether introduced by hoisting or not doesn't matter, we simply reuse it
+    defExpr = call[paramPos]
+  else:
+    for i in 0..<defExpr.safeLen:
+      hoistParamsUsedInDefault(c, call, letSection, defExpr[i])
+
 proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode =
   when defined(nimCompilerStackraceHints):
     setFrameMsg c.config$n.info & " " & $n.kind
@@ -2707,6 +2743,15 @@ proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode =
       result = semDirectOp(c, n, flags)
     else:
       result = semIndirectOp(c, n, flags)
+
+    if nfDefaultRefsParam in result.flags:
+      result = result.copyTree #XXX: Figure out what causes default param nodes to be shared.. (sigmatch bug?)
+      # We've found a default value that references another param.
+      # See the notes in `hoistParamsUsedInDefault` for more details.
+      var hoistedParams = newNodeI(nkLetSection, result.info)
+      for i in 1..<result.len:
+        hoistParamsUsedInDefault(c, result, hoistedParams, result[i])
+      result = newTreeIT(nkStmtListExpr, result.info, result.typ, hoistedParams, result)
   of nkWhen:
     if efWantStmt in flags:
       result = semWhen(c, n, true)
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index dc833ce34..f745f9075 100644
--- a/compiler/sempass2.nim
+++ b/compiler/sempass2.nim
@@ -1281,7 +1281,7 @@ proc trackStmt*(c: PContext; module: PSym; n: PNode, isTopLevel: bool) =
                 nkTypeSection, nkConverterDef, nkMethodDef, nkIteratorDef}:
     return
   let g = c.graph
-  var effects = newNode(nkEffectList, n.info)
+  var effects = newNodeI(nkEffectList, n.info)
   var t: TEffects
   initEffects(g, effects, module, t, c)
   t.isTopLevel = isTopLevel
diff --git a/compiler/semtypinst.nim b/compiler/semtypinst.nim
index 5fa3880af..68ba93ebc 100644
--- a/compiler/semtypinst.nim
+++ b/compiler/semtypinst.nim
@@ -207,7 +207,7 @@ proc replaceTypeVarsN(cl: var TReplTypeVars, n: PNode; start=0): PNode =
     result.sym = replaceTypeVarsS(cl, n.sym)
     if result.sym.typ.kind == tyVoid:
       # don't add the 'void' field
-      result = newNode(nkRecList, n.info)
+      result = newNodeI(nkRecList, n.info)
   of nkRecWhen:
     var branch: PNode = nil              # the branch to take
     for i in 0..<n.len:
diff --git a/compiler/transf.nim b/compiler/transf.nim
index 3333acbf1..8ff0664da 100644
--- a/compiler/transf.nim
+++ b/compiler/transf.nim
@@ -886,43 +886,6 @@ proc commonOptimizations*(g: ModuleGraph; c: PSym, n: PNode): PNode =
     else:
       result = n
 
-proc hoistParamsUsedInDefault(c: PTransf, call, letSection, defExpr: PNode): PNode =
-  # This takes care of complicated signatures such as:
-  # proc foo(a: int, b = a)
-  # proc bar(a: int, b: int, c = a + b)
-  #
-  # The recursion may confuse you. It performs two duties:
-  #
-  # 1) extracting all referenced params from default expressions
-  #    into a let section preceding the call
-  #
-  # 2) replacing the "references" within the default expression
-  #    with these extracted skLet symbols.
-  #
-  # The first duty is carried out directly in the code here, while the second
-  # duty is activated by returning a non-nil value. The caller is responsible
-  # for replacing the input to the function with the returned non-nil value.
-  # (which is the hoisted symbol)
-  if defExpr.kind == nkSym:
-    if defExpr.sym.kind == skParam and defExpr.sym.owner == call[0].sym:
-      let paramPos = defExpr.sym.position + 1
-
-      if call[paramPos].kind == nkSym and sfHoisted in call[paramPos].sym.flags:
-        # Already hoisted, we still need to return it in order to replace the
-        # placeholder expression in the default value.
-        return call[paramPos]
-
-      let hoistedVarSym = hoistExpr(letSection,
-                                    call[paramPos],
-                                    getIdent(c.graph.cache, genPrefix),
-                                    c.transCon.owner).newSymNode
-      call[paramPos] = hoistedVarSym
-      return hoistedVarSym
-  else:
-    for i in 0..<defExpr.safeLen:
-      let hoisted = hoistParamsUsedInDefault(c, call, letSection, defExpr[i])
-      if hoisted != nil: defExpr[i] = hoisted
-
 proc transform(c: PTransf, n: PNode): PNode =
   when false:
     var oldDeferAnchor: PNode
@@ -989,16 +952,6 @@ proc transform(c: PTransf, n: PNode): PNode =
   of nkBreakStmt: result = transformBreak(c, n)
   of nkCallKinds:
     result = transformCall(c, n)
-    var call = result
-    if nfDefaultRefsParam in call.flags:
-      # We've found a default value that references another param.
-      # See the notes in `hoistParamsUsedInDefault` for more details.
-      var hoistedParams = newNodeI(nkLetSection, call.info, 0)
-      for i in 1..<call.len:
-        let hoisted = hoistParamsUsedInDefault(c, call, hoistedParams, call[i])
-        if hoisted != nil: call[i] = hoisted
-      result = newTree(nkStmtListExpr, hoistedParams, call)
-      result.typ = call.typ
   of nkAddr, nkHiddenAddr:
     result = transformAddrDeref(c, n, nkDerefExpr, nkHiddenDeref)
   of nkDerefExpr, nkHiddenDeref:
diff --git a/compiler/vmdeps.nim b/compiler/vmdeps.nim
index 4ba7b23bf..37e7e4b38 100644
--- a/compiler/vmdeps.nim
+++ b/compiler/vmdeps.nim
@@ -19,8 +19,7 @@ proc opSlurp*(file: string, info: TLineInfo, module: PSym; conf: ConfigRef): str
     # we produce a fake include statement for every slurped filename, so that
     # the module dependencies are accurate:
     discard conf.fileInfoIdx(AbsoluteFile filename)
-    appendToModule(module, newNode(nkIncludeStmt, info, @[
-      newStrNode(nkStrLit, filename)]))
+    appendToModule(module, newTreeI(nkIncludeStmt, info, newStrNode(nkStrLit, filename)))
   except IOError:
     localError(conf, info, "cannot open file: " & file)
     result = ""
diff --git a/tests/defaultprocparam/tdefaultprocparam.nim b/tests/defaultprocparam/tdefaultprocparam.nim
index 5f8c1adab..0d62cc955 100644
--- a/tests/defaultprocparam/tdefaultprocparam.nim
+++ b/tests/defaultprocparam/tdefaultprocparam.nim
@@ -1,8 +1,83 @@
 discard """
 output: '''
 hi
+hi
+topLevel|topLevel|
+topLevel2|topLevel2|
+inProc|inProc|
+inProc2|inProc2|
+topLevel|9
+topLevel2|10
+inProc|7
+inProc2|8
+must have been the wind..
+I'm there
+must have been the wind..
+I'm there
+symbol'a'symbol'a'
+symbol'b'symbol'b'
+symbol'a'symbol'b'
+symbol'a'9
+symbol'b'9
+symbol'a'0
 '''
 """
 import mdefaultprocparam
 
 p()
+
+proc testP =
+  p()
+
+testP()
+
+proc p2(s: string, count = s): string = s & count
+
+proc testP2 =
+  echo p2 """inProc|"""
+  echo p2 """inProc2|"""
+
+echo p2 """topLevel|"""
+echo p2 """topLevel2|"""
+
+testP2()
+
+import macros
+macro dTT(a: typed) = echo a.treeRepr
+
+proc p3(s: string, count = len(s)): string = s & $count
+
+proc testP3 =
+  echo p3 """inProc|"""
+  echo p3 """inProc2|"""
+
+echo p3 """topLevel|"""
+echo p3 """topLevel2|"""
+
+testP3()
+
+proc cut(s: string, c = len(s)): string =
+  s[0..<s.len-c]
+
+echo "must have been the wind.." & cut "I'm gone"
+echo cut("I'm gone", 4) & "there"
+
+proc testCut =
+  echo "must have been the wind.." & cut "I'm gone"
+  echo cut("I'm gone", 4) & "there"
+
+testCut()
+
+var a = "symbol'a'"
+var b = "symbol'b'"
+
+block:
+  echo p2(a)
+block:
+  echo p2(b)
+block:
+  echo p2(a, b)
+block:
+  echo p3(a)
+  echo p3(b)
+  echo p3(a, 0)