Fix #5691 (#15158)

* Fix #5691
* Cleanup and thoughts
* Use scope approach
* Seperate defined/declared/declaredInScope magics
* Fix declaredInScope
* Update spec accordingly

compiler/ast.nim

@@ -617,7 +617,7 @@ var
 type
   TMagic* = enum # symbols that require compiler magic:
     mNone,
-    mDefined, mDefinedInScope, mCompiles, mArrGet, mArrPut, mAsgn,
+    mDefined, mDeclared, mDeclaredInScope, mCompiles, mArrGet, mArrPut, mAsgn,
     mLow, mHigh, mSizeOf, mAlignOf, mOffsetOf, mTypeTrait,
     mIs, mOf, mAddr, mType, mTypeOf,
     mPlugin, mEcho, mShallowCopy, mSlurp, mStaticExec, mStatic,

compiler/condsyms.nim

@@ -119,3 +119,4 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasStacktraceMsgs")
   defineSymbol("nimDoesntTrackDefects")
   defineSymbol("nimHasLentIterators")
+  defineSymbol("nimHasDeclaredMagic")

compiler/lookups.nim

@@ -93,6 +93,11 @@ proc skipAlias*(s: PSym; n: PNode; conf: ConfigRef): PSym =
 
 proc localSearchInScope*(c: PContext, s: PIdent): PSym =
   result = strTableGet(c.currentScope.symbols, s)
+  var shadow = c.currentScope
+  while result == nil and shadow.parent != nil and shadow.depthLevel == shadow.parent.depthLevel:
+    # We are in a shadow scope, check in the parent too
+    result = strTableGet(shadow.parent.symbols, s)
+    shadow = shadow.parent
 
 proc searchInScopes*(c: PContext, s: PIdent): PSym =
   for scope in walkScopes(c.currentScope):
@@ -231,6 +236,23 @@ proc addInterfaceOverloadableSymAt*(c: PContext, scope: PScope, sym: PSym) =
   addOverloadableSymAt(c, scope, sym)
   addInterfaceDeclAux(c, sym)
 
+proc openShadowScope*(c: PContext) =
+  c.currentScope = PScope(parent: c.currentScope,
+                          symbols: newStrTable(),
+                          depthLevel: c.scopeDepth)
+
+proc closeShadowScope*(c: PContext) =
+  c.closeScope
+
+proc mergeShadowScope*(c: PContext) =
+  let shadowScope = c.currentScope
+  c.rawCloseScope
+  for sym in shadowScope.symbols:
+    if sym.kind in OverloadableSyms:
+      c.addInterfaceOverloadableSymAt(c.currentScope, sym)
+    else:
+      c.addInterfaceDecl(sym)
+
 when defined(nimfix):
   # when we cannot find the identifier, retry with a changed identifier:
   proc altSpelling(x: PIdent): PIdent =

compiler/sem.nim

@@ -383,12 +383,10 @@ when not defined(nimHasSinkInference):
 
 include hlo, seminst, semcall
 
-when false:
-  # hopefully not required:
-  proc resetSemFlag(n: PNode) =
-    excl n.flags, nfSem
-    for i in 0..<n.safeLen:
-      resetSemFlag(n[i])
+proc resetSemFlag(n: PNode) =
+  excl n.flags, nfSem
+  for i in 0..<n.safeLen:
+    resetSemFlag(n[i])
 
 proc semAfterMacroCall(c: PContext, call, macroResult: PNode,
                        s: PSym, flags: TExprFlags): PNode =
