Disentangle grammar resolution and related PG code #1018
Conversation
|
| } | ||
|
|
||
| impl Resolution { | ||
| pub fn original(&self, name: &Identifier) -> &(Identifier, Item) { |
There was a problem hiding this comment.
Ah, I forgot that I still had it here. I need it to extract the lexical context for a given item.
I can't strictly replace it with a "get lexical context for an item" because some item may be reachable from different lexical contexts (i.e. pragma keyword) as required by our current lexer codegen/setup.
I'm happy with renaming it but are we fine with keeping this function for now?
There was a problem hiding this comment.
Sounds good. I suggest renaming it to its true purpose, and add this reasoning as a commment for the future.
| ctx.resolved.insert(ident.clone(), resolved.clone()); | ||
| return resolved; | ||
| } | ||
| Item::Token { item } => item as Rc<_>, | ||
| Item::Trivia { item } => item as Rc<_>, | ||
| Item::Fragment { item } => item as Rc<_>, | ||
| _ => unreachable!("Only terminals can be resolved here"), |
There was a problem hiding this comment.
nit: for here and other panics: I suggest adding the provided ident to the error message, to make it easier to debug what went wrong.
There was a problem hiding this comment.
Done in 2ffeb5e, I agree with the spirit, however, there's no real value IMO to do that for these unreachables specifically - it's checked above in the match whether the branch is concerned with a parser thunk (so nonterminal) or not; the code does not change often and the assertion seems fairly fundamental (what we consider a non-terminal).
For things like panic!("Empty error_recovery") it makes more sense but ideally we should validate or model that better in the DSL v2, instead.
| self.scanner_contexts | ||
| .get_mut(self.current_context_name.as_ref().unwrap()) | ||
| .expect("context must be set with `set_current_context`") | ||
| } | ||
|
|
||
| fn into_model(self) -> ParserModel { | ||
| // TODO: Separate it to a function that combines the accumulated state with the parser_fns |
There was a problem hiding this comment.
nit: I suggest attaching an issue number to these TODO comments, for future upkeeping. If it is a new issue, and it is not a high priority, we can put it in the Backlog project for now.
There was a problem hiding this comment.
I figured that the overhead of the issue is bigger than the fix - now, we combine it in the ParserModel::from_language and ScannerContextCollector::into_model returns only the relevant fields rather than immediately combining them with the ones in ParserFunctions.
|
Thanks @OmarTawfik for a swift review! |
Ticks the box in #638 (Keyword trie inclusion should be reworked to not require synthetic rules over all keywords)
This exposes the resolution step rather than treating it as an implementation detail and doesn't try to shoehorn the DSL v2 items to the old
Grammarmodel as much that the PG was based on.Moreover, this breaks up the existing grammar visitor and tries to collect or calculate more properties upfront directly from DSL v2 in order to be more explicit that they not need to depend on the collector state that tracked everything.
I didn't submit it initially because I felt I could polish it slightly further (see the
TODOnote) but since I'm focused on EDR now and this cleans up the last important box in #638 (the other one is simply reverting some rules for string literals), I think it's worth reviewing in the current state.