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]

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!

[ratmice]

So, one of the issues I've run across is here is with the difference method, and Span, like HashMap, the get value is general with the where clause where K: Borrow<Q>, but unfortunately the difference function does't have that kind of where clause.

So I don't currently see a nice/easy way to compare a HashSet<(Span, &str)>, and a HashSet<&str> for difference, such as a difference function that takes a closure. Perhaps I'm missing/overlooking some method?

Using difference closest I've been able to come up with so far was this kind of helper method, which sadly has to allocate a whole bunch of sets!

    pub fn unrecognized_keys_for_prefix(&self, prefix: &str, valid_keys: &HashSet<&str>) -> HashSet<(&str, Span)> {
        let found_keys_spanned = self.iter_prefix_keys(prefix).collect::<HashSet<_>>();
        let found_keys = found_keys_spanned.iter().map(|(s, _)| *s).collect::<HashSet<_>>();
        let unrecognized_keys = found_keys.difference(valid_keys).copied().collect::<HashSet<_>>();
        found_keys_spanned.iter().copied().filter(|(key, _)| unrecognized_keys.contains(*key)).collect::<HashSet<_>>()
    }

Edit: apparently If I avoid difference, and just use filter/contains directly which difference basically desugars to
it ends up much nicer.

    pub fn unrecognized_keys_for_prefix(
        &self,
        prefix: &str,
        valid_keys: &HashSet<&str>,
    ) -> impl Iterator<Item=(&str, Span)> {
        self.iter_prefix_keys(prefix)
            .filter(|(key, _)| !valid_keys.contains(*key))
    }

[ratmice]

So I think that gives us a rough draft for external checks at least.

I'm keeping in my head a list of follow up patches I should write down:

1. We can probably replace MarkMap entirely with HashMap, since we're no longer using any of the required or used marks.
2. nimbleparse current usage of hidden api, which we can migrate to codegen is pub
3. #[allow(unused)] that we'll also be able to remove once codegen is pub

[ratmice]

FWIW, this check shown in the docstring example is one that I would rather see done automatically during codegen rather than externally,
because otherwise people might not call (or notice they should call) register_header_prefix.
and then typo prefixes can fall through.

However that poses a problem, since nimbleparse doesn't know it needs to register_header_prefix for nimbleparse_lsp and all other downstream prefixes. We'd need some alternate mechanism like

%grmtools{
  cfgrammar.external_prefixes: ["nimbleparse_lsp"],
}

So that the header itself would register the prefix. It could still then catch typos like lrpr and suggest adding an external prefix:

%grmtools{
   lrpr.recoverer: CPCTPlus,
}

I'm not certain how I feel about essentially registering the header prefix from the header itself.
But it seems like it kind of allows us to force these kinds of error checks, so of course I'm curious to hear your thoughts.

[ltratt]

self.iter_prefix_keys(prefix).filter(|(key, _)| !valid_keys.contains(*key))

I admit that wasn't what I was thinking of, but it's not too bad, and I think we can easily tell the user this is (probably) what they want (rather than providing a helper method ourselves)!

[ratmice]

self.iter_prefix_keys(prefix).filter(|(key, _)| !valid_keys.contains(*key))

I admit that wasn't what I was thinking of, but it's not too bad, and I think we can easily tell the user this is (probably) what they want (rather than providing a helper method ourselves)!

Yeah, I don't think this is that bad, especially considering that it's mostly crates like nimbleparse_lsp, and perhaps
the occasional project that might need to check some custom fields. But I think the burden is going to be mostly on external crates which can easily provide their own helper function. The other registration thing I'm slightly more hesitant to push onto users though.