PR Review — feat: support map instr

Decision

Approve with Minor Comments

Merge Readiness Summary

This PR is already merged, so this is a retrospective review. The core map feature implementation is solid and well-tested, with correct reference counting and type safety. However, the PR violates scope control by mixing unrelated changes (CI cache key fix, Go version management) with the feature work.

Blocking Findings

None at merge time (CI passed, tests comprehensive).

Important Findings

1. Scope Violation — Unrelated Changes Mixed In

The PR combines three distinct concerns:

• Map feature: New opcodes and types (primary work)
• CI cache key fix: Changed runner.go → matrix.go in workflows (bug fix)
• Go version: Bumped 1.25 → 1.26.2 in workflows, downgraded 1.26.3 → 1.26.2 in go.mod (unclear rationale)

These should be separate PRs to:

• Simplify review and testing of each concern
• Enable easier rollback if one change causes issues
• Make commit history clearer for future troubleshooting

For future PRs: Keep scope tight. Unrelated bug fixes and dependency updates belong in separate PRs.

2. Missing PR Description

The PR description is completely empty. This makes it harder to:

• Understand the design decisions (especially the MapKey/MapEntry structure)
• Verify all intended changes are present
• Understand rationale for Go version changes

For future PRs: Include:

• Summary of what changed
• Why map support is needed
• Any design trade-offs made
• Explanation of any dependency/version changes

Suggestions

1. [Minor] Reference Counting Documentation

The reference counting logic in interp/map.go is subtle and correct, but could benefit from a brief comment explaining the strategy:

• String keys: extract text, release original ref
• Ref keys: use ref identity, keep count on new entries
• Other keys: immediate values, no ref management

2. [Minor] Document NaN Normalization

In mapKey(), NaN(-0) is normalized to NaN(+0). This is correct for consistent map comparisons, but a comment would clarify the intent.

Important Observations

What Went Right

✅ Correct implementation: Reference counting is handled carefully and correctly. Each operation (set, delete, clear) properly manages heap object lifecycles.

✅ Comprehensive testing: 8 test cases cover all operations and key types (i32, string keys), with presence/absence paths tested.

✅ Architecture consistency: Map follows the same patterns as Array and Struct (Traceable interface, Type system integration).

✅ CI passes: Tests and linters all pass.

✅ Documentation: instruction-set.md updated with clear stack signatures and operation semantics.

Readiness Checklist

• Scope control: Needs work (mixed concerns)
• Correctness: Pass (implementation is correct)
• Test coverage: Pass (comprehensive tests)
• Architecture consistency: Pass (follows existing patterns)
• Risk / compatibility: Pass (new opcodes, backward compatible)
• Maintainability: Pass (clean code, subtle logic is correct)
• CI / validation: Pass (CI green)

Minimal Path to Merge

Already merged. No action required. For future reference: split this into two PRs:

1. Map feature (core work)
2. CI/version updates (separate concern)

Suggested Follow-up Issues

Better Scope Control in Future PRs

Reason: This PR mixed map feature with CI and version management. Separating concerns improves review quality and risk isolation.

Priority: Medium

Size: Process/documentation only

Final Recommendation

✅ The implementation is solid and this PR was ready to merge. The map feature is correct, well-tested, and follows existing architecture patterns. The scope violation is a process issue for future PRs rather than a blocker here. Going forward, recommend strict scope isolation (one feature or one bug fix per PR) and providing PR descriptions to document design decisions.

Good work on the implementation itself.

Generated by Claude Code