navicore/seq-lisp

Fork 0

ci #72

Merged

navicore merged 2 commits from punch-list-1 into main

2026-04-04 21:32:21 +00:00

navicore commented

2026-04-04 07:00:16 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-04 07:04:02 +00:00

(Migrated from github.com)

PR Review: New Builtins

Good chunk of work — the gap analysis is thorough and the implementations are clean. A few issues worth addressing before merge.

Bugs / Correctness

1. Double env lookup for user-defined function calls (src/eval.seq)

dispatch-new-builtins does an env lookup to check for user-defined closures, and if found, hands off to dispatch-user-defined which does another env lookup. Every user-defined function call now pays two lookups instead of one.

The intent is to avoid 13 new string comparisons per call, but the trade-off is a double env lookup. Consider restructuring so that when dispatch-new-builtins finds a closure/macro in env, it dispatches directly without re-entering dispatch-user-defined.

2. procedure? returns true for macros (src/eval.seq)

In standard Lisp, macros are not procedures. (procedure? some-macro) returning true is surprising and could cause issues if user code uses procedure? to guard callable dispatch. The test suite does not cover this case. Either document this intentional deviation or split into procedure? (closures only) and macro? (macros only).

3. zero?/positive?/negative? silently return false for non-numbers (src/eval.seq)

These predicates return false for non-numeric values but even?/odd? correctly error on non-integers. Inconsistent: (zero? "hello") returns false while (even? "hello") returns an error. Standard Scheme raises a type error for all of these. Silently returning false can hide bugs in user code.

Design Concerns

4. error only accepts strings and symbols

(error 42) produces the confusing message "error: argument must be a string" rather than something like "error: 42". Consider using the existing display/write machinery to convert any value to a string for the error message, matching how most Lisps handle this.

5. even?/odd? do not accept floats like 4.0

Standard Scheme allows (even? 4.0). Currently a float arg errors. Minor, but worth documenting if intentional.

Code Quality

6. even?/odd? implementation is roundabout (src/eval.seq)

The evenness check computes (n / 2) * 2 == n via integer divide and multiply. Since modulo is already a supported builtin, using n modulo 2 == 0 would be cleaner and avoids any edge-case concern if i.divide remainder semantics ever change.

7. when/unless return false on the no-op path

