Since bincode is unmaintained use wincode instead.

[ratmice]

This is the final patch in the series, this should fix #616. Serialization with our complex bounds is quite a bit less straight forward with wincode than it was with bincode. Because the Configuration is parameterized in the serialization bounds.
Thus we end up having to put bounds on multiple serialization configurations in order to serialize data with that configuration.
At least that was the only way I could figure out how to implement it.

This doesn't provide any CTParserBuilder mechanisms for configuring the encoding used, so it currently always defaults to the VarInt encoding, but the plumbing is all there to do so.

This is part of a series of patches to various crates moving from the bincode crate to the wincode crate.
I believe the reverse order of dependencies is:

1. packedvec
2. sparsevec
3. vob
4. grmtools

I'm currently leaving how we want to deal with semver up in the air. Since these do change trait impls around,
It may be that as we release these we need to fiddle with the version numbers in Cargo.toml too.

[ratmice]

Since in #631 this was calling this function in hot loop,
I should try and put this match statement in the closure body I think.

[ratmice]

Should be fixed in d74ca12

[ratmice]

This was tested with local versions of all the crates using the following Cargo.toml

diff --git a/Cargo.toml b/Cargo.toml
index c1f3ce15..662a5ae8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -48,3 +48,8 @@ proc-macro2 = "1.0"
 prettyplease = "0.2.31"
 syn = "2.0"
 yaml-rust2 = "0.10.1"
+
+[patch.crates-io]
+vob = {path = "../vob", version = "3.0.4"}
+sparsevec = {path = "../sparsevec", version = "0.2.2"}
+packedvec = {path = "../packedvec", version = "1.2"}

[ratmice]

It is a bit unfortunate, but as far as I can tell it doesn't appear that we can just do something like:

LexerTypesT::StorageT: SchemaWrite<Config, Src = LexerTypesT::StorageT> + ...;
Config: for<c: wincode::config::Config>, c,

Because higher ranked trait bounds are limited to lifetimes.
Config here is also not dyn compatible because of the associated types and consts.
So if there are other wincode configurations we do want to try encoding with afaik we have to add a bounds
here for each of those.

[ratmice]

Working on the follow up commit that configures encoding between variable, and fixed...

[ratmice]

Still needs some work to prepare it for follow up patches.

[ratmice]

Should be None at this point like yacckind and recoverer, above.
Otherwise we can't differentiate between the default value, and user having set it.

[ratmice]

I went ahead and fixed this in f0da9ba

Which contains the whole configuration scheme, I did some looking at the generated sources,
and the variable in encoding files are both about half the size of the fixed int encodings,
but the fixed int encodings should should load faster.

If we don't want that, parts of the first patch can be simplified (Like the multiple Schema bounds on different configurations)

[ltratt]

@ratmice Let me handle vob and sparsevec to save you the effort,

[ratmice]

Woops, think I might have already pushed to my branch, but feel free.

[ratmice]

🤞 other than version bumping, I think this one is ready for being looked at whenever you've got time.

[ltratt]

I'm working my way through. I must admit that I didn't anticipate wincode having quite such an impact! Still, this is definitely the right thing to do.

[ltratt]

I have a few minor comments and one bikeshed: do we think it's obvious what "encoding_config" will be to users? I'm somewhat neutral on this: I did wonder if "serialisation_format" (ok, ok, I could live with a "z" in there :)) might be clearer, but I'm not sure.

[ltratt]

I think all of the necessary dependencies are now on crates.io.

[ratmice]

I went with SerialisationFormat in 2822302 so at least we're consistently british spelling!

[ratmice]

This is a pretty awkward API, the thing going on is that we've parameterized between
logical locations (command line arguments in nimbleparse, builder parameters in CTParserBuilder, and source locations read from %grmtools sections).

In the first two cases we have Value<Span> while in the second we have Value<Location>.

We could return a Vec<T> where T: Clone here, but in all the Value<Location> areas, this is going to be extremely unhelpful, and in the Span case, PrimaryLocation will still return at least one relevant location.

So we may drop some information regarding source locations when dealing with Span,
but we also avoid emitting redundant locations, when all those locations are like CommandLine

[ratmice]

So, it's kind of like we're either going to have this information loss, where we drop "secondary" locations,
or we're going to have a bunch of redundancy. So I think picking one of the locations is ok.

[ltratt]

Since we've got a major version bump coming up, we could consider an API change if it would help?

