feat: expose canonical action layout - #103
Merged
Merged
Conversation
Use the emitter's recursive action expansion as the single source of truth for native action widths, with explicit WIR/emission errors and coverage for nested structured actions. Fixes #102
Teakowa
commented
Aug 24, 2026
Teakowa
left a comment
Contributor
Author
There was a problem hiding this comment.
Review verdict: merge-ready
I reviewed the complete PR #103 diff against the narrowed scope and acceptance criteria of #102. I found no correctness, ownership, architecture, validation, or regression blocker.
Verified
action_width()does not maintain a second manually synchronized width table. It invokes the same recursiveEmitter::action()implementation used by canonical emission and counts at the sharedline()sink.- Nested
If/Else,While,ForGlobalVariable, andForPlayerVariableexpansion is therefore derived from the same implementation that emits native Workshop structure. - The API remains source-language-neutral. No
Switch,Case,Default,DoWhile, label, goto, CFG, fallthrough, or source-language break semantics were added to WIR/workshop-rs. - Invalid programs fail explicitly through
InvalidWIR; emission/layout failures remain explicit throughEmissionrather than producing guessed/partial widths. - The non-rule-final context is explicitly documented. This matches the relative-
Skipuse required by opy-rs#61 and preserves the existing rule-finalIfemission optimization instead of changing emitter semantics. - Tests cover leaf/native width, nested structured expansion, emitter/layout agreement, and explicit invalid-WIR failure.
- The implementation is small and does not redesign WIR or introduce a generic target/relocation system.
- CI run #220 is green on the reviewed head, including Rust 1.85/stable quality gates, catalog checks, and the five real-project scenarios.
Minor / metadata
docs/action-layout.mdis a durable public contract but is not linked from the canonicaldocs/README.mdindex. Add it under Architecture/ownership or a small compiler-consumer-contract section so the new public contract is discoverable.- The PR body is empty. The commit contains
Fixes #102, but PR-level scope/closure should still explicitly link#102(for exampleCloses #102) so the PR remains self-describing independent of merge strategy.
Neither item changes the implementation verdict. After those metadata/documentation cleanups, this is ready to merge and release; opy-rs#61 can then replace its local normalized_action_width() with the released workshop-rs API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
workshop_rs::emitter::action_widthfor canonical WIR action sequenceslinepathIf/Else,While,ForGlobalVariable, andForPlayerVariablelayoutsdocs/README.mdScope
This PR does not add source-language control-flow semantics,
Switch/Case/Default/DoWhile, labels, goto, CFG, fallthrough, break policy, or a relocation system. It does not modifyopy-rs#61.Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --locked -- -D warningscargo run --locked -p workshop-rs --bin workshop-catalog-gen -- checkcargo test --workspace --all-targets --locked(249 passed, 24 suites)Downstream contract
After this PR is merged and a release containing it is published,
opy-rs#61can replace its localnormalized_action_width()model withworkshop_rs::emitter::action_width(&program, &catalog, &locale, actions).map(|layout| layout.width), while retaining all OPY-specific switch/do-while semantics and diagnostics locally.Closes #102