Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

non-hygenic behavior for gensymmed name in template #24260

Open
arnetheduck opened this issue Oct 8, 2024 · 7 comments
Open

non-hygenic behavior for gensymmed name in template #24260

arnetheduck opened this issue Oct 8, 2024 · 7 comments

Comments

@arnetheduck
Copy link
Contributor

Description

import std/macros

macro evalOnceAs*(exp, alias: untyped): untyped =
  expectKind(alias, nnkIdent)

  let
    body = nnkStmtList.newTree()
    val =
      if exp.kind == nnkSym:
        # The symbol can be used directly
        # TODO dot expressions? etc..
        exp
      else:
        let val = genSym(ident = "evalOnce_" & $alias)
        body.add newLetStmt(val, exp)
        val
  body.add(
    newProc(
      name = genSym(nskTemplate, $alias),
      params = [getType(untyped)],
      body = val,
      procType = nnkTemplateDef,
    )
  )

  body

type ArrayBuf = object
  buf: array[32, byte]
  n: int

template data*(bParam: ArrayBuf): openArray =
  bParam.evalOnceAs(b)
  b.buf.toOpenArray(0, b.n - 1)


proc f() =
  var b: int
  var a: ArrayBuf

  echo a.data()

  echo b

var b: int

proc f2() =
  var a: ArrayBuf

  echo a.data()

  echo b

the name b is local to the template and not dirty - it should neither conflict with the other b nor leak out of the template

A variation, when b is in a different scope:


var b: int
proc f() =
  var a: ArrayBuf

  echo a.data()

  echo b

f()

Here, (buf: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], n: 0) gets printed.

Nim Version

2.0.8

Current Output

testit.nim(26, 20) Error: redefinition of 'b'; previous declaration here: /home/arnetheduck/status/nimbus-eth1/_exper/testit.nim(45, 7)

Expected Output

No response

Known Workarounds

No response

Additional Information

No response

@Araq
Copy link
Member

Araq commented Oct 8, 2024

Is this a regression?

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Oct 8, 2024

not sure, I haven't tested earlier versions. the code is "recent" in the sense that it wasn't tested with a conflicting name in the same scope. it was discovered by accident due to random unexpected values for a variable named b having the same type as the symbol inside the template, so it's certainly surprising.

@metagn
Copy link
Collaborator

metagn commented Oct 8, 2024

Templates don't work with macro-generated gensym names at all, because semTemplateDef defines the template regardless of sfGenSym, unlike all other routines which check for it in semProcAux.

import std/macros

macro fooTempl(name: untyped): untyped =
  result = newProc(
    name = genSym(nskTemplate, $name),
    params = [ident"untyped"],
    body = newLit(123),
    procType = nnkTemplateDef,
  )

fooTempl(bar)
echo bar() # 123

macro fooProc(name: untyped): untyped =
  result = newProc(
    name = genSym(nskProc, $name),
    params = [ident"auto"],
    body = newLit(123),
    procType = nnkProcDef,
  )

fooProc(bar2)
echo bar2() # undeclared identifier: bar2

The example in the issue still wouldn't work though because the data template has no way of capturing the gensym'd b generated in evalOnceAs. In fact any use of evalOnceAs would have the same problem.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Oct 8, 2024

The example in the issue still wouldn't work though because the

Not sure I follow .. in the example echo a.data() works as expected so clearly the template can access b gensymmed in the macro.

@metagn
Copy link
Collaborator

metagn commented Oct 8, 2024

The compiler doesn't consider when template names are marked gensym, so the fact that b is gensymmed is completely ignored and a raw template b is generated. It does consider it for other routines, let etc.

If this was fixed, i.e. templates did gensym their names like other routines, the generated sym would be inaccessible from outside of the macro, so the macro wouldn't work as expected. So the macro actually needs to not gensym the name of the template, i.e. it depends on this bug.

This would mean the code in the issue will still fail though, because of the limitation that the template data cannot automatically gensym b itself. We could maybe mitigate this with something like evalOnceAs(b {.gensym.}). This is a workaround, but it might be hard to repeat:

macro data*(bParam: ArrayBuf): openArray =
  let b = ident repr genSym(nskTemplate, "b")
  result = quote do:
    `bParam`.evalOnceAs(`b`)
    `b`.buf.toOpenArray(0, `b`.n - 1)

@arnetheduck
Copy link
Contributor Author

I look at it in a slightly different way: data is a hygienic template - no matter what it does (including evaluating macros that declare variables), names should not be leaking out of it - ie even if the macro leaks, that leak should be stopped at data.

If data called another dirty template, I would probably still not expect the symbols to leak beyond its scope unless data itself was dirty.

@arnetheduck
Copy link
Contributor Author

arnetheduck commented Oct 8, 2024

there's a similar macro in stdlib btw:

val = genSym()

it's also buggy:

import std/sequtils

var iter2 = 10

iterator xxx(): int {.closure.} =
  yield 5

proc f() =
  let x = xxx
  var v = toSeq(x)

  echo iter2, " ", v

f()

prints 0 @[5] (!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants