fix(compiler): apply OverPy's per-parameter False/True substitutions under optimizeForSize - #398
Conversation
…under optimizeForSize Replace the hand-written literal special cases with a table of OverPy 9.7.10's canReplace0ByFalse, canReplace1ByTrue, canReplace0ByNull and canReplaceNullVectorByNull flags, generated by tools/overpy/gen_literal_flags.cjs and applied to every call argument, array element and vector component. Replacement policy stays in opy-rs and no longer reads the catalog's broader acceptance coercions. Stop normalizing authored literals while lowering (0 to Null, False to 0, empty array to empty string), which only converged because the workshop-rs parser did the same. Unoptimized output now keeps the authored spelling, as the pinned reference does. Adopt workshop-rs 0.8.0, which preserves contextual literals. Fixes #388
Teakowa
left a comment
There was a problem hiding this comment.
Three blocking findings:
-
crates/opy-rs/src/compiler/size_optimization/literal_flags.rs/tools/overpy/gen_literal_flags.cjs: the generated upstreamcanReplace*table is interpreted directly by generic Rust to choose observable OverPy lowering. That conflicts with the currentlanguage-core.md/AGENTS.mdcontract: source-language behavior and coercion/lowering policy belong in discoverable typed Rust; generated/declarative data must not become a semantic control layer, and upstream implementation data should not be mechanically imported into the core. Encode this policy directly in opy-rs typed code and use the upstream table/probe for completeness verification, or obtain an explicit owner architecture decision changing that contract. -
Required verification is missing.
AGENTS.mdsays lowering/optimization/builtin-argument changes must runtools/overpy/probe_builtins.py. The PR verification does not report it, and the current CI run does not execute that probe. Run the full builtin-call probe and resolve any unexplained differences. -
Issue #388 explicitly requires Wright's canonical diff for Bastion
main.opyandexternalMain.opy; the PR says it was not run. The opy-rs structural gate is green for those same pinned entries, but review should not silently substitute a different check for an explicit AC. Run the Wright check, or have the owner update the AC if the structural gate is now the intended replacement.
Address review: the generated canReplace table drove lowering policy directly. Write the per-parameter policy as typed Rust in literal_slots.rs and keep the upstream flags only as verification data (tools/overpy/upstream-literal-flags.json), checked by slots_cover_the_upstream_flags in both directions. Verified with tools/overpy/probe_builtins.py (unexplained: 0) and the Bastion structural gate (0 differences).
Teakowa
left a comment
There was a problem hiding this comment.
Findings 1 and 2 are resolved. The generated upstream flags are now verification data only, while the policy is encoded in typed Rust, and the follow-up commit records the builtin probe with 0 unexplained differences.
One blocker remains: Issue #388 still explicitly requires Wright's canonical diff for Bastion main.opy and externalMain.opy, while the PR still says that check was not run. Run that acceptance check, or update the Issue contract if the opy-rs structural gate is now the intended replacement. No new code-level findings in the follow-up patch.
|
Thanks. All three findings are addressed in 5eb8b5c and the PR description.
Requesting re-review. |
Teakowa
left a comment
There was a problem hiding this comment.
LGTM. The three prior review findings are resolved, the Wright acceptance check is now documented for both Bastion entries, and the current CI run is fully green.
Fixes #388
Summary
workshop-rs0.8.0, which stops normalizing contextual literals (fix(parser)!: preserve contextual literal substitutions workshop-rs#300).#!optimizeForSize, spell0/1/Null/zero vectors as OverPy 9.7.10 does per parameter, from a table generated bytools/overpy/gen_literal_flags.cjs(OverPy'scanReplace0ByFalse,canReplace1ByTrue,canReplace0ByNull,canReplaceNullVectorByNull). Policy stays in opy-rs and no longer reads catalogparamCoercions.0→Null,False→0, empty array→"") that only converged because the old parser did the same. Unoptimized output keeps the authored spelling, as the pinned reference does (checked by compiling probes with pinned OverPy).Verification
cargo test --workspace,cargo clippy --workspace --all-targets,cargo fmt --check,tools/overpy/tests: pass.tools/overpy/probe_builtins.py: 4064 probes, unexplained: 0 (two approved gaps unchanged).tools/overpy/structural_gate.py: Bastionmain.opyandexternalMain.opyreport 0 differences against pinned OverPy.mainworktree with onlyworkshop-rsbumped to=0.8.0(itsmainpins 0.7.1, which predates fix(parser)!: preserve contextual literal substitutions workshop-rs#300), compiling Bastion (f693704) through this branch'sopy-provider, compared withworkshop_rs::roundtrip::equivalentagainst pinned OverPy 9.7.10 output.main.opy(zh-CN): equivalent.externalMain.opy(en-US): equivalent, also viawright semantic-compare.wright semantic-compareonly parses en-US, so the zh-CN comparison usedworkshop-rsdirectly with the zh-CN locale.