fix: Add evaluator unit tests for fn parameter destructuring patterns

[andreasronge]

Fixes #156

Summary

Adds comprehensive unit tests for the destructuring functionality in function parameters that was previously implemented in match_pattern/2, apply_fun/5, and eval_closure_arg/7.

Test Plan

• ✅ Vector destructuring with position extraction (first, second elements)
• ✅ Vector destructuring ignoring extra elements
• ✅ Vector destructuring error handling (insufficient elements, non-list types)
• ✅ Map destructuring extracting specified keys
• ✅ Map destructuring with multiple keys
• ✅ Map destructuring with default values
• ✅ Map destructuring error handling (non-map types)
• ✅ Nested destructuring patterns (vectors containing destructuring)

All tests pass locally and pre-commit checks pass.

🤖 Generated with Claude Code

[github-actions]

PR Review: Add evaluator unit tests for fn parameter destructuring patterns

Summary

This PR adds 10 comprehensive unit tests for evaluator destructuring functionality in function parameters. The tests follow existing patterns and provide thorough coverage of vector and map destructuring scenarios.

What's Good

• Tests are well-organized in a dedicated "closure with destructuring patterns" describe block
• Good use of pre-built AST nodes (as per issue guidance) rather than parsing
• Comprehensive coverage: vector, map, nested patterns, and error cases
• Helpful inline comments showing the equivalent Lisp syntax
• Strong assertions with specific expected values
• Consistent test naming convention with colons
• All tests pass

Issues (Must Fix)

1. Missing test case from spec - eval_test.exs

   • Problem: The issue's Test Plan table specifies a test for "wrong type for vector pattern" where a map is passed instead of a list: ((fn [[a b]] a) {:x 1}). The PR includes a test for passing an integer (42) instead of a list, but not for passing a map.
   • Impact: Incomplete test coverage per the issue specification
   • Fix: Add test case that passes a map {:map, [...]} to a vector destructuring pattern
   • Complexity: Mechanical (same pattern as existing "error on non-list argument" test)

2. Missing nested pattern test case - eval_test.exs

   • Problem: The issue's Test Plan specifies a nested pattern test: ((fn [[k {:keys [v]}]] v) [["key" {:v 42}]]) - a vector containing a symbol and a map pattern. The PR only tests nested vector-in-vector patterns, not vector containing map destructuring.
   • Impact: The mix of vector and map patterns in a nested structure is a distinct code path that should be tested
   • Fix: Add test case for vector pattern containing map pattern: {:destructure, {:seq, [{:var, :k}, {:destructure, {:keys, [:v], []}}]}}
   • Complexity: Mechanical (follows existing nested pattern test structure)

Suggestions (Optional)

1. OPTIONAL: Consider test helper extraction - Nice-to-have
   • The test setup pattern (building closure → let binding → call AST) is repeated in each test
   • Could extract a helper like build_closure_call(params, body, args)
   • Complexity: Moderate (needs design decisions on API)
   • Recommendation: Out of scope for this PR, could be future work

Security

No concerns - this is test code only.

Documentation

No updates needed - internal tests only (as stated in issue).

Verdict

Request Changes - Two test cases from the issue's Test Plan are missing:

1. Wrong type for vector pattern (map passed instead of list)
2. Nested vector+map destructuring pattern

These are mechanical additions following the existing test patterns.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Missing test for map passed to vector destructuring pattern

Location: test/ptc_runner/lisp/eval_test.exs:642
Problem: The issue #156 Test Plan specifies testing "wrong type for vector pattern" with a map passed instead of a list: ((fn [[a b]] a) {:x 1}). The PR includes a test for passing an integer (42), but not for passing a map.
Fix: Add a test case that passes a map {:map, [{{:keyword, :x}, 1}]} to a vector destructuring pattern. The test should verify it returns an error. Follow the same pattern as the existing "error on non-list argument" test at line 642.

Issue 2: Missing test for nested vector containing map destructuring

Location: test/ptc_runner/lisp/eval_test.exs:709
Problem: The issue #156 Test Plan specifies a nested pattern test: ((fn [[k {:keys [v]}]] v) [["key" {:v 42}]]) - a vector pattern containing a symbol and a map pattern. The PR only tests nested vector-in-vector patterns (line 709), not vector containing map destructuring.
Fix: Add a test case for vector pattern containing both a variable and a map pattern. The AST should be: {:destructure, {:seq, [{:var, :k}, {:destructure, {:keys, [:v], []}}]}} called with [{:vector, [{:string, "key"}, {:map, [{{:keyword, :v}, 42}]}]}]. Expected result: {:ok, 42, %{}}. Follow the same pattern as the existing nested test at line 709.

Please fix ALL issues above in a SINGLE commit. Make minimal changes to resolve each issue.

[andreasronge]

Auto-Triage Summary

Review Analysis

The review identified 2 missing test cases from issue #156's Test Plan that need to be added to complete the test coverage specification.

Decisions Made

#	Issue	Decision	Action
1	Missing test for map passed to vector destructuring pattern	FIX_NOW	Included in fix request above
2	Missing test for nested vector containing map destructuring	FIX_NOW	Included in fix request above
3	Optional: Consider test helper extraction	IGNORE	Out of scope for this PR; could be future work

