refactor: Tidy effect opcode handling#500
Merged
Merged
Conversation
Nothing emits Clear, yet it stayed a live materializer/VM arm. Remove the EffectKind and RuntimeEffect variants and every decode/materialize/trace/format arm, plus the load-pass and compile-verify no-op cases. The opcodes after it close the gap, so the effect opcode space stays contiguous: Null becomes 9, SuppressBegin 10, SuppressEnd 11.
zharinov
force-pushed
the
refactor/drop-dead-clear-effect
branch
from
June 21, 2026 16:26
fb45948 to
ddee25f
Compare
The forged-module tests hard-coded effect opcodes as raw numbers (`op == 6`, `2u16 << 10`, "opcode 11" in comments). The renumber broke one silently: `forged_preamble_without_root_struct` wrote `10u16` meaning `Null`, but after the renumber opcode 10 is `SuppressBegin`, so it passed for the wrong reason. Reference `EffectKind` and `EFFECT_PAYLOAD_BITS`/`EFFECT_PAYLOAD_MAX` instead, with an `effect_word` helper for the raw byte construction. A renumber or reorder of the opcodes now flows through the tests for free.
Struct effects were StructOpen(4), StructClose(5), Set(6), so the insert (Set) sat after the close — inconsistent with arrays, where Push sits between ArrayOpen and ArrayClose. Swap Set and StructClose so both composites read open / insert / close: Array{Open,Push,Close} = 1/2/3, Struct{Open,Set,Close} = 4/5/6.
zharinov
changed the title
"currently 12 defined", "(0-11)", and the `SuppressBegin (10)`/`SuppressEnd (11)` numbers duplicated the opcode table just above and rotted on every renumber. The table is the source of truth.
The enum-ordering comment restated what the variant values already show, and the `effect_word` doc restated its one-line body. Remove both.
zharinov
enabled auto-merge (squash)
June 21, 2026 17:10
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
1 participant
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
Cleareffect and close the opcode gap it left, so effect opcodes stay contiguous.Array{Open, Push, Close}= 1/2/3,Struct{Open, Set, Close}= 4/5/6.EffectKind+EFFECT_PAYLOAD_BITS) so a renumber or reorder flows through for free.Why
Clearwas a live runtime arm for an effect the compiler never emits. While tidying it, the opcode numbers turned out to be scattered as raw literals across the tests — one had already broken silently on the renumber. The numbers now live only in theEffectKindenum; everything else is symbolic.Notes
Focused commits — delete
Clear+ close the gap, make the forged tests symbolic, the struct open/insert/close swap, then a docs tidy. The swap stays tiny because the tests are symbolic by then.