Initial parsing of %user section as a Header.

[ratmice]

This still needs some work, but is an initial attempt at a %user section we can parse by reusing the %grmtools section parser, then restrict the types within it as a post processing stage.

In a subsequent patch we can iterate over the %user section and convert it into a nicer to use HashMap. Throwing invalid value errors on the more rust-like constructs available via HeaderValue

This is also currently missing any way to access the information from the section.

[ratmice]

I think that pretty much covers the approach that I was thinking would largely reuse the code we already have,
I did a cargo semver-checks --exclude lrpar-tests to double check that there is no semver bump needed.

[ratmice]

I suppose I was thinking about the dotted keys vs named sections
Technically it seems like we could have both a named section and a declaration of the same name,
giving users the ability to specify the name of their section (via sections_lookup here rather than hard coding %user

We could have both:

%foo {
  // The user specified a foo section
}

%foo later-grmtools-added-a-foo-declaration

The commitment would need to make is that we agree not to add sections other than %grmtools.
There I suppose is an obscure conflict in that it is assuming that the %foo declaration is not starting with a {, at the very beginning of the file at least.

This would reduce some duplication of the tool name

%user {
    nimbleparse_lsp.input_file_extension: ".foo"
    nimbleparse_lsp.grammar_path: "./foo.y"
}

could turn into:

%nimbleparse_lsp {
   input_file_extension: "foo",
   grammar_path: "./foo.y",
}

Actually it's probably a bit more complex than just adding it here, we'd need to ignore it for cases like nimbleparse which aren't expecting/passing any additional section_lookup entries. The approach taken in this patch, definitely eliminates that complexity.

[ltratt]

This raises another possibility (off the top of my head: not properly thought through etc etc!). Perhaps we only need a %grmtools section and we namespace all directives in there (accepting, for backwards compatibility reasons, that not everything will have a grmtools. prefix at first).

[ratmice]

I think there is actually still a bit of a reason to have separate directives in that we can check that each declaration in the %grmtools section is used easily, while it's more difficult for the %user section (because we aren't documenting/exposing the markmap that allows marking them as used)

Well I guess check that the grmtools. prefixed items are used only, and filter out other prefixes in the unused check, we'd have to split the . during the filter (only filtering out items with a non-grmtools non-empty prefix).

[ratmice]

The other thing I would say is that as it is currently written, with the hashmap like API, and the string grmtools.yacc_kind being a different key than yacckind, it's definitely going to double a lot of checks in the codebase while we still have legacy entries without the namespace. It seems like it might be quite a bit.

I don't know if trying to change the way that lookup is handled to use a default prefix/search defaulting to using a grmtools namespace would be viable. But that might avoid adding a second lookup including the namespace.

[ltratt]

Dumb idea: imagine when we see yacckind (and other legacy names) we immediately converted them to grmtools.yacc_kind so there was no duplication. Would that solve the problem? If it doesn't, then I think I should shut up, and we can move on with the PR :)

[ltratt]

I don't have an immediate opinion. We could simply bork if those are passed to a key that isn't grmtools.yacc_kind? That's probably the easiest thing for now, in the sense that it's strict, and we can relax it later if we realise that's the right thing.

[ratmice]

We can do that, but the problem is mostly how we expose grmtools. flags through what is currently the GrammarAST::user_section() -> HashMap<String, UserSectionValue> once it's merged in %grmtools it feels like it should also return the grmtools.* but that means we need to either strip fields like yacc_kind or add a variant to UserSectionValue convert them to, I think we can do something simpler than HeaderValue.
Like UserSectionValue::Constructor("YaccKind::Grmtools") and normalizing the string value.

But whether or not we allow them in user specified fields, whether and how we should/shouldn't expose the grmtools.* ones now that they're in the same section.

[ratmice]

Anyhow, I feel like adding a Constructor variant and converting yacc_kind back into a string shouldn't be that difficult, and even gives us a path to allowing user values to contain these constructor variants in the future if we're so inclined.

So I'll probably start working on it under that assumption, but curious if you have other ideas.

[ltratt]

how we expose grmtools. flags through what is currently the GrammarAST::user_section() -> HashMap<String, UserSectionValue>

[As before, I haven't thought about this carefully so caveat emptor!]

Could we do something like GrammarAST::headers_starting_with(prefix: &str) -> impl Iter<...>? Then if you want the grmtools headers you do ast.headers_starting_with("grmtools") and so on?

[ratmice]

Yeah, I think we could do something like that. We might also just have headers() and make the user perform a `filter(|(key, val)| key.starts_with("grmtools")).

Briefly I had considered whether entries with the grmtools prefix should be present when they are in the GrammarAST but not in the %grmtools section, but it is hard to be consistent with that when some fields originated as values in ctbuilder.rs rather than GrammarAST. Like warnings_are_errors was I believe never a field of GrammarAST. It also just doesn't really match the headers notion.

[ratmice]

Closing this for now while we try the other approach discussed, we can always reopen it if that doesn't work out.