Add API for user defined grmtools section entries in GrammarAST - #667
Add API for user defined grmtools section entries in GrammarAST#667ratmice wants to merge 5 commits into
Conversation
| ast_validity | ||
| .ast | ||
| .unused_grmtools_section_keys_for_crate("test"), | ||
| vec!["test.unused"] |
There was a problem hiding this comment.
CTParserBuilder and nimbleparse are still using the ast_validity.ast_grmtools_section.unused() directly, rather than unused_grmtools_section_keys_for_crate.
So this usage seen in the testsuite with test.foo keys is likely to still trigger an error in practice, I had kind of forgotten about this until just now.
Unsure if we want to relax those errors in this patch, or a subsequent one?
There was a problem hiding this comment.
This seems like something I need to investigate sooner rather than later, it isn't just that we're using unused, we're actually dealing with duplicate Header entries entirely, and there is some passing of that header value around mutably between lrlex/lrpar.
So we kind of need to move over to the new lookup API through the GrammarAST too.
There are still some aspects that aren't publicly exported through the GrammarAST, like required fields.
Edit: moved some comments here to a more relevant place.
| } | ||
|
|
||
| pub fn unused_grmtools_section_keys_for_crate(&self, crate_name: &str) -> Vec<String> { | ||
| if let Some(map) = &self.grmtools_section { |
There was a problem hiding this comment.
The primary reason I marked this as draft, was I was wondering if we should try to make this return a Span too? (Also the function name doesn't really roll off the tongue!)
| let test_num_span = src.find_span("test.num"); | ||
| let test_num_val_span = src.find_span("1234"); | ||
| let mut test_crate_expected = HashMap::new(); | ||
| test_crate_expected.insert( |
There was a problem hiding this comment.
Perhaps this whole test can be cleaned up by using a Vec<(key, span, value)> instead of a HashMap.
That was kind of a carry-over from the prior attempt.
There was a problem hiding this comment.
I tried to do some clean up here in eff4480
Perhaps I went overboard reducing the let bound variables?
| pub fn grmtools_section_value_for_crate( | ||
| &mut self, | ||
| crate_name: &str, | ||
| key_name: &str, |
There was a problem hiding this comment.
I was kind of undecided whether this should do the format!({crate_name}.{key_name}), or
just take the key as a single string, including the crate name.
I just picked one, randomly based on the unused takes the crate name as a separate parameter,
but no strong opinions.
| /// Performs a lookup in the grmtools section for an entry with the key `crate_name.key_name` and returns it. | ||
| /// If the entry is found it marks the key as `used`, for the purposes of `unused_grmtools_section_keys_for_crate`. | ||
| pub fn grmtools_section_value_for_crate( | ||
| &mut self, |
There was a problem hiding this comment.
Well, I'm concerned about this &mut self which is needed for the call to mark_used.
Given that GrammarAST has a lot of pub fields, and I don't think we currently
have any methods to obtain a mut GrammarAST outside of the cfgrammar crate where the tests reside.
Thus this seems like it might not work, maybe we could/should store it in the ASTWithValidityInfo somehow?
There was a problem hiding this comment.
I moved it in a96e799
I think with that I should be able to start on the CTParserBuilder parts.
| let mut yp = YaccParser::new(yacc_kind, src); | ||
| yp.parse().map_err(|e| errs.extend(e)).ok(); | ||
| let mut ast = yp.build(); | ||
| let (mut ast, _) = yp.build(); |
There was a problem hiding this comment.
We have to parse the header twice here, and we may be using the wrong instance.
The first parse is to pull out the YaccKind, and after that it goes through the normal route
through YaccParser::parse.
It probably marks entries as required in the instance returned from YaccParser::build, we
also use that instance in ASTWithValidityInfo::new() when the YaccKind is always known, so that would follow the same code path we use elsewhere.
1 participant
This is a second attempt at exposing querying of entries defined by downstream crates stored in the
%grmtoolssection.The first attempt was #665, this attempt is extended to allow crates to query for unused keys defined within their namespace. And is overall simpler due to being based on the new
header::Valuetype work done in #666 .