feat(compiler): add Workshop backend slices - #82
Conversation
Teakowa
left a comment
There was a problem hiding this comment.
One-pass audit against #37 and the current repository state. Verdict: changes required; do not merge as-is.
Blockers
-
The branch is not review/merge-clean against current
main. PR #78/#42 was squash-merged as1a434e4, but this branch still carries the pre-squash #78 commits (d18fc2b,c2877ae,7eb81ed) and then addsc5d24ce. GitHub reports the branch as diverged and the PR as non-mergeable, and the diff repeats already-merged #42 work. Rebase/cherry-pick the #37-specific work onto currentmainso the PR contains only its intended delta and does not risk dropping intervening mainline changes. -
Fixes #37is not satisfied by the implementation shown here. #37 is the authoritative backend issue forsemantics/settings-emission,preprocessing/directive-effects,hooks/post-compile-workshop,translations/locale-emission, andoptimization/backend-effects. Its scope explicitly requires evidence-backed observable optimization/replacement/directive effects, rule-prefix/presentation handling at the backend boundary, and minimized integration fixtures for settings, locale, backend directives, and hooks. This compiler path does not consumehir.preprocessing.optimizationorreplacements, and the docs still leave observable optimization/replacement effects as Coming soon. Either complete the approved #37 scope or have the owning planning decision revise the issue/closure contract; this PR must not close #37 as-is.The dependency gate is also unresolved: #37 requires #36 complete. #36 is still open and still records #42 as its active edge even though #42 has since closed. Revalidate/close that dependency (or explicitly revise #37) before merging a PR that closes #37.
Major
-
Post-compile-hook diagnostics are flattened and lose the structured script provenance the runtime already provides.
MacroError::Scriptcarriessource_name, line, column, and adjusted stack information, butcompile_sourceconverts every hook failure througherror.to_string()and retains only the OPY directive span. That contradicts this PR's “preserve hook diagnostics” claim and #37's structured/actionable provenance AC. Preserve the hook runtime's structured provenance instead of discarding it; the directive span can remain the OPY-side anchor/related location. -
The unsupported-locale diagnostic fabricates OPY source provenance.
compile_hir_with_localereceiveslocaleas an external compiler argument, but on an undeclared locale it attaches the span ofhir.preprocessing.translations. The matrix itself describes this feature as the--languageoutput selection contract; a#!translationsdeclaration is not the source of an invalid external locale argument. Unless locale selection is actually derived from that directive, this error should not point at it. Keep the span absent at this layer (or attach option provenance at the caller that owns the option) rather than blaming unrelated OPY source. -
The support-state upgrades outrun the executable evidence currently recorded.
end-to-end-supportedis defined as explicit OPY → HIR → WIR → validated Workshop emission evidence for the declared scope, buttranslations/locale-emissionis advanced using only upstream data plus the existing en-US oracle invocation; this PR adds a negative undeclared-locale unit test, not a positive non-en-US localized emission probe. The settings test only checks a few substrings even though a pinned settings oracle already exists, and the hook row's evidence list does not reference the new compiler-level final-Workshop integration. In addition, the matrix snapshot still says#!postCompileHookexecution is lowering-dependent while the row now says end-to-end-supported. Keep claims at the lower state until the affected executable evidence exists, or add the focused evidence required by #37 and update the snapshot/provenance consistently.
Other audit areas
The ownership direction is otherwise correct: OPY/backend behavior stays in opy-rs, while settings/catalog/localization/WIR/emission remain delegated to released workshop-rs; I found no reverse dependency or copied canonical Workshop data in the #37-specific work. Hook execution is correctly placed after successful Workshop emission and its completion string is separated from the pre-hook emitted artifact.
No CI/workflow runs are currently attached to head c5d24ce, so the verification commands in the PR body are author-declared evidence only at this point. Re-run the repository gates after the branch is repaired.
c5d24ce to
007d877
Compare
|
Addressed in 007d877.
Local verification rerun: fmt, clippy with |
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up one-pass audit on 007d877. The previous branch-history/#36 dependency/hook-diagnostic/locale-provenance findings are resolved, the PR is mergeable again, and the current CI run is green. Verdict: changes still required; do not merge this head as-is.
Blockers
-
Pure optimizer controls are now turned into hard compilation failures without the evidence #37 requires.
reject_unlowered_directives()rejects every non-defaultOptimizationState, so a valid OPY project using e.g.#!disableOptimizationsno longer reaches canonical Workshop emission. That conflicts directly with #37's scope: unsupported optimizer/output-form differences are to remain documented non-blocking presentation differences unless observable semantics require them. The current patch has not established such an observable-semantic requirement; it only establishes that there is no canonical optimizer lowering yet. Do not make unsupported optimizer shape/output-form controls correctness blockers by default. Keep those controls non-blocking unless corpus/oracle evidence demonstrates a semantic requirement, while separately failing explicit semantic replacement directives where silently ignoring them would change behavior. -
The fail-explicit guard only checks the final aggregate preprocessing state, so scoped/include-local backend effects can still be silently ignored. Opy HIR intentionally retains
DirectiveRecordentries withscope_depthand aPreprocessingSnapshotbecause block-scoped state transitions must remain inspectable.reject_unlowered_directives()checks onlypreprocessing.replacements.first()and the finalpreprocessing.optimization. A replacement/optimizer state introduced inside an include or lexical scope and restored before the end can therefore disappear from those final fields while remaining inpreprocessing.directives; compilation then proceeds without applying or rejecting that backend effect. That contradicts this PR's stated contract (“reject ... instead of silently ignoring”) and #37's no-silent-fallback AC. For effects that are semantically relevant enough to reject, evaluate the ordered/scoped directive records rather than only the final snapshot. For presentation-only optimizer controls, apply finding 1 instead of rejecting them.
Major
- The settings Oracle assertion can hide semantic string changes.
normalize_workshop_whitespace()removes all whitespace characters before comparing the emitted settings section with the pinned Oracle. That also removes whitespace inside quoted user values, so"a b"and"ab"compare equal. Since this test is being used as the executable basis for advancingsemantics/settings-emissiontoend-to-end-supported, normalization must preserve string contents. Normalize only structural formatting outside literals, or compare a settings-aware semantic representation that preserves values.
Minor
- The support-matrix snapshot provenance is internally stale. The snapshot note now claims the #37 integration slice, while
snapshot.dateremains2026-08-24andasOfCommitremainsc5714a4..., which predates this PR. Either advance the snapshot provenance to the state actually described by the note or keep the frozen snapshot note limited to what that commit/date represents.
Resolved from the previous review
- Branch is rebased/clean enough for GitHub to report
mergeable: true. - #37 is now correctly treated as an open parent/partial delivery (
Refs #37), not closed by this PR. - #36 was revalidated and closed.
- Post-compile hook errors retain structured script provenance in addition to the OPY directive anchor.
- Unsupported external locales no longer fabricate a
#!translationssource span. - Positive non-English locale and compiler-level hook/settings coverage were added.
- Ownership remains correct: OPY/backend-specific behavior stays in
opy-rs; canonical settings/catalog/localization/WIR/emission stay delegated toworkshop-rs.
After the two directive-handling blockers are corrected, re-run the existing repository gates; no additional broad test expansion is needed beyond fixing the settings assertion and adding a focused scoped-directive regression if that failure mode is not already covered.
007d877 to
9f7c62b
Compare
|
Addressed review 5051072214 in 9f7c62b.
Reran fmt, Clippy with |
Teakowa
left a comment
There was a problem hiding this comment.
Follow-up audit on 9f7c62b.
Verdict: changes still required. The previous optimizer/scoped-state/settings-comparison findings are substantially fixed, and the branch/CI/ownership picture is now clean, but one compiler-boundary correctness hole remains and the support artifact is internally contradictory.
Blocker
B1 — compile_hir can still silently ignore an active replacement state when directive history is absent
reject_unlowered_directives now only scans hir.preprocessing.directives for names beginning with replace. This correctly catches scoped replacement directives whose final state was later restored, but the previous active-state guard was removed entirely.
PreprocessingState exposes replacements and directives as separate serde-defaulted fields. directives is provenance/history; it is not a structural prerequisite for a non-empty replacements state, and HIR validation does not establish an invariant that the two must agree. Therefore a deserialized/constructed hir::Program accepted by the public compile_hir boundary can contain a non-empty active preprocessing.replacements with an empty/missing directives list. This path now reaches lowering and emits Workshop while silently dropping a semantic replacement that this PR explicitly says must fail rather than be approximated.
Please reject the union of (a) active replacement state and (b) historical replacement directive records. The history check is needed for restored scoped state; it should supplement, not replace, the active-state check. A boundary regression test with replacements != [] and directives == [] would protect the actual failure mode.
Major
M1 — compatibility/support-matrix.json still describes the pre-#37 state at the snapshot level
The feature rows in this PR promote settings, locale emission, and post-compile-hook execution to end-to-end-supported, but the same file's snapshot is still dated 2026-08-24, points at c5714a4028464f19f236f54d390e7b7237e434bd, and its note still states that #!postCompileHook execution against final Workshop text is lowering-dependent (#8).
That is now a direct contradiction inside the authoritative support artifact, not just stale cosmetic metadata. Update the snapshot provenance/note to the state this PR is actually claiming (or regenerate it through the repository's intended path) so consumers do not get two incompatible answers from one matrix.
Previous findings / audit notes
- Resolved: optimizer-only controls are non-blocking again, matching #37's explicit compatibility boundary.
- Resolved: replacement directive history is now checked, so scoped/restored replacement effects are not silently lost on the canonical source path.
- Resolved: the settings Oracle comparison no longer erases whitespace inside string literals; the regression assertion distinguishes
"a b"from"ab". - Resolved: #36 is closed; #82 correctly remains a bounded partial delivery and uses
Refs #37rather than closing #37. #!rulePrefix/#!rulePrefixTemplateare applied in OPY lowering before Workshop compilation; no duplicate Workshop semantics were introduced here.- Settings/catalog/locale/WIR/emission ownership remains delegated to
workshop-rs; no reverse dependency or copied canonical Workshop table was found. - Post-compile hooks still execute only after successful canonical Workshop emission and retain both directive and script-runtime provenance.
- Current head is mergeable and CI run #162 is green.
I did not find additional objective findings in the changed docs, dependency changes, support-matrix mirror, public hook record change, or CI surface.
9f7c62b to
98e14af
Compare
|
Addressed review 5051776859 in the updated head.
Reran fmt, Clippy with |
Teakowa
left a comment
There was a problem hiding this comment.
Final follow-up audit on 98e14af against #37 and the current repository state.
Verdict: no blocking code findings on this head. The previously reported compiler-boundary and support-artifact issues are resolved.
Verified
- Active replacement state and historical/scoped replacement records are both rejected, so
compile_hirno longer has thereplacements != [] && directives == []silent-fallback hole. The focused boundary regression covers that inverse case. - Optimizer-only controls remain non-blocking presentation differences, matching #37's explicit compatibility boundary.
- The support-matrix snapshot no longer contradicts the promoted settings/locale/hook rows; the bounded #37 slice and remaining optimizer/replacement gaps are described consistently.
- Settings still delegate validation/emission to
workshop-rs, and the Oracle comparison preserves quoted-string contents. - Locale selection remains catalog-driven with explicit unsupported-locale behavior.
#!postCompileHookstill runs only after successful canonical Workshop emission, receives the emitted text, and preserves directive plus script-runtime provenance on failure.#!rulePrefix/#!rulePrefixTemplatestay OPY-owned and are applied before Workshop lowering; no Workshop semantic tables were duplicated inopy-rs.- #82 remains a bounded partial delivery and correctly uses
Refs #37; #37 is still open and should remain open for its remaining approved scope. - The PR is currently mergeable and the diff remains scoped to the compiler/support/docs slice.
CI
CI run #163 is still in progress at review time. Rust 1.85 quality, Rust stable quality, and macOS JS-runtime jobs are green; only the Windows JS-runtime job is still running. Merge should wait for that final hosted gate to complete successfully.
I found no additional Blocker/Major/Minor findings in the changed dependencies, public hook record, compiler API, support-matrix mirror, or documentation surface.
Summary
mainScope
This is a bounded partial delivery for #37. It deliberately does not claim optimizer/replacement transformation parity, so it references rather than closes #37.
Verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-targets --all-featurespython3 -m unittest discover -s compatibility/testsRefs #37