Add grmtools_section to ast by ratmice · Pull Request #665 · softdevteam/grmtools

ratmice · 2026-08-26T19:54:54Z

This is a work in progress, it converts the HeaderValue into a simplified value type.
As of this time the conversion of the RustLike values (and their spans) is still largely untested.

User specified values will still trigger a Unused entry error. This is all I had time for today, will work on that tomorrow.

ratmice · 2026-08-26T19:55:49Z

+                    let array_span = Span::new(start_span.start(), end_span.end());
+                    let mut out = Vec::with_capacity(v.capacity());
+                    for setting in v {
+                        // To Call this function recursively we need to convert the `Setting<Span>` to a `HeaderValue<Span>`


Out of date comment, should be Value.

Should be fixed in 4200d44

ratmice · 2026-08-27T10:08:16Z

+            test_string_span_start,
+            test_string_span_start + "test.string".len(),
+        );
+        let test_string_val_span_start = src.find("Foo").unwrap();


Noting that the span here seems to exclude the quotation marks.

ratmice · 2026-08-27T10:09:51Z

+            "cfgrammar.yacckind".to_string(),
+            (
+                yacckind_span,
+                // The actual value we receive has been lower cased


At this point I'm perhaps regretting this case insensitivity of the rust values?
Note that we don't do it for string values so it doesn't apply to globbed filenames.

If we want to keep the case insensitivity, perhaps it is better to move it outside of the Header structure,
to the caller. It looks like key values are not case insensitive which would be harder to move out of the HashMap.

Edit: It looks like my checking for key case insensitivty was flawed, and they actually are case insensitive.

Updated the test in 7f4847a to highlight the case insensitivity

I have come, in my dotage, to prefer case sensitivity, so if we want to move to that, I'm all for it!

The main issue is that I don't know how to do that without a change of behavior (at least for key names).
But it seems like it might be a good time to pull the band-aid off I don't think the case insensitivity has actually been advertised anywhere.

I agree: we didn't commit and I think it's reasonable to assume it is case sensitive.

ratmice · 2026-08-27T11:20:11Z

                    s.push('(');
                    if let Some((arg_ns, _)) = arg_namespace {
                        s.push_str(&arg_ns);
+                        s.push_str("::");


Forgot to mention in the commit message this round of testing found two bugs in the RustLike values.

ltratt · 2026-08-27T11:50:33Z

            )
    }
