Add API for user defined grmtools section entries in GrammarAST

[ratmice]

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]

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]

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

[ratmice]

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]

Having thought about it, I'm in the coping stage where it feels like there isn't much the code generator can do about this. It doesn't know at what time all the header values have been resolved, since it gives mutable access to the header via PaserBuildEnv::header_mut(), currently the check_unused_header_keys_for_crate function is also on PaserBuildEnv.

Edit: Oops, disregard the paragraph below. I was looking at the lrlex::codegen, in lrpar::codegen we actually need to mutate the header/mark keys as used after the call to code_generator().
The reason we need it after is because of the inspect_rt callback which we use to implement test_files, it builds a RTParserBuilder, and marks the test_files entry as used.
It is worth noting though that test_files is explicitly marked as experimental in the book here.

Edit2: Given the above perhaps we code do the checking in ParserCodegen::generate it takes a ParserBuildEnv and returns a Result. This provides a place to do the check, but still has some open questions about Header<Location> vs Header<Span>, and which header to perform the check on during codegen.

It looks like it could work if we actually made this a private function, and called it from ParserBuildEnv::code_generator. That is the next point after all header values should have been marked used. Presumably we'd need some way perhaps in ParserBuildEnvArgs to pass it a list of crates to check.

[ratmice]

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:

1. we start wanting to add ways that downstream crates can set their values via CTParserBuilder (beyond adding them to their parser source)
2. 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]

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.

[ratmice]

I've tried a couple of times to write a sort of high level comment about design issues, but it keeps turning into a textbook/great american novel.

This was mostly talking about duplication between e.g. ASTWithValidityInfo and codegen:: module, where codegen:: currently is acting solely like a sink, and ASTWithValidityInfo is acting primarily as a source one is consuming entries, the other is providing them. But they overlap in providing this static analysis/checking of unused variables.

There is something to say though about the mechanisms provided by this patch, which is they currently aren't exposing any of this or making it usable from within CTParserBuilder. That deserves some high level description of what this patch is providing/background info.

In nimbleparse_lsp v2, we're basically calling ASTWithValidityInfo::from_str with the grammar source,
we plan on using the codegen:: module directly, passing the prebuilt ASTWithValidityInfo to the code generator
with ParserBuildEnvArgs::ast_with_validity_info, but between those two calls we can pull out any user specified keys.

One of the unforseen problems, which I guess I am encountering now, and didn't anticipate is that the intent was for the public API to kind of not expose the Location type, by providing a new() function which just uses an empty Header<Location>, but that leaves the Location type itself still getting exposed via errors. Even though the only error that can happen are Location::Span ones. One thing we could consider doing is adding a generic parameter to the codegen:: structures, so the hidden constructor ParserSrcEnv::new_with_header could return a ParserSrcEnv<LexerTypesT, Location> while ParserSrcEnv::new() could return a ParserSrcEnv<LexerTypesT, Span>, but that is a pretty significant change (and seems likely to be impossible).

It hadn't really occurred to me how/whether we plan on exposing this via CTParserBuilder or codegen:: there is additional complexity with adding that, since it involves Location and the additional Header<Location> values. That is to say codegen:: has two structures we could query. The one read from the file itself containing Spans and the one with all the default values having been resolved and plugged in with Locations. So this patch hasn't attempted to really come up with any answers about this latter problem.

[ratmice]

I'm going to go ahead and mark most of my waffling review comments as resolved for now.
But feel free to unresolve anything, if you feel like deserves more attention or a different approach.

It just feels like they're probably more distracting than helpful at this point.

[ltratt]

Dumb question: do we even need to force two variables (crate_name and key_name) on the user? Would key: &str make sense alone?

[ratmice]

We don't, I made the same comment in a review above too, for a different function.
I kind of flipped a coin, and did separate crate names and keys, due to the separate crate name elsewhere
like check_unused_... where the key name isn't passed in at all.

It didn't seem like a strong argument then, happy to change it.

[ltratt]

I think that if we use a single string, we get a nicer API in #667 (comment) too? Warning: I might be wrong.

[ltratt]

I wonder if instead of crate_name we have prefix, where "" means "match all" and any non-empty string implicitly ends with . (we don't want "x" to match "xy.z")?

[ratmice]

I'm not exactly certain how to interpret the suggestion,

One thing to note is that the current behavior, None is not actually "match all", but "match anything without any crate prefix".

Are you saying we should have

• Some("") should match all keys.
• None should match keys with no prefix.
• Some("foo") should match "foo.*"

Or did you just mean to replace the option with just the empty string?

[ltratt]

Ah! OK, so I think we had this discussion recently (or at least I'm hallucinating something similar). I think intuition approach is to say "everything has a prefix; for legacy keys, we add the prefix "grmtools" (or "lrpar" or ...)". So "match" then becomes "match a prefix". So match("") matches everything; match("grmools") matches grmtools' keys, and so on. Again, warning: I haven't thought deeply about this and this might not be the best API.

[ratmice]

Ahh, maybe we need to handle unknown unprefixed keys elsewhere then.

The way it currently works, unprefixed keys can squeek by, because the prefix is only added for known keys.
So if you have a key like unknown: "foo", it can end up in the header because it doesn't match any "grmtools", "lrpar", or "lrlex" keys.

Perhaps that should be an error when we try to insert the key into the Header instead of handling it at this point where they key just happens to end up not being used by anything.

This is kind of a "lazy" way to handle that error of an unknown key with no crate prefix.

[ltratt]

Aha, yes, I think unprefixed-keys-that-aren't-recognised-legacy-keys are probably best banned. Forcing namespacing on users seems like a good idea (at least right now).

[ratmice]

Okay, so the next question is do we actually want/need the match-all behavior?
I guess I see how it is useful for users to call on their own grammars to ensure all their keys are valid.

I had been looking at it from the perspective of grmtools, and nimbleparse_lsp, where there
maybe crate specific keys that these crates can't know about. So they can't perform the checks
for external crates.

I suppose that end-user checking is enough motiviation to add it.

[ltratt]

I think the "match all" behaviour falls out of the hat, so unless we ban it, it Just Works (TM). I think it's useful to have it, at least from grmtools internal purposes? But we could start without it (i.e. ban it), and only unban it later if we find someone has a real use case for it: I'm fine with doing that.

[ratmice]

I think there is a use for it, as an end-user you can typo the crate name which will get by the per-crate checks. So if you do the match-all, it'll catch that. But it's only really possible to do if you are certain all the keys have been used.

For instance if you want to add that check to build.rs in a grammar that contains nimbleparse_lsp keys, you'll need to mark them as used yourself (or I can provide a helper function).

That is to say this key checking becomes somewhat of a distributed problem.

[ltratt]

Good point -- I agree.