refactor(opy-rs): simplify redundant compiler paths - #406
Merged
Merged
Conversation
e54-bot
marked this pull request as ready for review
September 28, 2026 07:15
Teakowa
requested changes
Sep 28, 2026
Teakowa
left a comment
Contributor
There was a problem hiding this comment.
One behavior regression remains in the public HIR validator; details inline.
Teakowa
approved these changes
Sep 28, 2026
Teakowa
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The HIR validation error precedence regression is fixed, the regression test covers both Program::validate() and parse_value(), and CI is green on the updated head.
Merged
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
This PR contains the accumulated, behavior-preserving
opy-rssimplifications accepted by the user as the completed scope of the current task. The original 30% LOC target was not reached; the measured result is stated below. Earlier work removed discarded numeric text and ignored span plumbing from lowering, reused shared HIR traversal queries, consolidated loop action envelopes, removed infallible wrappers and redundant parser token clones, split lowering by responsibility, shared provider UTF-16 position conversion, simplified settings parsing, removed private HIR validation forwarding helpers, and centralized repeated compiler, parser, and preprocessing construction paths.Recent changes consolidate global/player variable lookup and switch/conditional-jump lowering, share compiler integration-failure and initializer-order diagnostics, and unify alias resolution, annotation spans, binary-expression construction, delimited-item parsing, lexer token/identifier handling, quoted settings scanning, HIR dump formatting, and CLI diagnostic conversion. Operator folds reuse existing unary/binary helpers; compiler passes share Workshop Value child traversal; compression/decompression share alphabet and component decoding; rule-name renderers share their template value schema; compiler and lowering share directive lookup. String merging no longer keeps a name-only token forwarder, Blizzard Global spacing reads widths from its canonical glyph table rather than building a temporary duplicate, reconstruction uses the lexer identifier rule, cased progress-bar text lowering now lives with its sole action-call consumer, and declaration lowering lives in a dedicated submodule. Declaration allocation checks implicit slot collisions and gathers global/player/subroutine entries in one source-order pass. Named allocation reserves special high slots and normal allocation through one entry point; declaration plans now own their names directly instead of building and converting an intermediate borrowed list. Macro expansion now mutates the already-private HIR clone in place, retaining statement-list splicing and macro replacement while removing per-variant reconstruction. The expander borrows source macro templates and parameter names, keeping only the mutable body copy needed for each expansion. Source-to-HIR rule formatting and compiler rule/settings formatting now share one streaming filter for the same invisible characters. Global and player initializer lowering now reuse the existing generated-rule path. Rule insertion and rule/condition/action provenance registration now share one compiler path. Logical
and/orfolding now shares one branch implementation while preserving strict and non-strict operand-return behavior. Literal string extraction and numeric vector extraction now have one shared implementation reused by both optimizer passes. Size optimization now shares the exact numeric0/1to Boolean conversion across loop bounds, modification operands, indexed-variable arguments, and chase values. Rule and subroutine parsing now carry annotation/directive results in one state value instead of six mutable arguments; this also removes atoo_many_argumentsexemption and discarded subroutine-state bindings. Reverse-emitter action and value calls now share manifest/member lookup and kind diagnostics, reuse one manifest-call path, and remove the forwarding-only member-action wrapper. Settings expression evaluation now indexes HIR constants by borrowed expressions, removing one deep-copy map and a second reference map. HIR validation now visits each condition once instead of recursing through both the generic child fields and a second condition path. Cased-glyph records now construct their shared five-field shape once per character; the alphabet output was compared with the pinned OverPy oracle. Logicalorandandparsers now share their left-associative chain loop. Lexer, f-string, and settings parsing share one simple escape map; macro argument substitution reuses the lexer identifier-continuation rule. Rule, declaration, and control-flow parser headers now share colon/body-indent handling; tabular and splitDictArray now share compression component-shape and magnitude checks. Declaration source-order visibility now delegates to one shared predicate while subroutine definitions retain their separate visibility rule. HIR program traversal now has one canonical walker reused by tooling. Indexed expression decomposition now returns one canonical root and source-order indices, replacing the compiler recursive target splitter and the source lowerer duplicate walk. Synthetic texture setup HIR now uses small constructors for spanless calls, receiver calls, and statements instead of repeating the same AST fields through the whole generated rule. The macro JavaScript runtime now calls its sole QuickJS implementation directly, removing the unused single-implementationJsEnginetrait and its replaceability claim. Self-modification detection now borrows the operand list and clones only the value kept in the rewritten action. Reverse reconstruction reuses its subroutine-name set instead of scanning the source table again, and compile reports move their exact text after normalization instead of cloning it. HIR expression and statement types own both read and write span selection; macro expansion uses those mutable accessors instead of maintaining backend-specific setters. Delimited call/event argument, array, and dictionary parsing now share the separator loop while each item parser keeps its own syntax and diagnostics. Reverse reconstruction shares disabled-rule diagnostics across subroutine and normal rules, and emits global/player variable tables through one formatter. No test, fixture, snapshot, or validation files were changed.Acceptance status
At the user's direction, the accumulated result is accepted as the completed scope of this task, although the original 30% target was not reached. Using the established scope and method (Rust files under
crates, excluding tests and benches), the count moved from 41,202 to 39,151 lines: 2,051 net lines removed (4.98%). Reaching the original target would require at least 10,310 further net deletions. The final candidate sweep found no broad deletion at that scale that preserves the requested behavior and immutable evaluation boundaries; apparent large repetitions belong to separate CST/HIR schemas, distinct compiler phases, or context-specific lowering.Verification
cargo fmt --all -- --check,git diff --check,cargo test --workspace --all-targets --all-features, andcargo clippy --workspace --all-targets --all-features -- -D warningson exact source at2773225(539 passed, 1 ignored across 12 suites)129c209passed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobs0ffdfeapassed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobse80ada8was superseded before every job completed: stable Rust, Rust 1.85, and macOS passed; compiler compatibility and Windows were still in progress481be7epassed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobsc9217c0passed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobs135a633passed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobs2773225passed: stable Rust, Rust 1.85, compiler compatibility, and both JS runtime jobs9e9039d: 5,152 probes (match: 4,208;both-reject: 586;native-accepts: 334;native-rejects: 24), 0 unexplained differences; the two approved gaps matched 12 probes each. The later863cc56change is limited to parser directive-state grouping. 69 manifest functions had no valid sample call.opy-compat project-compareduring the macro in-place refactor on 9 successful macro-bearing real-world corpus projects: old and new binaries produced identical structural comparison results (4 matched the pinned oracle; 5 retained the same existing differences)cargo test -p opy-rs --lib texture(3 passed, including the pinned texture setup oracle comparison)python3 -m unittest discover -s tools/overpy/tests(37 passed on exact source at2773225)and/orplus f-string escape, escaped settings string, and macro identifier-boundary inputs all reported equivalent with no differencesReviewed candidate retained
opy-provider::Server::handle_messagestill returnsOption<Value>although current production branches all return a response. Its in-module test directly calls.expect()on the result; changing that call would modify an immutable test, so the wrapper remains.