+
+    pub fn grmtools_section_values(


Do we need this given grmtools_section_values_for_crate? Maybe if we say "an empty crate name matches everything"?

Suprisingly took me a couple of tries, to keep the return type the same.

Because the closure is a generic parameter to the filter iterator type, and closures are uniquely typed.

However should be fixed in 065ad66

Hmm, I'd kind of like to perhaps think more about the way that crates query for these values.
The lingering question in my mind is "Can we somehow leverage the query mechanism so that downstream crates can enable the unused key checks" without a burdensome API?

This extensibility, and strict checking seem like conflicting goals, but I'm not really convinced yet that they are.

I'm fine with taking our time.

ltratt · 2026-08-27T11:51:20Z

+            "cfgrammar.yacckind".to_string(),
+            (
+                yacckind_span,
+                // The actual value we receive has been lower cased


I have come, in my dotage, to prefer case sensitivity, so if we want to move to that, I'm all for it!

ratmice · 2026-08-27T12:51:55Z

There is still one thing left to do before user values will be able to be added by downstream crates.
Currently this just exposes the ability to read the current grmtools section user values.
Once we relax the unused entry checks, outside crates should be able to add their own.

But this seemed like a good stopping point for this patch?

Edit: The above is not exactly correct, except when using the CTBuilder construction method.
If the crate constructs things directly using ASTWithValidationInfo it can avoid the checks (Like the tests added here).

ltratt · 2026-08-27T13:07:30Z

Point taken. Yes, I think this is a good stopping point. Ready to squash?

ratmice · 2026-08-27T13:20:02Z

I'd just like to think about, and perhaps play around with the thoughts in #665 (comment) first before we commit to this.

My thought is mainly that if we had grmtools_section_value_for_crate("cratename", "key") that could mark the key as used. Where the current API grmtools_section_values_for_crate we can't really tell if any specific key is used or not.

Now somehow we need to deal with the fact that nimbleparse and ctbuilder don't know about downstream crates.
So it should somehow only enable the check for crates which get queried (This implies it isn't actually much help if you typo the crate name, but might still catch errors in key names)

Edit: Anyhow i'll think on it for a day or so, let me know whether you think it'd be worthwhile for catching errors, or too complex for the benefits it gives. One thing I think is that it seems like this would require lazily converting from Header rather than the eager conversion to HashMap<String, GrmtoolsSectionValue> because Header is what contains the mark_used ability.

Edit2: An alternate thought is just having a key registration mechanism register_key_for_crate(crate, key) separate from the query mechanism. That would allow us to complain if keys were unregistered.

Edit3: I think one way to expose the check is just changing the Header.check_unused_header_keys to check_unused_header_keys_for_crate(crate).

ratmice · 2026-08-28T11:30:30Z

So, I think I've got a good plan for how this could work, the only issue I'm having is the HeaderValue -> GrmtoolsSectionValue conversion makes us return owned GrmtoolsSectionValue types. When calling the lookup and doing the conversion lazily then, we end up cloning the value at lookup time for the conversion.

We could avoid that if we added the Span/Location generic to GrmtoolsSectionValue, and switched internally from HeaderValue to the simpler type GrmtoolsSectionValue. I believe that is possible anyways, but probably we'd want to do first as a separate patch if we're interested in doing it.

It will probably make some errors worse, in the sense that if you have a bad yacckind: Original(AnUnrecognizedActionKind) we'd basically be attributing this to a bad GrmtoolsSectionValue::RustLike("Original(AnUnrecognizedActionKind)") so it would call out the yacckind rather than the YaccOriginalActionKind as the problem.

If we choose not to do that it just means we have to return owned rather than borrowed values during lookup.
Personally (even though I wrote it) I've always felt the HeaderValue structure is a pretty painful to use/understand/look at. It mostly evolved from the way the parser produces values. So if we can get the parser to produce GrmtoolsSectionValue directly it might clean up the code a bit.

Let me know if you want me to experiment with that?

Edit: I hould probably note that I don't think there is much of an efficiency difference, we're probably just moving the clone from lookup time, to parse time. Because it is unlikely people ever do multiple lookups.

Edit: I posted #666 as a experiment in switching HeaderValue to use the GrmtoolsSectionValue type.

ltratt · 2026-08-31T10:47:44Z

Dumb question: is #666 intended to maybe replace this PR?

ratmice · 2026-08-31T11:18:46Z

Kind of a step towards a replacement PR I would say.

Like I could do some changes alternately on this PR, but I think the functions for querying values would have kind of funny types.

Like we currently have this function:

    pub fn grmtools_section_values_for_crate(
        &self,
        crate_name: &str,
    ) -> impl Iterator<Item = (&String, &(Span, GrmtoolsSectionValue))>

but the only way that works is because GrammarAST owns a HashMap<String, GrmtoolsSectionValue>

In #666 it's setting it up to switch to GrammarAST to owning a Header,
With the current Header owning HeaderValue the lookup would have to return owned GrmtoolsSectionValue

    pub fn grmtools_section_value_for_crate(
        &self,
        crate_name: &str,
        value: &str,
    ) -> Option<(&String, (Span, GrmtoolsSectionValue))>

By basing this patch on top of #666 we can get back to a more HashMap familiar API like

    pub fn grmtools_section_value_for_crate(
        &self,
        crate_name: &str,
        value: &str,
    ) -> Option<(&String, &(Span, GrmtoolsSectionValue))>

So the thought was mostly rebuilding this patch on top of #666 gives us an API more similar to
the HashMap::get which returns references rather than having to clone because it's doing a conversion from an internal type.

So in a yak shaving way, it's kind of laying the ground work for a second iteration of this patch.

ltratt · 2026-08-31T11:24:41Z

I think I get it. I'll review #666 in that light.

ratmice · 2026-08-31T11:31:28Z

Edit: Actually disregard this, I think because it's #[non_exhaustive] it isn't actually semver breakage.

It dawns on me there is some weirdness we might discuss about GrammarAST and the patch on top of #666 where we reimplement this PR.

in that currently all of GrammarAST values are pub but it seems unlikely we'd want Header to be pub.
which means that GrammarAST won't be able to be built outside with a

GrammarAST {
   ...
}

I don't think that method of constructing AST's is actually usable, since all the validation is private.
That might be one thing to keep in mind, I think it is technically a semver break, but it isn't an API pathway that currently leads to the ability to construct a parser from the GrammarAST.

Add grmtools_section to ast

966252a

ratmice commented Aug 26, 2026

View reviewed changes

ratmice added 2 commits August 27, 2026 03:03

Add test cases for grmtools_section_values_for_crate

6609692

Fix out of date comment

4200d44

ratmice commented Aug 27, 2026

View reviewed changes

ratmice added 3 commits August 27, 2026 03:26

Improve test readability

d766f4e

More tests and test cleanup

8295c6a

Exhaustively check GrmtoolsSectionValue::RustLike values

4926874

ratmice commented Aug 27, 2026

View reviewed changes

test key case insensitivity

7f4847a

ratmice assigned ltratt Aug 27, 2026

ratmice marked this pull request as ready for review August 27, 2026 11:51

ltratt reviewed Aug 27, 2026

View reviewed changes

ratmice added 3 commits August 27, 2026 05:29

Make grmtools section case sensitive

e3a4c71

rustfmt

c52fd9d

Combine grmtools_section_values with the for_crate version

065ad66

ratmice mentioned this pull request Aug 28, 2026

Simplify Header::Value #666

Open

Uh oh!

Conversation

ratmice commented Aug 26, 2026

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 Aug 27, 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!

Choose a reason for hiding this comment

Uh oh!

ratmice Aug 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ltratt commented Aug 27, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ltratt commented Aug 31, 2026

Uh oh!

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

Uh oh!

Uh oh!

ltratt commented Aug 31, 2026

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ratmice Aug 27, 2026 •

edited

Loading

ratmice Aug 27, 2026 •

edited

Loading

ratmice commented Aug 27, 2026 •

edited

Loading

ratmice commented Aug 27, 2026 •

edited

Loading

ratmice commented Aug 28, 2026 •

edited

Loading

ratmice commented Aug 31, 2026 •

edited

Loading

ratmice commented Aug 31, 2026 •

edited

Loading