Add API for user defined grmtools section entries in GrammarAST by ratmice · Pull Request #667 · softdevteam/grmtools

ratmice · 2026-09-11T05:47:40Z

This is a second attempt at exposing querying of entries defined by downstream crates stored in the %grmtools section.
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::Value type work done in #666 .

ratmice · 2026-09-11T05:51:42Z

+            ast_validity
+                .ast
+                .unused_grmtools_section_keys_for_crate("test"),
+            vec!["test.unused"]


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?

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.

Making CTParserBuilder work should be fixed now in adb16f4

ratmice · 2026-09-11T05:53:35Z

+    }
+
+    pub fn unused_grmtools_section_keys_for_crate(&self, crate_name: &str) -> Vec<String> {
+        if let Some(map) = &self.grmtools_section {


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!)

Turns out there was a lot more reasons this should have been marked as draft,
but this should be fixed in e8012c9

ratmice · 2026-09-11T06:05:10Z

+        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(


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.

I tried to do some clean up here in eff4480

Perhaps I went overboard reducing the let bound variables?

ratmice · 2026-09-11T06:14:23Z

+    pub fn grmtools_section_value_for_crate(
+        &mut self,
+        crate_name: &str,
+        key_name: &str,


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.

ratmice · 2026-09-11T08:19:38Z

+    /// 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,


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?

I moved it in a96e799

I think with that I should be able to start on the CTParserBuilder parts (adb16f4).

ratmice · 2026-09-11T17:04:42Z

                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();


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.

Should be fixed in 86a4457

ratmice · 2026-09-12T07:45:14Z

+        let build_env = src_env
+            .build_env(ParserBuildEnvArgs::new().mod_name(Some("test_module")))
+            .unwrap();
+        build_env


This may still need some work, on how to expose this check in a way that will work with downstream crates that want to use the code_generator. But this stuff is all private still anyways.

The thing to note is that this check still happens in CTParserBuilder so code_generator is not automatically checking for unused keys as a byproduct of code generation.

Hence having to reimplement the checks in these test cases as calls to build_env.check_unused...

Regardless of whether downstream crates automatically get checking for unused keys,
they can now perform the check themselves to obtain the same results as CTParserBuilder?

ratmice · 2026-09-12T08:08:48Z

It took me a while thinking about how to handle that there are multiple header instances across various crates.
for instance we have a Header<Span> in cfgrammar::yacc::ASTWithValidityInfo and a Header<Location> in CTParserBuilder/lrpar::codegen. These all get merged into the Header<Location> and each of their owners may call mark_used on them and then later check the unused status of some key.

I came to the conclusion that it isn't actually a problem, because these Header<Location> only know about keys for grmtools crates and only check for those. Further, those header instances aren't visible to downstream crates.

The key to this working is that each of these crates is going to be marking and checking the keys for the crates they know in the header they'll check, so we don't really have much of an issue with a key being marked used in one Header then checked for used status in another.

But I don't imagine that is either an obvious as either problem or solution. There's really two ways this could be a problem:

we start wanting to add ways that downstream crates can set their values via CTParserBuilder (beyond adding them to their parser source)
downstream crates perform a check_unused on cfgrammar/lrpar/lrlex crate entries.

The second case actually will currently fail as we're only calling mark_used in the Header<Location> rather than the ast_with_validation_info.grmtools_section, the fix that comes to mind is ensuring mark_used is called for both Header instances.

Edit: I'll work on a fix for this second case, and feel like we can just accept that we won't do the first?
Edit 2: There is one complication in that this means we need mutable rather than shared references to the ASTWithValidityInfo it doesn't seem that it is possible for e.g. lrpar::codegen:: to satisfy the borrow checker with an &mut ASTWithValidityInfo.

So i'm wondering if we just perform the checks e.g. during codegen, and allow inconsistent results if a user tries to call unused_grmtools_section_keys_for_crate("cfgrammar") e.g. for an external crate like cfgrammar/lrpar/lrlex on their own.

ratmice · 2026-09-12T08:46:27Z

+            .unwrap();
+        build_env
+            .check_unused_header_keys_for_crate(Some("cfgrammar"))
+            .unwrap();


So the main problem I'm thinking about in the ginormous comment is that if we change these
build_env.check_unused_header_keys_for_crate(Some("cfgrammar")) calls to:

ast_with_validity_info.unused_grmtools_section_keys_for_crate("cfgrammar")
These start failing, because the codegen process is merging keys, and then working on the merged structure rather than the one in the AST.

Further it only has a shared AST, and it seems difficult to change that to a mutable one.

The best thing that has come to mind is calling mark_used in YaccParser::parse() or build this is before the keys actually get used, but before ownership is moved.

That is done in fcba11b

ratmice · 2026-09-12T10:44:37Z

Marking as ready, because I think I've covered all the issues I can think of as best I can.
Though this has definitely not been as smooth of a patch process as I hoped/thought it would have been.

Add API for user defined grmtools section entries in GrammarAST

59f354a

ratmice commented Sep 11, 2026

View reviewed changes

ratmice added 2 commits September 10, 2026 23:49

Clean up new test cases

eff4480

Remove unneeded to_string() in test

14537c3

ratmice commented Sep 11, 2026

View reviewed changes

Move ownership of field to ASTWithValidityInfo

a96e799

ratmice commented Sep 11, 2026

View reviewed changes

ratmice added 2 commits September 11, 2026 10:12

Use the grmtools section from YaccParser::build

86a4457

First attempt at allowing downstream crate keys in CTParserBuilder

adb16f4

ratmice commented Sep 12, 2026

View reviewed changes

ratmice assigned ltratt Sep 12, 2026

ratmice commented Sep 12, 2026

View reviewed changes

ratmice added 4 commits September 12, 2026 02:08

Use same naming convention as the codegen module

fd3fa86

Preemptively call mark_used on grmtools crate entries in header

fcba11b

stray whitespace

751a4d4

Make unused header entries check return spans

e8012c9

ratmice marked this pull request as ready for review September 12, 2026 10:40

Rename lookup function to use similar naming scheme.

9bb74bf

Uh oh!

Conversation

ratmice commented Sep 11, 2026

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice commented Sep 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 12, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice Sep 11, 2026 •

edited

Loading

ratmice commented Sep 12, 2026 •

edited

Loading

ratmice Sep 12, 2026 •

edited

Loading