Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces automatic expansion of safe, unobserved optional (P?) occurrences in GLR grammars to prevent zero-consuming reduce cycles, and reports unexpandable nullable cycles as generation errors. The changes span the build script, derive macro, LSP diagnostics, and parser. The review comments suggest two valuable optimizations: first, batching the expansion of optional helpers in expand_glr_epsilon_self_loop_optionals to avoid repeatedly rebuilding the state machine; second, deduplicating the error formatting logic for ParseError::GlrNullableReduceCycle in the build script.
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 rewrite_target = None; | ||
| 'states: for (state_idx, state) in states.iter().enumerate() { | ||
| for reduce_rules in state.reduce_map.values() { | ||
| for &rule_idx in reduce_rules { | ||
| let Some((nonterm_idx, local_rule_idx)) = self.rule_id_to_indices(rule_idx) | ||
| else { | ||
| continue; | ||
| }; | ||
| if !self | ||
| .is_optional_epsilon_self_loop_candidate(nonterm_idx, local_rule_idx) | ||
| { | ||
| continue; | ||
| } | ||
| if state | ||
| .shift_goto_map_nonterm | ||
| .iter() | ||
| .any(|(nonterm, target)| { | ||
| *nonterm == nonterm_idx && *target == state_idx | ||
| }) | ||
| { | ||
| rewrite_target = Some(nonterm_idx); | ||
| break 'states; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let Some(optional_idx) = rewrite_target else { | ||
| break; | ||
| }; | ||
| if !self.expand_optional_occurrences(optional_idx) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The current implementation of expand_glr_epsilon_self_loop_optionals runs a loop up to self.nonterminals.len() times, and in each iteration, it builds the entire LR/LALR state machine just to find and expand a single optional helper. For larger grammars, building the state machine is a very expensive operation.\n\nWe can significantly optimize this by collecting all candidate optional helpers that participate in a self-loop in the current state machine, and expanding all of them in a single pass before rebuilding the state machine. This reduces the number of state machine builds from O(K) (where K is the number of self-looping optionals) to O(1) in almost all cases.
let mut rewrite_targets = BTreeSet::new();\n for (state_idx, state) in states.iter().enumerate() {\n for reduce_rules in state.reduce_map.values() {\n for &rule_idx in reduce_rules {\n let Some((nonterm_idx, local_rule_idx)) = self.rule_id_to_indices(rule_idx)\n else {\n continue\n };\n if !self\n .is_optional_epsilon_self_loop_candidate(nonterm_idx, local_rule_idx)\n {\n continue\n }\n if state\n .shift_goto_map_nonterm\n .iter\n .any(|(nonterm, target)| {\n *nonterm == nonterm_idx && *target == state_idx\n })\n {\n rewrite_targets.insert(nonterm_idx);\n }\n }\n }\n }\n\n if rewrite_targets.is_empty() {\n break;\n }\n let mut changed_any = false;\n for optional_idx in rewrite_targets {\n if self.expand_optional_occurrences(optional_idx) {\n changed_any = true;\n }\n }\n if !changed_any {\n break;\n }| ParseError::GlrNullableReduceCycle { | ||
| location, | ||
| nullable_rule, | ||
| state, | ||
| reason, | ||
| help, | ||
| } => { | ||
| let range = span_manager.get_byterange(&location).unwrap_or(0..0); | ||
| Diagnostic::error() | ||
| .with_message("GLR nullable reduce cycle cannot be expanded safely") | ||
| .with_labels(vec![Label::primary(file_id, range).with_message( | ||
| "this nullable production participates in a zero-consuming GLR cycle", | ||
| )]) | ||
| .with_notes(vec![ | ||
| format!( | ||
| "reducing `{nullable_rule}` in state {state} returns to the same state without consuming input" | ||
| ), | ||
| format!("why RustyLR cannot rewrite it automatically: {reason}"), | ||
| format!("how to fix it: {help}"), | ||
| ]) | ||
| } |
There was a problem hiding this comment.
This error formatting block for ParseError::GlrNullableReduceCycle is identical to the one defined at lines 892-912. To improve maintainability and avoid code duplication, consider extracting this formatting logic into a helper function or method on Builder (e.g., fn format_glr_nullable_cycle_error(...) -> Diagnostic<usize>).
close #91
P?occurrences when their empty branch would create a zero-consuming GLR reduce self-loop.glr_optional_expandedinfo diagnostics showing the original production and generated replacement productions.P?,P*, and user-defined nullable productions.