refactor(codegen): collapse standalone block writers into mixin methods (#206 follow-up) - #211
Conversation
…ds (#206 follow-up) PR #206 left the standalone `pub fn writeXxx(...)` declarations alongside the new `Mixin(Self).writeXxx(self, ...)` methods because the test surface + a couple of external call sites depended on the explicit-arg form. Migrating those callers to the Codegen-context shape removes the duplication: each concern lives in exactly one place (the mixin method), driven by Codegen's accumulated state. - 4 test files refactored to construct a Codegen + dispatch through it (engine_events_discovery_test, backend_wiring_tests, flow_scanner/coercions_tests, flow_scanner/flow_decls_tests). Added `emptyCodegen(allocator)` to test/helpers.zig for the zero-state case. - root.zig's `generateGameShim` migrated to dispatch through a Codegen context for `writePluginEventsBlock`. - main_zig.zig's re-export shim trimmed: standalone block-writer + lifecycle-builder re-exports dropped. What remains: submodule namespaces, the Codegen alias, scan/preview pass-throughs, and LoadStyle (kept because lifecycle siblings consume it as a value). - 12 standalone fns inlined into their mixin methods (5 in plugin_registries, 2 in scene_manifests, 2 in lifecycle/loop, 3 in lifecycle/callback). Signatures now read off `self.*` instead of the explicit arg list; doc comments + bit-exact emission shape preserved verbatim. The 3 standalone helpers in `blocks/asset_wiring.zig` and the one in `blocks/resource_loader.zig` stay `pub` — they're consumed by `lifecycle/{loop,callback}.zig` as sibling-module utilities, not part of the external surface. The Mixin in each of those files still delegates (the mixin reads no state from self in either case, the methods only exist for orchestrator-side dispatch uniformity). Bit-identical generated output across all 8 reachable bundled examples (scripts/gen_all_examples.sh, 3 runs). sokol_imgui has a pre-existing setup failure (local labelle-imgui dep missing) and is unaffected by this change. zig build test green (555/559 — same baseline as today's main).
PR SummaryMedium Risk Overview Public API: Unchanged externally: Reviewed by Cursor Bugbot for commit 822ecc2. Bugbot is set up for automated code reviews on this repo. Configure here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codegen architecture by collapsing standalone block-writer and lifecycle-builder functions into mixin methods on the Codegen context, establishing a mixin-only surface. Corresponding updates were made to the orchestrator, tests, and shims to dispatch through the Codegen context. The review feedback identifies a potential issue in the resolve functions of both PluginFlowNodes and PluginCoercions where module names starting with a digit are not correctly handled, which would cause resolution to fail since generated identifiers are prefixed with an underscore.
| try bw.writeAll(" pub fn resolve(comptime dotted: []const u8) ?[]const u8 {\n"); | ||
| try bw.writeAll(" const dot = std.mem.indexOfScalar(u8, dotted, '.') orelse return null;\n"); | ||
| try bw.writeAll(" const module = dotted[0..dot];\n"); | ||
| try bw.writeAll(" const node = dotted[dot + 1 ..];\n"); | ||
| try bw.writeAll(" if (node.len == 0) return null;\n"); | ||
| try bw.writeAll(" const qualified = module ++ \"__\" ++ node;\n"); | ||
| try bw.writeAll(" if (!@hasDecl(@This(), qualified)) return null;\n"); | ||
| try bw.writeAll(" return qualified;\n"); | ||
| try bw.writeAll(" }\n"); |
There was a problem hiding this comment.
If a plugin or module name starts with a digit (e.g., 3d_physics), the generated identifier is prefixed with an underscore to be a valid Zig identifier. However, the resolve function currently joins the module and node names directly without checking if the module name starts with a digit. This will cause resolution to fail for such modules.
Please update the resolve function to prepend an underscore if the module name starts with a digit, ensuring consistency with the generated identifiers.
try bw.writeAll(" pub fn resolve(comptime dotted: []const u8) ?[]const u8 {\n");
try bw.writeAll(" const dot = std.mem.indexOfScalar(u8, dotted, '.') orelse return null;\n");
try bw.writeAll(" const module = dotted[0..dot];\n");
try bw.writeAll(" const node = dotted[dot + 1 ..];\n");
try bw.writeAll(" if (node.len == 0) return null;\n");
try bw.writeAll(" const qualified = if (module.len > 0 and module[0] >= '0' and module[0] <= '9')\n");
try bw.writeAll(" \"_\" ++ module ++ \"__\" ++ node\n");
try bw.writeAll(" else\n");
try bw.writeAll(" module ++ \"__\" ++ node;\n");
try bw.writeAll(" if (!@hasDecl(@This(), qualified)) return null;\n");
try bw.writeAll(" return qualified;\n");
try bw.writeAll(" }\n");
References
- Zig identifiers must not start with a digit. When converting strings (such as file paths) to Zig identifiers, prefix the result with an underscore if the input starts with a digit to ensure the generated code is valid.
There was a problem hiding this comment.
Verified the finding is correct — sanitizePluginIdent (scan.zig:819) does prepend _ for digit-leading module names, so the generated resolve body's module ++ "__" ++ name join fails @hasDecl for those modules. But the bug pre-dates this PR (same string lives on main), and this PR's contract is bit-identical generated output — applying the fix here would force regenerating every example baseline and entangle a correctness change with the refactor.
Filed as #212 to fix in a follow-up; the suggested two-line change there mirrors what you proposed.
| try bw.writeAll(" pub fn resolve(comptime dotted: []const u8) ?[]const u8 {\n"); | ||
| try bw.writeAll(" const dot = std.mem.indexOfScalar(u8, dotted, '.') orelse return null;\n"); | ||
| try bw.writeAll(" const module = dotted[0..dot];\n"); | ||
| try bw.writeAll(" const name = dotted[dot + 1 ..];\n"); | ||
| try bw.writeAll(" if (name.len == 0) return null;\n"); | ||
| try bw.writeAll(" const qualified = module ++ \"__\" ++ name;\n"); | ||
| try bw.writeAll(" if (!@hasDecl(@This(), qualified)) return null;\n"); | ||
| try bw.writeAll(" return qualified;\n"); | ||
| try bw.writeAll(" }\n"); |
There was a problem hiding this comment.
If a plugin or module name starts with a digit (e.g., 3d_physics), the generated identifier is prefixed with an underscore to be a valid Zig identifier. However, the resolve function currently joins the module and name directly without checking if the module name starts with a digit. This will cause resolution to fail for such modules.
Please update the resolve function to prepend an underscore if the module name starts with a digit, ensuring consistency with the generated identifiers.
try bw.writeAll(" pub fn resolve(comptime dotted: []const u8) ?[]const u8 {\n");
try bw.writeAll(" const dot = std.mem.indexOfScalar(u8, dotted, '.') orelse return null;\n");
try bw.writeAll(" const module = dotted[0..dot];\n");
try bw.writeAll(" const name = dotted[dot + 1 ..];\n");
try bw.writeAll(" if (name.len == 0) return null;\n");
try bw.writeAll(" const qualified = if (module.len > 0 and module[0] >= '0' and module[0] <= '9')\n");
try bw.writeAll(" \"_\" ++ module ++ \"__\" ++ name\n");
try bw.writeAll(" else\n");
try bw.writeAll(" module ++ \"__\" ++ name;\n");
try bw.writeAll(" if (!@hasDecl(@This(), qualified)) return null;\n");
try bw.writeAll(" return qualified;\n");
try bw.writeAll(" }\n");
References
- Zig identifiers must not start with a digit. When converting strings (such as file paths) to Zig identifiers, prefix the result with an underscore if the input starts with a digit to ensure the generated code is valid
There was a problem hiding this comment.
Verified the finding is correct — sanitizePluginIdent (scan.zig:819) does prepend _ for digit-leading module names, so the generated resolve body's module ++ "__" ++ name join fails @hasDecl for those modules. But the bug pre-dates this PR (same string lives on main), and this PR's contract is bit-identical generated output — applying the fix here would force regenerating every example baseline and entangle a correctness change with the refactor.
Filed as #212 to fix in a follow-up; the suggested two-line change there mirrors what you proposed.
|
@copilot review |
…216) The assembler's `sanitizePluginIdent` prefixes a `_` to plugin module names that start with a digit (Zig identifier rule), so a plugin `3d_renderer` ships its FlowNodes / Coercions decls under `_3d_renderer__<name>`. But the generated `resolve()` body computed `qualified = module ++ "__" ++ node` straight off the user-supplied dotted name, so `resolve("3d_renderer.foo")` produced `3d_renderer__foo` — silently missing the `_`-prefixed decl and returning `null` for every flow / coercion reference into a digit-leading plugin. Mirror the digit-prefix + non-identifier collapse in the emitted resolver via a new comptime `sanitizeModuleIdent` helper inside the generated `PluginFlowNodes` / `PluginCoercions` structs. Pure comptime, zero runtime cost. Adds a regression test that asserts the new helper + sanitized join lands on the resolver shape; pre-fix the test fails because the emitted body still says `module ++ "__" ++ node`. Caught by gemini-code-assist on #211 but kept out of that refactor's bit-identical-output scope; filed as #212.
Summary
pub fn writeXxx(...)declarations are collapsed into the matchingMixin(Self).writeXxx(self, ...)methods. Every external caller (orchestrator + tests +root.zig:generateGameShim) now dispatches through aCodegencontext, matching howmain_template.zigalready worked.blocks/plugin_registries.zig(5),blocks/scene_manifests.zig(2),lifecycle/loop.zig(2),lifecycle/callback.zig(3). Bodies now read state offself.*instead of taking explicit args.main_zig.zigre-export shim trimmed from 111 → 95 lines: dropped all per-fn re-exports, kept submodule namespaces +Codegenalias + scan/preview type pass-throughs +LoadStyle(still passed as a value by lifecycle siblings).blocks/asset_wiring.zigandblocks/resource_loader.zigkeep their standalonepub fnforms — they're consumed bylifecycle/{loop,callback}.zigas sibling-module utilities, not part of the external surface. The Mixin methods still delegate uniformly so the orchestrator can callctx.writeXxx(...)consistently.Test plan
zig build test --summary all—555/559 tests passed (4 skipped), same baseline as today'smain.scripts/gen_all_examples.sh— 8/9 bundled examples bit-identical (main.zig,game.zig,build.zig,build.zig.zon) across baseline + 2 follow-up runs.sokol_imguihas a pre-existing setup failure (missing locallabelle-imguidep) and is unaffected by this PR — it failed identically onorigin/main.Files
src/codegen/blocks/{plugin_registries,scene_manifests}.zig— inlined into Mixinsrc/codegen/lifecycle/{loop,callback}.zig— inlined into Mixinsrc/codegen/context.zig— file header updated to document mixin-only surfacesrc/main_zig.zig— re-export shim trimmedsrc/root.zig—generateGameShimdispatcheswritePluginEventsBlockvia a Codegen contexttest/{backend_wiring_tests,engine_events_discovery_test}.zigandtest/flow_scanner/{coercions_tests,flow_decls_tests}.zig— refactored to build a Codegen + dispatch through ittest/helpers.zig— newemptyCodegen(allocator)helper for the zero-state mixin tests