navicore/seq-lisp

Fork 0

Host eval is clean — no bootstrap overhead, no self-hosted functions … #71

Merged

navicore merged 2 commits from map-filter-regression-fix into main

2026-03-29 06:34:09 +00:00

navicore commented

2026-03-29 05:31:19 +00:00

(Migrated from github.com)

…in the host path, no regressions. The self-hosted reader/eval lives in

lib/reader/ as an additive library.

…in the host path, no regressions. The self-hosted reader/eval lives in lib/reader/ as an additive library.

claude[bot] commented

2026-03-29 05:35:07 +00:00

(Migrated from github.com)

PR Review: Host eval cleanup — remove bootstrap, restore native map/filter/fold

Overall this is a sound direction — removing the bootstrap overhead from the hot path is the right call, and the design doc is admirably honest about what regressed and why. A few issues worth addressing before merge.

Critical: `not` silently disappears from host eval

The old bootstrap provided not to the environment. This PR removes the bootstrap entirely but does not add not to the dispatch chain. As a result, (not x) in the host eval now produces "undefined symbol: not" rather than a boolean.

;; This now fails silently (returns snil via the eval wrapper)
(not #t)  ; => Error: undefined symbol: not

The test suite doesn't catch this because the only not tests go through seval-string (self-hosted eval path), not the host eval. tests/lisp/core/predicates.slisp has no not tests at all.

Fix: add a native dispatch entry for not (it's a trivial one-liner in Seq) alongside the other predicates, or add it to dispatch-predicates. Either way, a test in predicates.slisp should accompany it.

Stale comment in `lib/bootstrap.slisp`

The file header still reads:

The canonical source is the embedded string in src/eval.seq (bootstrap-source). This file is for readability only and is NOT loaded by the interpreter. Edit eval.seq to change bootstrap behavior.

But bootstrap-source was deleted in this PR. The comment is now incorrect and will confuse anyone who reads it. The file should either be deleted (it serves no purpose if there's no longer an embedded bootstrap) or its header updated to reflect that it's no longer backed by anything in src/eval.seq.

Positive: iterative map/filter in self-hosted eval

The new map/filter via reverse + fold in lib/reader/eval.slisp is the right fix for the self-hosted stack overflow. Defining fold first (so map/filter can use it) is correct, and reverse is confirmed available as a builtin in the self-hosted eval.

;; Clean and stack-safe
(define (map f lst)
  (reverse (fold (lambda (acc x) (cons (f x) acc)) '() lst)))

Positive: bootstrap overhead removed from hot paths

The previous approach re-evaluated the bootstrap source string on every call to eval, eval-print, run-file, run-repl, etc. Replacing bootstrap-env with env-empty across src/eval.seq and src/repl.seq eliminates this. No objections.

Minor: `map`/`filter` only accept closures, not symbols

eval-map-with-env and eval-filter-with-env evaluate the function argument and then check sclosure?. This means (map car '((1 2) (3 4))) will fail because car isn't an environment-bound closure. The apply builtin handles this via the symbol-detection path (constructs (car (quote ...)) and re-evaluates), but map and filter don't get that treatment.

This appears to be pre-existing behavior (the self-hosted closures had the same limitation), so not a regression from this PR, but worth noting as a known gap. The design doc's statement that "all interesting functions are recursive" is true, but first-class builtins in map would meaningfully expand the language's expressiveness.

Then-count arithmetic

15 thens for the updated dispatch-group4 looks correct (was 12, added 3 for map/filter/fold).

Summary

Issue	Severity
`not` removed from host eval, no tests cover it	Bug — should fix before merge
`lib/bootstrap.slisp` header references deleted `bootstrap-source`	Misleading documentation
Self-hosted map/filter now iterative via fold	Good fix
Bootstrap overhead removed from all entry points	Good cleanup
`map`/`filter` don't accept builtin symbols as fn arg	Known pre-existing limitation

## PR Review: Host eval cleanup — remove bootstrap, restore native map/filter/fold Overall this is a sound direction — removing the bootstrap overhead from the hot path is the right call, and the design doc is admirably honest about what regressed and why. A few issues worth addressing before merge. --- ### Critical: `not` silently disappears from host eval The old bootstrap provided `not` to the environment. This PR removes the bootstrap entirely but does **not** add `not` to the dispatch chain. As a result, `(not x)` in the host eval now produces "undefined symbol: not" rather than a boolean. ```lisp ;; This now fails silently (returns snil via the eval wrapper) (not #t) ; => Error: undefined symbol: not ``` The test suite doesn't catch this because the only `not` tests go through `seval-string` (self-hosted eval path), not the host eval. `tests/lisp/core/predicates.slisp` has no `not` tests at all. Fix: add a native dispatch entry for `not` (it's a trivial one-liner in Seq) alongside the other predicates, or add it to `dispatch-predicates`. Either way, a test in `predicates.slisp` should accompany it. --- ### Stale comment in `lib/bootstrap.slisp` The file header still reads: > The canonical source is the embedded string in `src/eval.seq` (`bootstrap-source`). This file is for readability only and is NOT loaded by the interpreter. Edit `eval.seq` to change bootstrap behavior. But `bootstrap-source` was deleted in this PR. The comment is now incorrect and will confuse anyone who reads it. The file should either be deleted (it serves no purpose if there's no longer an embedded bootstrap) or its header updated to reflect that it's no longer backed by anything in `src/eval.seq`. --- ### Positive: iterative map/filter in self-hosted eval The new `map`/`filter` via `reverse` + `fold` in `lib/reader/eval.slisp` is the right fix for the self-hosted stack overflow. Defining `fold` first (so `map`/`filter` can use it) is correct, and `reverse` is confirmed available as a builtin in the self-hosted eval. ```lisp ;; Clean and stack-safe (define (map f lst) (reverse (fold (lambda (acc x) (cons (f x) acc)) '() lst))) ``` --- ### Positive: bootstrap overhead removed from hot paths The previous approach re-evaluated the bootstrap source string on every call to `eval`, `eval-print`, `run-file`, `run-repl`, etc. Replacing `bootstrap-env` with `env-empty` across `src/eval.seq` and `src/repl.seq` eliminates this. No objections. --- ### Minor: `map`/`filter` only accept closures, not symbols `eval-map-with-env` and `eval-filter-with-env` evaluate the function argument and then check `sclosure?`. This means `(map car '((1 2) (3 4)))` will fail because `car` isn't an environment-bound closure. The `apply` builtin handles this via the symbol-detection path (constructs `(car (quote ...))` and re-evaluates), but `map` and `filter` don't get that treatment. This appears to be pre-existing behavior (the self-hosted closures had the same limitation), so not a regression from this PR, but worth noting as a known gap. The design doc's statement that "all interesting functions are recursive" is true, but first-class builtins in `map` would meaningfully expand the language's expressiveness. --- ### Then-count arithmetic 15 `then`s for the updated `dispatch-group4` looks correct (was 12, added 3 for map/filter/fold). --- ### Summary | Issue | Severity | |---|---| | `not` removed from host eval, no tests cover it | **Bug — should fix before merge** | | `lib/bootstrap.slisp` header references deleted `bootstrap-source` | Misleading documentation | | Self-hosted map/filter now iterative via fold | Good fix | | Bootstrap overhead removed from all entry points | Good cleanup | | `map`/`filter` don't accept builtin symbols as fn arg | Known pre-existing limitation |

claude[bot] commented

2026-03-29 06:00:34 +00:00

(Migrated from github.com)

PR Review: Host eval cleanup — bootstrap overhead removed

This is a clean architectural win. Removing the bootstrap eval path from the host cuts startup overhead and eliminates the self-hosted/native duplication that caused the stack overflow regressions. Overall the implementation is sound. A few things worth noting:

Potential issue: empty if-body in `eval-not-with-env` (eval.seq ~line 4209)

dup eval-err? if
else
  eval-ok-value
  ...
then

The empty if body works correctly — the error EvalResult stays on the stack and is returned — but this idiom is easy to misread and could silently swallow a logic error if the stack depth ever changes. The more idiomatic pattern used elsewhere in this file has an explicit error branch. Not a blocker, but worth a comment at minimum.

Missing regression test for large-list map/filter

The stated motivation for reverting map/filter/fold to native was a stack overflow at ~10,000 elements. There is no new test exercising this boundary. Something like:

(test 'map-large-list
  (assert-equal (length (map (lambda (x) (+ x 1)) (range 10000 '()))) 10000))

Without this, the regression could silently reappear if someone changes the dispatch again.

Self-hosted eval: `reverse` dependency confirmed safe

The new map/filter in lib/reader/eval.slisp call reverse, which is a native builtin (line 334 of eval.slisp). The approach is correct: fold accumulates in reverse, then a single reverse pass gives O(n) behaviour without deep recursion. No issue here.

`lib/bootstrap.slisp` header could be clearer

The file says it is a "HISTORICAL REFERENCE" but still contains live-looking SeqLisp code. The design doc already acknowledges the duplication problem (option 3: delete the file). If it stays, a more prominent note that this code is not evaluated anywhere would help avoid confusion for new contributors.

`then` chain counts look correct

dispatch-builtins: 15 thens for 15 branches (cons/car/cdr/list/append/reverse/length/nth/last/take/drop/map/filter/fold/apply) ✓
dispatch-predicates: 11 thens for 11 branches (null?/number?/integer?/float?/string?/symbol?/list?/boolean?/equal?/gensym/not) ✓

Positive notes

The not semantics are correct: only #f is falsy; numbers, nil, and other symbols are truthy. Consistent with the new test cases at predicates.slisp lines 43-48.
The five new not tests are well chosen (false, true, number, nil, symbol).
The design doc update honestly calls out the mutual-recursion regression and the stack-depth limitation for the self-hosted eval — good for setting expectations.
Removing bootstrap-env from eval-print, run-file, run-repl, and the stdin path is consistently applied across both eval.seq and repl.seq.

Summary: Approve with the suggestion to add a large-list regression test and clarify the lib/bootstrap.slisp status. The empty-if idiom in eval-not-with-env is a minor style point, not a correctness problem.

## PR Review: Host eval cleanup — bootstrap overhead removed This is a clean architectural win. Removing the bootstrap eval path from the host cuts startup overhead and eliminates the self-hosted/native duplication that caused the stack overflow regressions. Overall the implementation is sound. A few things worth noting: --- ### Potential issue: empty if-body in `eval-not-with-env` (eval.seq ~line 4209) ```seq dup eval-err? if else eval-ok-value ... then ``` The empty `if` body works correctly — the error EvalResult stays on the stack and is returned — but this idiom is easy to misread and could silently swallow a logic error if the stack depth ever changes. The more idiomatic pattern used elsewhere in this file has an explicit error branch. Not a blocker, but worth a comment at minimum. --- ### Missing regression test for large-list map/filter The stated motivation for reverting `map`/`filter`/`fold` to native was a stack overflow at ~10,000 elements. There is no new test exercising this boundary. Something like: ```scheme (test 'map-large-list (assert-equal (length (map (lambda (x) (+ x 1)) (range 10000 '()))) 10000)) ``` Without this, the regression could silently reappear if someone changes the dispatch again. --- ### Self-hosted eval: `reverse` dependency confirmed safe The new `map`/`filter` in `lib/reader/eval.slisp` call `reverse`, which is a native builtin (line 334 of `eval.slisp`). The approach is correct: `fold` accumulates in reverse, then a single `reverse` pass gives O(n) behaviour without deep recursion. No issue here. --- ### `lib/bootstrap.slisp` header could be clearer The file says it is a "HISTORICAL REFERENCE" but still contains live-looking SeqLisp code. The design doc already acknowledges the duplication problem (option 3: delete the file). If it stays, a more prominent note that this code **is not evaluated anywhere** would help avoid confusion for new contributors. --- ### `then` chain counts look correct - `dispatch-builtins`: 15 `then`s for 15 branches (cons/car/cdr/list/append/reverse/length/nth/last/take/drop/map/filter/fold/apply) ✓ - `dispatch-predicates`: 11 `then`s for 11 branches (null?/number?/integer?/float?/string?/symbol?/list?/boolean?/equal?/gensym/not) ✓ --- ### Positive notes - The `not` semantics are correct: only `#f` is falsy; numbers, nil, and other symbols are truthy. Consistent with the new test cases at predicates.slisp lines 43-48. - The five new `not` tests are well chosen (false, true, number, nil, symbol). - The design doc update honestly calls out the mutual-recursion regression and the stack-depth limitation for the self-hosted eval — good for setting expectations. - Removing `bootstrap-env` from `eval-print`, `run-file`, `run-repl`, and the stdin path is consistently applied across both `eval.seq` and `repl.seq`. --- **Summary:** Approve with the suggestion to add a large-list regression test and clarify the `lib/bootstrap.slisp` status. The empty-if idiom in `eval-not-with-env` is a minor style point, not a correctness problem.