Removed inner Context wrapping & add generic based start extractor#86
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the parser stack representation in rusty_lr by replacing the custom DataStack trait with a new SemanticValue trait and a StartExtractor trait. This allows the deterministic and non-deterministic Context implementations to store semantic values directly in a standard Vec, simplifying the generated code structure and removing the need to generate individual DataStack wrapper structs. Additionally, the documentation, examples, and tests have been updated to reflect these changes. Feedback on this PR suggests improving error handling in the deterministic Context::accept method by replacing a generic .unwrap() with a descriptive panic message via .unwrap_or_else(), ensuring consistency with the non-deterministic implementation.
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.
| Ok((self.data_stack.pop_start().unwrap(), self.userdata)) | ||
| // pop eof | ||
| self.data_stack.pop(); | ||
| let start = self.data_stack.pop().unwrap(); |
There was a problem hiding this comment.
Using .unwrap() here can lead to a generic panic without context if the stack is unexpectedly empty. It is safer and more consistent with the non-deterministic context implementation (and the very next line) to use .unwrap_or_else() with a descriptive panic message.
let start = self.data_stack.pop().unwrap_or_else(|| {\n panic!(\n \"Accepted path for virtual start branch {} has no start value\",\n Start::BRANCH_INDEX\n )\n });
1. Transition of DataStack -> SemanticValue
DataStacktrait and replaced it with theSemanticValuetrait.DataStackwas a trait representingVec<Enum Value>, the newSemanticValuerepresents a singleEnum Value.new_empty(),new_terminal( term ), andreduce_action()functions.Contextwill now holdVec<SemanticValue>instead ofDataStack.Contextwrapper was created only when there were two or morestart_rule_names. However, sinceSemanticValuealone cannot determine theStartTypeor extract the inner value, aContextwrapper will now be created even if there is only onestart_rule_name.acceptandaccept_allwithin the wrapper.accept, it callsfeed_eof(), pops one EOF, pops one value, and then extracts and returns the value corresponding toStartusing amatchexpression on the spot.Contextwrapper is implemented regardless of the number ofstart_rule_names, the conditionalifbranching based on the count ofstart_rule_namesinemit.rswas removed.2. Removed inner Context wrapping & add generic based start extractor
Context. Instead, decided to simply apply an alias to the originalrusty_lr_core'sContext.Contextso they are not visible to the user.StartType-related elements into Generics.Startdata fromSemanticValueinsideaccept, created a separate trait or struct inemit(e.g.,<Start1>Extracter,<Start2>Extracter) and hadContexthold it.CloneorDisplayforContextcan be automatically handled by moving them to trait bounds.