Standard Scheme returns an unspecified value when the when condition is false. Returning false is a pragmatic choice for this Lisp but worth a comment, since user code may accidentally rely on (when #f ...) being falsy.

Test Coverage

Good coverage overall. A few gaps:

(procedure? +) — builtins are not closures, so this returns false. A test would document this behavior explicitly.
(error 42) — test the non-string path to verify the error message is sensible.
(zero? "hello") vs (even? "hello") — add tests that document or fix the inconsistent type handling.
Short-circuit tests for and/or are present and good.

Minor

The defmacro test rename from when/unless to my-when/my-unless is the right call to avoid shadowing the new builtins.
The gap analysis doc update is well-structured and a useful reference.

Overall this is solid work. The main things to fix before merge are the double env lookup (point 1) and the type consistency of numeric predicates (point 3).

## PR Review: New Builtins Good chunk of work — the gap analysis is thorough and the implementations are clean. A few issues worth addressing before merge. --- ### Bugs / Correctness **1. Double env lookup for user-defined function calls** (src/eval.seq) dispatch-new-builtins does an env lookup to check for user-defined closures, and if found, hands off to dispatch-user-defined which does another env lookup. Every user-defined function call now pays two lookups instead of one. The intent is to avoid 13 new string comparisons per call, but the trade-off is a double env lookup. Consider restructuring so that when dispatch-new-builtins finds a closure/macro in env, it dispatches directly without re-entering dispatch-user-defined. **2. procedure? returns true for macros** (src/eval.seq) In standard Lisp, macros are not procedures. (procedure? some-macro) returning true is surprising and could cause issues if user code uses procedure? to guard callable dispatch. The test suite does not cover this case. Either document this intentional deviation or split into procedure? (closures only) and macro? (macros only). **3. zero?/positive?/negative? silently return false for non-numbers** (src/eval.seq) These predicates return false for non-numeric values but even?/odd? correctly error on non-integers. Inconsistent: (zero? "hello") returns false while (even? "hello") returns an error. Standard Scheme raises a type error for all of these. Silently returning false can hide bugs in user code. --- ### Design Concerns **4. error only accepts strings and symbols** (error 42) produces the confusing message "error: argument must be a string" rather than something like "error: 42". Consider using the existing display/write machinery to convert any value to a string for the error message, matching how most Lisps handle this. **5. even?/odd? do not accept floats like 4.0** Standard Scheme allows (even? 4.0). Currently a float arg errors. Minor, but worth documenting if intentional. --- ### Code Quality **6. even?/odd? implementation is roundabout** (src/eval.seq) The evenness check computes (n / 2) * 2 == n via integer divide and multiply. Since modulo is already a supported builtin, using n modulo 2 == 0 would be cleaner and avoids any edge-case concern if i.divide remainder semantics ever change. **7. when/unless return false on the no-op path** Standard Scheme returns an unspecified value when the when condition is false. Returning false is a pragmatic choice for this Lisp but worth a comment, since user code may accidentally rely on (when #f ...) being falsy. --- ### Test Coverage Good coverage overall. A few gaps: - (procedure? +) — builtins are not closures, so this returns false. A test would document this behavior explicitly. - (error 42) — test the non-string path to verify the error message is sensible. - (zero? "hello") vs (even? "hello") — add tests that document or fix the inconsistent type handling. - Short-circuit tests for and/or are present and good. --- ### Minor - The defmacro test rename from when/unless to my-when/my-unless is the right call to avoid shadowing the new builtins. - The gap analysis doc update is well-structured and a useful reference. Overall this is solid work. The main things to fix before merge are the double env lookup (point 1) and the type consistency of numeric predicates (point 3).

claude[bot] commented

2026-04-04 17:53:07 +00:00

(Migrated from github.com)

PR Review: New Builtins (and/or/when/unless/error, numeric predicates, pair?, procedure?, newline)

Good batch of additions. The code is well-structured and test coverage is solid. A few observations below.

Implementation Notes

even?/odd? via division round-trip

The current implementation checks n == (n/2)*2, which works correctly (including negative numbers). However, if i.modulo is available at the Seq level, using modulo would be simpler and easier to read. The top-level modulo builtin exists in the Lisp layer - worth checking if the underlying Seq op is accessible.

procedure? silently returns false for builtins

(procedure? car) will return false because builtins are not closures. This is a real divergence from user expectations. The test suite only covers lambdas and macros. At a minimum, a comment in the implementation noting this limitation would help future readers. It might also be worth adding an explicit test documenting the known limitation.

dispatch-new-builtins double env lookup edge case

When env-lookup-raw finds a value that is neither a closure nor a macro, the code drops it and falls through to dispatch-new-builtins-chain, then dispatch-user-defined - which does another env lookup. Minor inefficiency, functionally correct.

Test Gaps

No newline test

newline is implemented and registered as a builtin in lsp.slisp and eval.seq, but there is no test for it. Even a smoke test would be useful.

or-all-false test name is slightly misleading

The or-all-false test asserts equality with 0, not false, because or returns the last value even when all are falsy. A rename like or-last-value-when-all-false would make the intent clearer.

What is Good

Short-circuit semantics for and/or are correct - including return-value-not-just-boolean behavior.
The and-short-circuit and or-short-circuit tests using (error ...) elegantly verify that short-circuiting actually happens.
Renaming when/unless in defmacro.slisp to my-when/my-unless was the right call to avoid shadowing the new builtins.
eval-when-with-env / eval-unless-with-env correctly use eval-begin-exprs for multi-body support, and the *-multi-body tests cover it.
The performance comment in dispatch-new-builtins explaining why env lookup happens first is appreciated.
Good error messages throughout.

Summary

Nothing blocking. Two items most worth addressing before merge:

A newline smoke test (trivial).
A comment or explicit test documenting that procedure? returns false for builtins (otherwise users will hit a confusing behavior with no explanation).

## PR Review: New Builtins (and/or/when/unless/error, numeric predicates, pair?, procedure?, newline) Good batch of additions. The code is well-structured and test coverage is solid. A few observations below. --- ### Implementation Notes **even?/odd? via division round-trip** The current implementation checks n == (n/2)*2, which works correctly (including negative numbers). However, if i.modulo is available at the Seq level, using modulo would be simpler and easier to read. The top-level modulo builtin exists in the Lisp layer - worth checking if the underlying Seq op is accessible. **procedure? silently returns false for builtins** (procedure? car) will return false because builtins are not closures. This is a real divergence from user expectations. The test suite only covers lambdas and macros. At a minimum, a comment in the implementation noting this limitation would help future readers. It might also be worth adding an explicit test documenting the known limitation. **dispatch-new-builtins double env lookup edge case** When env-lookup-raw finds a value that is neither a closure nor a macro, the code drops it and falls through to dispatch-new-builtins-chain, then dispatch-user-defined - which does another env lookup. Minor inefficiency, functionally correct. --- ### Test Gaps **No newline test** newline is implemented and registered as a builtin in lsp.slisp and eval.seq, but there is no test for it. Even a smoke test would be useful. **or-all-false test name is slightly misleading** The or-all-false test asserts equality with 0, not false, because or returns the last value even when all are falsy. A rename like or-last-value-when-all-false would make the intent clearer. --- ### What is Good - Short-circuit semantics for and/or are correct - including return-value-not-just-boolean behavior. - The and-short-circuit and or-short-circuit tests using (error ...) elegantly verify that short-circuiting actually happens. - Renaming when/unless in defmacro.slisp to my-when/my-unless was the right call to avoid shadowing the new builtins. - eval-when-with-env / eval-unless-with-env correctly use eval-begin-exprs for multi-body support, and the *-multi-body tests cover it. - The performance comment in dispatch-new-builtins explaining why env lookup happens first is appreciated. - Good error messages throughout. --- ### Summary Nothing blocking. Two items most worth addressing before merge: 1. A newline smoke test (trivial). 2. A comment or explicit test documenting that procedure? returns false for builtins (otherwise users will hit a confusing behavior with no explanation).