make genericsOpenSym work at instantiation time, new behavior in `o…

…penSym` (#24111) alternative to #24101 #23892 changed the opensym experimental switch so that it has to be enabled in the context of the generic/template declarations capturing the symbols, not the context of the instantiation of the generics/templates. This was to be in line with where the compiler gives the warnings and changes behavior in a potentially breaking way. However `results` [depends on the old behavior](https://github.com/arnetheduck/nim-results/blob/71d404b314479a6205bfd050f4fe5fe49cdafc69/results.nim#L1428), so that the callers of the macros provided by results always take advantage of the opensym behavior. To accomodate this, we change the behavior of the old experimental option that `results` uses, `genericsOpenSym`, so that ignores the information of whether or not symbols are intentionally opened and always gives the opensym behavior as long as it's enabled at instantiation time. This should keep `results` working as is. However this differs from the normal opensym switch in that it doesn't generate `nnkOpenSym`. Before it was just a generics-only version of `openSym` along with `templateOpenSym` which was only for templates. So `templateOpenSym` is removed along with this change, but no one appears to have used it.
nim-lang · Sep 18, 2024 · 0c3573e · 0c3573e
1 parent 79b17b7
commit 0c3573e
Show file tree

Hide file tree

Showing 11 changed files with 125 additions and 28 deletions.
diff --git a/changelog.md b/changelog.md
@@ -128,8 +128,7 @@ is often an easy workaround.
   context changes.
 
   Since this change may affect runtime behavior, the experimental switch
-  `openSym`, or `genericsOpenSym` and `templateOpenSym` for only the respective
-  routines, needs to be enabled; and a warning is given in the case where an
+  `openSym` needs to be enabled; and a warning is given in the case where an
   injected symbol would replace a captured symbol not bound by `bind` and
   the experimental switch isn't enabled.
 
@@ -150,7 +149,7 @@ is often an easy workaround.
         value # warning: a new `value` has been injected, use `bind` or turn on `experimental:openSym`
   echo oldTempl() # "captured"
 
-  {.experimental: "openSym".} # or {.experimental: "genericsOpenSym".} for just generic procs
+  {.experimental: "openSym".}
 
   proc bar[T](): string =
     foo(123):
@@ -163,8 +162,6 @@ is often an easy workaround.
       return value
   assert baz[int]() == "captured"
 
-  # {.experimental: "templateOpenSym".} would be needed here if genericsOpenSym was used
-
   template barTempl(): string =
     block:
       foo(123):
@@ -185,6 +182,34 @@ is often an easy workaround.
   experimental feature should still handle `nnkOpenSym`, as the node kind would
   simply not be generated as opposed to being removed.
 
+  Another experimental switch `genericsOpenSym` exists that enables this behavior
+  at instantiation time, meaning templates etc can enable it specifically when
+  they are being called. However this does not generate `nnkOpenSym` nodes
+  (unless the other switch is enabled) and so doesn't reflect the regular
+  behavior of the switch.
+
+  ```nim
+  const value = "captured"
+  template foo(x: int, body: untyped): untyped =
+    let value {.inject.} = "injected"
+    {.push experimental: "genericsOpenSym".}
+    body
+    {.pop.}
+
+  proc bar[T](): string =
+    foo(123):
+      return value
+  echo bar[int]() # "injected"
+
+  template barTempl(): string =
+    block:
+      var res: string
+      foo(123):
+        res = value
+      res
+  assert barTempl() == "injected"
+  ```
+
 ## 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.

diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
@@ -167,4 +167,5 @@ proc initDefines*(symbols: StringTableRef) =
 
   defineSymbol("nimHasVtables")
   defineSymbol("nimHasGenericsOpenSym2")
+  defineSymbol("nimHasGenericsOpenSym3")
   defineSymbol("nimHasJsNoLambdaLifting")
diff --git a/compiler/options.nim b/compiler/options.nim
@@ -226,8 +226,8 @@ type
     strictCaseObjects,
     inferGenericTypes,
     openSym, # remove nfDisabledOpenSym when this is default
-    # separated alternatives to above:
-    genericsOpenSym, templateOpenSym,
+    # alternative to above:
+    genericsOpenSym
     vtables
 
   LegacyFeature* = enum

diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim
@@ -187,6 +187,7 @@ proc semOpenSym(c: PContext, n: PNode, flags: TExprFlags, expectedType: PType,
           break
       o = o.owner
   # nothing found
+  n.flags.excl nfDisabledOpenSym
   if not warnDisabled and isSym:
     result = semExpr(c, n, flags, expectedType)
   else:
@@ -197,7 +198,9 @@ proc semOpenSym(c: PContext, n: PNode, flags: TExprFlags, expectedType: PType,
 
 proc semSymChoice(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType: PType = nil): PNode =
   if n.kind == nkOpenSymChoice:
-    result = semOpenSym(c, n, flags, expectedType, warnDisabled = nfDisabledOpenSym in n.flags)
+    result = semOpenSym(c, n, flags, expectedType,
+      warnDisabled = nfDisabledOpenSym in n.flags and
+        genericsOpenSym notin c.features)
     if result != nil:
       return
   result = n
@@ -3293,8 +3296,12 @@ proc semExpr(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType: PType
   of nkSym:
     let s = n.sym
     if nfDisabledOpenSym in n.flags:
-      let res = semOpenSym(c, n, flags, expectedType, warnDisabled = true)
-      assert res == nil
+      let override = genericsOpenSym in c.features
+      let res = semOpenSym(c, n, flags, expectedType,
+        warnDisabled = not override)
+      if res != nil:
+        assert override
+        return res
     # 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)

diff --git a/compiler/semgnrc.nim b/compiler/semgnrc.nim
@@ -74,7 +74,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
     else:
       result = symChoice(c, n, s, scOpen)
       if canOpenSym(s):
-        if {openSym, genericsOpenSym} * c.features != {}:
+        if openSym in c.features:
           if result.kind == nkSym:
             result = newOpenSym(result)
           else:
@@ -112,7 +112,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
         # we are in a generic context and `prepareNode` will be called
         result = newSymNodeTypeDesc(s, c.idgen, n.info)
         if canOpenSym(result.sym):
-          if {openSym, genericsOpenSym} * c.features != {}:
+          if openSym in c.features:
             result = newOpenSym(result)
           else:
             result.flags.incl nfDisabledOpenSym
@@ -122,7 +122,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
     else:
       result = newSymNodeTypeDesc(s, c.idgen, n.info)
       if canOpenSym(result.sym):
-        if {openSym, genericsOpenSym} * c.features != {}:
+        if openSym in c.features:
           result = newOpenSym(result)
         else:
           result.flags.incl nfDisabledOpenSym
@@ -141,7 +141,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
         return
       result = newSymNodeTypeDesc(s, c.idgen, n.info)
       if canOpenSym(result.sym):
-        if {openSym, genericsOpenSym} * c.features != {}:
+        if openSym in c.features:
           result = newOpenSym(result)
         else:
           result.flags.incl nfDisabledOpenSym
@@ -153,7 +153,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
       # we are in a generic context and `prepareNode` will be called
       result = newSymNodeTypeDesc(s, c.idgen, n.info)
       if canOpenSym(result.sym):
-        if {openSym, genericsOpenSym} * c.features != {}:
+        if openSym in c.features:
           result = newOpenSym(result)
         else:
           result.flags.incl nfDisabledOpenSym
@@ -164,7 +164,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym,
   else:
     result = newSymNode(s, n.info)
     if canOpenSym(result.sym):
-      if {openSym, genericsOpenSym} * c.features != {}:
+      if openSym in c.features:
         result = newOpenSym(result)
       else:
         result.flags.incl nfDisabledOpenSym

diff --git a/compiler/semtempl.nim b/compiler/semtempl.nim
@@ -233,7 +233,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField, isAmbiguous: bo
   of OverloadableSyms:
     result = symChoice(c.c, n, s, scOpen, isField)
     if not isField and result.kind in {nkSym, nkOpenSymChoice}:
-      if {openSym, templateOpenSym} * c.c.features != {}:
+      if openSym in c.c.features:
         if result.kind == nkSym:
           result = newOpenSym(result)
         else:
@@ -246,7 +246,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField, isAmbiguous: bo
     else:
       result = newSymNodeTypeDesc(s, c.c.idgen, n.info)
       if not isField and s.owner != c.owner:
-        if {openSym, templateOpenSym} * c.c.features != {}:
+        if openSym in c.c.features:
           result = newOpenSym(result)
         else:
           result.flags.incl nfDisabledOpenSym
@@ -264,7 +264,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField, isAmbiguous: bo
       if not isField and not (s.owner == c.owner and
           s.typ != nil and s.typ.kind == tyGenericParam) and
           result.kind in {nkSym, nkOpenSymChoice}:
-        if {openSym, templateOpenSym} * c.c.features != {}:
+        if openSym in c.c.features:
           if result.kind == nkSym:
             result = newOpenSym(result)
           else:
@@ -277,7 +277,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField, isAmbiguous: bo
     else:
       result = newSymNode(s, n.info)
       if not isField:
-        if {openSym, templateOpenSym} * c.c.features != {}:
+        if openSym in c.c.features:
           result = newOpenSym(result)
         else:
           result.flags.incl nfDisabledOpenSym

diff --git a/doc/manual_experimental.md b/doc/manual_experimental.md
@@ -2533,8 +2533,7 @@ 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
-`openSym`, or `genericsOpenSym` and `templateOpenSym` for only the respective
-routines, needs to be enabled; and a warning is given in the case where an
+`openSym` needs to be enabled; and a warning is given in the case where an
 injected symbol would replace a captured symbol not bound by `bind` and
 the experimental switch isn't enabled.
 
@@ -2555,7 +2554,7 @@ template oldTempl(): string =
       value # warning: a new `value` has been injected, use `bind` or turn on `experimental:openSym`
 echo oldTempl() # "captured"
 
-{.experimental: "openSym".} # or {.experimental: "genericsOpenSym".} for just generic procs
+{.experimental: "openSym".}
 
 proc bar[T](): string =
   foo(123):
@@ -2568,8 +2567,6 @@ proc baz[T](): string =
     return value
 assert baz[int]() == "captured"
 
-# {.experimental: "templateOpenSym".} would be needed here if genericsOpenSym was used
-
 template barTempl(): string =
   block:
     foo(123):
@@ -2590,6 +2587,34 @@ 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.
 
+Another experimental switch `genericsOpenSym` exists that enables this behavior
+at instantiation time, meaning templates etc can enable it specifically when
+they are being called. However this does not generate `nnkOpenSym` nodes
+(unless the other switch is enabled) and so doesn't reflect the regular
+behavior of the switch.
+
+```nim
+const value = "captured"
+template foo(x: int, body: untyped): untyped =
+  let value {.inject.} = "injected"
+  {.push experimental: "genericsOpenSym".}
+  body
+  {.pop.}
+
+proc bar[T](): string =
+  foo(123):
+    return value
+echo bar[int]() # "injected"
+
+template barTempl(): string =
+  block:
+    var res: string
+    foo(123):
+      res = value
+    res
+assert barTempl() == "injected"
+```
+
 
 VTable for methods
 ==================

diff --git a/tests/generics/mopensymimport2.nim b/tests/generics/mopensymimport2.nim
@@ -1,4 +1,4 @@
-{.experimental: "genericsOpenSym".}
+{.experimental: "openSym".}
 
 import mopensymimport1
 

diff --git a/tests/generics/tmacroinjectedsym.nim b/tests/generics/tmacroinjectedsym.nim
@@ -1,4 +1,4 @@
-{.experimental: "genericsOpenSym".}
+{.experimental: "openSym".}
 
 block: # issue #22605, normal call syntax
   const error = "bad"

diff --git a/tests/template/topensym.nim b/tests/template/topensym.nim
@@ -1,4 +1,4 @@
-{.experimental: "templateOpenSym".}
+{.experimental: "openSym".}
 
 block: # issue #24002
   type Result[T, E] = object

diff --git a/tests/template/topensymoverride.nim b/tests/template/topensymoverride.nim
@@ -0,0 +1,39 @@
+discard """
+  matrix: "--skipParentCfg --filenames:legacyRelProj"
+"""
+
+const value = "captured"
+template fooOld(x: int, body: untyped): untyped =
+  let value {.inject.} = "injected"
+  body
+template foo(x: int, body: untyped): untyped =
+  let value {.inject.} = "injected"
+  {.push experimental: "genericsOpenSym".}
+  body
+  {.pop.}
+
+proc old[T](): string =
+  fooOld(123):
+    return value
+doAssert old[int]() == "captured"
+
+template oldTempl(): string =
+  block:
+    var res: string
+    fooOld(123):
+      res = value
+    res
+doAssert oldTempl() == "captured"
+
+proc bar[T](): string =
+  foo(123):
+    return value
+doAssert bar[int]() == "injected"
+
+template barTempl(): string =
+  block:
+    var res: string
+    foo(123):
+      res = value
+    res
+doAssert barTempl() == "injected"