dundalek / GrammKit

Generate diagrams for parser grammars
http://dundalek.com/GrammKit/
The Unlicense
236 stars 19 forks source link

Cannot read property 'type' of undefined #14

Closed ldegen closed 5 years ago

ldegen commented 5 years ago

Hi, I am running into an issue after upgrading from 0.3.1 to 0.6.3. Most of my grammar still works, but GrammKit chokes on one particular rule. I modified the example from README.md to reproduce the problem:

var grammkit = require('grammkit');
var parse = require('pegjs/lib/parser').parse;

var grammar = parse('Atom "_atom"\n  = !\'AND\' !\'OR\' !\'NOT\' [^\\t\\n\\r ()]+ { return text().trim();}');
console.log(grammkit.diagram(grammar.rules[0]));

This produces the following error:

…/node_modules/grammkit/lib/rd-optimize-loops.js:4
  if (node.type === 'Sequence') {
           ^

TypeError: Cannot read property 'type' of undefined
    at traverse (…/node_modules/grammkit/lib/rd-optimize-loops.js:4:12)
    at Array.map (<anonymous>)
    at traverse (…/node_modules/grammkit/lib/rd-optimize-loops.js:45:33)
    at Array.map (<anonymous>)
    at traverse (…/node_modules/grammkit/lib/rd-optimize-loops.js:45:33)
    at Array.map (<anonymous>)
    at traverse (…/node_modules/grammkit/lib/rd-optimize-loops.js:5:31)
    at Array.map (<anonymous>)
    at traverse (…/node_modules/grammkit/lib/rd-optimize-loops.js:45:33)
    at diagramRd (…/node_modules/grammkit/lib/diagram.js:6:10)

I'll try to dig a bit deeper, but maybe you already have an idea?

Cheers, --lu

ldegen commented 5 years ago

Actually, the following is enough to reproduce the error:

Atom = [^\t\n\r ()]+

It seems that https://github.com/dundalek/GrammKit/blob/5a5c4b9bdaf8fa329ef551459a202e90862fe2ed/lib/peg-rd.js#L50 expects the presence of an attribute rawText, which pegjs does not produce. Maybe this isn't actually a change in GrammKit, but one in pegjs (I upgraded both dependencies) that triggers this.

ldegen commented 5 years ago

It seems pegjs has removed that from its own grammar parser. But all the information relevant to GrammKit is actually still there. Have a look at https://github.com/pegjs/pegjs/blob/c17e9b2df7e10ae60af2b642172fe7fd2f7c589d/src/parser.pegjs#L343

I think it would be possible to re-synthesize the text that should be displayed in the diagram. I'll try to put this into a PR.

ldegen commented 5 years ago

@dundalek regarding your comments to #15:

After one night of sleep, I think fixing the version alone would not really solve the problem. The actual problem I think is that GrammKit depends on non-public API (i.e. the internal parser tree from pegjs), that may change without notice even in patch level increments. OTOH, the grammar for the grammar files is public API so it should be fairly stable. The closest thing to a specification they have is this: https://pegjs.org/documentation#grammar-syntax-and-semantics. So why not depend on that? You could just implement your own parser for this grammar, which should be easy -- you can start by copy-pasting their parser, but you can modify its semantics to produce whatever AST you need. It should also be easy enough to accommodate for changes in their grammar, since your own parser will probably always be structurally very close to theirs.

Anyway, these are just my thoughts. :-) For the time being I will heed your advice and downgrade to pegjs-0.8.0.

ldegen commented 5 years ago

fyi: pegjs-0.9.0 does not trigger this particular problem; I guess the breaking change was introduced in 0.10.0.

dundalek commented 5 years ago

Hi @ldegen, I incorporated your fix and made other changes to upgrade to pegjs-0.10.0. Please let me know if you encounter other issues.