Refine feed planning and recovery strategy#95
Conversation
ehwan
commented
Jul 4, 2026
- Add deterministic pre-feed planning so feed_location simulates CFG-level reductions/shifts first and only mutates stacks after a valid terminal shift is confirmed.
- Reuse the same deterministic planning path for can_feed, can_accept, can_panic, and synthetic error token recovery.
- Restrict recovery to true NoAction cases: GLR recovery now runs only when every active branch is grammatically unable to consume the terminal.
- Change GLR feed success to return FeedSuccess, allowing successful feeds to report errors from pruned sibling branches.
- Remove NoPrecedence, remove GLR state-0 error recovery, and shrink internal GLR failure payloads to avoid unnecessary cloning.
There was a problem hiding this comment.
Code Review
This pull request refactors the token feeding mechanism in both deterministic and GLR parsers to use a pre-feed simulation phase. In deterministic parsing, this prevents partial stack mutations on grammatical failures. In GLR parsing, feeding now returns a FeedSuccess struct containing optional pruned-branch errors, and panic-mode recovery is only entered when all branches fail with NoAction. The review feedback suggests optimizing performance in the GLR branch loop by hoisting the extra_state_stack allocation out of the loop, and adding a defensive check in the deterministic pre-feed simulation to prevent potential integer underflow.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut feedable_branch_seen = false; | ||
| for Branch { node, userdata } in current_branches.drain(..) { | ||
| if let Err((node, _, _, userdata)) = self.feed_location_impl( | ||
| node, | ||
| TerminalSymbol::Terminal(term.clone()), | ||
| class, | ||
| location.clone(), | ||
| userdata, | ||
| ) { | ||
| // store to fallback nodes in case of all nodes failed to shift | ||
| // Classify this branch before mutating the graph-structured stack. Only branches that | ||
| // fail this CFG simulation are considered `NoAction` branches. | ||
| let mut extra_state_stack = Vec::new(); |
There was a problem hiding this comment.
Allocating a new Vec for extra_state_stack inside the current_branches loop can be inefficient, especially when there are many active branches or during frequent token feeding. Moving the allocation outside of the loop and clearing it on each iteration avoids unnecessary reallocations.
let mut feedable_branch_seen = false;
let mut extra_state_stack = Vec::new();
for Branch { node, userdata } in current_branches.drain(..) {
// Classify this branch before mutating the graph-structured stack. Only branches that
// fail this CFG simulation are considered `NoAction` branches.
extra_state_stack.clear();| if tokens_len <= extra_state_stack.len() { | ||
| extra_state_stack.truncate(extra_state_stack.len() - tokens_len); | ||
| } else { | ||
| let left = tokens_len - extra_state_stack.len(); | ||
| extra_state_stack.clear(); | ||
| stack_len -= left; | ||
| } |
There was a problem hiding this comment.
To prevent potential integer underflow panics in debug mode (or wrapping in release mode) when left > stack_len, add a defensive check to ensure left <= stack_len before performing the subtraction.
if tokens_len <= extra_state_stack.len() {
extra_state_stack.truncate(extra_state_stack.len() - tokens_len);
} else {
let left = tokens_len - extra_state_stack.len();
extra_state_stack.clear();
if left > stack_len {
return Err(FeedPlanError {
state: self.simulated_state(&extra_state_stack, stack_len),
});
}
stack_len -= left;
}