From b1bfba9a31bf23f716f6bc495b33f72473067045 Mon Sep 17 00:00:00 2001 From: metagn Date: Tue, 9 Apr 2024 15:37:34 +0300 Subject: [PATCH] stop gensym identifiers hijacking routine decl names in templates (#23392) fixes #23326 In a routine declaration node in a template, if the routine is marked as `gensym`, the compiler adds it as a new symbol to a preliminary scope of the template. If it's not marked as gensym, then it searches the preliminary scope of the template for the name of the routine, then when it matches a template parameter or a gensym identifier, the compiler replaces the name node with a symbol node of the found symbol. This makes sense for the template parameter since it has to be replaced later, but not really for the gensym identifier, as it doesn't allow us to inject a routine with the same name as an identifier previously declared as gensym (the problem in #23326 is when this is in another `when` branch). However this is the only channel to reuse a gensym symbol in a declaration, so maybe removing it has side effects. For example if we have: ```nim proc foo(x: int) {.gensym.} = discard proc foo(x: float) {.gensym.} = discard ``` it will not behave the same as ```nim proc foo(x: int) {.gensym.} = discard proc foo(x: float) = discard ``` behaved previously, which maybe allowed overloading over the gensym'd symbols. A note to the "undeclared identifier" error message has also been added for a potential error code that implicitly depended on the old behavior might give, namely ``undeclared identifier: 'abc`gensym123'``, which happens when in a template an identifier is first declared gensym in code that doesn't compile, then as a routine which injects by default, then the identifier is used. (cherry picked from commit 73b0b0d31c156392c64b8b088402695c7e27f0f0) --- compiler/lookups.nim | 6 ++++- compiler/semtempl.nim | 2 +- testament/important_packages.nim | 6 ++--- tests/errmsgs/tinconsistentgensym.nim | 25 ++++++++++++++++++ tests/template/tgensymhijack.nim | 37 +++++++++++++++++++++++++++ 5 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 tests/errmsgs/tinconsistentgensym.nim create mode 100644 tests/template/tgensymhijack.nim diff --git a/compiler/lookups.nim b/compiler/lookups.nim index afa2716d8a..6301368986 100644 --- a/compiler/lookups.nim +++ b/compiler/lookups.nim @@ -566,7 +566,11 @@ proc errorUndeclaredIdentifier*(c: PContext; info: TLineInfo; name: string, extr if name == "_": err = "the special identifier '_' is ignored in declarations and cannot be used" else: - err = "undeclared identifier: '" & name & "'" & extra + err = "undeclared identifier: '" & name & "'" + if "`gensym" in name: + err.add "; if declared in a template, this identifier may be inconsistently marked inject or gensym" + if extra.len != 0: + err.add extra if c.recursiveDep.len > 0: err.add "\nThis might be caused by a recursive module dependency:\n" err.add c.recursiveDep diff --git a/compiler/semtempl.nim b/compiler/semtempl.nim index 36b7838ec6..871a10db98 100644 --- a/compiler/semtempl.nim +++ b/compiler/semtempl.nim @@ -260,7 +260,7 @@ proc semRoutineInTemplName(c: var TemplCtx, n: PNode): PNode = if n.kind == nkIdent: let s = qualifiedLookUp(c.c, n, {}) if s != nil: - if s.owner == c.owner and (s.kind == skParam or sfGenSym in s.flags): + if s.owner == c.owner and s.kind == skParam: incl(s.flags, sfUsed) result = newSymNode(s, n.info) onUse(n.info, s) diff --git a/testament/important_packages.nim b/testament/important_packages.nim index 462bf8a30d..65f48d5537 100644 --- a/testament/important_packages.nim +++ b/testament/important_packages.nim @@ -60,7 +60,7 @@ pkg "compactdict" pkg "comprehension", "nimble test", "https://github.com/alehander92/comprehension" pkg "cowstrings" pkg "criterion", allowFailure = true # needs testing binary -pkg "datamancer" +pkg "datamancer", "nimble install -y arraymancer@#HEAD; nimble test" pkg "dashing", "nim c tests/functional.nim" pkg "delaunay" pkg "docopt" @@ -72,7 +72,7 @@ pkg "fragments", "nim c -r fragments/dsl.nim", allowFailure = true # pending htt pkg "fusion" pkg "gara" pkg "glob" -pkg "ggplotnim", "nim c -d:noCairo -r tests/tests.nim" +pkg "ggplotnim", "nimble install -y arraymancer@#HEAD; nim c -d:noCairo -r tests/tests.nim" pkg "gittyup", "nimble test", "https://github.com/disruptek/gittyup", allowFailure = true pkg "gnuplot", "nim c gnuplot.nim" # pkg "gram", "nim c -r --mm:arc --define:danger tests/test.nim", "https://github.com/disruptek/gram" @@ -124,7 +124,7 @@ pkg "nimx", "nim c test/main.nim", allowFailure = true pkg "nitter", "nim c src/nitter.nim", "https://github.com/zedeus/nitter" pkg "norm", "testament r tests/common/tmodel.nim" pkg "npeg", "nimble testarc" -pkg "numericalnim", "nimble nimCI" +pkg "numericalnim", "nimble install -y arraymancer@#HEAD; nimble nimCI" pkg "optionsutils" pkg "ormin", "nim c -o:orminn ormin.nim" pkg "parsetoml" diff --git a/tests/errmsgs/tinconsistentgensym.nim b/tests/errmsgs/tinconsistentgensym.nim new file mode 100644 index 0000000000..026c17fcaa --- /dev/null +++ b/tests/errmsgs/tinconsistentgensym.nim @@ -0,0 +1,25 @@ +discard """ + cmd: "nim check --hints:off $file" +""" + +block: + template foo = + when false: + let x = 123 + else: + template x: untyped = 456 + echo x #[tt.Error + ^ undeclared identifier: 'x`gensym0'; if declared in a template, this identifier may be inconsistently marked inject or gensym]# + foo() + +block: + template foo(y: static bool) = + block: + when y: + let x {.gensym.} = 123 + else: + let x {.inject.} = 456 + echo x #[tt.Error + ^ undeclared identifier: 'x']# + foo(false) + foo(true) diff --git a/tests/template/tgensymhijack.nim b/tests/template/tgensymhijack.nim new file mode 100644 index 0000000000..72ff3d4957 --- /dev/null +++ b/tests/template/tgensymhijack.nim @@ -0,0 +1,37 @@ +# issue #23326 + +type Result*[E] = object + e*: E + +proc error*[E](v: Result[E]): E = discard + +template valueOr*[E](self: Result[E], def: untyped): int = + when E isnot void: + when false: + # Comment line below to make it work + template error(): E {.used, gensym.} = s.e + discard + else: + template error(): E {.used, inject.} = + self.e + + def + else: + def + + +block: + let rErr = Result[string](e: "a") + let rErrV = rErr.valueOr: + ord(error[0]) + +block: + template foo(x: static bool): untyped = + when x: + let a = 123 + else: + template a: untyped {.gensym.} = 456 + a + + doAssert foo(false) == 456 + doAssert foo(true) == 123