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

Error thrown for some rules with single elemets #32

Closed
nasser opened this issue Sep 3, 2018 · 4 comments
Closed

Error thrown for some rules with single elemets #32

nasser opened this issue Sep 3, 2018 · 4 comments

Comments

@nasser
Copy link

nasser commented Sep 3, 2018

These work:

grammar test
number <-  [0-9]? "foo" %action
grammar test
number <-  [0-9]+ %action
grammar test
number <-  [0-9]?

But this fails with a grammar parsing error

grammar test
number <-  [0-9]? %action
$ canopy grammar.peg
Line 2: expected [\s], "#", [a-zA-Z_], "(", "&", "!", '"', "'", "`", "[", ".", "<", "/"
number <-  [0-9]? %action
                  ^
SyntaxError: Line 2: expected [\s], "#", [a-zA-Z_], "(", "&", "!", '"', "'", "`", "[", ".", "<", "/"
number <-  [0-9]? %action
                  ^
    at Parser.parse (/usr/lib/node_modules/canopy/lib/canopy.js:2788:11)
    at Object.parse (/usr/lib/node_modules/canopy/lib/canopy.js:2794:19)
    at Canopy.Compiler.parseTree (/usr/lib/node_modules/canopy/lib/canopy.js:4527:20)
    at Canopy.Compiler.toSource (/usr/lib/node_modules/canopy/lib/canopy.js:4539:10)
    at Object.compile (/usr/lib/node_modules/canopy/lib/canopy.js:19:21)
    at Object.<anonymous> (/usr/lib/node_modules/canopy/bin/canopy:25:24)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
@jcoglan
Copy link
Owner

jcoglan commented Aug 3, 2020

@nasser Sorry it took me so long to respond to you. There's a reason this doesn't work but it's also prompted me to add support for the thing you're trying to do.

Actions are for constructing new nodes, i.e. instead of Canopy constructing its own parse tree nodes, it delegates to your action to do that. Therefore they only apply to rules that make new nodes which are:

  • terminals like "string", [chars] or .
  • sequences
  • repetitions

They don't apply to the ? operator because this doesn't construct any nodes itself -- the expression before it constructs a node, and the ? operator just means it's fine if that expression doesn't match anything.

So for example in ("hello world")?, the terminals "hello" and "world" each construct a node, and the sequence ("hello" "world") constructs a new node by combining those, but then the ? operator just passes the sequence node through rather than building any nodes itself. The result of expr? is either the node that expr creates, or nothing.

However I think there is possibly an ergonomic benefit in making expr? %action work, and based on your examples I think it would make sense to interpret it as (expr %action)?. That is, action is used to build the node for expr if it matches, and then ? returns the result if any.

Does that make sense? I've just pushed ca4041d which implements this functionality and I'd value your feedback.

@byteit101
Copy link

I just ran into this too. The ca4041d commit solves some of the problems, but my brief attempts to fix the others didn't work as I expected.

I have a rule of the form:

grammar test
root <- number %action2 
number <-  "NaN" %action1

I was able to get it to parse last night by a similar approach to ca4041d, but not to generate a call to action2.

byteit101 added a commit to byteit101/canopy that referenced this issue May 6, 2021
@byteit101
Copy link

I managed to figure out an approach to solve this that will be fixed by #49

@jcoglan
Copy link
Owner

jcoglan commented Sep 26, 2021

I'm going to close this because I believe the central problem identified in this issue (making %action work with rule?) has been addressed. Anyone that wants to discuss expanding the scope of this to references should comment on #49.

@jcoglan jcoglan closed this as completed Sep 26, 2021
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

3 participants