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

arrow notation barfs on comments #51

Open
ericprud opened this issue Oct 6, 2019 · 8 comments
Open

arrow notation barfs on comments #51

ericprud opened this issue Oct 6, 2019 · 8 comments
Labels

Comments

@ericprud
Copy link

ericprud commented Oct 6, 2019

zaaaach/jison parses

expr: 'foo' -> 1 // comment
;

jison-gho 0.6.1-216 and master fail if there's a comment following the semantic action.

(zaaaaach also parses extra ';'s

expr: 'foo' -> 1; // comment
;

but I think that's a bug.)

@GerHobbelt GerHobbelt added the bug label Oct 6, 2019
@GerHobbelt
Copy link
Owner

GerHobbelt commented Oct 6, 2019

I would consider the latter variant a grammar bug as

expr: 'foo' -> 1 // comment
;

SHOULD be parsed by the machine as semantic action 1 // comment, i.e. a action including comment, while

expr: 'foo' -> 1; // comment

(note the edit) SHOULD be parsed by the machine as semantic action 1 in a jison grammar rule, which itself is now followed by the comment comment, which is summarily ignored.

Good catch, by the way. Must find out where this went wrong. Without looking, my bet is that it's the -> semaction coding style parse section that doesn't properly consider comments as part of such single-line single-expression actions.

That hunch suggest you might also try this, which I expect should also fix the issue:

  • this should fly

    expr: 'foo' -> { 1 // comment }
    ;
    
  • and then this one should as well:

    expr: 'foo' -> { 1; // comment }
    ;
    

    which incidentally makes it visually more obvious where the semicolon after 1 belongs. (🤔
    Sunday morning and no idea how to verbalize this more intelligently. 🤔 )

@ericprud
Copy link
Author

ericprud commented Oct 6, 2019

For now, I continued all of my comments on the following line a la:

nonLiteralKind:
      IT_IRI	-> { nodeKind: "iri" }
// CONT  t: 1iriPattern
    | IT_NONLITERAL	-> { nodeKind: "nonliteral" }
// CONT  t: 1nonliteralLength
    ;

It's reversible so I can make my way back when the parser's happy with arrows with comments.

Tx for being so attentive!

ericprud pushed a commit to shexjs/shex.js that referenced this issue Oct 12, 2019
@ericprud
Copy link
Author

ericprud commented Nov 6, 2020

Can I help with this? I'm typically pingable on gitter 9:00-19:00, 21:30-24:00 CEST.

@GerHobbelt
Copy link
Owner

GerHobbelt commented Nov 6, 2020 via email

@ericprud
Copy link
Author

ericprud commented Nov 6, 2020

I'll be around. Not much leaving the house these days.

@GerHobbelt
Copy link
Owner

GerHobbelt commented Nov 16, 2020

Ok. This took a lot longer than intended. Had the overestimated confidence that I would be able to run a few jison-gho tests in a few hours. That turned out into a big fight with the bleeding edge codebase which didn't want to cooperate after a long bout of inattentiveness, so I had to try a few spots, ending up with having to roll the last release forward in micro steps as the dev box node env has changed over the months (year-plus!) since it got a good workout last.

Currently I have (should push this by now 🤔 [Edit: done] ) a bleeding edge which is halfway there (latest lexer grammar working, latest BNF parser grammar failing badly and therefor rolled back to bnf parser from last release plus some minimal changes) and compiling grammars again.

The good news -- though most of it unrelated to your problem yet

  • working bleeding edge is now ES6 capable; one can use let and const in user action blocks without jison-gho generating parsers with fatal let variable collision failures.
  • arrow functions are now at least processed somewhat decently when there's a comment around, BUT

The bad news

  • current bleeding edge DOES NOT cope at all with curly braces in arrow actions
  • and it needs a BNF parser spec overhaul (that is half-way done in the totally buggered old mainline bleeding edge which started this fight) to get right.

Ergo: this issue will need some more work before I'ld call it anywhere near "done".


Meanwhile, thank you VERY MUCH for your patience-in-the-extreme. Wow. Sticking around for a year while I'm silent is impressive (for lack of better words). I cannot guarantee better performance in the future but I don't know how else I can convey to you that I am humbled. I can only hope jison-gho is still useful to you despite the chaotic bits.


If you feel extremely daring, the bleeding edge I'll push in a few minutes today will be in the t3 branch -- master still is a total b0rk b0rk b0rk today -- but when you go in there and run make to build a jison-gho of your own, be VERY aware that jison bootstraps itself by using the jison installed by npm in node_modules/jison-gho/ and that will FAIL with horrid errors unless some very nasty patching is done until the next official release of jison-gho (which will be a 0.7.0-something; too much has changed since 0.6.x) -- that patching action has to be documented in CONTRIBUTING.md tonight!


That's the current state of affairs; things are moving, possibly a glacial pace, but at least now you know a bit about what going on.

HTH

GerHobbelt added a commit that referenced this issue Nov 16, 2020
GerHobbelt added a commit that referenced this issue Nov 16, 2020
- use the pre-patched jison from the previous commit to build these as using an older jison won't fly. Hence you MUST copy the dist/ directory to node_modules/jison-gho/dist/ before running `make` or your ass is graas.
- patch the code generator(s) to check if action code chunks include `let` or `const` keywords and if they do, wrap them in a {...} scope block to prevent collisions with other action code blocks in the same generated large switch/case statement.
- drop ES5 usage in the build paths everywhere. This means we're not testing the validity of the BABEL output anymore. ES5 is on the way out.
- **WARNING/NOTICE**: this dist/ is KNOWN-GOOD-ENOUGH for bootstrapping jison. Bootstrapping jison with ANY EARLIER BUILD WILL FAIL due to the ES6 features used in the action code blocks and the way the older versions generated lexer/parser code, which will result in load-time fatal collisions reported by `node` as multiple `let` statements will compete for supremacy.
- issue 51 examples have been tweaked to compile and run a basic test run -- at least sample 1 does, while the others still fail, even when marked GOOD: to make those work we'll need the bnf.y grammar overhaul from master and then some extra work.
- while this /dist/ is known-good-enough, arrow-actions in the /examples/ grammars will fail to generate a viable parser due to lingering overzealous {...} brace cleanup code that's been in jison-gho for quite a while: hence #51 is DEFINITELY NOT FIXED YET!
- augmented the bnf.y grammar spec to produce a slightly less horrid and unintelligible error report for arrow-action code blocks which fail to test-compile: you'ld often get error reports about unexpected () braces, which are NOT in your action code line(s) but in the auto-generated wrapper instead -- which is required to test-compile the arrow-action code.
GerHobbelt added a commit that referenced this issue Nov 16, 2020
…lled `braceArrowActionCode()`

- micro step towards merging this branch with the (currently still faulty!) master branch and make that one the main dev line once again.
  + this will take some significant migratory work on bnf.l + bnf.y before we can merge branches, though! master has a significantly advanced, yet buggy, new grammar spec, which never got finished as the plugs got pulled due to RL developments which are nearing the end after a year now   |:-(
- regenerated library files
- updated test reference files
  - as usual done brute-force via `make clean-dumpfiles ; make`
- removing testset file lists as globby is back in action -- see also sindresorhus/globby#155 (comment)
- added another test example grammar for #51 ; also added the required input file and minimal lexer for producing a Minimum Viable Test Product running the generated grammars (if there's any success there)
@GerHobbelt
Copy link
Owner

In case you feel extremely daring as mentioned above, you'll need this for hints on that /node_modules/jison-gho/dist/ bootstrapping process you will need while riding the very edge of the bleeding edge:
https://github.com/GerHobbelt/jison/blob/t3/CONTRIBUTING.md

@ericprud
Copy link
Author

ericprud commented Nov 20, 2020

and I applaud your persistence in picking up a cold project. I'll try to fiddle around with the grammar in the bleeding edge and maybe even add its build products into a branch of shex.js in order to see how those hacks fares there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants