feat(compiler): close catalog-backed lowering - #78
Conversation
Teakowa
left a comment
There was a problem hiding this comment.
Blocking review — one-pass audit against #42 / #36. The implementation direction is sound, CI is green, and the patch preserves the ownership boundary, but this PR cannot close #42 yet.
Blockers
1. The chase evidence currently locks an unverified Workshop operand form instead of matching the pinned semantic evidence
catalog_backed_contextual_chase_calls_match_the_pinned_oracle asserts:
Chase Global Variable Over Time(Global.Round_Attack_Time, 0, 30, None);
but the pinned OverPy oracle for the same chase-condition-agentlab fixture emits:
Chase Global Variable Over Time(Round_Attack_Time, 0, 30, None);
This is not merely an output-format preference: the first parameter is the global-variable selector, while Global.Round_Attack_Time is the normal value-form spelling of a global-variable read. The current workshop-rs emitter special-cases player chase operands but passes the global operand through ordinary value emission, and its own round-trip tests currently pin that Global.g form. Structural WIR validation and canonical-id validation therefore do not prove the emitted operand role is legal/correct.
Because #42 requires no unexpected semantic divergence, do not encode the consumer-side difference as success. Resolve this at the owning boundary: verify/fix the chase variable-operand emission contract in workshop-rs, release a consumable contract, then update opy-rs and make this fixture use the same semantic oracle-equivalence path as the receiver fixture. If reproducible Workshop runtime evidence proves both spellings are legal and observably equivalent, record that evidence explicitly instead; the current repo evidence does not establish it.
2. The explicit real-world corpus acceptance criterion is not exercised by the native compiler
#42 (and parent #36) requires representative real-world corpus cases to reach deterministic Workshop emission with no unexpected divergence in the supported slice. The new opy-compiler tests only load compatibility/fixtures/synthetic/*. receiver-calls and chase-condition-agentlab have useful provenance links to real discoveries/projects, but both are intentionally classified as synthetic-original fixtures.
The Python compatibility tests in the PR verification validate fixture/oracle metadata and the diff machinery; they do not run real-world/* through opy-compiler. Therefore the green CI does not supply the real-world native-compilation evidence required to auto-close #42.
Add durable executable evidence for at least one representative provenance-linked real-world case/slice through opy-compiler (deterministic emission plus the appropriate semantic/oracle comparison). If a whole imported project is blocked by unrelated non-goal features, a minimized regression with project/revision/path provenance is appropriate; do not silently substitute a synthetic-only gate for the issue AC.
Major
3. The support-matrix note assigns OPY receiver/member surface ownership to the wrong repo
The updated semantics/receiver-members note says that the “full receiver member lists” remain Workshop-owned. That conflicts with the manifest's existing ownership contract: member functions, aliases, receiver categories, signatures, and source names are OPY source-language API metadata owned by opy-rs; workshop-rs owns the canonical Workshop target identities/catalog, content domains, localization, validation, and emission.
Please reword the matrix note so future work is not routed to the wrong owner. Keeping Workshop catalog/member target semantics lowering-dependent is correct; the OPY member list itself is not Workshop-owned.
Minor
No additional merge-blocking minor findings. Once the blockers are addressed, chase-enums would also benefit from reusing assert_matches_oracle(...) instead of several substring assertions, since the helper and pinned oracle already exist and that avoids accidentally proving only selected lines.
Verified good
#41/#52prerequisites are complete.- HIR remains Workshop-independent; catalog binding stays in
opy-compiler. - Manifest
catalogIdlinks are consumed without copying canonical Workshop tables. - Receiver/catalog gaps and unknown enum members remain explicit source-attributed failures.
memberAccessis a documented canonical WIR helper inworkshop-rs, so that path is not an ownership violation.compile_hirruns bothProgram::validateand canonical-id validation before emission.- Support-matrix states remain conservatively
lowering-dependentrather than claiming full support. - The PR is narrowly scoped to the compiler, focused evidence, and synchronized compatibility metadata.
- Current CI for
d18fc2bd3805e2061f18039d1ae640a21ec63f8cis green.
Teakowa
left a comment
There was a problem hiding this comment.
Correction to my previous blocking review: retract the chaseOverTime global-variable operand finding. The distinction I conflated was OPY source syntax vs emitted Workshop syntax. In OPY, chaseOverTime(Round_Attack_Time, ...) correctly refers to the global variable without a Global. qualifier; lowering/emission may legitimately produce Chase Global Variable Over Time(Global.Round_Attack_Time, ...) in Workshop text. That difference is not by itself a semantic divergence and should not block this PR.
The remaining blocking concern is the #42 acceptance evidence: the new native compiler tests exercise synthetic fixtures only, while #42/#36 explicitly require representative real-world corpus cases to reach deterministic Workshop emission. The support-matrix ownership wording concern also remains.
Exercise the provenance-recorded overpy-cake project through native OPY HIR lowering, canonical WIR validation, and deterministic Workshop emission. Clarify OPY versus Workshop ownership in the support matrix. Fixes #42
|
Review findings addressed in
Verification on the PR branch:
|
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up one-pass audit at c2877aed. The two substantive findings from the previous review are now addressed: the PR executes the actual provenance-recorded real-world/overpy-cake source through OPY HIR → opy-compiler → validated canonical WIR → deterministic Workshop emission, and the support-matrix receiver/member ownership wording now correctly keeps the OPY source surface in opy-rs while Workshop target semantics remain workshop-rs-owned.
Blocker
Current CI is red because the unknown-enum negative test no longer reaches the integration boundary
Both Rust quality jobs fail in unknown_catalog_enum_member_is_a_source_attributed_gap. On the current PR merge result, opy_rs::compile(...) rejects ChaseTimeReeval.NOPE first with the source-layer unknown-enum-member diagnostic, so the test panics at expect("frontend resolves") before opy-compiler is exercised.
Do not weaken the OPY semantic diagnostic to make this test pass. The source layer is correctly rejecting an OPY-known invalid enum member. If #42 still needs an integration-boundary negative test for a stale/unknown Workshop catalog member, construct or otherwise reach a valid HIR identity that the OPY layer accepts but the Workshop catalog rejects; otherwise remove this unreachable duplicate test if the existing source diagnostic plus canonical-id validation already cover the contract.
Current CI run 33154371356: formatting and Clippy pass; both stable and 1.85 Rust jobs fail only in the test step on this case. real_world_cake_exercises_catalog_lowering_end_to_end itself passes.
Major / Minor
No additional Major or Minor findings from the updated diff. The real-world test deliberately avoids whole-output oracle reparsing because the pinned cake output contains the known ambiguous Visible To spelling; for #42's bounded catalog slice, deterministic emission plus canonical WIR/catalog validation and the existing focused oracle-backed synthetic cases are sufficient here without imposing output-text identity.
Once the failing negative test is corrected and the new head is green, I do not see another objective blocker to closing #42 / #36 from this PR.
Keep the source-layer unknown-enum-member diagnostic authoritative while retaining the reachable catalog-gap integration negative test. Refs #42
|
Review 5049634081 follow-up is fixed in commit
Local verification passed:
The branch is pushed; fresh hosted checks are now running. |
|
Fresh workflow is fully green on : Rust 1.85.0, stable, macOS JS runtime, and Windows JS runtime all passed. PR merge state is . |
|
Correction: fresh workflow 33158708355 is fully green on commit 7eb81ed: Rust 1.85.0, stable, macOS JS runtime, and Windows JS runtime all passed. PR merge state is CLEAN. |
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up one-pass audit on 7eb81ed8c4e95a4fd255cc164b57cb5ca66e613e: no remaining Blocker / Major / Minor findings.
The previous CI blocker is correctly resolved by removing the unreachable integration test rather than weakening the authoritative OPY unknown-enum-member diagnostic. The reachable catalog-gap integration negative test remains.
Revalidated against #42 / #36:
- catalog-backed builtin/member/enum lowering remains bounded and canonical through
workshop-rs; - structural WIR and canonical-id validation remain in the native compiler path;
real-world/overpy-cakenow provides provenance-recorded native end-to-end evidence with deterministic Workshop emission and representative catalog-backed action/value calls;- support-matrix ownership wording correctly keeps the OPY receiver/member source surface in
opy-rsand Workshop target semantics/catalog validation inworkshop-rs; - unsupported/gap states remain explicit rather than being counted as support;
- the latest CI run is green on stable and Rust 1.85.0, with the JS runtime matrix also green.
From review evidence, PR #78 can satisfy and close #42 when merged. With #42 merged/closed, #36's current active edge is also objectively cleared; #36 should be rechecked against the merged main state before closing.
Summary
catalogIdlinksappend, and bare member access into canonical WIRworkshop-rscatalogVerification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-targets --all-featurespython3 -m unittest discover -s compatibility/testsFixes #42