refactor!: simplify runtime and normalize naming#148
Conversation
Simplify interpreter state and helper ownership, preserve CFG and type-pool invariants, and apply consistent package-wide symbol names. BREAKING CHANGE: rename optimizer, pass, analysis, transform, function-builder, heap-limit, slot, and register APIs as documented in docs/symbol-naming-audit.md.
📝 WalkthroughWalkthroughThe pull request applies a broad naming audit across public APIs and internals, updates optimizer and builder usage, strengthens verifier type tracking, refactors interpreter/JIT state handling, and updates documentation and tests. ChangesAPI and optimization pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 29.54% 29.57% +0.02%
==========================================
Files 86 86
Lines 60158 60181 +23
==========================================
+ Hits 17775 17798 +23
- Misses 41116 41117 +1
+ Partials 1267 1266 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
interp/jit_plan.go (1)
108-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
inputas a function name shadows the conventional*compileInputparameter name.
staticPlan(input *compileInput)(Line 198) and likely other functions in this file take a*compileInputparameter also namedinput, which now shadows the package-levelinput()constructor within those scopes. It compiles fine today, but it's confusing to read and would silently break if any of those functions ever needed to call the constructor.♻️ Suggested rename to avoid the collision
-func input(i *Interpreter, addr int) (*compileInput, bool) { +func newInput(i *Interpreter, addr int) (*compileInput, bool) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/jit_plan.go` around lines 108 - 123, Rename the package-level input constructor to a distinct name and update every call site, including staticPlan and any other references, so it no longer collides with *compileInput parameters named input.interp/jit_arm64.go (1)
870-891: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRef-kind
localLoadedclear happens beforeloadLocal's success is known.
f.state[idx] &^= localLoadedruns unconditionally forKindReflocals beforel.loadLocal(...)is called and checked. TodayloadLocalcannot fail forKindRef(its only internal failure branch is gated onKindI64), so this is currently safe, but it violates the stated invariant that failed lowering must not mutate frame facts, and would silently break if a Ref-specific failure path is ever added toloadLocal.🛡️ Defensive reordering
func (l arm64Lowerer) localGet(ctx *lowering, op step) bool { f := ctx.frame() idx := int(op.args[0]) if idx >= len(f.kinds) { return false } - if f.kinds[idx] == types.KindRef { - f.state[idx] &^= localLoaded - } if !l.loadLocal(ctx, f, idx, op.ip) { return false } + if f.kinds[idx] == types.KindRef { + f.state[idx] &^= localLoaded + }Based on learnings, this is not tied to a specific retrieved-learning note; the concern is grounded directly in the
interp/**/*.gocoding guideline on JIT handler mutation-on-failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/jit_arm64.go` around lines 870 - 891, Move the Ref-local f.state[idx] &^= localLoaded mutation in arm64Lowerer.localGet to occur only after l.loadLocal succeeds. Preserve the existing early return on loadLocal failure so failed lowering leaves frame facts unchanged, while retaining the current reference-retention and push behavior on success.Source: Coding guidelines
analysis/gvn.go (1)
109-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlace
newNumberingafter its caller.
RuncallsnewNumbering, butnewNumberingis declared earlier in the file. Move the helper belowRunso callers precede callees.As per coding guidelines, callers must be declared before callees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@analysis/gvn.go` at line 109, Move the newNumbering function declaration below Run in analysis/gvn.go, leaving its implementation unchanged so the caller appears before the callee.Source: Coding guidelines
transform/cd_test.go (1)
66-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise type deduplication and remapping here.
This case omits
MAP_NEWand uses unique type indices, so it would pass even if the new scan or rewrite logic failed to remap operands. Add duplicate ref/map types and assert the compacted type table plus rewritten indices.Suggested coverage update
{ name: "all type operands", program: program.New( []instr.Instruction{ instr.New(instr.REF_TEST, 0), - instr.New(instr.REF_CAST, 1), - instr.New(instr.MAP_NEW_DEFAULT, 2), + instr.New(instr.REF_CAST, 0), + instr.New(instr.MAP_NEW, 1), + instr.New(instr.MAP_NEW_DEFAULT, 1), }, program.WithTypes( - types.TypeString, types.TypeError, types.NewMapType(types.TypeI32, types.TypeI32), + types.TypeString, + types.NewMapType(types.TypeI32, types.TypeI32), ), ), expected: program.New( []instr.Instruction{ instr.New(instr.REF_TEST, 0), - instr.New(instr.REF_CAST, 1), - instr.New(instr.MAP_NEW_DEFAULT, 2), + instr.New(instr.REF_CAST, 0), + instr.New(instr.MAP_NEW, 1), + instr.New(instr.MAP_NEW_DEFAULT, 1), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transform/cd_test.go` around lines 66 - 84, Update the “all type operands” case in the transform tests to include duplicate reference and map types, including MAP_NEW, so deduplication and operand remapping are exercised. Assert the expected compacted type table and rewritten indices for REF_TEST, REF_CAST, MAP_NEW_DEFAULT, and MAP_NEW rather than retaining unique indices.asm/regalloc.go (1)
109-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or wire the unused
blockhelper.
golangci-lintreports(*allocator).blockas unused at Line 109. Unless a generated or build-tagged caller is intentionally omitted, delete this dead helper or add the missing call path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asm/regalloc.go` around lines 109 - 113, Remove the unused allocator.block method unless a valid generated or build-tagged caller is required; otherwise wire it into the intended register-blocking path. Verify references to (*allocator).block and preserve the existing blocked and avail updates wherever that behavior is needed.Source: Linters/SAST tools
program/verify.go (1)
81-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
newCheckeris defined before its callerVerify.Elsewhere in this file callers consistently precede their callees (
step→call,merge→unify,step→acceptsType), but herenewChecker(81) is defined aboveVerify(93), which calls it. As per coding guidelines,**/*.gofiles should place "callers before callees."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@program/verify.go` around lines 81 - 108, Reorder the declarations in program/verify.go so Verify appears before newChecker, keeping both implementations and behavior unchanged. Verify should remain the caller and newChecker its following callee, matching the file’s callers-before-callees convention.Source: Coding guidelines
interp/interp.go (1)
1790-1821: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
globalKinds()interp/interp.go:1790-1807can be dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@interp/interp.go` around lines 1790 - 1821, Remove the unused Interpreter.globalKinds method from interp/interp.go. Leave globalReprs and all other global-kind handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@analysis/blocks.go`:
- Around line 137-139: Move the BlocksAnalysis.Run method so it is declared
before the Blocks function, preserving its existing implementation and behavior
while satisfying the repository’s caller-before-callee declaration order.
In `@interp/jit.go`:
- Line 451: Update the frame snapshot logic around frames[i].state so it
preserves every localState value from f.state, including localLoaded and
localDirty bits, instead of leaving the newly allocated entries zeroed. Keep the
independent snapshot allocation while copying the source state contents into it
before later lowering uses the snapshot.
In `@program/verify.go`:
- Around line 458-470: Update the LOCAL_SET and LOCAL_TEE handling in the
verifier to validate the stack top with acceptsType against
c.locals[inst.Operand(0)] after the underflow check, matching GLOBAL_SET and
GLOBAL_TEE. Return ErrTypeMismatch on failure while preserving the existing
success and underflow behavior.
---
Nitpick comments:
In `@analysis/gvn.go`:
- Line 109: Move the newNumbering function declaration below Run in
analysis/gvn.go, leaving its implementation unchanged so the caller appears
before the callee.
In `@asm/regalloc.go`:
- Around line 109-113: Remove the unused allocator.block method unless a valid
generated or build-tagged caller is required; otherwise wire it into the
intended register-blocking path. Verify references to (*allocator).block and
preserve the existing blocked and avail updates wherever that behavior is
needed.
In `@interp/interp.go`:
- Around line 1790-1821: Remove the unused Interpreter.globalKinds method from
interp/interp.go. Leave globalReprs and all other global-kind handling
unchanged.
In `@interp/jit_arm64.go`:
- Around line 870-891: Move the Ref-local f.state[idx] &^= localLoaded mutation
in arm64Lowerer.localGet to occur only after l.loadLocal succeeds. Preserve the
existing early return on loadLocal failure so failed lowering leaves frame facts
unchanged, while retaining the current reference-retention and push behavior on
success.
In `@interp/jit_plan.go`:
- Around line 108-123: Rename the package-level input constructor to a distinct
name and update every call site, including staticPlan and any other references,
so it no longer collides with *compileInput parameters named input.
In `@program/verify.go`:
- Around line 81-108: Reorder the declarations in program/verify.go so Verify
appears before newChecker, keeping both implementations and behavior unchanged.
Verify should remain the caller and newChecker its following callee, matching
the file’s callers-before-callees convention.
In `@transform/cd_test.go`:
- Around line 66-84: Update the “all type operands” case in the transform tests
to include duplicate reference and map types, including MAP_NEW, so
deduplication and operand remapping are exercised. Assert the expected compacted
type table and rewritten indices for REF_TEST, REF_CAST, MAP_NEW_DEFAULT, and
MAP_NEW rather than retaining unique indices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cadb540f-3d11-4b80-8890-659fb1d85a8d
📒 Files selected for processing (68)
AGENTS.mdREADME.ko.mdREADME.mdanalysis/blocks.goanalysis/blocks_test.goanalysis/gvn.goanalysis/gvn_test.goasm/assembler.goasm/link.goasm/reg.goasm/reg_test.goasm/regalloc.goasm/rewriter.gobenchmarks/call.gocli/repl.gocli/repl_test.godebug/debugger.godocs/README.mddocs/architecture.mddocs/coding-patterns.mddocs/host-integration.mddocs/jit-internals.mddocs/memory-model.mddocs/pass-system.mddocs/roadmap.mddocs/symbol-naming-audit.mddocs/testing.mddocs/verification.mdinterp/host.gointerp/interp.gointerp/interp_test.gointerp/jit.gointerp/jit_arm64.gointerp/jit_arm64_test.gointerp/jit_plan.gointerp/marshal.gointerp/pool.gointerp/pool_test.gointerp/trace.gooptimize/example_test.gooptimize/fuzz_test.gooptimize/optimizer.gooptimize/optimizer_test.gopass/pipeline.gopass/pipeline_test.goprof/collector.goprof/jit.goprof/jit_metrics.goprogram/builder.goprogram/parse.goprogram/parse_test.goprogram/verify.goprogram/verify_test.gotransform/as.gotransform/as_test.gotransform/cd.gotransform/cd_test.gotransform/cf.gotransform/cf_test.gotransform/dce.gotransform/dce_test.gotransform/gvn.gotransform/gvn_test.gotypes/function.gotypes/function_test.gotypes/parse_test.gotypes/struct.gotypes/struct_test.go
| func (p *BlocksAnalysis) Run(m *pass.Manager, fn *types.Function) ([]*BasicBlock, error) { | ||
| return Blocks(fn) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move BlocksAnalysis.Run before Blocks.
Run calls Blocks, but it is declared afterward. Move this wrapper above Blocks to satisfy the repository rule that callers precede callees.
Suggested reorder
+func (p *BlocksAnalysis) Run(m *pass.Manager, fn *types.Function) ([]*BasicBlock, error) {
+ return Blocks(fn)
+}
+
func Blocks(fn *types.Function) ([]*BasicBlock, error) {
...
}
-
-func (p *BlocksAnalysis) Run(m *pass.Manager, fn *types.Function) ([]*BasicBlock, error) {
- return Blocks(fn)
-}As per coding guidelines, declaration order should place callers before callees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@analysis/blocks.go` around lines 137 - 139, Move the BlocksAnalysis.Run
method so it is declared before the Blocks function, preserving its existing
implementation and behavior while satisfying the repository’s
caller-before-callee declaration order.
Source: Coding guidelines
| frames[i].locals = make([]value, len(f.locals)) | ||
| frames[i].loaded = make([]bool, len(f.loaded)) | ||
| frames[i].dirty = make([]bool, len(f.dirty)) | ||
| frames[i].state = make([]localState, len(f.state)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Copy local-state bits into snapshots.
frames[i] = f copies only the slice header; this make replaces it with zeroed entries and discards every localLoaded/localDirty bit from f.state. Later lowering can therefore omit required local reload/flush handling and diverge from threaded execution.
Proposed fix
- frames[i].state = make([]localState, len(f.state))
+ frames[i].state = make([]localState, len(f.state))
+ copy(frames[i].state, f.state)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| frames[i].state = make([]localState, len(f.state)) | |
| frames[i].state = make([]localState, len(f.state)) | |
| copy(frames[i].state, f.state) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@interp/jit.go` at line 451, Update the frame snapshot logic around
frames[i].state so it preserves every localState value from f.state, including
localLoaded and localDirty bits, instead of leaving the newly allocated entries
zeroed. Keep the independent snapshot allocation while copying the source state
contents into it before later lowering uses the snapshot.
| case instr.LOCAL_TEE: | ||
| if st.len() == 0 { | ||
| return false, c.fail(ip, op, ErrStackUnderflow) | ||
| } | ||
| return false, nil | ||
| case instr.GLOBAL_TEE: | ||
| if st.len() == 0 { | ||
| return false, c.fail(ip, op, ErrStackUnderflow) | ||
| } | ||
| if !acceptsType(st.top(), c.prog.Globals[inst.Operand(0)]) { | ||
| return false, c.fail(ip, op, ErrTypeMismatch) | ||
| } | ||
| return false, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether locals get any equivalent runtime type-cast safety net
# (mirroring SetGlobal's Cast check), and whether LOCAL_SET/LOCAL_TEE were
# already exempt from type checking before this diff.
rg -nP -C3 '\bLOCAL_SET\b|\bLOCAL_TEE\b' program/verify.go interp/interp.go
rg -nP -C3 'func.*SetLocal' interp/interp.goRepository: siyul-park/minivm
Length of output: 1249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the verifier around local/global get/set/tee handling.
sed -n '180,230p' program/verify.go
echo '---'
sed -n '450,500p' program/verify.go
echo '---'
sed -n '680,705p' program/verify.go
# Inspect the interpreter's local write path for any runtime type enforcement.
echo '--- interp ---'
sed -n '470,540p' interp/interp.goRepository: siyul-park/minivm
Length of output: 6175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect verifier call/type resolution and the interpreter's corresponding call path.
sed -n '560,620p' program/verify.go
echo '---'
sed -n '620,700p' program/verify.go
echo '--- interp call ---'
rg -n -C4 '\bCALL\b|func .*Call\(' interp/interp.go program/verify.goRepository: siyul-park/minivm
Length of output: 7696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect local slot typing/boxing and any verifier rules that constrain locals.
rg -n -C3 '\btypes\.Boxed\b|\bKindRef\b|\bLOCAL_SET\b|\bLOCAL_TEE\b|\bLocal\(' types interp program
echo '--- types ---'
sed -n '1,260p' types/*.goRepository: siyul-park/minivm
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect LOCAL_SET and the declared stack effects for local opcodes.
sed -n '230,280p' program/verify.go
echo '---'
sed -n '500,560p' program/verify.go
echo '--- instr ---'
rg -n -C2 '\bLOCAL_SET\b|\bLOCAL_TEE\b' instrRepository: siyul-park/minivm
Length of output: 3548
LOCAL_SET and LOCAL_TEE still skip the declared local type check.
LOCAL_GET now attaches c.locals[...] to the stack slot, but both local write paths only enforce underflow/kind. That lets a typed local hold an incompatible value and later be consumed as if it matched the declaration, which is the same unsoundness GLOBAL_SET/GLOBAL_TEE now avoid. Add the same acceptsType(..., c.locals[inst.Operand(0)]) check to both local writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@program/verify.go` around lines 458 - 470, Update the LOCAL_SET and LOCAL_TEE
handling in the verifier to validate the stack top with acceptsType against
c.locals[inst.Operand(0)] after the underflow check, matching GLOBAL_SET and
GLOBAL_TEE. Return ErrTypeMismatch on failure while preserving the existing
success and underflow behavior.
Changes Made
RETURN_CALLas a terminal CFG instructionStructType.CastBlocksAnalysis,GVNAnalysis,GVN,DCEPass,GVNPass,FoldPass, andDedupPassoptimize.New,Optimizer.Add, andPipeline.AddFunctionBuilder.Params/Returns/Locals/Captures,Function.Slots, andWithHeapLimitNewMap,NewMapWithCapacity,NewTypedMap, andNewMapForTypepublic and semantically distinct.Related Issues
None.
Additional Information
This PR intentionally contains breaking API renames because the public API has not reached a stable release. No deprecated compatibility aliases were added.
Verification:
make check(cd benchmarks && go test -race ./...)(cd benchmarks && go vet ./...)git diff --checkmake benchmark-prAll commands completed successfully on Darwin/ARM64 (Apple M4 Pro). The benchmark gate was used as a regression smoke test; this PR makes no controlled performance-improvement claim.
Summary by CodeRabbit
New Features
RETURN_CALLin control-flow analysis.API Updates
WithHeapLimit.Verifyto accept only a program.Documentation