refactor: simplification implement - #168

internal/cmd/geninterp/generate_test.go (1)

16-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Place the scenarios under t.Run sub-cases.

TestGenerator runs five distinct scenarios in one linear body: build, first generation, clean -check, stale file, and missing file. A failure in an early scenario aborts the rest, and the failure output does not name the scenario. Wrap each scenario in t.Run while keeping them sequential so the shared temporary directory state stays valid.

Based on the coding guideline "Use one top-level test per public symbol (Test<Func> or Test<Type>_<Method>), place sub-cases under t.Run".

🤖 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 `@internal/cmd/geninterp/generate_test.go` around lines 16 - 58, Refactor
TestGenerator so each scenario—build, initial generation, clean -check,
stale-file check, and missing-file check—is enclosed in a descriptive sequential
t.Run sub-case. Preserve the shared temp directory and generated-file state
between sub-cases, and keep the existing assertions and command ordering intact.

Source: Coding guidelines

benchmarks/control_test.go (2)

149-171: 📐 Maintainability & Code Quality | 🔵 Trivial

Verify program.Builder.MustBuild() and reduce duplicated build-and-panic boilerplate.

iterativeFib repeats prog, err := b.Build(); if err != nil { panic(err) }; return prog. call_test.go already uses types.NewFunctionBuilder(...).MustBuild() for function builders. Check whether program.Builder exposes an equivalent MustBuild() method. This duplication repeats in sieve in this file, and in benchmarks/memory_test.go and benchmarks/numeric_test.go. See the consolidated comment for the full list of sites.

🤖 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 `@benchmarks/control_test.go` around lines 149 - 171, Replace the manual Build
error-handling sequence in iterativeFib with program.Builder’s MustBuild method
if available, returning its result directly. Apply the same change to the
corresponding sieve and benchmark builder sites identified in the consolidated
review, while preserving each function’s existing behavior.

---

149-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a shared program-builder panic helper.