@@ -403,8 +401,7 @@ proc semAfterMacroCall(c: PContext, call, macroResult: PNode,
   c.friendModules.add(s.owner.getModule)
   idSynchronizationPoint(5000)
   result = macroResult
-  excl(result.flags, nfSem)
-  #resetSemFlag n
+  resetSemFlag result
   if s.typ[0] == nil:
     result = semStmt(c, result, flags)
   else:

compiler/semcall.nim

@@ -94,10 +94,6 @@ proc pickBestCandidate(c: PContext, headSymbol: PNode,
     if c.currentScope.symbols.counter == counterInitial or syms.len != 0:
       matches(c, n, orig, z)
       if z.state == csMatch:
-        #if sym.name.s == "==" and (n.info ?? "temp3"):
-        #  echo typeToString(sym.typ)
-        #  writeMatches(z)
-
         # little hack so that iterators are preferred over everything else:
         if sym.kind == skIterator: inc(z.exactMatches, 200)
         case best.state

compiler/semexprs.nim

@@ -1854,29 +1854,32 @@ proc semYield(c: PContext, n: PNode): PNode =
   elif c.p.owner.typ[0] != nil:
     localError(c.config, n.info, errGenerated, "yield statement must yield a value")
 
-proc lookUpForDefined(c: PContext, i: PIdent, onlyCurrentScope: bool): PSym =
-  if onlyCurrentScope:
-    result = localSearchInScope(c, i)
-  else:
-    result = searchInScopes(c, i) # no need for stub loading
+proc semDefined(c: PContext, n: PNode): PNode =
+  checkSonsLen(n, 2, c.config)
+  # we replace this node by a 'true' or 'false' node:
+  result = newIntNode(nkIntLit, 0)
+  result.intVal = ord isDefined(c.config, considerQuotedIdent(c, n[1], n).s)
+  result.info = n.info
+  result.typ = getSysType(c.graph, n.info, tyBool)
 
-proc lookUpForDefined(c: PContext, n: PNode, onlyCurrentScope: bool): PSym =
+proc lookUpForDeclared(c: PContext, n: PNode, onlyCurrentScope: bool): PSym =
   case n.kind
-  of nkIdent:
-    result = lookUpForDefined(c, n.ident, onlyCurrentScope)
+  of nkIdent, nkAccQuoted:
+    result = if onlyCurrentScope:
+               localSearchInScope(c, considerQuotedIdent(c, n))
+             else:
+               searchInScopes(c, considerQuotedIdent(c, n))
   of nkDotExpr:
     result = nil
     if onlyCurrentScope: return
     checkSonsLen(n, 2, c.config)
-    var m = lookUpForDefined(c, n[0], onlyCurrentScope)
+    var m = lookUpForDeclared(c, n[0], onlyCurrentScope)
     if m != nil and m.kind == skModule:
       let ident = considerQuotedIdent(c, n[1], n)
       if m == c.module:
         result = strTableGet(c.topLevelScope.symbols, ident)
       else:
         result = strTableGet(m.tab, ident)
-  of nkAccQuoted:
-    result = lookUpForDefined(c, considerQuotedIdent(c, n), onlyCurrentScope)
   of nkSym:
     result = n.sym
   of nkOpenSymChoice, nkClosedSymChoice:
@@ -1885,15 +1888,11 @@ proc lookUpForDefined(c: PContext, n: PNode, onlyCurrentScope: bool): PSym =
     localError(c.config, n.info, "identifier expected, but got: " & renderTree(n))
     result = nil
 
-proc semDefined(c: PContext, n: PNode, onlyCurrentScope: bool): PNode =
+proc semDeclared(c: PContext, n: PNode, onlyCurrentScope: bool): PNode =
   checkSonsLen(n, 2, c.config)
   # we replace this node by a 'true' or 'false' node:
   result = newIntNode(nkIntLit, 0)
-  if not onlyCurrentScope and considerQuotedIdent(c, n[0], n).s == "defined":
-    let d = considerQuotedIdent(c, n[1], n)
-    result.intVal = ord isDefined(c.config, d.s)
-  elif lookUpForDefined(c, n[1], onlyCurrentScope) != nil:
-    result.intVal = 1
+  result.intVal = ord lookUpForDeclared(c, n[1], onlyCurrentScope) != nil
   result.info = n.info
   result.typ = getSysType(c.graph, n.info, tyBool)
 
@@ -2187,10 +2186,13 @@ proc semMagic(c: PContext, n: PNode, s: PSym, flags: TExprFlags): PNode =
     result = semTypeOf(c, n)
   of mDefined:
     markUsed(c, n.info, s)
-    result = semDefined(c, setMs(n, s), false)
-  of mDefinedInScope:
+    result = semDefined(c, setMs(n, s))
+  of mDeclared:
     markUsed(c, n.info, s)
-    result = semDefined(c, setMs(n, s), true)
+    result = semDeclared(c, setMs(n, s), false)
+  of mDeclaredInScope:
+    markUsed(c, n.info, s)
+    result = semDeclared(c, setMs(n, s), true)
   of mCompiles:
     markUsed(c, n.info, s)
     result = semCompiles(c, setMs(n, s), flags)

compiler/semgnrc.nim

@@ -228,7 +228,7 @@ proc semGenericStmt(c: PContext, n: PNode,
     var mixinContext = false
     if s != nil:
       incl(s.flags, sfUsed)
-      mixinContext = s.magic in {mDefined, mDefinedInScope, mCompiles, mAstToStr}
+      mixinContext = s.magic in {mDefined, mDeclared, mDeclaredInScope, mCompiles, mAstToStr}
       let whichChoice = if s.id in ctx.toBind: scClosed
                         elif s.isMixedIn: scForceOpen
                         else: scOpen

compiler/semstmts.nim

@@ -1834,7 +1834,7 @@ proc semProcAux(c: PContext, n: PNode, kind: TSymKind,
   result = semProcAnnotation(c, n, validPragmas)
   if result != nil: return result
   result = n
-  checkSonsLen(n, bodyPos + 1, c.config)
+  checkMinSonsLen(n, bodyPos + 1, c.config)
   var s: PSym
   var typeIsDetermined = false
   var isAnon = false

compiler/sigmatch.nim

@@ -2324,9 +2324,11 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
     formal: PSym # current routine parameter
 
   template noMatch() =
+    c.mergeShadowScope #merge so that we don't have to resem for later overloads
     m.state = csNoMatch
     m.firstMismatch.arg = a
     m.firstMismatch.formal = formal
+    return
 
   template checkConstraint(n: untyped) {.dirty.} =
     if not formal.constraint.isNil:
@@ -2335,7 +2337,6 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
         inc(m.genericMatches, 100)
       else:
         noMatch()
-        return
 
     if formal.typ.kind in {tyVar}:
       let argConverter = if arg.kind == nkHiddenDeref: arg[0] else: arg
@@ -2343,11 +2344,9 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
         if argConverter.typ.kind notin {tyVar}:
           m.firstMismatch.kind = kVarNeeded
           noMatch()
-          return
       elif not n.isLValue:
         m.firstMismatch.kind = kVarNeeded
         noMatch()
-        return
 
   m.state = csMatch # until proven otherwise
   m.firstMismatch = MismatchInfo()
@@ -2359,6 +2358,9 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
   formal = if formalLen > 1: m.callee.n[1].sym else: nil
 
   while a < n.len:
+
+    c.openShadowScope
+
     if a >= formalLen-1 and f < formalLen and m.callee.n[f].typ.isVarargsUntyped:
       formal = m.callee.n[f].sym
       incl(marker, formal.position)
@@ -2383,12 +2385,10 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
       if n[a][0].kind != nkIdent:
         localError(c.config, n[a].info, "named parameter has to be an identifier")
         noMatch()
-        return
       formal = getNamedParamFromList(m.callee.n, n[a][0].ident)
       if formal == nil:
         # no error message!
         noMatch()
-        return
       if containsOrIncl(marker, formal.position):
         m.firstMismatch.kind = kAlreadyGiven
         # already in namedParams, so no match
@@ -2397,7 +2397,6 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
         # different parameter names could match later on):
         when false: localError(n[a].info, errCannotBindXTwice, formal.name.s)
         noMatch()
-        return
       m.baseTypeMatch = false
       m.typedescMatched = false
       n[a][1] = prepareOperand(c, formal.typ, n[a][1])
@@ -2407,7 +2406,6 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
       m.firstMismatch.kind = kTypeMismatch
       if arg == nil:
         noMatch()
-        return
       checkConstraint(n[a][1])
       if m.baseTypeMatch:
         #assert(container == nil)
@@ -2448,16 +2446,13 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
             checkConstraint(n[a])
           else:
             noMatch()
-            return
         else:
           m.firstMismatch.kind = kExtraArg
           noMatch()
-          return
       else:
         if m.callee.n[f].kind != nkSym:
           internalError(c.config, n[a].info, "matches")
           noMatch()
-          return
         formal = m.callee.n[f].sym
         m.firstMismatch.kind = kTypeMismatch
         if containsOrIncl(marker, formal.position) and container.isNil:
@@ -2465,7 +2460,6 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
           # positional param already in namedParams: (see above remark)
           when false: localError(n[a].info, errCannotBindXTwice, formal.name.s)
           noMatch()
-          return
 
         if formal.typ.isVarargsUntyped:
           if container.isNil:
@@ -2482,7 +2476,6 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
                                     n[a], nOrig[a])
           if arg == nil:
             noMatch()
-            return
           if m.baseTypeMatch:
             assert formal.typ.kind == tyVarargs
             #assert(container == nil)
@@ -2511,8 +2504,13 @@ proc matchesAux(c: PContext, n, nOrig: PNode,
             localError(c.config, n[a].info, "cannot convert $1 to $2" % [
               typeToString(n[a].typ), typeToString(formal.typ) ])
             noMatch()
-            return
         checkConstraint(n[a])
+
+    if m.state == csMatch and not(m.calleeSym != nil and m.calleeSym.kind in {skTemplate, skMacro}):
+      c.mergeShadowScope
+    else:
+      c.closeShadowScope
+
     inc(a)
   # for some edge cases (see tdont_return_unowned_from_owned test case)
   m.firstMismatch.arg = a
@@ -2544,7 +2542,7 @@ proc matches*(c: PContext, n, nOrig: PNode, m: var TCandidate) =
   if m.state == csNoMatch: return
   # check that every formal parameter got a value:
   for f in 1..<m.callee.n.len:
-    var formal = m.callee.n[f].sym
+    let formal = m.callee.n[f].sym
     if not containsOrIncl(marker, formal.position):
       if formal.ast == nil:
         if formal.typ.kind == tyVarargs:

doc/manual.rst

@@ -5019,23 +5019,16 @@ delayed until template instantiation time:
     :status: 1
 
   template t(body: typed) =
+    proc p = echo "hey"
     block:
       body
 
   t:
-    var i = 1
-    echo i
+    p()  # fails with 'undeclared identifier: p'
 
-  t:
-    var i = 2  # fails with 'attempt to redeclare i'
-    echo i
-
-The above code fails with the mysterious error message that ``i`` has already
-been declared. The reason for this is that the ``var i = ...`` bodies need to
-be type-checked before they are passed to the ``body`` parameter and type
-checking in Nim implies symbol lookups. For the symbol lookups to succeed
-``i`` needs to be added to the current (i.e. outer) scope. After type checking
-these additions to the symbol table are not rolled back (for better or worse).
+The above code fails with the error message that ``p`` is not declared.
+The reason for this is that the ``p()`` body is type-checked before getting
+passed to the ``body`` parameter and type checking in Nim implies symbol lookups.
 The same code works with ``untyped`` as the passed body is not required to be
 type-checked:
 
@@ -5043,16 +5036,12 @@ type-checked:
     :test: "nim c $1"
 
   template t(body: untyped) =
+    proc p = echo "hey"
     block:
       body
 
   t:
-    var i = 1
-    echo i
-
-  t:
-    var i = 2  # compiles
-    echo i
+    p()  # compiles
 
 
 Varargs of untyped

lib/system.nim

@@ -150,25 +150,30 @@ else:
   template runnableExamples*(doccmd = "", body: untyped) =
     discard
 
-proc declared*(x: untyped): bool {.magic: "Defined", noSideEffect, compileTime.}
-  ## Special compile-time procedure that checks whether `x` is
-  ## declared. `x` has to be an identifier or a qualified identifier.
-  ##
-  ## See also:
-  ## * `declaredInScope <#declaredInScope,untyped>`_
-  ##
-  ## This can be used to check whether a library provides a certain
-  ## feature or not:
-  ##
-  ## .. code-block:: Nim
-  ##   when not declared(strutils.toUpper):
-  ##     # provide our own toUpper proc here, because strutils is
-  ##     # missing it.
+when defined(nimHasDeclaredMagic):
+  proc declared*(x: untyped): bool {.magic: "Declared", noSideEffect, compileTime.}
+    ## Special compile-time procedure that checks whether `x` is
+    ## declared. `x` has to be an identifier or a qualified identifier.
+    ##
+    ## See also:
+    ## * `declaredInScope <#declaredInScope,untyped>`_
+    ##
+    ## This can be used to check whether a library provides a certain
+    ## feature or not:
+    ##
+    ## .. code-block:: Nim
+    ##   when not declared(strutils.toUpper):
+    ##     # provide our own toUpper proc here, because strutils is
+    ##     # missing it.
+else:
+  proc declared*(x: untyped): bool {.magic: "Defined", noSideEffect, compileTime.}
 
-proc declaredInScope*(x: untyped): bool {.
-  magic: "DefinedInScope", noSideEffect, compileTime.}
-  ## Special compile-time procedure that checks whether `x` is
-  ## declared in the current scope. `x` has to be an identifier.
+when defined(nimHasDeclaredMagic):
+  proc declaredInScope*(x: untyped): bool {.magic: "DeclaredInScope", noSideEffect, compileTime.}
+    ## Special compile-time procedure that checks whether `x` is
+    ## declared in the current scope. `x` has to be an identifier.
+else:
+  proc declaredInScope*(x: untyped): bool {.magic: "DefinedInScope", noSideEffect, compileTime.}
 
 proc `addr`*[T](x: var T): ptr T {.magic: "Addr", noSideEffect.} =
   ## Builtin `addr` operator for taking the address of a memory location.

tests/macros/tmacros_various.nim

@@ -22,6 +22,7 @@ a[1]: 45
 x: some string
 ([("key", "val"), ("keyB", "2")], [("val", "key"), ("2", "keyB")])
 ([("key", "val"), ("keyB", "2")], [("val", "key"), ("2", "keyB")])
+0
 '''
 """
 
@@ -222,3 +223,17 @@ block getImplTransformed:
   doAssert "toExpand" notin code
     # template is expanded (but that would already be the case with
     # `a.getImpl.repr`, unlike the other transformations mentioned above
+
+
+# test macro resemming
+macro makeVar(): untyped =
+  quote:
+    var tensorY {.inject.}: int
+
+macro noop(a: typed): untyped =
+  a
+
+noop:
+  makeVar
+echo tensorY
+

tests/macros/tmacrotypes.nim

@@ -43,6 +43,8 @@ macro checkType(ex: typed): untyped =
   echo ex.getTypeInst.repr, "; ", ex.typeKind, "; ", ex.getType.repr, "; ", ex.getTypeImpl.repr
 
 macro checkProcType(fn: typed): untyped =
+  if fn.kind == nnkProcDef:
+    result = fn
   let fn_sym = if fn.kind == nnkProcDef: fn[0] else: fn
   echo fn_sym, "; ", fn_sym.typeKind, "; ", fn_sym.getType.repr, "; ", fn_sym.getTypeImpl.repr
 

tests/template/tparams_gensymed.nim

@@ -336,3 +336,61 @@ test(hello):
 var data = 5
 
 hello(data)
+
+# bug #5691
+
+template bar(x: typed) = discard
+macro barry(x: typed) = discard
+
+var a = 0
+
+bar:
+  var a = 10
+
+barry:
+  var a = 20
+
+bar:
+  var b = 10
+
+barry:
+  var b = 20
+
+var b = 30
+
+# template bar(x: static int) = discard
+#You may think that this should work:
+# bar((var c = 1; echo "hey"; c))
+# echo c
+#But it must not! Since this would be incorrect:
+# bar((var b = 3; const c = 1; echo "hey"; c))
+# echo b # <- b wouldn't exist
+
+discard not (let xx = 1; true)
+discard xx
+
+template barrel(a: typed): untyped = a
+
+barrel:
+  var aa* = 1
+  var bb = 3
+  export bb
+
+# Test declaredInScope within params
+template test1: untyped =
+  when not declaredInScope(thing):
+    var thing {.inject.}: int
+
+proc chunkedReadLoop =
+  test1
+  test1
+
+template test2: untyped =
+  when not not not declaredInScope(thing):
+    var thing {.inject.}: int
+
+proc chunkedReadLoop2 =
+  test2
+  test2
+
+test1(); test2()

tests/template/tparamscope.nim

@@ -0,0 +1,10 @@
+discard """
+  errormsg: "undeclared identifier: 'a'"
+  line: 10
+"""
+
+
+template secondArg(a, b: typed): untyped =
+  b
+
+echo secondArg((var a = 1; 1), a)