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

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.

Out of date comment, should be Value<Span>.

Should be fixed in 4200d44

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

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.

Forgot to mention in the commit message this round of testing found two bugs in the RustLike 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.

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

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

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

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<Span> 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).

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<Span> to the simpler type GrmtoolsSectionValue<Span>. 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.

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

use simpler GrmtoolsSectionValue throughout #666

Open

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

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

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

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

                           )
                   }
+                  pub fn grmtools_section_values(

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

Uh oh!

Add grmtools_section to ast - #665

Add grmtools_section to ast#665ratmice wants to merge 10 commits intosoftdevteam:mastersoftdevteam/grmtools:masterfrom ratmice:grmtools_section_user_entries_part2ratmice/grmtools:grmtools_section_user_entries_part2Copy head branch name to clipboard

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add grmtools_section to ast#665
ratmice wants to merge 10 commits into
softdevteam:masterfrom
ratmice:grmtools_section_user_entries_part2

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