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

Captures not working inside of proc with generic arguments #68

Closed
kerrycobb opened this issue Sep 21, 2023 · 21 comments
Closed

Captures not working inside of proc with generic arguments #68

kerrycobb opened this issue Sep 21, 2023 · 21 comments

Comments

@kerrycobb
Copy link

kerrycobb commented Sep 21, 2023

The > symbol used in an expression inside of a proc causes the following error: Error: wrong number of arguments. It seems like it is being recognized as the system operator rather than part of the expression.

Two examples of it not working:

import npeg

proc test(T: typedesc, str: string) = 
  let p = peg "start":
    start <- >"Test"
  var m = p.match(str)
  echo m.captures
test(string, "Test")
import npeg

proc test[T](obj: T, str: string) = 
  let p = peg "start":
    start <- >"Test"
  var m = p.match(str)
  echo m.captures
test(1, "Test")

If the '>' is removed, things work. But of course I cannot use the matched element. This is the only symbol that is causing me problems and the parser I've been trying to write is using almost all of them.

import npeg

proc test(T: typedesc, str: string) = 
  let p = peg "start":
    start <- "Test"
  var m = p.match(str)
  echo m.captures
test(string, "Test")
import npeg 

proc test[T](obj: T, str: string) = 
  let p = peg "start":
    start <- "Test"
  var m = p.match(str)
  echo m.captures
test(1, "Test")
@zevv
Copy link
Owner

zevv commented Sep 21, 2023

Well, I'm flabbergasted about this one - I'll spend some time trying to understand what is happening, but I would not be surprised if this is a Nim bug.

@kerrycobb
Copy link
Author

That was my suspicion as well but I thought I should start here.

@zevv
Copy link
Owner

zevv commented Sep 21, 2023

I have it minimized to this:

import macros

macro foo*(n: untyped): untyped =
  echo "foo 1 ", n.astGenRepr

macro foo*(n, n2: untyped): untyped =
  echo "foo 2 ", n.astGenRepr, " ", n2.astGenRepr

proc test[T](v: T, str: string) =
  foo >"test" 

test("", "Test")

Take away the second foo macro, and stuff just works.

@disruptek: you are a smart man, any clue?

@zevv
Copy link
Owner

zevv commented Sep 24, 2023

Indeed, there is a Nim bug, which seems to have been there since always. Funny that his never popped up before for anyone while using NPeg.

