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

Reductions with empty productions #12

Closed
SquidDev opened this issue Jan 1, 2024 · 4 comments
Closed

Reductions with empty productions #12

SquidDev opened this issue Jan 1, 2024 · 4 comments

Comments

@SquidDev
Copy link
Contributor

SquidDev commented Jan 1, 2024

Apologies for the slew of issues here (though thank you for all the work you've done on fixing them! ❤️).

I'm continuing to update my current code to use the latest version of lrgrep, and have hit a problem where reductions for empty productions do not appear to be matched.

I've committed a small reproduction case, but just to explain what's going on here.

We have the following grammar which accepts any number of characters between two brackets (e.g. (), (a), (aaa)).

let sentence := "(" ; ~ = chars ; ")" ; EOF ; <>

let chars := ~ = list(char) ; <List.rev>
let char := ~ = C ; <>

I'd like to match sentences without a closing bracket, and so have the following rule:

rule error_message = parse error
| OPAREN ; [chars]
{ "Unclosed '('" }

While this does match sentences like (aaa, it does not match just (. Curiously this does work with LRgrep 2 (so using OPAREN ; chars ; !), so I'm assuming this is a regression rather than intentional behaviour?

Sorry I can't provide more info! Still going down the rabbit hole of trying to understand how lrgrep works.

@let-def let-def closed this as completed in d799170 Jan 2, 2024
@let-def
Copy link
Owner

let-def commented Jan 2, 2024

Thanks for the bug report (and cheers to 2024! :) ).

This is indeed a regression. I thought I had handled that case but it was subtler than I expected...

Long story short: when initiating a reduction there is a special case (a "limit condition") because we don't know anything about the state of the stack yet, so we have to peek at the first state, which is not part of the reduction but allow us to identify the possible reductions coming next. And the code was not handling this case correctly, because it forgot to "unconsume" this first state (the corresponding expression would have been something like OPAREN ; [OPAREN; chars], which is completely incorrect but is what the code was trying to match when chars is empty).
Yet another form of off-by-one :).

And looking at it again, I got an idea that might make it possible to get rid of this special case, I will experiment...

@let-def let-def reopened this Jan 2, 2024
@let-def let-def closed this as completed Jan 2, 2024
@let-def
Copy link
Owner

let-def commented Jan 2, 2024

And thanks for the tiny example, I will keep it in the code base it is very convenient for experimenting.

@let-def
Copy link
Owner

let-def commented Jan 2, 2024

I think I found a way to simplify this case, the current master has a more uniform treatment of reductions.

@SquidDev
Copy link
Contributor Author

SquidDev commented Jan 2, 2024

Thank you so much - just tried out current master and it works perfectly! Really liking the new syntax, I've managed to get rid of all my hacks, and there's some rules which are significantly cleaner now.

Happy new year to you too! :)

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

2 participants