Reasoning

Issues 1-2: FIX_NOW

• Both test cases are explicitly listed in issue Add evaluator unit tests for fn parameter destructuring patterns #156's Test Plan table
• Mechanical additions following existing test patterns in the PR
• In-scope: PR's purpose is to implement the Test Plan from issue Add evaluator unit tests for fn parameter destructuring patterns #156
• Simple complexity: Just need to build correct AST nodes using established patterns

Issue 3: IGNORE

• Marked as "OPTIONAL" and "nice-to-have" by reviewer
• Moderate complexity requiring API design decisions
• Reviewer explicitly recommended "could be future work"
• Not required for PR acceptance

Status

• FIX_NOW items: 2 (batched in single fix comment above)
• Issues created: 0
• Items ignored: 1

Next Steps: Awaiting claude.yml workflow to apply fixes from the comment above.

[andreasronge]

Fix Summary

Fixed both issues from the PR review:

Issue 1: Vector destructuring with map argument

• Added test case "vector destructuring: error on map argument"
• Tests that passing a map {:map, [{{:keyword, :x}, 1}]} to a vector destructuring pattern returns an error
• Follows the same pattern as the existing integer argument error test

Issue 2: Nested vector containing map destructuring

• Added test case "vector destructuring with nested patterns: vector containing map"
• Tests the pattern [k {:keys [v]}] called with [["key" {:v 42}]]
• Verifies correct extraction of nested values (expects result: 42)
• Covers the distinct code path for mixed vector-map nested patterns

Verification

• All tests pass (954 tests)
• Code formatting verified
• Compilation successful with warnings-as-errors
• Pre-commit checks passed

Both test cases implement the exact AST structures specified in the issue #156 Test Plan.

[github-actions]

PR Review: Add evaluator unit tests for fn parameter destructuring patterns

Summary

This PR adds 12 comprehensive unit tests for the evaluator's destructuring functionality in function parameters. The implementation now correctly matches the issue #156 Test Plan and spec section 4.2 requirements, following a previous review cycle that addressed two missing test cases.

What's Good

• Complete coverage - All test cases from the spec (section 4.2) and issue Add evaluator unit tests for fn parameter destructuring patterns #156 Test Plan are now covered:
  • Vector destructuring (extract first, second, ignore extra, insufficient elements, wrong types)
  • Map destructuring (single key, multiple keys, defaults, wrong types)
  • Nested patterns (vector-in-vector, vector containing map)
• Well-organized - Tests are grouped in a clear "closure with destructuring patterns" describe block
• Helpful comments - Each test includes an inline comment showing the equivalent Lisp syntax
• Strong assertions - Uses specific expected values ({:ok, 1, %{}}, {:ok, 42, %{}}) rather than shape matching
• Consistent patterns - Follows existing test patterns in the file (pre-built AST nodes, Env.initial(), dummy_tool/2)
• Pre-commit verified - All 82 tests pass in the file

Issues (Must Fix)

None. The previous review's issues have been addressed:

1. ✅ Added test for map passed to vector destructuring ("error on map argument", line 725)
2. ✅ Added test for nested vector containing map destructuring ("vector containing map", line 738)

Suggestions (Optional)

1. OPTIONAL: Test helper extraction - Nice-to-have
   • The pattern params → body → closure_def → bindings → call_ast → eval is repeated across all 12 new tests
   • Could extract a helper like eval_closure_call(params, body, args) to reduce boilerplate
   • Note: This matches an existing pattern in the file (lines 523-560), so any refactoring should be done holistically
   • Recommendation: Out of scope for this PR; could be a separate refactoring issue if desired

Security

No concerns - test code only.

Documentation

No updates needed - internal unit tests only (per issue #156 guidance).

Verification Against Spec

Checked docs/ptc-lisp-fn-destructuring-spec.md section 4.2:

• ✅ Vector destructuring basic extraction (a, b from [1 2])
• ✅ Map destructuring with :keys
• ✅ Extra elements ignored
• ✅ Error on insufficient elements
• ✅ Nested patterns tested

Verdict

Approve - All required test cases from issue #156 are implemented and passing. The tests are well-structured, use strong assertions, and follow existing codebase patterns.

[andreasronge]

Auto-Triage Summary - Cycle 1

Review Analysis

The latest PR review shows Approve verdict. All previously identified issues from the first review cycle have been successfully fixed:

• ✅ Added test for map passed to vector destructuring pattern
• ✅ Added test for nested vector containing map destructuring

The PR now contains 12 comprehensive unit tests that fully implement the Test Plan from issue #156.

Decisions Made

#	Issue	Decision	Reason
1	OPTIONAL: Test helper extraction	IGNORE	Explicitly marked as "nice-to-have" and "out of scope for this PR" by reviewer. Could be tracked in a separate refactoring issue if desired.

Status

• FIX_NOW items: 0
• Issues created: 0
• Items ignored: 1

Outcome

✅ PR is ready to merge - All required test cases are implemented and passing. The ready-to-merge label has been added.

Auto-merge will handle merging once all required checks pass.