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
|
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.
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