Refactor Parser Trait to Static Methods and Parameterize Context with Parser Generic#58
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the rusty_lr API by removing the requirement to pass a parser instance reference to context methods (such as feed, feed_location, accept, can_feed, trace, and backtrace). Instead, the Context struct is now parameterized with the Parser type P, and parser tables/rules are accessed statically. The reviewer raised two critical architectural concerns: first, the new Clone implementation for nondeterministic Context unnecessarily clones transient fields, introducing a breaking Data::ReduceActionError: Clone bound that forces user-defined error types to implement Clone. Second, removing &self from get_rules and get_states forces a 'static lifetime bound on associated types, which prevents zero-copy parsing.
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.
| type Term: 'static; | ||
| /// The type of terminal classes. | ||
| type TermClass: terminalclass::TerminalClass<Term = Self::Term>; | ||
| type TermClass: terminalclass::TerminalClass<Term = Self::Term> + 'static; | ||
| /// The type of non-terminal symbols. | ||
| type NonTerm: nonterminal::NonTerminal; | ||
| type NonTerm: nonterminal::NonTerminal + 'static; | ||
| /// The type of the parser state. | ||
| type State: State<TermClass = Self::TermClass, NonTerm = Self::NonTerm>; | ||
| type State: State<TermClass = Self::TermClass, NonTerm = Self::NonTerm> + 'static; | ||
|
|
||
| /// Get list of production rules | ||
| fn get_rules(&self) -> &[crate::rule::ProductionRule<Self::TermClass, Self::NonTerm>]; | ||
| /// Get list of states | ||
| fn get_states(&self) -> &[Self::State]; | ||
| /// Get list of production rules. Returns a static slice since rules are generated at compile time. | ||
| fn get_rules() -> &'static [crate::rule::ProductionRule<Self::TermClass, Self::NonTerm>]; | ||
| /// Get list of states. Returns a static slice since states are generated at compile time. | ||
| fn get_states() -> &'static [Self::State]; | ||
| /// Get the type of precedence for i'th level. | ||
| /// `None` if i'th level was defined as %precedence (no reduce type). | ||
| fn precedence_types(&self, level: u8) -> Option<crate::rule::ReduceType>; | ||
| /// Determined entirely at compile time and is static. | ||
| fn precedence_types(level: u8) -> Option<crate::rule::ReduceType>; |
There was a problem hiding this comment.
By removing &self from get_rules and get_states and returning &'static slices, you have introduced a 'static lifetime bound on the associated types Term, TermClass, NonTerm, and State.
This is a significant architectural limitation because it completely prevents the parser from supporting zero-copy parsing (where tokens or non-terminals borrow from the input string with a lifetime shorter than 'static, e.g., Token<'a>).
If zero-copy parsing is a desired feature or a potential future requirement for this library, you might want to reconsider this refactoring, or parameterize the Parser trait with a lifetime (e.g., Parser<'a>) so that static methods can return slices bound to that lifetime instead of forcing 'static globally.
There was a problem hiding this comment.
It's okay to have breaking change.
Also, there are no plans to support lifetime-ed Token as a input.
Summary of Changes
This pull request refactors the
Parsertrait and parser execution contexts (deterministicandnondeterministic) to improve usability, clean up signature parameters, and enhance code simplicity.Key improvements:
ParserMethods: Removed&selfand trait object requirements from allParsertrait methods (get_rules,get_states,precedence_types), changing them into static functions returning static slices/options. Bounded trait associated types with'static.P: Parsertype directly on the executionContext(andNodeRefIteratorfor GLR) along withPhantomData<P>. This eliminates the need to pass parser references to context method calls (e.g..feed(),.accept(),.can_feed(),.expected_token_str(),.trace(), etc.) and avoids turbofish specifications.pub struct GrammarParser;) are now generated instead of struct instances with static reference fields. The unusednew()constructor was completely removed.README.md,GLR.md,rusty_lr_executable/README.md) to reflect the new, simpler syntax.Detailed Modifications
1.
rusty_lr_corerusty_lr_core/src/parser/mod.rs:&self/&selfreferences fromParser::get_rules(),Parser::get_states(), andParser::precedence_types().'staticlifetime bounds toParserassociated types (Term,TermClass,NonTerm,State) andDataStackassociated types (Term,NonTerm).rusty_lr_core/src/parser/deterministic/context.rs:ContextasContext<P: Parser, Data: DataStack, StateIndex>containing_phantom: PhantomData<P>.parser: &Pparameter from all member methods.rusty_lr_core/src/parser/nondeterministic/context.rs:ContextandNodeRefIteratorto acceptP: Parseras the first generic argument.CloneforNodeRefIteratorto bypassP: Clonerequirement and updatedContext'sClonebounds.parserarguments from all methods.2.
rusty_lr_parserrusty_lr_parser/src/emit.rs:pub struct #parser_struct_name;) for the parsers without fields.impl Parser for #parser_struct_nameutilizing staticOnceLockcaches.new()constructor.rusty_lr_parser/src/parser/lexer.rs:parserargument fromfeed_recursiveand internal calls.rusty_lr_parser/src/grammar.rs:feed_recursiveandaccept.rusty_lr_parser/src/parser/parser_expanded.rs:new()methods and utilize static trait implementations.3. Examples & Documentation
calculator,calculator_u8,glr, andjsonexample binaries to initialize contexts and call.feed()/.accept()without passing the parser reference.README.md,GLR.md, andrusty_lr_executable/README.mdto show the new simplified usage.Context::new()and feed tokens directly.