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

Identical AST succeeds, but fails when produced by a macro, with "cannot use symbol of kind 'proc' as a 'field'" #11182

Closed
cyisfor opened this issue May 6, 2019 · 10 comments

Comments

@cyisfor
Copy link

cyisfor commented May 6, 2019

The macro produces an identical AST, except with "Sym" instead of "Ident" some places, for hygiene, but compilation fails with a mysterious error. I'm not even using proc nodes!

Example

import macros
macro examine(x: untyped): untyped =
  debugEcho(x.treeRepr)
  return x

type Kind = enum
  Thingy = 23

examine:  
  type
    thingy = object
      case kind: Kind
      of Thingy:
        value: int


macro fail(): untyped =
  result = quote:
    type
      thingy = object
        case kind: Kind
        of Thingy:
          value: int
  debugEcho(result.treeRepr)

const broken = true
  
when broken:
  fail()

Current Output

Error: cannot use symbol of kind 'proc' as a 'field'

Expected Output

Compilation succeeds.

Possible Solution

Any Sym node in a macro is resolved to an Ident (aka a field) node by the hygiene system, so the symbol Thingy will turn into Thingy1234567 for the actual code, thus preventing macros from polluting the global namespace. This is done in compiler/sem.nim in newSymG where n.sym takes the Ident out of a not yet set-in-stone Sym, but a TSymKind is also passed to that function, causing the above error if the underlying n.sym kind is different. That makes sense, because if a symbol resolves to something that is not an Ident, then using it in the AST as an Ident would produce invalid code.

The problem is when a symbol just happens to be the same name as a procedure from any imported module. In the above example, using kind as the Ident naming type thingy is perfectly valid code, but when done inside a macro, kind is supposed to be replaced with kind1234etc during the conversion from Sym to Ident. That should happen, but what happens instead is the Sym resolution process examines all imported procedures first, finding a procedure named kind (wildcard imported from the macros package). So kind has the symbol type of skProc now. Even though kind would make a perfectly good field, and kind1234etc would make an even more distinguised field, the code errors out thinking that we stuffed an entire procedure into a symbol that was only supposed to have an ident.

This can be avoided by using your psychic powers and genius level code analysis abilities to instantly calculate all imported symbols, so that when you write code, you never choose a symbol for your identifier that has the same name as an imported procedure, thus:

etc
macro nofail(): untyped =
  result = quote:
    type
      thingy = object
        case kind12345: Kind
        of Thingy:
          value: int
etc

Or another way of thinking about it is when writing symbols in macros, always tack on some large random number to the end, and you probably won't have symbols that are the same name as any procedure you imported. You can also do this:

import macros except kind
etc...

Provided your psychic abilities are at least powerful enough to divine which module your colliding symbol was imported from, because Nim won't give you any help with that.

Another solution is to do this:

from macros import treeRepr, quote, newIdentNode

But then you have to slog through the errors when importing quote should also pull in newIdentNode, since quote uses newIdentNode, but instead it errors out until you manually add it in. But after you've added all the super secret surprise imports, then it's somewhat possible for someone analyzing your code to figure out what it does, without doing an exhaustive search through all the symbols of every import, as is required with the import X syntax.

Some actual solutions that aren't hacks would be:

  • fix the symbol resolution process somehow to account for the case where 'proc' symbols actually can be 'field' symbols.
  • fix importing so that it imports needed symbols rather than erroring out and telling you that you need to import them, thus making from X import syntax actually useful and reducing name collisions.
  • fix the error in compiler/sem.nim to tell you which 'proc' symbol is causing the error, rather than it being "any possible symbol in your macro"

So... that's my suggestion. Not sure exactly how to do it though.

Additional Information

Nim Compiler Version 0.19.9 [Linux: amd64]

git hash: 8a918075e5adf4529b90e3516dd3d31321a06cce
active boot switches: -d:release
@bluenote10
Copy link
Contributor

bluenote10 commented May 6, 2019

It looks like the kind field has been bound to the kind proc in macros.nim. And the reason why the type gets bound is probably because quote takes a typed argument.

In my opinion that is also the source of the problem, quote should not take typed argument and automatically bind all symbols. It's weird anyway and requires probably some special treatment, because the AST can contain undefined symbols. But in general the opinion is that quote is broken, and @krux is working on a new solution, which on first glance will not have the problem.

If you just want to make it work: You know the trick with converting all the symbols in an AST back to idents? (Here is a snippet, quite a few of my recently opened tickets were related to using symbols instead of idents). Alternatively you can use getAst on a template -- this gives better control which symbols get bound.

@cyisfor
Copy link
Author

cyisfor commented May 6, 2019

It looks like the kind field has been bound to the kind proc in macros.nim. And the reason why the type gets bound is probably because quote takes a typed argument.

Oh, I didn't know the argument to quote was typed. That might be another solution, to make it untyped! I don't think people expect quote to have type awareness, at least.

the trick with converting all the symbols in an AST back to idents?

I thought about that, but it seemed like too much of a confusing hack to find every nnkSym and run gensym to make an appropriate identifier for it, the way that the compiler should already be doing automatically.

Alternatively you can use getAst on a template -- this gives better control which symbols get bound.

I... never considered that. That's actually not entirely a bad idea. Only trouble is you can't have templates with a variable sized list of arguments, that I'm aware of, so a template like

template make_object(name: untyped, fields: varargs[untyped]) =
  type name = object
    #{(fields)=>+=>(field => >>>*** field: int ***<<<)+magic pixies}

would have to be a macro, if it wanted to do anything with the syntax of each field, other than just plunk down the fields in between [ and ]

@bluenote10
Copy link
Contributor

That's true, in case it helps to solve your problem this code does exactly that.

