-
Notifications
You must be signed in to change notification settings - Fork 286
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
Insert an empty lookahead in the lane table in reduce conflict states #898
Insert an empty lookahead in the lane table in reduce conflict states #898
Conversation
We need to make sure the state in the conflict is in the lane table. All the existing test cases happened to have a shift case in the conflict state, even if the shift wasn't part of the actual conflict. In the event of a conflicting state with only reduce actions, we'll need the lookahead store, so we can check it as a successor of the predecessors we found. fixes lalrpop#897
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm motivated to have this merged in but it would be nice to have an additional set of eyes just in case this is somehow "too easy" of a fix.
Wow, yeah, I guess there was quite the history of this. Those bugs do all look like the same issue. It will be good to have it all fixed. I agree about wanting more eyes. I do think the change makes sense, and the new behavior here seems to agree with the documentation, Niko's lane table blog post, and the original lane table paper as much as I understand them. That said, this is my first time diving deep into the details of the lane table algorithm, so I'm far from a lane table expert. It would be nice if we could get @nikomatsakis to take a look at this, since he's got to be the maintainer with the most in depth knowledge of the lane table algorithm. |
This is one of those classic cases -- a one-line diff but maybe big implications :) |
Man, I had to do quite some work to bring this back into cache. I'm glad that I left as many comments as I did! I still haven't reconstructed the full algorithm in my head but, from what I can see, this change appears pretty safe and quite reasonable. Adding an empty entry into the conflict table seems like it should basically always be an ok thing to do. I don't yet see the complete line in the code that leads from this entry not existing to the panic -- I guess that somehow either lalrpop/lalrpop/src/lr1/lane_table/construct/merge.rs Lines 203 to 204 in 7bc39d3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I worked this through a bit more in my head and I feel comfortable with this change. Nice work, @dburgener!
Thanks, @nikomatsakis!
Not sure if your final comment means you're good on this now, but yes, my understanding is that you are correct here. In the common case, the predecessors of the conflicting state get added with it as successors here. We later start walking the lane table, which gets us to this line, which panics, because the original state is a successor of something else, but was never inserted itself. In all our previously existing tests, the conflicting state had a reduce/reduce conflict, but there was also a non-conflicting possible shift from that state, which resulted in the state being stored in the lane table. But there's nothing to say there must also be a shift, that was just a coincidence of the provided grammars. |
Thanks for the final pointers. I was happy with the change but I didn't see *exactly* which lines led to the failure (I figured they had to be there...)
…On Fri, May 24, 2024, at 10:33 AM, Daniel Burgener wrote:
Thanks, @nikomatsakis <https://github.com/nikomatsakis>!
> I don't yet see the complete line in the code that leads from this entry not existing to the panic -- I guess that somehow either source or target is otherwise not present in the Map here? I'm not sure where that occurs exactly.
>
Not sure if your final comment means you're good on this now, but yes, my understanding is that you are correct here. In the common case, the predecessors of the conflicting state get added with it as successors here <https://github.com/lalrpop/lalrpop/blob/85361447bf6710875dfd0d3c42d61e31b2c54e79/lalrpop/src/lr1/lane_table/lane/mod.rs#L102>. We later start walking the lane table <https://github.com/lalrpop/lalrpop/blob/85361447bf6710875dfd0d3c42d61e31b2c54e79/lalrpop/src/lr1/lane_table/construct/mod.rs#L199>, which gets us to this line <https://github.com/lalrpop/lalrpop/blob/85361447bf6710875dfd0d3c42d61e31b2c54e79/lalrpop/src/lr1/lane_table/construct/merge.rs#L204>, which panics, because the original state is a successor of something else, but was never inserted itself.
In all our previously existing tests, the conflicting state had a reduce/reduce conflict, but there was also a non-conflicting possible shift from that state, which resulted in the state being stored in the lane table. But there's nothing to say there must also be a shift, that was just a coincidence of the provided grammars.
—
Reply to this email directly, view it on GitHub <#898 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZVZM5I5QD4445IOP4LZD5FSXAVCNFSM6AAAAABH4GPUTCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRZGY3TKMZVGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
We need to make sure the state in the conflict is in the lane table. All the existing test cases happened to have a shift case in the conflict state, even if the shift wasn't part of the actual conflict. In the event of a conflicting state with only reduce actions, we'll need the lookahead store, so we can check it as a successor of the predecessors we found.
fixes #897