Workaround for now would be to define your own >() template in scope that will get precedence over `system.>()", something like this.

template `>`(a: untyped): untyped =
  discard

Let's see if the bug gets a fix anytime soon.

@kerrycobb
Copy link
Author

Thanks for digging into this. I'm encountering another problem now. The proposed work around does make the original example work though!

This works:

import npeg
proc test1(T: typedesc, str: string) = 
  template `>`(a: untyped): untyped = discard
  let p = peg "start":
    start <- >"test"
  var m = p.match(str)
test1(int, "test")

However, adding an additional rule causes an error:

# Error: Expected PEG rule name but got nnkSy
import npeg
proc test2(T: typedesc, str: string) = 
  template `>`(a: untyped): untyped = discard
  let p = peg "start":
    test   <- >"test"
    start  <- test
  var m = p.match(str)
test2(int, "test")

The template doesn't interfere at all with the non-generic case though:

import npeg
proc test3(str: string) = 
  template `>`(a: untyped): untyped = discard
  let p = peg "start":
    test   <- >"test"
    start  <- test
  var m = p.match(str)
test3("test")

@zevv
Copy link
Owner

zevv commented Sep 24, 2023

Hm your second example compiles fine for me on latest devel; What Nim version are you on?

@kerrycobb
Copy link
Author

kerrycobb commented Sep 24, 2023

I encounter the error with 2.0.0 as well as devel on a mac amd64.

Here's the stack trace:
stack trace: (most recent call last)
/Users/kerry/.nimble/pkgs2/npeg-1.2.1-c5451eea5d1f656700f04ad4b234d99ab0b78ed7/npeg.nim(86, 3) peg
/Users/kerry/.nimble/pkgs2/npeg-1.2.1-c5451eea5d1f656700f04ad4b234d99ab0b78ed7/npeg.nim(76, 29) pegAux
/Users/kerry/.nimble/pkgs2/npeg-1.2.1-c5451eea5d1f656700f04ad4b234d99ab0b78ed7/npeg/parsepatt.nim(217, 9) parseGrammar
/Users/kerry/Dropbox/repos/PhylogeNi/test.nim(54, 6) template/generic instantiation of test2 from here
/Users/kerry/Dropbox/repos/PhylogeNi/test.nim(49, 15) template/generic instantiation of peg from here
/Users/kerry/Dropbox/repos/PhylogeNi/test.nim(50, 12) Error: Expected PEG rule name but got nnkSym

@kerrycobb kerrycobb reopened this Sep 24, 2023
@zevv
Copy link
Owner

zevv commented Sep 24, 2023

Well that;'s not making any sense, I must be doing something wrong here.

I just have this one file t.nim:

# Error: Expected PEG rule name but got nnkSy
import npeg
proc test2(T: typedesc, str: string) = 
  template `>`(a: untyped): untyped = discard
  let p = peg "start":
    test   <- >"test"
    start  <- test
  var m = p.match(str)
test2(int, "test")

And then

$ nim -v
nim -v
Nim Compiler Version 2.1.1 [Linux: amd64]
Compiled at 2023-09-24
Copyright (c) 2006-2023 by Andreas Rumpf

git hash: 1b0447c208a8ec03c1abf5c511b3949a882a8996
active boot switches: -d:release -d:danger


$ nim r t.nim
Hint: mm: orc; threads: on; opt: none (DEBUG BUILD, `-d:release` generates faster code)
70903 lines; 1.048s; 110.586MiB peakmem; proj: /home/ico/sandbox/prjs/npeg/t.nim; out: /home/ico/sandbox/prjs/npeg/t [SuccessX]

Same with my 2.0:

Nim Compiler Version 2.0.0 [Linux: amd64]

@kerrycobb
Copy link
Author

My file was called test.nim. And I couldn't run it on mac or linux. Choosing another name fixed that error.

But now there is another problem. It seems to be due to using a recursive rule. And it happens regardless of the > operator being used or not.

import npeg

var str = "(())"

# This fails with "Error: node lacks field: strVa"
proc parse*(T: typedesc, str:string) = 
  let p = peg "parser":
    elem  <- internal 
    internal   <- '(' * ?elem * ')' 
    parser     <- internal
  let r = p.match(str)
parse(int, str)

# But this works
proc parse*(str:string) = 
  let p = peg "parser":
    elem  <- internal 
    internal   <- '(' * ?elem * ')' 
    parser     <- internal
  let r = p.match(str)
parse(str)

@zevv
Copy link
Owner

zevv commented Sep 25, 2023

My file was called test.nim. And I couldn't run it on mac or linux. Choosing another name fixed that error.

Yeah, things like that happened to me more than once. Check if you have a stray nim.cfg or test.nims file lingering around in that same directory, that can get picked up by the compiler and change your compilation settings.

proc parse*(T: typedesc, str:string) = 
  let p = peg "parser":
    elem  <- internal 
    internal   <- '(' * ?elem * ')' 
    parser     <- internal
  let r = p.match(str)
parse(int, str)

I guess this is the same Nim bug as the >, but now happening on the * operator; try to put something liek this in scope:

template `*`(a, b: untyped): untyped = discard

Our Nim bug is now one of the other many bugs and slowly moving down in the list, so not sure if this will get fixed anytime soon.

I had the nasty plan of adding your snippet to the NPeg test suite - since NPeg is part of Nims "important packages", its tests are ran as part of Nim's CI; deliberately breaking Nim CI might speed things up, but I will not be making any friends with that I guess.

@kerrycobb
Copy link
Author

kerrycobb commented Sep 25, 2023

Yeah, things like that happened to me more than once. Check if you have a stray nim.cfg or test.nims file lingering >around in that same directory, that can get picked up by the compiler and change your compilation settings.

Thanks for the tip!

I guess this is the same Nim bug as the >, but now happening on the * operator; try to put something liek this in >scope:

This was indeed the case! And now my parser is working! I had to define a template like that for every operator that is being used. This felt like a nice way to keep it a bit out of the way and have clarity about why it's there.

template genericBugWorkAround() =
  # Workaround for bug in Nim
  # https://github.com/zevv/npeg/issues/68
  # https://github.com/nim-lang/Nim/issues/22740
  template `>`(a: untyped): untyped = discard
  template `*`(a: untyped): untyped = discard
  template `-`(a: untyped): untyped = discard
  template `+`(a: untyped): untyped = discard
  template `?`(a: untyped): untyped = discard
  template `!`(a: untyped): untyped = discard
  template `$`(a: untyped): untyped = discard
  
proc parse*(T: typedesc, str:string): T = 
  genericBugWorkAround()
  let p = peg "parser":
  ...

Our Nim bug is now one of the other many bugs and slowly moving down in the list, so not sure if this will get fixed >anytime soon.

I had the nasty plan of adding your snippet to the NPeg test suite - since NPeg is part of Nims "important >packages", its tests are ran as part of Nim's CI; deliberately breaking Nim CI might speed things up, but I will not be >making any friends with that I guess.

Haha! I'll let you be the judge of that. I'm content with the workaround and can move forward. Maybe just a note in the docs somewhere for now? I really don't have a good sense for how high of a priority that bug should be.

Thanks for your efforts in helping me find a solution! And also for the work you've done making this fantastic tool.

@zevv
Copy link
Owner

zevv commented Sep 25, 2023

This was indeed the case! And now my parser is working! I had to define a template like that for every
operator that is being used. This felt like a nice way to keep it a bit out of the way and have clarity about
why it's there.

Well, the least I can do: 049b4ca

import npeg

proc parse*(T: typedesc, str:string) = 
  nimBug22740()
  let p = peg "parser":
    elem       <- > internal 
    internal   <- '(' * ?elem * ')' 
    parser     <- internal
  let r = p.match(str)
parse(int, "(())")

@kerrycobb
Copy link
Author

I may have spoken too soon. It was working when I wasn't trying to import the parser into another module. When I do that I am encountering more errors.

# a.nim
import npeg

proc parse*(T: typedesc, str:string) = 
  let p = peg "parser":
    parser  <- "test" 
  let r = p.match(str)
#b.nim
import ./a

parse(int, "test")

Returns Error: type mismatch: got <Captures[system.char], int> but expected one of: ...

# a.nim
import npeg

proc parse*(T: typedesc, str:string) = 
  template `>`(a: untyped): untyped = discard
  let p = peg "parser":
    parser  <- >"test" 
  let r = p.match(str)
#b.nim
import ./a

parse(int, "test")

Returns Error: undeclared identifier: 'NPegStackOverflowError' candidates (edit distance, scope distance); see '--spellSuggest': ...

@zevv
Copy link
Owner

zevv commented Sep 25, 2023

I'll spend some time this evening to see if I can understand what is happening here.

Also, I now remember - vividly - why I left Nim behind and moved on to other languages...

saem added a commit to saem/nimskull that referenced this issue Sep 27, 2023
Unmatched `immediate` `macro`s and `template`s within a generic context
no longer result in a compiler error.

## Details

Immediate macros and templates are not dispatched via `sigmatch` and
instead `semgnrc` directly calls `evaltempl.evalTemplate`, in turn
`evalTemplateArgs` is called which can produce an error if an incorrect
number of arguments are provided. `evalTemplateArgs` is also used by
`semMacroExpr`, so similar issues would also exist for macros.

Now the error from `evalTemplateArgs` is immediately returned by
`evalTemplate` and treated as a mismatch in `semgnrc.semGenericStmt`.
Doing so prevents a compiler crash, attempt to access the sons of an error
node. A test has been added as part of this change, which was originally
from: zevv/npeg#68 (comment)
@zevv
Copy link
Owner

zevv commented Sep 30, 2023

I'm sorry @kerrycobb, but it seems I can't get this to work, nor can I find a feasible workaround. It seems that we have again hit some limit of Nim's macro system.

I hope NPeg is still usable to you it its current form.

I'll leave this issue open, maybe someone else will come by and find a way to get this done.

@kerrycobb
Copy link
Author

@zevv, thank you for trying!

That is unfortunate. Maybe I can create a macro or template that generates a parser for each of the types that I wish to be able to use with the parser as a workaround for now. I'm not going to have time to try that out for a few weeks though. I'll post here if I am successful.

Have you gained any incite that could be shared over in nim-lang/Nim#22740 or perhaps as a new issue if more appropriate?

I'll likely still find uses for Npeg aside from my current project but this throws a pretty big wrench into the plans I had. It's not for anything real important though. Just an exploration into the utility of Nim for some application. I remain optimistic for it eventually.

@zevv
Copy link
Owner

zevv commented Oct 1, 2023

Well, this is rather ridiculous.

Change a.nim to this to make it work:

# a.nim       
import npeg                                                 
                                               
discard patt "test"                                              
                                       
proc parse*(T: typedesc, str:string) =
  let p = peg "parser":               
    parser  <- "test"                 
  let r = p.match(str)   

@kerrycobb
Copy link
Author

kerrycobb commented Oct 2, 2023

I've managed to get everything working! Here is a minimal example that should be expandable to most use cases:

# a.nim

import npeg

proc parse*(T: typedesc, str:string) = 
  template `*`(a: untyped): untyped = discard
  template `-`(a: untyped): untyped = discard
  template `+`(a: untyped): untyped = discard
  template `$`(a:untyped): untyped = discard  # Only useful if all code is in a single module
  let p = peg "parser":
    parser  <- >"test": 
      echo capture[1].s # Syntactic sugar `$` does not work
  let r = p.match(str)

proc workAround() =  
  parse(string, "") # Any valid type used here will make this work with all other valid types.
# b.nim

import ./a

parse(int, "test")

The operators defined with the templates above are the ones that I've found need to have declarations. I previously mentioned ! and ? but it's not actually the case. So it is any Nim operator which expects more than one argument. $ isn't useful unless all of the code is in a single module. But it is necessary.

The discard patt "test" does indeed make that example work. However if you try to capture with > it breaks again.

@zevv
Copy link
Owner

zevv commented Oct 3, 2023

Glad to hear you got it all to work, that was quite a journey!

If it's ok with you I'll close the issue for now, but I'll add a link to it from the documentation in case anyone else runs into the same problem.

@kerrycobb
Copy link
Author

Sounds good to me. Thanks again for your efforts!

@zevv zevv closed this as completed Oct 3, 2023
@zevv
Copy link
Owner

zevv commented Oct 3, 2023

github-merge-queue bot pushed a commit to nim-works/nimskull that referenced this issue Oct 3, 2023
## Summary

Unmatched `immediate` `macro`s and `template`s within a generic context
no longer result in a compiler error.

## Details

Immediate macros and templates are not dispatched via `sigmatch` and
instead `semgnrc` directly calls `evaltempl.evalTemplate`, in turn
`evalTemplateArgs` is called which can produce an error if an incorrect
number of arguments are provided. `evalTemplateArgs` is also used by
`semMacroExpr`, so similar issues would also exist for macros.

Now the error from `evalTemplateArgs` is immediately returned by
`evalTemplate` and treated as a mismatch in `semgnrc.semGenericStmt`.
Doing so prevents a compiler crash, attempt to access the sons of an
error
node. A test has been added as part of this change, which was
originally
from: zevv/npeg#68 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants