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]

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]

With various changes, primarily 0ca6955 and 45fca2b

Along with changing the place where we call mark_used and mutate the Header value have allowed these checks to be performed earlier, and largely eliminated the problem of multiple Header values, culminating in allowing this now be done as a side effect of calling code_generator.

So I think this should be fixed. Also, this was the last thing I was struggling with/unhappy about in this PR.

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

[ratmice]

Should be fixed in 75127c6

[ltratt]

At first glance this does look like it improves things?

[ratmice]

Yeah, I think so.

Edit: Moved my other part of the coment elsewhere.

[ratmice]

If this route seems okay, perhaps as a follow up patch we could get rid of the generic type parameter T to HeaderValue<T>/Value<T> and just inline the Span argument directly?

[ratmice]

So, one thing with this function that could be improved is that it gets called for each unused key, for each crate. It could if passed a set of crate names, do a single pass checking if the prefix of each unused key exists in the set of crate_prefixes.

So instead of doing a pass for each crate, it could do a single pass for all crates.

This wouldn't be hard to do now, I think it'd even be deterministic for the testsuite,
since MarkMap doesn't sort based on randomness.

I'm not sure if it is worth the trouble with the low number of crates and keys though?

[ratmice]

I had done this in c3b3f6c

[ratmice]

If it helps I think we could split this into two patches, one that removes Header from CTParserBuilder and adds it to ASTWithValidityInfo, then a second patch which actually adds the unused check API?

[ltratt]

I think I might be tempted to name this header_value_get(&mut self, key: &str) and downplay the "crate" bit slightly. I do think that strongly encouraging users to use crate names as the prefix is a good idea, but I'm sure someone (maybe us!) will at some point have a good reason for deviating from that.

[ratmice]

Renamed in c3b3f6c

[ltratt]

Might this better be iter_header_values(&self, prefix: &str) -> impl Iterator<...>? I think we can usefully avoid forcing collect / Vec on the user.

[ratmice]

I'll have to try it again, I think I tried to avoid the collect, and the iterator return value could outlive the borrowed &self, but maybe that was trying to get rid of the key_name.clone() call too.

[ratmice]

Also I think it should be iter_unused_header_values

[ratmice]

Should be fixed in c3b3f6c

I suppose, technically it returns a key rather than a value, but uniform function naming is more important?

[ltratt]

I'm not quite sure what the purpose of this function is (at least in the external API).

[ratmice]

Hmm, good question, I think historically there has only been one required key YaccKind, and
we have basically migrated all those checks to specific error checks that check explicitly for that with errors like ParserSrcEnvError::MissingYaccKind, rather than a check that the yacckind key is set in the header.

But previously CTParserBuilder unless it knew the YaccKind would set this as required in the header.
Which would then cause an error if there was no %grmtools{yacckind: ...}

[ltratt]

Ah, so maybe this relates to my question in #667 (comment). grmtools itself wants to check for unused headers with its prefix(es), so perhaps the questions is where that check is.

[ratmice]

Yeah, so this is the implementation of that check, the check is actually performed in the ParserBuildEnv::code_generator() function, returning an error if this check fails.

https://github.com/softdevteam/grmtools/pull/667/changes#diff-7a0605cd47a548bf700da1ff07486de5143ee3048757e0da3df60c6be542b852R496-R507

Edit: It's worth noting that check in code_generator was intended to handle both grmtools crates, and external crates too. There is an example of that in the test cases https://github.com/softdevteam/grmtools/pull/667/changes#diff-7a0605cd47a548bf700da1ff07486de5143ee3048757e0da3df60c6be542b852R1344-R1351

There was also a question whether this particular function should take a HashSet, instead of a crate name, and perform all the checks at once, leaving codegen to do all the cross-crate coordination

Edit: I think try and migrate to the HashSet api, to get rid of some accrued ugliness in this series, making this API from ast.rs #[doc(hidden)]

[ratmice]

It looks like that check you mention is missing from nimbleparse, which doesn't currently use codegen (not yet pub).

Though it's worth noting that you can build an RTParserBuilder from the result of ParserCodegen::finish() without calling ParserCodegen::generate(), so in theory we could modify nimbleparse to use that same check.

We could also perform the checks manually similar to how they are implemented in codegen though.

[ratmice]

I went ahead and removed this function in c3b3f6c since CTParserBuilder/nimbleparse no longer needed anything like it.

Also updated to use HashSet, marked most of the ast.rs api as hidden.

[ltratt]

Is this function useful if it's unused? [I ask this innocently!]

[ratmice]

This is primarily intended for external crates to call, but since we haven't made codegen pub,
essentially crates like nimbleparse_lsp would call src_env.add_value_checks_for_crate("nimbleparse_lsp") then on build()

it would cause unused checks for cfgrammar, all the crates that were registered.
maybe register_key_prefix or something to that effect would be a better name.

This isn't used because it was actually easier to just add cfgrammar, and all the default grmtools crates at once on construction. Rather than building and mutating for each crate.

[ratmice]

I renamed it in 70ce6f1 hopefully that better reflects its eventual intended usage.

[ltratt]

I think I get where this is going now. There's one implicit thing which I had perhaps missed earlier: we're allowing external users to mark keys as used/unused so they can then easily get the unused keys relevant to them. That's quite an interesting feature, but I must admit that my initial reaction is to wonder if we shouldn't push that functionality onto them. In other words, if I'm crate C and I define keys C.A and C.B, should it be my job to "read all keys beginning with C, and if there are any that aren't A or B, warn/error to the user"?

[ratmice]

I think where the "check your own" namespace breaks down is with namespace typos, like say cfgramma.yacckind, ideally when the user is doing a check for crate prefix src_env.add_value_checks_for_crate(""), this centralized prefix checking code can catch that while it couldn't catch it if it's only checking cfgrammar.

But yeah, I don't really have a good playbook/map for how to best implement this kind of feature, like
I know I really don't like generic metadata fields where nobody has any ideas about the keys/entries,
and it's impossible to check or catch typos because of that. It's definitely possible I'm over complicating things because of my metadata table pet peeve though.

[ratmice]

Just one minor difference to note about your comment, which is about allowing external crates to mark their values.

A lot of the explicit API is what grmtools is using using internally, to mark keys as used, The codegen api here is largely trying to add a more implicit API. Where as a side-effect of looking up keys, they get marked as used. And as a side effect of bulding a code generator, they get checked for being unused. The hope was that external crates wouldn't actually have to do the tracking or deal with an API surface to get the checks.

Currently in this patch they still require an explicit registration for that check via the src_env.register_header_value_prefix("crate_name") call, but perhaps we could also register the prefix for unused checks implicitly on key lookup too?

I think we'd still need an explicit check if we want a check that matches all namespaces. currently src_env.register_header_value_prefix("")

Is there a preference between implict/explicit? Also do you think the crate typo resistance is worth doing it in the crate rather than having external crates check their own namespace?

[ratmice]

Hmm, it's worth noting I set this as hidden, so here we're calling a doc(hidden) function in nimbleparse.

Just now, after the fact I'm recalling that nimbleparse is supposed to be more of a working example. In which case we probably want to avoid calling hidden API?

Ideally once codegen is pub, this can be changed to use that and the hidden API will then be confined to being used from codegen.

[ltratt]

Just now, after the fact I'm recalling that nimbleparse is supposed to be more of a working example. In which case we probably want to avoid calling hidden API?

Well, if nimbleparse needs this API, it suggests it should be pub and not mark as ununsed. But if that API is unstable, I agree: it would be better (if possible) to find a stable API for nimbleparse to use.

[ratmice]

FWIW, I tried to write a relatively short document examining the API from the 3 perspectives of

1. In tree tools like nimbleparse
2. Out of tree downstream tools like nimbleparse_lsp
3. End-user crates like build.rs

Tried to keep it short, while being a bit more thorough than what I had done in comments here.

https://gist.github.com/ratmice/ff2678c672105766b9805b8eb91097f2

[ltratt]

I think I follow your use cases. A quick note of my personal bias: over time I've come to prefer minimalistic APIs because they tend to allow usecases that didn't occur to me. That isn't always a good thing, of course, but it tends to be my starting point.

On that basis, I think the simplest (and dumbest) API only needs two things:

1. get_key(key: &str) -> Option<&str>.
2. iter_key_prefixes(prefix: &str) -> impl Iterator<...> returns an iterator over all keys starting with prefix.

One thing, though, I've taken away from your work is that the prefix idea is a really good one.

In particular, it not only gives two crates a de facto way of separating their keys ("use your crate name unless you have a good reason not to") but it gives a de jure way of stopping them treading on each other's toes. In other words register_prefix(prefix: &str) -> Error<(), ...> means that if (somehow!) two crates try registering the same prefix, the second one will go splat in a deterministic and easily understood way.

Once we've accepted that, it makes sense to define a "prefix" as something like [a-zA-Z_][a-zA-Z_0-9]*. In particular, iter_key_prefixes("a") would return "a.b" but not "ab.c".

In terms of "mark things as used", I think I'm inclined to push that onto users because one error I think they'll make is "is key X defined? oh, I didn't expect that" but then X would be marked as used automatically. In other words, we're accidentally pushing some policy decisions (that make sense for grmtools) onto users in a way that might confuse them.

The good news is that they can very easily work out which keys aren't use with something like:

let unused = x.iter_key_prefixes("crate").collect::<HashSet<_>>().difference(HashSet::from(&["b"]);

where b is the key they know was used.

Sorry for the ramble: let me know what you think!

[ratmice]

Totally understand the desire for a simple API, one with just something like iter_key_prefixes I initially started out with. I only moved to a get_key style API when I realized one could use that to infer usage automagically (I'm also not certain that is a good idea, but it seemed to save on having a separate mark_used style API).

One of the things the centralized use checking is aiming to save users from is when they set some setting, and somehow mess it up, that if the setting doesn't get applied for whatever reason, the idea is it should notify them rather than ignore the setting. This is kind of inhibited by the everybody checks their own keys/prefix

I think lrlex examples might be the best

%grmtools {
// Posix escape setting is not set because we typo'd in the prefix, 
  lrlx.posix_escapes
}

So it can be the user thinks the setting goes into effect, the lrlex crate checked all the keys in it's prefix for validity.
But by the distributed nature of prefixes this one crept through unchecked. Users would need to somehow know and check that lrlx is not actually valid prefix but a typo for lrlex.

So for that reason I thought it was worthwhile to have a centralized checking mechanism, that can point out that
no lrlx prefix was registered nor did it ever use a posix_escapes key.

Maybe there is some way we can push that onto users with an API like fn iter_prefixes() -> &["cfgrammar", "lrpar"], which they could check against crate names? Or require the register_prefix call and it can complain that lrlx wasn't registered (presumably grmtools libraries will be registering lrlex) rather than the user registering lrlx.

Anyhow, I'm a little bit uncertain whether end-users are going to bother adding these kinds of extra checks if they don't just happen during the build process even if they occasionally find errors.

Also sorry for (all of) the rambling too!

Edit: I think it's worth explicitly pointing out the somewhat contradictory use case requirements here where we have nimbleparse which is like: "there might be a lrlx crate nimbleparse doesn't know about that this flag was intended for", and build.rs where "this is a typo which we would like to catch".

It seems (as an end-user/build.rs check) we couldn't quite do something like

assert_eq!(ast_with_validity_info.iter_prefixes(), build_env.registered_prefixes())

but something to the effect of

for prefix in ast_with_validity_info.iter_prefixes() {
   assert!(build_env.registered_prefixes().contains(prefix))
}

might, because there is no guarantee that there will be entries for every registered crate?

[ltratt]

I think it's worth explicitly pointing out the somewhat contradictory use case requirements here where we have nimbleparse which is like: "there might be a lrlx crate nimbleparse doesn't know about that this flag was intended for

I hadn't thought about this. I'm not quite sure what to do here: if a flag is for another tool, should nimbleparse ignore it? My immediate reaction is that I can argue this both ways!

[ratmice]

I hadn't thought about this. I'm not quite sure what to do here: if a flag is for another tool, should nimbleparse ignore it? My immediate reaction is that I can argue this both ways!

Ahh, I hadn't really considered any approach besides nimbleparse ignoring it. It at least feels like nimbleparse ignoring actions acts kind of like a precedent that it may ignore parts of the grammar that it is ignorant of or incapable of handling?

[ratmice]

So the one thing that came to mind today, is that we turned an unregistered prefix into a warning nimbleparse already just prints warnings, and doesn't behave as though warnings are errors.

While the CT*Builder instances treat warnings as errors, that could be one way we can have the behavior somewhat uniform between nimbleparse and the rest of the crates.

We could also maybe add something like nimbleparse.register_external_prefixes: ["nimbleparse_lsp"] (or perhaps lrpar. and lrlex. instead) to suppress the warning?

That would give us:

• Warnings on unrecognized prefix and/or errors depending upon whether warnings are errors.
• The ability to suppress warnings for unregistered unrecognized prefixes all together

[ltratt]

I hadn't really considered any approach besides nimbleparse ignoring it

Having slept on this, I think this is the better default. I think you're right that having nimbleparse warn about such keys is a good idea, though (at least I think that's what you're suggesting). That will be a nice reminder to users that nimbleparse is doing a "best effort" but if those keys have some semantic influence on the grammar, nimbleparse can't know what to do about it.

In terms of the "should grmtools track 'is this key used' in the API", on reflection I think it probably is better if we shove that responsibility onto the users. The existence of difference in the Rust std lib makes it very easy for them to do this themselves, and they can then use whatever semantics is appropriate for their use case. That said, I can be convinced either way: it's not a big deal.

[ratmice]

In terms of the "should grmtools track 'is this key used' in the API", on reflection I think it probably is better if we shove that responsibility onto the users. The existence of difference in the Rust std lib makes it very easy for them to do this themselves, and they can then use whatever semantics is appropriate for their use case. That said, I can be convinced either way: it's not a big deal.

The issue I suppose I'm struggling with is that difference gives us one of the two sorts of checks easily.
Lets say the two checks are "complete" an "incomplete" checking (Edit: maybe it would be better to say "exhaustive" and "non-exhaustive" checks instead).

nimbleparse and the like are the "incomplete" case which only know a subset of the keys, can only check keys for their known prefix. This one is easily done outside via difference,

The other case is where we have e.g. a build.rs and we know the complete list of keys for all crates.
Then we want to check that the union of all the crate keys, against the difference of all the keys in the header.
By registering prefixes, and checking against the set of known crates/prefixes.

Perhaps the right thing to do is start out trying to implement the complete check with the external responsibility for the checks, and see how involved that is and whether it is something we would recommend, or whether the burden justifies the build.rs additions.

To do the "complete" check, we definitely need to adjust how I was planning on doing though, since the lists of known keys are spread out across all the crates though. So we can't just easily union them together.

Anyhow, I'll give it a go trying to implement both these checks as external checks. With the "complete" check just checking against sets of prefixes, which allows us to avoid the need for the union of all known keys for all crates in use.

We can see how that looks and go from there?

[ltratt]

Perhaps the right thing to do is start out trying to implement the complete check with the external responsibility for the checks, and see how involved that is and whether it is something we would recommend, or whether the burden justifies the build.rs additions.

That seems like a good plan to me and we should definitely feel free to reconsider if we realise things are going in the wrong direction!