Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the LSP diagnostics for rusty_lr by including the specific shift and reduce rules involved in conflicts. It introduces helper functions to format LR(0) items and productions, updates the conflict message formatting, and adds comprehensive unit tests to verify the diagnostic output. The reviewer pointed out potential panic risks in format_lr0_item and format_production due to direct indexing of grammar.builder.rules, which could crash the LSP server. It is recommended to use .get() to access these rules safely and handle the None cases gracefully.
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.
| fn format_lr0_item(grammar: &Grammar, item: LR0ItemRef) -> String { | ||
| grammar.builder.rules[item.production_idx] | ||
| .rule | ||
| .clone() | ||
| .map( | ||
| |class| grammar.class_pretty_name_list(class, 5), | ||
| |nonterm| grammar.nonterm_pretty_name(nonterm), | ||
| ) | ||
| .into_shifted(item.dot) | ||
| .to_string() | ||
| } |
There was a problem hiding this comment.
Direct indexing of grammar.builder.rules with item.production_idx can cause a panic if the index is out of bounds. Since this code runs inside an LSP server, any panic will crash the entire language server process, resulting in a poor user experience. It is safer to use .get() and handle the None case gracefully.
fn format_lr0_item(grammar: &Grammar, item: LR0ItemRef) -> String {
if let Some(rule) = grammar.builder.rules.get(item.production_idx) {
rule.rule
.clone()
.map(
|class| grammar.class_pretty_name_list(class, 5),
|nonterm| grammar.nonterm_pretty_name(nonterm),
)
.into_shifted(item.dot)
.to_string()
} else {
format!("<unknown rule {}>", item.production_idx)
}
}| fn format_production(grammar: &Grammar, rule: usize) -> String { | ||
| grammar.builder.rules[rule] | ||
| .rule | ||
| .clone() | ||
| .map( | ||
| |class| grammar.class_pretty_name_list(class, 5), | ||
| |nonterm| grammar.nonterm_pretty_name(nonterm), | ||
| ) | ||
| .to_string() | ||
| } |
There was a problem hiding this comment.
Direct indexing of grammar.builder.rules with rule can cause a panic if the index is out of bounds. To prevent crashing the LSP server, use .get() to access the rule safely.
fn format_production(grammar: &Grammar, rule: usize) -> String {
if let Some(rule_info) = grammar.builder.rules.get(rule) {
rule_info.rule
.clone()
.map(
|class| grammar.class_pretty_name_list(class, 5),
|nonterm| grammar.nonterm_pretty_name(nonterm),
)
.to_string()
} else {
format!("<unknown rule {rule}>")
}
}
Summary