From 27381cc60213e19aa34664176bd358ca5e45bd5a Mon Sep 17 00:00:00 2001 From: metagn Date: Wed, 18 Sep 2024 20:27:09 +0300 Subject: [PATCH] make `genericsOpenSym` work at instantiation time, new behavior in `openSym` (#24111) alternative to #24101 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. (cherry picked from commit 0c3573e4a0628bbaa8b09dcd854bdc2702948bbc) --- changelog.md | 35 +++++++++++++++++++++---- compiler/condsyms.nim | 1 + compiler/options.nim | 4 +-- compiler/semexprs.nim | 13 +++++++--- compiler/semgnrc.nim | 8 +++--- compiler/semtempl.nim | 6 ++--- doc/manual_experimental.md | 36 +++++++++++++++++++++---- tests/generics/mopensymimport2.nim | 2 +- tests/generics/tmacroinjectedsym.nim | 2 +- tests/template/topensym.nim | 2 +- tests/template/topensymoverride.nim | 39 ++++++++++++++++++++++++++++ 11 files changed, 123 insertions(+), 25 deletions(-) create mode 100644 tests/template/topensymoverride.nim diff --git a/changelog.md b/changelog.md index 67877183c6..43a0d8ecbf 100644 --- a/changelog.md +++ b/changelog.md @@ -39,8 +39,7 @@ slots when enlarging a sequence. 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. @@ -61,7 +60,7 @@ slots when enlarging a sequence. 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): @@ -74,8 +73,6 @@ slots when enlarging a sequence. return value assert baz[int]() == "captured" - # {.experimental: "templateOpenSym".} would be needed here if genericsOpenSym was used - template barTempl(): string = block: foo(123): @@ -96,6 +93,34 @@ slots when enlarging a sequence. 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 diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim index 2582976b8f..3d5b3344b9 100644 --- a/compiler/condsyms.nim +++ b/compiler/condsyms.nim @@ -162,3 +162,4 @@ proc initDefines*(symbols: StringTableRef) = defineSymbol("nimHasCastExtendedVm") defineSymbol("nimHasGenericsOpenSym2") defineSymbol("nimHasNolineTooLong") + defineSymbol("nimHasGenericsOpenSym3") diff --git a/compiler/options.nim b/compiler/options.nim index 7b5f55bbd2..7b364e53b6 100644 --- a/compiler/options.nim +++ b/compiler/options.nim @@ -222,8 +222,8 @@ type strictDefs, strictCaseObjects, openSym, # remove nfDisabledOpenSym when this is default - # separated alternatives to above: - genericsOpenSym, templateOpenSym + # alternative to above: + genericsOpenSym LegacyFeature* = enum allowSemcheckedAstModification, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index 62bae7ab84..82ef27a191 100644 --- 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 @@ -3210,8 +3213,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 index b93944c772..4e06a87e6a 100644 --- a/compiler/semgnrc.nim +++ b/compiler/semgnrc.nim @@ -72,7 +72,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: @@ -108,7 +108,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 @@ -122,7 +122,7 @@ proc semGenericStmtSymbol(c: PContext, n: PNode, s: PSym, (s.typ.flags * {tfGenericTypeParam, tfImplicitTypeParam} == {}): 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 @@ -133,7 +133,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 index 581e2b98fb..23ebdbba37 100644 --- a/compiler/semtempl.nim +++ b/compiler/semtempl.nim @@ -232,7 +232,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField: bool): PNode = 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: @@ -245,7 +245,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField: bool): PNode = 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 @@ -260,7 +260,7 @@ proc semTemplSymbol(c: var TemplCtx, n: PNode, s: PSym; isField: bool): PNode = 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 index dffd328257..e6c1db1f2c 100644 --- a/doc/manual_experimental.md +++ b/doc/manual_experimental.md @@ -2336,8 +2336,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. @@ -2358,7 +2357,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): @@ -2371,8 +2370,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): @@ -2392,3 +2389,32 @@ exactly 1 `nnkSym` 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. + +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" +``` + diff --git a/tests/generics/mopensymimport2.nim b/tests/generics/mopensymimport2.nim index 1e1cda301c..c17aafd001 100644 --- 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 index a2771a9e88..4b63749123 100644 --- 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 index 9393e19716..2f930407b9 100644 --- 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 new file mode 100644 index 0000000000..3d4bb59f14 --- /dev/null +++ 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"