Lrpar codegen by ratmice · Pull Request #657 · softdevteam/grmtools

ratmice · 2026-08-07T03:52:18Z

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...

BuildEnvArgs is kind of a minimalist equivalent of the current builder, it's just full of Option values.
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.
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.

…truct.

ltratt · 2026-08-07T08:19:47Z

+    src: &'a str,
+    // We store the path here so we can generate a module name from it if needed.
+    // But should never use it for filesystem interaction within this module.
+    path: &'a Path,


I think that in some future use-cases we might not even have a Path for a given input (e.g. if it's from stdin or a buffer in memory)? If so, I wonder if we should just make this Option<String> for the path? [Maybe not for this PR, but as a simple follow-up.] That might avoid a (borderline-but-not-quite pathological) case where the Path disappears and we don't get a valid pathname out of it later.

I think we kind of need something here stdin, e.g. we derive the default mod_name from it, and error formatting will need some label for error printing, I do think it could be a string though.

ltratt · 2026-08-07T08:25:06Z

+pub(crate) struct ParserBuildEnvArgs<'a> {
+    /// This allows the parser to originate from from a pre-parsed AST, rather than
+    /// parsing a grammar definition given as source string into an AST.
+    ast_originated: Option<&'a ASTWithValidityInfo>,


I ideally wondered if there are only two routes to us getting the AST: pre-parsed AST or string. I think "there are only those two routes" but perhaps I'm being narrow-minded!

Either way, I wondered if we could maybe tweak this field name a bit. Maybe just ast_validity_info with a docstring like "If the input came from a preparsed AST, this will be Some"?

That works for me.

ltratt · 2026-08-07T08:36:05Z

+}
+
+impl<'a> ParserSrcEnv<'a> {
+    pub(crate) fn new_with_defaults(


Will there ever be another new method? Should we just call this new?

I originally had it named new, but changed it in case we want a method new like so:

pub fn new(src, path) -> Self { self.new_with_defaults(src, path, Header::new()) }

The thought was that this could allow us to make Codegen public without making Header public,
the public API though would only be usable with grammars that have filled out grmtools sections.
And wouldn't allow specifying default values via the header. So a little less flexible than what the builder can do, but something.

Maybe this method should be new_with_header?

Definitely a better name.

ltratt · 2026-08-07T08:37:37Z

+        }
+    }
+
+    fn extract_ast_validation(


Any reason to prefix these methods with extract_? I'm not saying it's wrong, but I wondered if it has more semantic implication than is immediately obvious to me. I also suspect these should document that they mark headers as used?

I had meant to rename these resolve_ but I forgot, is resolve any better?
Essentially these are resolving them to some final/absolute value, from all the possible ways they could have defaults set for them.

ltratt · 2026-08-07T08:38:06Z

+        }
+    }
+
+    pub(crate) fn build_env<LexerTypesT>(


Dumb question: could/should this ever be called more than once? If "no" should this consume self?

I think calling it more than once would be a waste and inefficient, there is seems no actual harm in it though.
(I should say, there is no reason to, but there is no reason it couldn't either)

However his can't really consume self though:
Codegen::generate() and many functions (all error handling) after build_env require a &ParserSrcEnv reference. I didn't really think about this, and whether BuildEnv should take ownership of the SrcEnv after the call to build_env though.

FWIW, it looks like Codegen::generate also uses it for error handling.
One thing to note though is what I said in the PR description:

This may not be totally perfect basis for traits and external usage, for example it currently returns Box instead of typed errors.

As an example nimbleparse_lsp wants to send spans directly to the editor, so it wants a totally separate error formatting code than tools like nimbleparse or CTParserBuilder.

Perhaps that changes things.

So, it appears that not all of these errors actually have typed errors (aren't coming from a YaccGrammarError).

For instance the following error, I'm not exactly certain why this error is only being caught so late.
It's the only one that can happen during generate(...), and so is why we need to pass in a src_env.

grmtools/lrpar/src/lib/ctbuilder.rs

Lines 1659 to 1663 in 3a9d88f

s.push_str(&diag.underline_span_with_text(

rule_span,

"Rule missing action type".to_string(),

'^',

));

I'm assuming if we can't catch it earlier for some reason, we could always add a CodegenError type?

ltratt · 2026-08-07T08:39:35Z

+    }
+}
+
+impl<'a, LexerTypesT> ParserBuildEnv<'a, LexerTypesT>


Given that this is struct is private to the crate, I wondered if these getter methods are serving a useful purpose?

So, the only real vital getter that I recall is fn ast_validation it is vital, because that that is how we get the list of warnings, to error from warnings_are_errors in CTParserBuilder.

A lot of the other getters are less vital, the reason they were added was that I couldn't keep into self while codegen was partially migrated from CTParserBuilder to codegen, where we didn't have access to the private variables from outside the module.

But what that does mean, is that if we have an external Codegen trait, that the ParserCodegen struct could be derived from an external source. So if we did make it public, there are getters for all the things needed by the codegen module itself.

I don't exactly know about trait boundaries and the like given that e.g. BuildEnv::code_generator actually returns a CodeGen whether that would make it impossible to use with some external codegen impl.

That was my thinking behind keeping them anyways.

ltratt · 2026-08-07T08:40:16Z

+        &self.stable
+    }
+
+    pub(crate) fn take_parser(


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

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.

ltratt · 2026-08-07T08:44:18Z

+            // `lrpar::Node`` is deprecated within the lrpar crate, but not from within this module,
+            // Once it is removed from `lrpar`, we should move the declaration here entirely.
+            Some(quote! {
+                        #[allow(unused_imports)]


We can probably dedent this a bit (cargo fmt doesn't tend to deal well with macros, so we often have to do it by hand).

ratmice added 14 commits August 6, 2026 19:00

Initial steps towards a lrpar::codegen module.

5a0f5f2

Build the grm, state graph, and state table as part of the code_gen s…

dd71ab0

…truct.

Prepare rebuild_cache to be moved

d51e3b7

Move rebuild_cache to the codegen module

b8c0755

Prepare output_file and friends to be moved

653a285

Move gen_user_actions to the codegen module

1c6e6ac

Move gen_rule_consts to the codegen module

9690e11

Move gen_token_epp to codegen module

7c7f428

Move gen_parse_function to the codegen module

2d95ae4

Move gen_wrappers to codegen module

a521c26

Move most of output_file to the codegen module

6a39e1b

Move additional codegen helpers

b55acef

Remove extraneous pub(crate) since everything is moved

22b6d7f

Remove/fix any lint suppressions

cf24f1d

ratmice assigned ltratt Aug 7, 2026

ltratt reviewed Aug 7, 2026

View reviewed changes

	s.push_str(&diag.underline_span_with_text(
	rule_span,
	"Rule missing action type".to_string(),
	'^',
	));

Uh oh!

Conversation

ratmice commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratmice Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ratmice commented Aug 7, 2026 •

edited

Loading

ratmice Aug 7, 2026 •

edited

Loading

ratmice Aug 7, 2026 •

edited

Loading