Header recoverykind

[ratmice]

This adds one commit(1cc1df3) on top of the patches PR #549.
Adding RecoveryKind as a allowed/working field within the %grmtools section.

Edit: Rebased this on master

[ltratt]

Is there any way to build a test to cover this functionality?

[ratmice]

I was struggling to consider how to test it, because we can't really get the recoverykind out of the ctbuilder.
We could get the header out with that callback mechanism we discussed, but that doesn't really check the conversion.

I think we can kind of modify cttests to do something, but it doesn't yet support sending recoverer to the builder.
but if we make that change we can in lib.rs send bad input and look for ParseRepair?.

Anyways, you remind me I tested nimbleparse, by hand but forgot to test CTParserBuilder, looks like there is some kind of an issue with marking used there; but it doesn't jump out at me yet. (Edit: Oops, I was just testing on master which of course doesn't use recoverer yet)

[ratmice]

I wonder about an approach involving #[cfg(test)] conditional methods to CTParserBuilder with callbacks,
It seems nasty though, because the obvious way is to store those callbacks in Self and changing the struct, which is never good.

#[cfg(test)]
fn test_callback_recoverkind(self: CTParserBuilder, Fn(RecoveryKind) -> Result<(), Box<dyn Error>>) -> Result<(), Box<dyn Error>> {
}

[ratmice]

So, my gut leaning towards using the callback mechanism, we'd discussed in #549 I mean I was hoping the callback mechanism would be useful for testing, and so this seems like a good case. We can #[cfg(test)] the method for now, while leaving the callback field always a member of the builder.

I figure we if we pass the &mut Header, the YaccGrammar, and the RecoveryKind, to the callback
(right before we write the grammar to the OUT_DIR, but after we've constructed it), and return a Result<(), Box<dyn Error>> from that callback.

That'll give testing crates everything they need to construct an RTParserBuilder and run through their test cases.
But it also gives us access to the RecoverKind we need here.

[ltratt]

That'll give testing crates everything they need to construct an RTParserBuilder and run through their test cases.
But it also gives us access to the RecoverKind we need here.

It does sound like it's quite probably the way to go, even in the context of this PR?

[ratmice]

Assuming it works, running into some silly lifetime issues though.

[ratmice]

So, this works for our testing purposes

I was not able to make it #[cfg(test)] because the testing code is in cttests/build.rs, and I don't really know,
but it appears that in my experience build.rs doesn't get built separately for #[cfg(test)]?

That means we can't easily play around with this in the testsuite without making it public.
I guess we could write this in a #[test] but then it isn't running with cargo variables etc.

Going to go ahead and mark this as draft now, unless we can avoid making it pub

Edit: I still have yet to test that this is enough to build an RTParserBuilder from build.rs.

[ratmice]

So, in my first attempt there are a few things missing to be able to actually
construct an RTParserBuilder from within the process_header callback (this is probably badly named, perhaps fn inspect(...)

StateTable<StorageT> is easy to add, and then you can construct one, however it's difficult to call parse,
the main difficulty is getting at a lexer from within the callback, as you don't actually have a way to get the path to the lex file, to try and construct the lexer.

In theory we could maybe add the lex file path to the header from CTLexerBuilder that is the only idea I've had so far.

[ratmice]

Adding this PathBuf for lexer_path is kind of a hack, we're accruing lots of parameters,
I'm not sure if in this case we could add the lexer_path to Lex's Header and we could give CTParserBuilder a copy of CTLexerBuilders header (we may actually need to!)

Anyhow what i'm trying to say is we could consider putting the lexer path inside the header too instead of having a specific parameter for it.

[ltratt]

what i'm trying to say is we could consider putting the lexer path inside the header too

I don't want to be too dogmatic about this, as this might be the right solution, but I tend to lean towards thinking of the path as an "external" thing rather than part of the header.

[ratmice]

It is also worth noting that this whole, don't build the parser at all unless it parses these things is fairly unforgiving. It doesn't leave much for ways to poke the broken parser. I'd say it is no replacement for a testing mechanism. But I do see the appeal of "do not generate the parser unless it fulfills these baseline functionality".

In that sense, I'm uncertain about the example here.

[ltratt]

Dumb question: are we currently thinking of this as an internal or a potentially external API? If the former, I would consider this a "test" rather than an "example".

[ratmice]

Because we can't #[cfg(test)] it easily and use it from within cttests, that makes it pub, so this is experimenting with considering it an external API.

Hence my concern above about the example being pretty unforgiving, and not likely something you'd want to use as a replacement for #[test], in other words I worry that it will be taken as a test rather than a way to abort compilation.

Maybe the right thing to do is to run the cttests recoverykind tests under a #[test] rather than within build.rs, so we can avoid making it pub until we're ready, I suppose it is already dealing with non-source-dir paths to the generated yacc files.

[ratmice]

The plan to move it into a #[test] under lrpar/cttests/src/lib.rs doesn't actually work,
because while cttests lives under the lrpar/ in the file system, from the cargo/crate perspective they
are completely separate crates, so adding it to #[cfg(test)] still doesn't make the callback visible within cttests.

[ratmice]

I guess I could do it within a #[test] under lrpar/

[ratmice]

I went ahead and reverted the addition of the example in fad02ac, and the making it pub,
and added these tests to ctbuilder::tests in fcaefc9

This means we aren't actually building the resulting sources, but it's not nothing?

[ratmice]

So with the change scaled back down to just doing testing, and avoiding adding any new pub items, I went ahead and unmarked as draft.

[ltratt]

Do we need to make this doc(hidden) too? I think it will show up in rustdoc otherwise (but I might be wrong).

[ratmice]

We shouldn't need to, by default rustdoc only documents the specified cfg values, or the ones specified by cfg(doc)

https://doc.rust-lang.org/rustdoc/advanced-features.html#cfgdoc-documenting-platform-specific-or-feature-specific-information

[ltratt]

Please squash.

[ratmice]

Squashed.

[ratmice]

Should be fixed in 367f715.

Apparently rustfmt actually wanted that random indentation change.