[ratmice]

I hadn't given it a whole lot of thought, In that the whole mixing of spans and logical error locations is kind of new territory for me, so I've kind of been winging it. One thing we could consider is the way that the AST Value<T> is currently an enum which has the pair of e.g. String, and Span, but also String, and Location.

We could consider making it a

enum Setting {
   Num(u64, usize)
   Flag(bool, usize),
   Array(Vec<Setting>, usize, usize)
}
struct ValueRepr<T> {
    value: Value,
    locations: Vec<T>   
}

With that the Location::CommandLine and Location::Other("From<...>") could just push one location on the locations stack, and we'd use the usize indexes to point to that one location.

With spans, each of these could refer to different locations on the locations stack.

I think one of the questions is whether we could get the lrpar/cttests/grmtools_section.test to emit that structure.
Because we test that the AST structure is exactly the same for the handwritten parser, and the generated parser in the testsuite. It's hard for me to imagine how this structure representation could be emitted from the generated parser's rules. I'll try and give it a think overnight.

[ltratt]

Hmm, perhaps the current muddle is the best we can do right now.

[ratmice]

Well, I did think of one more place that could be cleaned up in e8a5dab, Edit: the other thing to note is that this is all #[doc(hidden)] too.

[ltratt]

Good point!

[ratmice]

Also let me know if you want me to split this one back into two separate PRs.
I just didn't want to get into a situation where some choice made in the first patch meant we had to make some incompatible change to move forward on making the serialization_fromat configurable.

I'm pretty sure though now that isn't the case.

[ltratt]

I went with SerialisationFormat in 2822302 so at least we're consistently british spelling!

Sorry :)

[ltratt]

I'm fine with this as one PR.

[ltratt]

I think this is ready for squashing?

[ratmice]

Squashed, but unfortunately while I was almost done rebasing, I noticed a pesky line that shouldn't be there (but was actually inconsequential because of the way quote encodes Option types as their contents, and None as empty strings)

Plus I apparently accidentally rebased on top of current master, rather than what I originally based the patch on.
So there are a few changes.

Edit: Actually I may have introduced one issue in the squash accidentally.

[ratmice]

Alright, squashed correctly this time.

[ltratt]

Before we consider making a release: since we've got a major release bump coming, is there anything else breaking we want to include?

[ratmice]

I'm pretty much heads down in another project, and don't really have any API breaking changes that would be helpful paged in.
One thing that comes to mind is a way to run the codegen/builders and say capture the output to a string, or Write trait.
So you could then run the code through something like rust-analyzer, or static analysis, from a .y file passed as a string
rather than as a filename.

With the intent of eventually having rust-analyzer cargo check the embedded actions as you're editing yacc files.
This is kind of where I was heading when I started cleaning up the codegen process using the quote crate.
But never got started on that aspect of it. That seems like something to consider.

I don't know how the rust-analyzer aspects will work, but we probably don't need that figured out to get grmtools in shape for it.

[ltratt]

One thing that comes to mind is a way to run the codegen/builders and say capture the output to a string, or Write trait.

I would love this! Do you think it would require breaking changes? Do you have a rough sense fo how one might go about it?

[ratmice]

My hope was to try and do it without breaking changes, by adding a new module, codegen.rs,
It would largely be taking the existing ctbuilder.rs, from the build() on down, but removing the filesystem interactions.
Then replacing the builder implementation with calls into the codegen module, so the existing builder would go:

let input_yacc_str = input_file.read(..)
let mut output_yacc_rs_str = String::new();
codegen::generate_yacc_rust(input_yacc_str, &mut output_yacc_rs);
output_file.write(output_yacc_rs_str)

If only to avoid introducing more API surface to CTParserBuilder, which it seems good to keep as the primary API for cargo/build.rs. I think it's mostly just a refactor, but not totally positive.

Edit:
There are probably a lot of things I'm not taking into account, like all the configuration methods the builder provides,
but those can largely be configured in the yacc file now, I'm not sure how to do it without making the Header datatype more public.

One thing we could do is consider a separate builder-like API just for configuring, which would largely mirror the configuration aspects of CTParserBuilder. So we could split it into two modules (builder settings, and codegen) then CTParserBuilder would just be a wrapper around these that adds cargo + filesystem interaction. (So essentially builder settings struct would just be for hiding the Header struct, and providing the current builder configuration to the codegen phase)