Very rough first draft at codegen module

[ratmice]

Here is a (not very pretty) first draft which is mostly just pulling code as-is out of CTLexerBuilder::build() into smaller, more manageable functions. I don't think anything here is really properly named, codegen at times seems like it's doing more parsing/validation than actual code generation (where it never uses it's self type!)

I thought that the splitting of the CTTokenMapBuilder turned out okay, however it is quite a bit simpler with less moving parts.

My thoughts are to try and rename the LexCodegen structure to something like LexCodegenBuilder,
and then try see if we can manage to come up with a good Self type for the codegen parts.

Edit:
I haven't tried to make anything pub here yet just pub(crate), aiming for close to verbatim code movement.
I do have an idea of what a second draft might look like, we'll see if it works soon...

But so far trying to make e.g. a two pass API for checking then codegen has proven elusive,
Primarily because of data dependencies between the checking phases, and callbacks like lrpar_config, I've managed to get some nicer API to compile, but not pass the testsuite.

[ratmice]

Well, I did manage to get a second draft up and working, and it feels like it's starting to resemble an API...
The data dependencies ended up being the lexerdef.set_rule_ids_spanned call, and the rule_ids_map.

So the way I tried to work around this was

1. Construct the LexCodegen pretty early, before all the elements it's composed of have been checked for validity.
2. Do the checking, and grab mutable references to any elements that need to be updated from the codegen.
3. Finally once all the checking and updating has been done, generate code.

So it isn't exactly a 2 pass system of validation/generation, but it kind of behaves like one unless you're taking mutable references.

I still haven't made anything pub though.
I need to do another pass over the changes, I had added some interim cloning (which ended up causing issues, before switching to mut refs) and need to make sure I got rid of all that.

[ratmice]

It was only towards the end of getting this working that I realized we need to keep the builder around
after build() to call cgb.lex_diag() some more.

Thus we should probably come up with a better name than LexCodegenBuilder since it isn't exactly the builder pattern anymore. I was thinking maybe LexCodegenSourceEnv or something

[ratmice]

One more issue with this build method, is the list of arguments isn't easily extensible in the way a real builder would be. Maybe we should put all these args actually into the struct.

Edit: For this we'll probably need to add the Clone trait to LexerKind. I'm a bit uncertain about that
In particular when it comes to future expansion with LRStreamingLexerer I'd always imagine we would need to pass a data channel in through LexerKind using some kind of spsc which wouldn't necessarily need to impl clone?

I'll explore a different option.

[ratmice]

ba0891b was my attempt at both these issues,
I renamed LexCodegenBuilder to LexSrcEnv, and introduced a struct LexCodegenArgs for stuff which LexSrcEnv takes ownership of but can't easily be owned by it's Self.

[ratmice]

This is starting to feel like we could start to consider pulling out of draft mode, and discuss making these pub(crate) functions pub. One thing I'm uncertain of though is whether these structures are going to translate directly into lrpar.

Edit: Regarding lrpar, at least one thing I'm noticing is that lrpar needs mechanisms for dealing with warnings.
While lrlex doesn't currently have warnings, this will probably end up changing the abstraction a little (not certain about the specifics).
It might be worth adding an empty warnings and vector and type into lrlex, just to keep the abstractions the same.
Perhaps that might be done separately though?

Edit 2x: The caveat to this is that I'm pretty uncertain in my ability to actually anticipate where warnings would arise from though in the api

[ratmice]

Perhaps this header value seems like the biggest impediment to making any of this API public, as it is currently a #[doc(hidden)] part of the API. It stores all the things like all the LexFlags coming from the CTLexerBuilder.
It the parsed_header is subsequently built by the %grmtools section. Those two header values then get merged below.

I say this partly because there was a moment at some point where I was confused why we weren't dropping a bunch of CTLexerBuilder options on the floor.

One thing we can perhaps consider is making the constructor public,
so downstream you can construct an empty one, outside of CTLexerBuilder codegen options could only be set when embedded in the %grmtools section.

I haven't had much in the way of other good ideas here. It isn't something I actually paid any mind to when initially making the patch.

[ratmice]

Probably a better way is that we could just make a pub version of new without the header argument at all like...

  pub fn new(src: &'a str, path: &'a path) -> Self {
     Self::new_with_header(src, path, Header::new())
  }

Not sure why I didn't think of it earlier.

[ratmice]

One slight step further to allow configurability of default values without making header public might be to make the type public but keep all the methods hidden, then we can have a way to obtain a Header from a CTLexerBuilder,
So you could call LexSrcEnv::new(src, path, ct_lexer_builder.header()). With that people could still configure it through the builders, but the type stays mostly opaque.

Edit:
Alternately we could consider just adding method like code_generator to CTLexerBuilder so the LexerSrcEnv could stay entirely private. I suppose the difficulty with that is that then there are two routes to generating code from the builder, the one people should use build(), and code_generator() that people will trip over.

[ratmice]

Should mention this implements the lrlex parts of #648

[ltratt]

I'm a bit behind on reviewing, but will hopefully get to this one soon!

[ratmice]

no worries, I understand, and also am also still trying to figure out an impl for lrpar.

[ratmice]

One thing that has occurred to me is that maybe it would have been nicer if I did one commit for each moved code block, instead of just moving a bunch of code blocks.

Let me know if you think that kind of approach would help, I could probably split it up.

Edit: On second thought it may not be possible without adding Clone bounds to some variables.

[ltratt]

Apologies again for being slow on this one: I wanted to do my best to digest it and think about the consequences! I broadly get where this is going: I agree with you that trying to think of the right names is always challenging when doing this sort of refactoring.

I'm intrigued by this:

But so far trying to make e.g. a two pass API for checking then codegen has proven elusive. Primarily because of data dependencies between the checking phases, and callbacks like lrpar_config, I've managed to get some nicer API to compile, but not pass the testsuite.

Let's assume we could get such an API past the test suite: what would the impact on users be? Would we be able to provide them a painless upgrade path or would they have to hack things around?

[ratmice]

I did actually manage to get a two-pass API to work (I guess it would be two-ish, i'll explain), there were two primary things that allowed it to work,
moving ownership into the codegen, and then allowing a lexerdef_mut accessor to borrow a mutable reference.
The other was a code organization regarding logical error handling.

When I say logical error handling, I mean cases where the control flow is roughly like

let foo = ...;
if something {
   return Err(...)
} else {
   let bar = foo.do_stuff();
}

The thing to note is that if foo.do_stuff() has no observable side effects (like the codegen passes) this can be reordered equivalently to

let foo = ...;
let bar = foo.do_stuff();
if something {
   return Err(...);
}

These two things, grabbing mutable borrows, and the reordering went a bit hand-in-hand.
But basically ended up with two-ish passes:

let src_env = LexSrcEnv::new(...);
let codegen_args = LexCodeGenArgs{...};
let codegen = src_env.code_generator(args)?;
// borrow code_gen mutably and e.g. modify lexerdef
let output_rs_src = codegen.generate();

At least if you consider "code_generator(args)?" and generate() to be the two passes.
Most of the other separation between those structs is just ownership.
With that I was able to get it at least pretty close to two pass and pass the test suite.

I did forget to update my initial message.

[ltratt]

Interesting! I wonder if we should take a brief step back and ask ourselves: knowing what we know now, about how people use these tools, and how we would like them to be able to use them in the future, what would the perfect API look like? We could sketch that out, and then work out (a) how big a change it would be for us to make a reality (b) if there's a plausible upgrade path for users. The reason for this is that it might free us from some of the accidental nastiness that, inevitably, has been baked in over time as we've evolved things (and, of course, many bad decisions I took back in the early days!).

[ratmice]

I'm actually pretty happy with they way (this series) turned out at the moment.

It's funny actually a lot of the things that were proving difficult are some of the things I've added (fairly recently even).
Things like the running an RTParserBuilder with test inputs (These are exactly the kinds of things that we needed to reorder the error handling/codegen building).

The other thing that requires some finesse is things like the communication with cargo being println("cargo:{}", ...),
which doesn't belong in the codegen. That sort of stuff is something we're always going to have to work around with all APIs (this has more impact in lrpar due to cargo:warning=.

So I think because of cargo we're always going to have this kind of control flow where we're borrowing references
between building the code generator and generating code?

[ratmice]

I'll definitely have a think on these things though, but definitely nothing instantly jumps out at me!

[ltratt]

Dumb idea: perhaps one of the problems is that we've hard-coded the output of codegen? I wonder if we had some sort of trait Codegen (perhaps even more abstract an idea than "codegen"?!) it would help us isolate more of the bits that are currently intertwined?