Lrpar codegen

[ratmice]

I think this is probably as reviewable as I'm going to manage to make this large of a patch.
The basic idea behind this patch is to have a kind of "functional pipeline", for doing code generation,

(SrcEnv?, BuildEnvArgs) -> BuildEnv? -> Codegen(SrcEnv, BuildEnv) -> rust_code?

A bit of an oversimplification, as there are some other minor details...

1. BuildEnvArgs is kind of a minimalist equivalent of the current builder, it's just full of Option values.
2. BuildEnv is full of derived values, it mostly strips off the Option, but it also contains values like ASTWithValidityInfo that are derived from all the other args.
3. Codegen then owns the YaccGrammar, StateTable and StateGraphs, which you can take ownership of after generating code.

This may not be totally perfect basis for traits and external usage, for example it currently returns Box<dyn Error> instead of typed errors. But it should be a pretty faithful conversion of the existing process, into a more targeted/self contained module.

[ltratt]

This one's interesting because it consumes self. Why/when do we use it in that way?

[ratmice]

This is for the return values to CTParserBuilder::build we construct a CTParser from it in the Ok case.
And I think we need to take ownership of the StateTable and StateGraph for CTConflictsError in some of the error cases too.

[ratmice]

I was wondering if fn finish(self) -> (YaccGrammar, StateGraph, StateTable) or finished might be a better
name, since technically it's taking more than just the parser?

I don't know if it's really finishing or finalizing anything, but maybe it conveys better that we're done with self and taking ownership of all the interesting values.

[ltratt]

I agree, that's clearer.

[ratmice]

Changed to finish in 57cd704

[ratmice]

My attempt to avoid conflicts didn't work (We modified lines that are removed too!),
so this'll need a rebase or reopening as a new PR.

But this does run though cargo test, despite some errors are temporarily panics, because it doesn't have the error handling we moved into cfgrammar.

[ratmice]

So I think ideally we'd be able to retain some mention of CTParserBuilder in this error text, which
in my patch series we've lost, to me it indicates that mod_name might want to be routed through Header, so it can pick up a location of CTParserBuilder or so.

There seem like a number of places in this patch where the origin of some error condition might be improved, but I'm hesitant to try and do too much/everything in this specific series.

[ltratt]

I agree: we don't have to do everything in one go.

[ltratt]

I suspect it's worth merging master in as a merge commit sooner rather than later, as it'll make the eventual rebase easier.

[ratmice]

I'm actually not really worried about the rebase, since all the conflicts should be of the form delete both the incoming and current changes (probably famous last words). But will try and figure out how to merge master without rebasing.

I believe that 5143009 should bring it in line with the current master branch

[ltratt]

I think we're probably ready to squash?

[ratmice]

I think so, but I wanted to think given all the simplifications we've done, whether we can move back to the more simpler model like we started with with lrlex (or if we'd want to).

I think it might be feasible now to have the two-phase thing, but I kind of like the SrcEnv, BuildEnv split, and the differences between errors are pretty stark. (e.g. Span on errors only happens in SrcEnv which makes sense).
I think we'd basically have to squash those errors together in the same type to go back to the two phase approach.
But if we ignore that, now that we aren't borrowing both the SrcEnv and the BuildEnv it seems like it could be possible combine them?

I'd like to mull that over a bit, hear what you think first.

[ltratt]

Taking a step back is always a good idea if it might help us reach a better decision. I'm a little bit unsure about the best way ahead, as I admit I've slightly muddled myself in the detail.

[ratmice]

I think if it's going to work, basically we'd be combining the ParserSrcEnv and ParserBuildEnv struct fields,
then we'd move the ParserSrcEnv::build_env function into ParserSrcEnv::new_with_header and make that return a Result<Self, ParserSrcEnvError>.

So, ParserSrcEnv would now build a ASTWithValidationInfo on new. Then we could put the code_generator method on that. We maybe could keep the error split by having different errors returned by new_with_header and code_generator on the same type.

But I think it might allow us to squash those two types together. I believe the main reason I had to separate them is we needed to borrow ParserSrcEnv::yacc_diag(), and then also borrow ParserBuildEnv::ast_with_validation_info(), but we removed the former.

That said, I do kind of like how this current patch is structured in the sense that building a src env, and a build env, etc seems like a logical sequence.

Edit: There is probably a simple way we can test this by making a constructor on ParserBuildEnv that internally calls ParserSrcEnv::new_with_header().build_env() I'll play around with the idea tomorrow

[ratmice]

So, I did end up quickly testing that out, and it does work but it didn't really spark joy, mostly because I had forgotten about the args parameter to the constructor, which it has to bring in too.

    pub(crate) fn new_with_header(
        src: &'a str,
        path: Option<&Path>,
        header: Header<Location>,
        args: ParserBuildEnvArgs<'a>,
    ) -> Result<ParserBuildEnv, ParserSrcEnvError> {
        ParserSrcEnv::new_with_header(src, path, header).build_env(args)
    }

It is perhaps one less type we have to make public, but the construction progression doesn't seem to feel nearly as nice.

[ltratt]

I tend to agree.