hazel
hazel copied to clipboard
hazelgrove
Integrate menhir parser and elaborator tests
@green726 are you taking a look at this PR or what is the plan? @7h3kk1d
This was still in dev from my perspective but @green726 is going to take over it from now and let me know if they need assistance.
Sounds good, thanks for the feedback. I'll make those syntactic changes and wait for the new UExp/DHExp to update then I'll mark as ready for review again.
@cyrus- sure, I can. Are there any other changes you would like me to make?
@green726 getting it up to date and writing more tests demonstrating all of the syntactic forms in the parser would be great. I can give it another review once that works. maybe @7h3kk1d can take a look as well?
@green726 I believe I saw failures similar to your last commit when my local reason version was out of sync with make deps. Let me know if you need any help debugging.
@7h3kk1d Ah, thank you! How were you able to fix it? It's interesting that its the formatter that throws the error rather than the compiler.
Oh, nevermind about that error. It seems it was just do to a "let" that should was not commented out and should have been. Thank you though!
Not sure if there's a reason it wouldn't work but could we move the core -> menhir conversion functions out of core and into the menhir package and flip the dependency order?
I could also be missing something that's already been talked about.
Not sure if there's a reason it wouldn't work but could we move the core -> menhir conversion functions out of core and into the menhir package and flip the dependency order?
I could also be missing something that's already been talked about.
When I first added the menhir parser I believe there was a reason I ordered the dependencies in this way (iirc there was some conflict caused if it went the opposite direction). It's possible that I'm either misremembering, something has changed now that would make it possible, or I just simply did something wrong initially. I can take a look back through and see if there's any reason why it can't be in menhir (did you see anything that jumped out?)
Not sure if there's a reason it wouldn't work but could we move the core -> menhir conversion functions out of core and into the menhir package and flip the dependency order? I could also be missing something that's already been talked about.
When I first added the menhir parser I believe there was a reason I ordered the dependencies in this way (iirc there was some conflict caused if it went the opposite direction). It's possible that I'm either misremembering, something has changed now that would make it possible, or I just simply did something wrong initially. I can take a look back through and see if there's any reason why it can't be in menhir (did you see anything that jumped out?)
I did not but I didn't look closely
There's also some warnings we want to address:
dune build @src/fmt --auto-promote src --profile dev
File "src/haz3lmenhir/Parser.mly", line 35, characters 7-9:
Warning: the token AS is unused.
File "src/haz3lmenhir/Parser.mly", line 14, characters 7-22:
Warning: the token BAD_CONSTRUCTOR is unused.
File "src/haz3lmenhir/Parser.mly", line 76, characters 7-12:
Warning: the token B_AND is unused.
Warning: 24 states have shift/reduce conflicts.
Warning: 2 states have reduce/reduce conflicts.
Warning: 454 shift/reduce conflicts were arbitrarily resolved.
Warning: 85 reduce/reduce conflicts were arbitrarily resolved.
It also might be useful to pull in the coverage branch to see which forms haven't been exercised.
Sorry, but what does the coverage branch do and what does it mean for a form to be "exercised"? Sorry, I'm not too good with the hazel terminology :). Thank you!
There's also some warnings we want to address:
dune build @src/fmt --auto-promote src --profile dev File "src/haz3lmenhir/Parser.mly", line 35, characters 7-9: Warning: the token AS is unused. File "src/haz3lmenhir/Parser.mly", line 14, characters 7-22: Warning: the token BAD_CONSTRUCTOR is unused. File "src/haz3lmenhir/Parser.mly", line 76, characters 7-12: Warning: the token B_AND is unused. Warning: 24 states have shift/reduce conflicts. Warning: 2 states have reduce/reduce conflicts. Warning: 454 shift/reduce conflicts were arbitrarily resolved. Warning: 85 reduce/reduce conflicts were arbitrarily resolved.
Yes, I'll look at the grammar and see if I can fix those 24 ambiguities. I'll let you know once I've pushed a commit that fixes them.