@krux02
Copy link
Contributor

krux02 commented May 6, 2019

when you add another overload of kind that isn't used it works:

import macros

macro examine(x: untyped): untyped =
  debugEcho(x.treeRepr)
  return x

type Kind = enum
  Thingy = 23

examine:
  type
    thingy = object
      case kind: Kind
      of Thingy:
        value: int


proc kind(arg: string): void = discard  # <--- *** this is new ***

macro fail(): untyped =
  result = quote:
    type
      thingy = object
        case kind: Kind
        of Thingy:
          value: int
  debugEcho(result.treeRepr)

const broken = true

when broken:
  fail()

output

StmtList
  TypeSection
    TypeDef
      Ident "thingy"
      Empty
      ObjectTy
        Empty
        Empty
        RecList
          RecCase
            IdentDefs
              Ident "kind"
              Ident "Kind"
              Empty
            OfBranch
              Ident "Thingy"
              RecList
                IdentDefs
                  Ident "value"
                  Ident "int"
                  Empty
TypeSection
  TypeDef
    Sym "thingy"
    Empty
    ObjectTy
      Empty
      Empty
      RecList
        RecCase
          IdentDefs
            OpenSymChoice    # <- *** this is new ***
              Sym "kind" 
              Sym "kind"
            Sym "Kind"
            Empty
          OfBranch
            Sym "Thingy"
            RecList
              IdentDefs
                Ident "value"
                Sym "int"
                Empty

two alternatives

  • You can add mixin kind in the first line of the quote body.
  • let kindIdent = ident"kind" ... case `kindIdent`: Kind.

@mratsim
Copy link
Collaborator

mratsim commented May 6, 2019

Now that explains some issues I had where I had to build the AST from scratch because quote would fail with a cryptic error.

@krux02
Copy link
Contributor

krux02 commented May 6, 2019

@mratsim can you link or mention the issues you had? I just talked this morning with Araq about how strange symbols in templates get resolved, and then I saw this issue, which is cause by exactly the problem I mentioned.

@mratsim
Copy link
Collaborator

mratsim commented May 6, 2019

Unfortunately I didn't raise issues at the time because it was part of a huge macro base and I interrupted myself a lot to raise various bugs at the time so I wanted to get things done.

Looking back into the history of my project, I had this "Make import with no use compile. Stuck at undeclared identifier contiguousIndex_" where I changed a getAST call to result quote do to apparently solve a bug where a proc/macro is not used even if imported.

The next commit is mratsim/laser@e6db3be which makes the code compile with a long comment:

  let index = newIdentNode("contiguousIndex_")
  # [...] skipped

  let # quote do, doesn't interpolate properly to static
      # We get "undeclared identifier contiguousIndex_" otherwise
    use_simd_node = newLit use_simd
    nowait_node = newLit nowait

use_simd and nowait where compile-time bool, AFAIK they are currently transformed into int by quote do if you don't use newLit due to #7375, but somehow it seems like at the time I got a misleading undeclared identifier error instead of the "expected" #7375 error.

@Araq Araq added the quote do label Aug 22, 2019
@timotheecour
Copy link
Member

this isn't related to quote do

reduced with 0 imports:

when true:
  proc fn1()=discard
  proc fn2()=discard
  proc fn2(a: int)=discard
  proc fn3()=discard
  template baz =
    var fn1: int # works
    # mixin fn3 # workaround: would make it "work" but with a wrong AST still
    type A = object
      fn2: int
      fn3: int # Error: cannot use symbol of kind 'proc' as a 'field'
  baz()

proposal fix

fields should be gensym'd (as do var's)

details

when true:
  import macros
  proc fn1()=discard
  proc fn2()=discard
  proc fn2(a: int)=discard
  proc fn3()=discard
  macro fail(): untyped =
    result = quote:
      var fn1: int # works
      # mixin fn3 # workaround: would make it "work"
      type A = object
        fn2: int
        fn3: int # Error: cannot use symbol of kind 'proc' as a 'field'
    debugEcho(result.treeRepr)
    debugEcho(result.repr)
  fail()

Error: cannot use symbol of kind 'proc' as a 'field'

StmtList
  VarSection
    IdentDefs
      Ident "fn1`gensym0"
      Sym "int"
      Empty
  TypeSection
    TypeDef
      Ident "A`gensym0"
      Empty
      ObjectTy
        Empty
        Empty
        RecList
          IdentDefs
            OpenSymChoice
              Sym "fn2"
              Sym "fn2"
            Sym "int"
            Empty
          IdentDefs
            Sym "fn3"
            Sym "int"
            Empty

var fn1`gensym0: int
type
  A`gensym0 = object
    fn2: int
    fn3: int

@metagn
Copy link
Collaborator

metagn commented Aug 28, 2023

Read basically none of the text above, but from what I understand the issue described here is fixed by #22106, the original example now gives:

StmtList
  TypeSection
    TypeDef
      Ident "thingy"
      Empty
      ObjectTy
        Empty
        Empty
        RecList
          RecCase
            IdentDefs
              Ident "kind"
              Ident "Kind"
              Empty
            OfBranch
              Ident "Thingy"
              RecList
                IdentDefs
                  Ident "value"
                  Ident "int"
                  Empty
(9, 1) template/generic instantiation of `examine` from here
(12, 7) Error: low(kind) must be 0 for discriminant

I'm guessing this is fine, changing Thingy = 23 to Thingy = 0 makes the example compile

@metagn
Copy link
Collaborator

metagn commented Sep 19, 2023

Original test cases in this issue are too convoluted for what was such a simple problem. Just closing.

@metagn metagn closed this as completed Sep 19, 2023
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

8 participants