program.Builder only offers Build(), while these benchmark helpers repeat the same if err != nil { panic(err) } return prog block five times across three files. Encapsulate this pattern in a small package-level helper in benchmarks/*; avoid using MustBuild() on types.FunctionBuilder unless constructing that API for program.Builder as well.

🤖 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 `@benchmarks/control_test.go` around lines 149 - 218, Extract a package-level
helper for building a program.Builder that panics on Build errors and returns
the resulting program. Replace the repeated build/error/return blocks in
iterativeFib and sieve in benchmarks/control_test.go, and the corresponding
blocks in benchmarks/memory_test.go and benchmarks/numeric_test.go, with this
helper; do not use types.FunctionBuilder.MustBuild unless the same API is added
for program.Builder.

internal/cmd/geninterp/lower.go (2)

1238-1323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document the intentional asymmetric coro handling in replace().

In the targetSlots == 1 branch, the fresh-frame path sets f.coro = 0 (Line 1257), but the frame-reuse path (Lines 1264-1278) never resets f.coro, leaving it at whatever value the replaced frame held. The same pattern repeats in the targetSlots != 1 branch: fresh-frame sets f.coro = 0 (Line 1300), reuse path (Lines 1307-1320) leaves it untouched. This is intentional: replace() implements tail calls (RETURN_CALL), and preserving f.coro keeps a tail-called function running on behalf of the same coroutine, matching the "preserves coroutine identity through function tail calls" test.

The intent is not obvious from the generated code alone. A future edit that "fixes" the missing reset in the reuse path would silently break coroutine identity across tail calls. Add a jen.Comment(...) at each reuse site to record why the reset is skipped.

📝 Proposed comment additions

 			jen.Id("f").Dot("release").Op("=").Add(jen.Lit(releaseTarget)),
+			jen.Comment("coro is intentionally preserved across a tail call so a resumed coroutine keeps its identity."),
 			jen.Id("i").Dot("sp").Op("=").Id("base").Op("+").Id("params").Op("+").Id("locals"),
 			jen.Id(label).Op(":").Add(jen.Null()),

Apply the equivalent comment before the second i.sp = base + params + locals at Line 1320.

🤖 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 `@internal/cmd/geninterp/lower.go` around lines 1238 - 1323, Add
jen.Comment(...) immediately before each frame-reuse path’s `i.sp = base +
params + locals` assignment in `replace`, covering both the `targetSlots == 1`
and `targetSlots != 1` branches. Document that `f.coro` is intentionally not
reset during tail-call frame reuse so coroutine identity is preserved; leave the
existing fresh-frame resets unchanged.

---

3673-3736: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate the repeated scalar-kind case list in refGet and refSet.

Both functions embed the identical case list types.I1, types.I8, types.I32, types.I64, types.F32, types.F64 (Lines 3680-3684 and 3725-3729). Extract a shared helper that returns this case list so the two opcodes cannot drift out of sync if a new scalar kind is added later.

♻️ Proposed extraction

func scalarKindCases() []jen.Code {
	return []jen.Code{
		jen.Id("types").Dot("I1"), jen.Id("types").Dot("I8"),
		jen.Id("types").Dot("I32"), jen.Id("types").Dot("I64"),
		jen.Id("types").Dot("F32"), jen.Id("types").Dot("F64"),
	}
}

Then reference scalarKindCases()... in place of the inlined list in both refGet and refSet.

🤖 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 `@internal/cmd/geninterp/lower.go` around lines 3673 - 3736, Extract the shared
scalar-kind case list into a scalarKindCases helper returning the six existing
jen.Code entries, then use its expanded result in the jen.Case calls within
refGet and refSet. Preserve the current scalar kinds and default ErrTypeMismatch
behavior while removing both duplicated inline lists.

interp/coroutine_test.go (3)

26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Name the top-level test after the public symbol under test.

The repository test convention requires Test<Func> or Test<Type>_<Method>. TestCoroutineReferences does not map to an exported symbol of package interp. The behaviors here are observed through Interpreter.Run, so TestInterpreter_Run with these sub-cases matches the convention. If a separate grouping is preferred, keep the sub-cases and rename the top-level function to a public symbol name.

As per coding guidelines: "Use one top-level test per public symbol (Test<Func> or Test<Type>_<Method>), place sub-cases under t.Run".

🤖 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/coroutine_test.go` at line 26, Rename the top-level test function
TestCoroutineReferences to TestInterpreter_Run so it follows the repository
convention and reflects the public symbol under test. Keep the existing
coroutine scenarios as sub-cases under t.Run.

Source: Coding guidelines

---

69-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Explain the 19 instruction-pointer literal.

19 is the encoded byte offset of the RESUME instruction in this module. A change to instruction widths moves that offset. require.True(t, collected) keeps the failure visible, so correctness is preserved. Add a short comment that states which instruction the offset targets, so a future edit can be updated quickly.

🤖 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/coroutine_test.go` around lines 69 - 73, Add a brief comment next to
the IP comparison in the interp.WithHook callback explaining that 19 is the
encoded byte offset of the RESUME instruction, making clear the literal must be
updated if instruction widths change.

---

220-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

State how the heap cap proves the release.

require.Equal(t, 3, vm.Len()) checks the stack only. The real proof is require.NoError(t, vm.Run(...)) with WithHeapLimit(5): if the discarded completion reference leaked, the last REF_NEW would return ErrHeapExhausted. Add a one-line comment that records this dependency, so a future heap-size change does not silently remove the coverage.

🤖 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/coroutine_test.go` around lines 220 - 228, The test currently relies
on the heap limit to detect leaked completion references, but this dependency is
undocumented. Add a one-line comment near the vm.Run assertion explaining that
WithHeapLimit(5) makes a leaked discarded reference cause the final REF_NEW to
return ErrHeapExhausted, preserving the release coverage.

interp/pool_test.go (2)

584-643: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the second declaration into the loop.

second is declared at line 584 but assigned and read only inside the iteration body. Declaring it inside the loop keeps the measured value local and removes the cross-iteration carry-over. The timer handling is correct: b.StopTimer() before the loop and b.StartTimer() only around pool.Get measures the shared-JIT miss 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 `@interp/pool_test.go` around lines 584 - 643, Move the var second
*interp.Interpreter declaration into the benchmark iteration loop, immediately
before its first assignment from pool.Get. Keep all existing uses, timer
handling, and pool lifecycle logic unchanged while eliminating cross-iteration
state.

---

120-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Record that 0xffffffff encodes -1.

The removed i32operand helper made the sign explicit. instr.New takes an unsigned operand, so the literal is correct. Add a named constant or a short comment, so the negative branch value stays readable.

♻️ Proposed change

 			Bind(neg).
-			Emit(instr.New(instr.I32_CONST, 0xffffffff)).
+			// 0xffffffff is the two's-complement encoding of i32 -1.
+			Emit(instr.New(instr.I32_CONST, 0xffffffff)).

🤖 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/pool_test.go` at line 120, Document the `0xffffffff` operand in the
test around `instr.New` as the encoded `-1` branch value, using a named constant
or concise comment. Keep the unsigned literal and existing instruction behavior
unchanged.

interp/host_test.go (1)

490-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Close the interpreter with defer inside the loop body.

Line 498 closes the interpreter as the last statement. If any earlier require in the same iteration fails, the test goroutine stops and the interpreter is never closed. The same pattern exists at lines 254-264 and 290-295. Wrap each iteration in a closure with defer i.Close(), or register t.Cleanup. This keeps failures from leaking interpreter resources into the rest of the test binary.

🤖 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/host_test.go` around lines 490 - 499, Update the test loop iterations
that create interpreters, including the shown host-object case and the analogous
sections near the other referenced cases, to register interpreter cleanup
immediately after creation using a closure-scoped defer or t.Cleanup. Remove the
explicit final i.Close calls where cleanup is registered, while preserving the
existing assertions and error handling.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes