opensym as node kind + fixed experimental switch (#23892)

refs https://github.com/nim-lang/Nim/pull/23873#discussion_r1687995060,
fixes #23386, fixes #23385, supersedes #23572

Turns the `nfOpenSym` node flag implemented in #23091 and extended in
#23102 and #23873, into a node kind `nkOpenSym` that forms a unary node
containing either `nkSym` or `nkOpenSymChoice`. Since this affects
macros working on generic proc AST, the node kind is now only generated
when the experimental switch `genericsOpenSym` is enabled, and a new
node flag `nfDisabledOpenSym` is set to the `nkSym` or `nkOpenSymChoice`
when the switch is not enabled so that we can give a warning.

Now that the experimental switch has more reasonable semantics, we
define `nimHasGenericsOpenSym2`.

changelog.md

@@ -78,7 +78,9 @@ is often an easy workaround.
 - An experimental option `genericsOpenSym` has been added to allow captured
   symbols in generic routine bodies to be replaced by symbols injected locally
   by templates/macros at instantiation time. `bind` may be used to keep the
-  captured symbols over the injected ones regardless of enabling the option.
+  captured symbols over the injected ones regardless of enabling the option,
+  but other methods like renaming the captured symbols should be used instead
+  so that the code is not affected by context changes.
 
   Since this change may affect runtime behavior, the experimental switch
   `genericsOpenSym` needs to be enabled, and a warning is given in the case
@@ -110,6 +112,13 @@ is often an easy workaround.
   assert baz[int]() == "captured"
   ```
 
+  This option also generates a new node kind `nnkOpenSym` which contains
+  exactly 1 of either an `nnkSym` or an `nnkOpenSymChoice` node. In the future
+  this might be merged with a slightly modified `nnkOpenSymChoice` node but
+  macros that want to support the experimental feature should still handle
+  `nnkOpenSym`, as the node kind would simply not be generated as opposed to
+  being removed.
+
 ## Compiler changes
 
 - `--nimcache` using a relative path as the argument in a config file is now relative to the config file instead of the current directory.

compiler/ast.nim

@@ -325,9 +325,9 @@ type
     nfFirstWrite # this node is a first write
     nfHasComment # node has a comment
     nfSkipFieldChecking # node skips field visable checking
-    nfOpenSym # node is a captured sym but can be overriden by local symbols
-              # ideally a unary node containing nkSym/nkOpenSymChoice or an
-              # extension over nkOpenSymChoice
+    nfDisabledOpenSym # temporary: node should be nkOpenSym but cannot
+                      # because genericsOpenSym experimental switch is disabled
+                      # gives warning instead
 
   TNodeFlags* = set[TNodeFlag]
   TTypeFlag* = enum   # keep below 32 for efficiency reasons (now: 47)
@@ -882,7 +882,7 @@ const
                                       nfFromTemplate, nfDefaultRefsParam,
                                       nfExecuteOnReload, nfLastRead,
                                       nfFirstWrite, nfSkipFieldChecking,
-                                      nfOpenSym}
+                                      nfDisabledOpenSym}
   namePos* = 0
   patternPos* = 1    # empty except for term rewriting macros
   genericParamsPos* = 2
@@ -924,6 +924,7 @@ proc getPIdent*(a: PNode): PIdent {.inline.} =
   of nkSym: a.sym.name
   of nkIdent: a.ident
   of nkOpenSymChoice, nkClosedSymChoice: a.sons[0].sym.name
+  of nkOpenSym: getPIdent(a.sons[0])
   else: nil
 
 const

compiler/condsyms.nim

@@ -166,4 +166,5 @@ proc initDefines*(symbols: StringTableRef) =
   defineSymbol("nimHasWarnStdPrefix")
 
   defineSymbol("nimHasVtables")
+  defineSymbol("nimHasGenericsOpenSym2")
   defineSymbol("nimHasJsNoLambdaLifting")

compiler/docgen.nim

@@ -831,7 +831,7 @@ proc getName(n: PNode): string =
     result = "`"
     for i in 0..<n.len: result.add(getName(n[i]))
     result = "`"
-  of nkOpenSymChoice, nkClosedSymChoice:
+  of nkOpenSymChoice, nkClosedSymChoice, nkOpenSym:
     result = getName(n[0])
   else:
     result = ""
@@ -849,7 +849,7 @@ proc getNameIdent(cache: IdentCache; n: PNode): PIdent =
     var r = ""
     for i in 0..<n.len: r.add(getNameIdent(cache, n[i]).s)
     result = getIdent(cache, r)
-  of nkOpenSymChoice, nkClosedSymChoice:
+  of nkOpenSymChoice, nkClosedSymChoice, nkOpenSym:
     result = getNameIdent(cache, n[0])
   else:
     result = nil
@@ -863,7 +863,7 @@ proc getRstName(n: PNode): PRstNode =
   of nkAccQuoted:
     result = getRstName(n[0])
     for i in 1..<n.len: result.text.add(getRstName(n[i]).text)
-  of nkOpenSymChoice, nkClosedSymChoice:
+  of nkOpenSymChoice, nkClosedSymChoice, nkOpenSym:
     result = getRstName(n[0])
   else:
     result = nil

compiler/ic/ic.nim

@@ -837,7 +837,7 @@ proc loadNodes*(c: var PackedDecoder; g: var PackedModuleGraph; thisModule: int;
     result.ident = getIdent(c.cache, g[thisModule].fromDisk.strings[n.litId])
   of nkSym:
     result.sym = loadSym(c, g, thisModule, PackedItemId(module: LitId(0), item: tree[n].soperand))
-    if result.typ == nil and nfOpenSym notin result.flags:
+    if result.typ == nil:
       result.typ = result.sym.typ
   of externIntLit:
     result.intVal = g[thisModule].fromDisk.numbers[n.litId]
@@ -851,7 +851,7 @@ proc loadNodes*(c: var PackedDecoder; g: var PackedModuleGraph; thisModule: int;
     assert n2.kind == nkNone
     transitionNoneToSym(result)
     result.sym = loadSym(c, g, thisModule, PackedItemId(module: n1.litId, item: tree[n2].soperand))
-    if result.typ == nil and nfOpenSym notin result.flags:
+    if result.typ == nil:
       result.typ = result.sym.typ
   else:
     for n0 in sonsReadonly(tree, n):

compiler/lookups.nim

@@ -63,6 +63,8 @@ proc considerQuotedIdent*(c: PContext; n: PNode, origin: PNode = nil): PIdent =
       result = n[0].sym.name
     else:
       handleError(n, origin)
+  of nkOpenSym:
+    result = considerQuotedIdent(c, n[0], origin)
   else:
     handleError(n, origin)
 
@@ -701,6 +703,10 @@ proc qualifiedLookUp*(c: PContext, n: PNode, flags: set[TLookupFlag]): PSym =
     if result != nil and result.kind == skStub: loadStub(result)
 
 proc initOverloadIter*(o: var TOverloadIter, c: PContext, n: PNode): PSym =
+  if n.kind == nkOpenSym:
+    # maybe the logic in semexprs should be mirrored here instead
+    # for now it only seems this is called for `pickSym` in `getTypeIdent` 
+    return initOverloadIter(o, c, n[0])
   o.importIdx = -1
   o.marked = initIntSet()
   case n.kind

compiler/nodekinds.nim

@@ -204,6 +204,7 @@ type
     nkModuleRef           # for .rod file support: A (moduleId, itemId) pair
     nkReplayAction        # for .rod file support: A replay action
     nkNilRodNode          # for .rod file support: a 'nil' PNode
+    nkOpenSym             # container for captured sym that can be overriden by local symbols
 
 const
   nkCallKinds* = {nkCall, nkInfix, nkPrefix, nkPostfix,

compiler/options.nim

@@ -226,7 +226,7 @@ type
     strictDefs,
     strictCaseObjects,
     inferGenericTypes,
-    genericsOpenSym,
+    genericsOpenSym, # remove nfDisabledOpenSym when this switch is default
     vtables
 
   LegacyFeature* = enum

compiler/renderer.nim

@@ -514,6 +514,7 @@ proc lsub(g: TSrcGen; n: PNode): int =
     result = if n.len > 0: lcomma(g, n) + 2 else: len("{:}")
   of nkClosedSymChoice, nkOpenSymChoice:
     if n.len > 0: result += lsub(g, n[0])
+  of nkOpenSym: result = lsub(g, n[0])
   of nkTupleTy: result = lcomma(g, n) + len("tuple[]")
   of nkTupleClassTy: result = len("tuple")
   of nkDotExpr: result = lsons(g, n) + 1
@@ -1013,7 +1014,7 @@ proc bracketKind*(g: TSrcGen, n: PNode): BracketKind =
 proc skipHiddenNodes(n: PNode): PNode =
   result = n
   while result != nil:
-    if result.kind in {nkHiddenStdConv, nkHiddenSubConv, nkHiddenCallConv} and result.len > 1:
+    if result.kind in {nkHiddenStdConv, nkHiddenSubConv, nkHiddenCallConv, nkOpenSym} and result.len > 1:
       result = result[1]
     elif result.kind in {nkCheckedFieldExpr, nkHiddenAddr, nkHiddenDeref, nkStringToCString, nkCStringToString} and
         result.len > 0:
@@ -1275,6 +1276,7 @@ proc gsub(g: var TSrcGen, n: PNode, c: TContext, fromStmtList = false) =
       put(g, tkParRi, if n.kind == nkOpenSymChoice: "|...)" else: ")")
     else:
       gsub(g, n, 0)
+  of nkOpenSym: gsub(g, n, 0)
   of nkPar, nkClosure:
     put(g, tkParLe, "(")
     gcomma(g, n, c)

compiler/semexprs.nim

@@ -157,13 +157,14 @@ proc semSymChoice(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType: P
   if result.kind == nkSym:
     result = semSym(c, result, result.sym, flags)
 
-proc semOpenSym(c: PContext, n: PNode, s: PSym, flags: TExprFlags, expectedType: PType): PNode =
-  ## sem a node marked `nfOpenSym`, that is, captured symbols that can be
+proc semOpenSym(c: PContext, n: PNode, flags: TExprFlags, expectedType: PType,
+                warnDisabled = false): PNode =
+  ## sem the child of an `nkOpenSym` node, that is, captured symbols that can be
   ## replaced by newly injected symbols in generics. `s` must be the captured
   ## symbol if the original node is an `nkSym` node; and `nil` if it is an
   ## `nkOpenSymChoice`, in which case only non-overloadable injected symbols
   ## will be considered.
-  result = nil
+  let isSym = n.kind == nkSym
   let ident = n.getPIdent
   assert ident != nil
   let id = newIdentNode(ident, n.info)
@@ -176,36 +177,41 @@ proc semOpenSym(c: PContext, n: PNode, s: PSym, flags: TExprFlags, expectedType:
   # but of the overloadable sym kinds, semExpr does not handle skModule, skMacro, skTemplate
   # as overloaded in the case where `nkIdent` finds them first
   if s2 != nil and not c.isAmbiguous and
-      ((s == nil and s2.kind notin OverloadableSyms-{skModule, skMacro, skTemplate}) or
-        (s != nil and s2 != s)):
+      ((isSym and s2 != n.sym) or
+        (not isSym and s2.kind notin OverloadableSyms-{skModule, skMacro, skTemplate})):
     # only consider symbols defined under current proc:
     var o = s2.owner
     while o != nil:
       if o == c.p.owner:
-        if genericsOpenSym in c.features:
+        if not warnDisabled:
           result = semExpr(c, id, flags, expectedType)
           return
         else:
           var msg =
             "a new symbol '" & ident.s & "' has been injected during " &
             "instantiation of " & c.p.owner.name.s & ", however "
-          if s == nil:
+          if isSym:
+            msg.add(
+              getSymRepr(c.config, n.sym) & " captured at " &
+              "the proc declaration will be used instead; " &
+              "either enable --experimental:genericsOpenSym to use the " &
+              "injected symbol or `bind` this captured symbol explicitly")
+          else:
             msg.add(
               "overloads of " & ident.s & " will be used instead; " &
               "either enable --experimental:genericsOpenSym to use the " &
               "injected symbol or `bind` this symbol explicitly")
-          else:
-            msg.add(
-              getSymRepr(c.config, s) & " captured at " &
-              "the proc declaration will be used instead; " &
-              "either enable --experimental:genericsOpenSym to use the " &
-              "injected symbol or `bind` this captured symbol explicitly")
           message(c.config, n.info, warnGenericsIgnoredInjection, msg)
           break
       o = o.owner
-  if s == nil:
-    # set symchoice node type back to None
-    n.typ = newTypeS(tyNone, c)
+  # nothing found
+  if not warnDisabled:
+    result = semExpr(c, n, flags, expectedType)
+  else:
+    result = nil
+    if not isSym:
+      # set symchoice node type back to None
+      n.typ = newTypeS(tyNone, c)
 
 proc inlineConst(c: PContext, n: PNode, s: PSym): PNode {.inline.} =
   result = copyTree(s.astdef)
@@ -2191,6 +2197,8 @@ proc lookUpForDeclared(c: PContext, n: PNode, onlyCurrentScope: bool): PSym =
     result = n.sym
   of nkOpenSymChoice, nkClosedSymChoice:
     result = n[0].sym
+  of nkOpenSym:
+    result = lookUpForDeclared(c, n[0], onlyCurrentScope)
   else:
     localError(c.config, n.info, "identifier expected, but got: " & renderTree(n))
     result = nil
@@ -2643,7 +2651,9 @@ proc semWhen(c: PContext, n: PNode, semCheck = true): PNode =
   var typ = commonTypeBegin
   if n.len in 1..2 and n[0].kind == nkElifBranch and (
       n.len == 1 or n[1].kind == nkElse):
-    let exprNode = n[0][0]
+    var exprNode = n[0][0]
+    if exprNode.kind == nkOpenSym:
+      exprNode = exprNode[0]
     if exprNode.kind == nkIdent:
       whenNimvm = lookUp(c, exprNode).magic == mNimvm
     elif exprNode.kind == nkSym:
@@ -3192,20 +3202,22 @@ proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType: PType
     if isSymChoice(result):
       result = semSymChoice(c, result, flags, expectedType)
   of nkClosedSymChoice, nkOpenSymChoice:
-    if n.kind == nkOpenSymChoice and nfOpenSym in n.flags:
-      result = semOpenSym(c, n, nil, flags, expectedType)
-      if result != nil:
-        return
+    if nfDisabledOpenSym in n.flags:
+      let res = semOpenSym(c, n, flags, expectedType, warnDisabled = true)
+      assert res == nil
     result = semSymChoice(c, n, flags, expectedType)
   of nkSym:
     let s = n.sym
-    if nfOpenSym in n.flags:
-      result = semOpenSym(c, n, s, flags, expectedType)
-      if result != nil:
-        return
+    if nfDisabledOpenSym in n.flags:
+      let res = semOpenSym(c, n, flags, expectedType, warnDisabled = true)
+      assert res == nil
     # because of the changed symbol binding, this does not mean that we
     # don't have to check the symbol for semantics here again!
     result = semSym(c, n, s, flags)
+  of nkOpenSym:
+    assert n.len == 1
+    let inner = n[0]
+    result = semOpenSym(c, inner, flags, expectedType)
   of nkEmpty, nkNone, nkCommentStmt, nkType:
     discard
   of nkNilLit:

compiler/semgnrc.nim

@@ -59,6 +59,9 @@ template isMixedIn(sym): bool =
 template canOpenSym(s): bool =
   {withinMixin, withinConcept} * flags == {withinMixin} and s.id notin ctx.toBind
 
+proc newOpenSym*(n: PNode): PNode {.inline.} =
+  result = newTreeI(nkOpenSym, n.info, n)
+
 proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
                           ctx: var GenericCtx; flags: TSemGenericFlags,
                           fromDotExpr=false): PNode =
@@ -73,8 +76,11 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
     else:
       result = symChoice(c, n, s, scOpen)
       if canOpenSym(s):
-        result.flags.incl nfOpenSym
-        result.typ = nil
+        if genericsOpenSym in c.features:
+          result = newOpenSym(result)
+        else:
+          result.flags.incl nfDisabledOpenSym
+          result.typ = nil
   case s.kind
   of skUnknown:
     # Introduced in this pass! Leave it as an identifier.
@@ -103,8 +109,11 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
     else:
       result = newSymNodeTypeDesc(s, c.idgen, n.info)
       if canOpenSym(result.sym):
-        result.flags.incl nfOpenSym
-        result.typ = nil
+        if genericsOpenSym in c.features:
+          result = newOpenSym(result)
+        else:
+          result.flags.incl nfDisabledOpenSym
+          result.typ = nil
     onUse(n.info, s)
   of skParam:
     result = n
@@ -114,16 +123,22 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
        (s.typ.flags * {tfGenericTypeParam, tfImplicitTypeParam} == {}):
       result = newSymNodeTypeDesc(s, c.idgen, n.info)
       if canOpenSym(result.sym):
-        result.flags.incl nfOpenSym
-        result.typ = nil
+        if genericsOpenSym in c.features:
+          result = newOpenSym(result)
+        else:
+          result.flags.incl nfDisabledOpenSym
+          result.typ = nil
     else:
       result = n
     onUse(n.info, s)
   else:
     result = newSymNode(s, n.info)
     if canOpenSym(result.sym):
-      result.flags.incl nfOpenSym
-      result.typ = nil
+      if genericsOpenSym in c.features:
+        result = newOpenSym(result)
+      else:
+        result.flags.incl nfDisabledOpenSym
+        result.typ = nil
     onUse(n.info, s)
 
 proc lookup(c: PContext, n: PNode, flags: TSemGenericFlags,

compiler/semtypes.nim

@@ -2207,6 +2207,7 @@ proc semTypeNode(c: PContext, n: PNode, prev: PType): PType =
   of nkType: result = n.typ
   of nkStmtListType: result = semStmtListType(c, n, prev)
   of nkBlockType: result = semBlockType(c, n, prev)
+  of nkOpenSym: result = semTypeNode(c, n[0], prev)
   else:
     result = semTypeExpr(c, n, prev)
     when false:

doc/manual_experimental.md

@@ -2528,7 +2528,9 @@ Injected symbols in generic procs
 With the experimental option `genericsOpenSym`, captured symbols in generic
 routine bodies may be replaced by symbols injected locally by templates/macros
 at instantiation time. `bind` may be used to keep the captured symbols over
-the injected ones regardless of enabling the option.
+the injected ones regardless of enabling the option, but other methods like
+renaming the captured symbols should be used instead so that the code is not
+affected by context changes.
   
 Since this change may affect runtime behavior, the experimental switch
 `genericsOpenSym` needs to be enabled, and a warning is given in the case
@@ -2560,6 +2562,13 @@ proc baz[T](): string =
 assert baz[int]() == "captured"
 ```
 
+This option also generates a new node kind `nnkOpenSym` which contains
+exactly 1 of either an `nnkSym` or an `nnkOpenSymChoice` node. In the future
+this might be merged with a slightly modified `nnkOpenSymChoice` node but
+macros that want to support the experimental feature should still handle
+`nnkOpenSym`, as the node kind would simply not be generated as opposed to
+being removed.
+
 
 VTable for methods
 ==================

lib/core/macros.nim

@@ -93,6 +93,8 @@ type
     nnkFuncDef,
     nnkTupleConstr,
     nnkError,  ## erroneous AST node
+    nnkModuleRef, nnkReplayAction, nnkNilRodNode ## internal IC nodes
+    nnkOpenSym
 
   NimNodeKinds* = set[NimNodeKind]
   NimTypeKind* = enum  # some types are no longer used, see ast.nim
@@ -1407,7 +1409,7 @@ proc `$`*(node: NimNode): string =
     result = node.basename.strVal & "*"
   of nnkStrLit..nnkTripleStrLit, nnkCommentStmt, nnkSym, nnkIdent:
     result = node.strVal
-  of nnkOpenSymChoice, nnkClosedSymChoice:
+  of nnkOpenSymChoice, nnkClosedSymChoice, nnkOpenSym:
     result = $node[0]
   of nnkAccQuoted:
     result = ""

lib/pure/sugar.nim

@@ -345,7 +345,7 @@ proc collectImpl(init, body: NimNode): NimNode {.since: (1, 1).} =
   let res = genSym(nskVar, "collectResult")
   var bracketExpr: NimNode
   if init != nil:
-    expectKind init, {nnkCall, nnkIdent, nnkSym, nnkClosedSymChoice, nnkOpenSymChoice}
+    expectKind init, {nnkCall, nnkIdent, nnkSym, nnkClosedSymChoice, nnkOpenSymChoice, nnkOpenSym}
     bracketExpr = newTree(nnkBracketExpr,
       if init.kind in {nnkCall, nnkClosedSymChoice, nnkOpenSymChoice}:
         freshIdentNodes(init[0]) else: freshIdentNodes(init))

tests/generics/mopensymimport1.nim

@@ -0,0 +1,34 @@
+type
+  Result*[T, E] = object
+    when T is void:
+      when E is void:
+        oResultPrivate*: bool
+      else:
+        case oResultPrivate*: bool
+        of false:
+          eResultPrivate*: E
+        of true:
+          discard
+    else:
+      when E is void:
+        case oResultPrivate*: bool
+        of false:
+          discard
+        of true:
+          vResultPrivate*: T
+      else:
+        case oResultPrivate*: bool
+        of false:
+          eResultPrivate*: E
+        of true:
+          vResultPrivate*: T
+
+template valueOr*[T: not void, E](self: Result[T, E], def: untyped): untyped =
+  let s = (self) # TODO avoid copy
+  case s.oResultPrivate
+  of true:
+    s.vResultPrivate
+  of false:
+    when E isnot void:
+      template error: untyped {.used, inject.} = s.eResultPrivate
+    def

tests/generics/mopensymimport2.nim

@@ -0,0 +1,16 @@
+{.experimental: "genericsOpenSym".}
+
+import mopensymimport1
+
+type Xxx = enum
+  error
+  value
+
+proc f(): Result[int, cstring] =
+  Result[int, cstring](oResultPrivate: false, eResultPrivate: "f")
+
+proc g*(T: type): string =
+  let x = f().valueOr:
+    return $error
+
+  "ok"

tests/generics/tmacroinjectedsymwarning.nim

@@ -46,6 +46,11 @@ proc f(): Result[int, cstring] =
 
 proc g(T: type): string =
   let x = f().valueOr:
+    {.push warningAsError[GenericsIgnoredInjection]: on.}
+    # test spurious error
+    discard true
+    let _ = f
+    {.pop.}
     return $error #[tt.Warning
             ^ a new symbol 'error' has been injected during instantiation of g, however 'error' [enumField declared in tmacroinjectedsymwarning.nim(6, 3)] captured at the proc declaration will be used instead; either enable --experimental:genericsOpenSym to use the injected symbol or `bind` this captured symbol explicitly [GenericsIgnoredInjection]]#
 

tests/generics/topensymimport.nim

@@ -0,0 +1,5 @@
+# issue #23386
+
+import mopensymimport2
+
+doAssert g(int) == "f"