fix: Align PTC-Lisp semantics with Clojure specification (#245)

[andreasronge]

Summary

Implemented three targeted semantic fixes to align PTC-Lisp with Clojure specification:

1. or operator: Now tracks and returns the last evaluated value (whether truthy or falsy) instead of only returning truthy values or nil
2. some predicate: Returns the result of applying the predicate to the first matching element (truthy result) instead of just a boolean
3. Subtraction operator: Fixed two issues:
   • Corrected reduce order to compute left-to-right (accumulator - element) instead of right-to-left
   • Moved unary minus handler before general variadic handler to ensure it's reachable

Test Plan

Added comprehensive tests for all three fixes:

• or now correctly returns last falsy value when all are falsy
• some returns first truthy predicate result (e.g., true for (some nil? [1 nil 3]))
• Subtraction computes correctly:
  • Unary: (- 10) → -10
  • Binary: (- 10 3) → 7
  • Variadic: (- 10 3 2 1) → 4 (not -8)

All existing tests pass (1126 tests, 0 failures).

Closes #245

[github-actions]

PR Review: fix: Align PTC-Lisp semantics with Clojure specification (#245)

Summary

This PR implements three targeted semantic fixes to align PTC-Lisp with Clojure behavior: or operator tracking last evaluated value, some returning predicate results instead of booleans, and fixing subtraction operator order and clause ordering. The implementation is correct and well-tested.

What's Good

• Correct implementation of all three fixes: Each semantic issue identified in fix: Align PTC-Lisp semantics with Clojure specification #245 is properly addressed
• Good clause ordering fix: Moving the unary minus handler before the general variadic handler ensures it's reachable
• Proper reduce order fix: Using fn x, acc -> fun2.(acc, x) end correctly implements left-to-right evaluation
• Comprehensive test coverage: 8 new tests covering all three fixes with both unit and integration tests
• The or fix is elegant: The do_eval_or_rest/6 helper properly tracks the last evaluated value through recursion
• Applied consistently: The reduce order fix is applied to both {:variadic, ...} and {:variadic_nonempty, ...} handlers

Issues (Must Fix)

1. Documentation not updated for some - docs/ptc-lisp-specification.md:780
   • Problem: The specification still says some returns "True if any match" but the implementation now returns the first truthy predicate result (matching Clojure semantics)
   • Impact: Documentation is inconsistent with implementation; LLMs relying on the spec may generate code expecting boolean returns
   • Fix: Update line 780 to match Clojure semantics:

     | `some` | `(some pred coll)` | First truthy result of pred, or nil |

   • Complexity: Mechanical (single line change)

Suggestions (Optional)

None - the implementation is clean and focused.

Security

No security concerns - this is a semantic fix to internal interpreter logic with no external attack surface.

Documentation

The specification at docs/ptc-lisp-specification.md needs to be updated to reflect the new some semantics (see Issue #1 above). The or documentation at line 902 already shows the correct behavior for returning first truthy value; the fix for returning last falsy when all are falsy is an edge case consistent with the documented behavior.

Verdict

Request Changes - The code changes are correct and well-tested, but the specification documentation needs a one-line update to reflect the new some semantics before merge.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Update some documentation to match Clojure semantics

Location: docs/ptc-lisp-specification.md:780
Problem: The specification still says some returns "True if any match" but the implementation now returns the first truthy predicate result (matching Clojure semantics)
Fix: Change line 780 from:

| `some` | `(some pred coll)` | True if any match |

to:

| `some` | `(some pred coll)` | First truthy result of pred, or nil |

Please fix in a SINGLE commit. Make minimal changes to resolve the issue.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Update some documentation to match Clojure semantics	FIX_NOW	Included in fix request above

Status

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

Reasoning

The documentation fix is in-scope for this PR since the PR explicitly changed some semantics to return the first truthy predicate result instead of a boolean. Updating the specification to reflect this change is a mechanical documentation update that completes the work started in the PR.

[andreasronge]

Fix Summary

Issue 1: Update some documentation ✅

Status: Fixed

Updated docs/ptc-lisp-specification.md:780 to reflect the new Clojure semantics for some:

• Before: True if any match
• After: First truthy result of pred, or nil

This aligns the specification documentation with the implementation, which now correctly returns the first truthy predicate result instead of a boolean.

Verification

• ✅ mix precommit passed (format, compile, credo, tests)
• ✅ All 1126 tests pass
• ✅ Single focused commit with minimal changes

[github-actions]

PR Review: fix: Align PTC-Lisp semantics with Clojure specification (#245)

Summary

This PR correctly implements three semantic fixes to align PTC-Lisp with Clojure specification: or operator returning last evaluated value, some predicate returning first truthy result, and subtraction operator fixes. The implementation is clean, well-tested, and the previous review cycle issues have been addressed.

What's Good

• Correct or fix: The do_eval_or_rest/6 helper elegantly tracks the last evaluated value through recursion while maintaining short-circuit behavior
• Proper some fix: Using Enum.find_value/2 is the idiomatic Elixir solution that matches Clojure semantics
• Complete subtraction fix: Addresses both the clause ordering issue (unary minus handler moved before general variadic handler) and the reduce order issue (using fn x, acc -> fun2.(acc, x) end)
• Consistent application: The reduce order fix is correctly applied to both {:variadic, ...} and {:variadic_nonempty, ...} handlers at lines 444 and 461
• Documentation updated: The specification at docs/ptc-lisp-specification.md:780 now correctly states First truthy result of pred, or nil
• Comprehensive tests: 8 new tests covering unit tests (eval_control_flow_test.exs) and integration tests (integration_test.exs) for all three fixes
• Good test comments: Tests reference issue fix: Align PTC-Lisp semantics with Clojure specification #245 for traceability

Issues (Must Fix)

None - the previous review cycle identified that the some documentation needed updating, and that fix is included in this PR.

Suggestions (Optional)

None - the implementation is focused and minimal.

Security

No security concerns - these are semantic fixes to internal interpreter logic with no external attack surface.

Documentation

• ✅ docs/ptc-lisp-specification.md updated to reflect new some semantics
• ✅ The or documentation examples at line 902 are consistent with the implementation (shows returning first truthy value; the edge case of returning last falsy is an implementation detail that aligns with Clojure)

Verdict

Approve - The implementation correctly addresses all three semantic issues from #245 with comprehensive tests and proper documentation updates. Ready for merge.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
-	No issues found	N/A	PR approved and ready to merge

Status

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

Reasoning

The latest review verdict is Approve with no issues identified. The previous review cycle's documentation issue was already fixed (updating some documentation at line 780 to reflect Clojure semantics). All tests pass and the PR is ready for merge.

Label ready-to-merge has been added.