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

Build failure only when --optimize-grammar is set; grammar with not referenced rule #70

Open
breml opened this issue Jul 17, 2018 · 5 comments · Fixed by #83
Open

Build failure only when --optimize-grammar is set; grammar with not referenced rule #70

breml opened this issue Jul 17, 2018 · 5 comments · Fixed by #83

Comments

@breml
Copy link
Collaborator

breml commented Jul 17, 2018

The following grammar results in build failure, if generated with --optimize-grammar. The same grammar builds with out problem, if the option is not set.

Minimal example:

X ← z:Z
Y ← Z
Z ← ('Z' { return nil, nil })?

Build error:

(*parser).callonZ3 undefined (type *parser has no method callonZ3)
@mna
Copy link
Owner

mna commented May 9, 2019

Hello Lucas,

I ran into the same issue and created this repo with what I had as minimal reproducible grammar (yours is smaller, though): https://github.com/mna/pigeon-bug-optimize-grammar-2. I tested it with your fix in the pending PR and it does work, is there any side-effect preventing the fix from being merged?

Thanks,
Martin

@breml
Copy link
Collaborator Author

breml commented May 10, 2019

Hi @mna
If I remember correctly, the reason why I did not yet merge this PR is, that it only solves part of the problem. I had cases where the optimize still failed. That being said, we can merge this one and at least improve the situation, even though there are still failing (edge) cases.

@mna
Copy link
Owner

mna commented May 10, 2019

Ah, yeah indeed I tried it on my full grammar and it does fail elsewhere with this fix. Might be worth holding up, as it might break grammars that are otherwise working today (for those that didn't run into the issue that it solves). I'll try to find some time to investigate further next week.

Thanks,
Martin

@breml
Copy link
Collaborator Author

breml commented May 17, 2019

I found the old commit and my notes about the bigger problem I found when I wrote the PR #71.

The thing is, that the added search algorithm in ast_optimize.go is not able to find unnecessary rules, if these rules do form a cycle, but the entire rules cycle is no longer reachable from one of the entry points.
Such cycles can be produced by the grammar optimizer. I found such a case in the go-thrift grammer when I tried to use the optimize option. I reduced the grammar to the following minimal example, extracted out of the optimized grammar from go-thrift:

{
package issue70_cycle
}

TypeDef ← annotations:TypeAnnotations?

FieldType ← SubType

SubType <- typ:FieldType annotations:TypeAnnotations? 

TypeAnnotations ← annotations:TypeAnnotation*

TypeAnnotation ← value:(value:'X' { return value, nil })?

So the bottom line is, that the search algorithm needs to be extended with something like this:

  • start from the protected rules (entrypoints)
  • follow all the rule uses rules and mark the used rules
  • all the not marked rules can be deleted

@breml breml closed this as completed in #83 May 20, 2019
breml pushed a commit that referenced this issue May 20, 2019
Fix references when unused rules are removed
Add missing cloneExpr when optimizing ZeroOrOneExpr

Improves situation for #70
@breml
Copy link
Collaborator Author

breml commented May 20, 2019

Optimizer also needs to detect that rules can be dropped, if they are only referred to by itself (closed cycles), see comment.

@breml breml reopened this May 20, 2019
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

Successfully merging a pull request may close this issue.

2 participants