navicore/seq-lisp

Fork 0

Step 1 (TCO) is done. #69

Merged

navicore merged 3 commits from fix-tco-self-hosting into main

2026-03-28 18:06:00 +00:00

navicore commented

2026-03-28 13:12:22 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-03-28 13:16:02 +00:00

(Migrated from github.com)

PR Review: Step 1 (TCO) via Trampoline

Overall this is a clean, well-structured implementation of the trampoline pattern for TCO. The approach is sound and the key changes are easy to follow.

Correctness: Thunk Tag Collision Risk

The sentinel __thunk__ is just a symbol in user-visible data. If any user program returns a list starting with that symbol, thunk? will silently misidentify it as a deferred thunk and corrupt the result. For example, '(__thunk__ a b) in user code would cause incorrect behavior.

A more robust approach: use a tagged wrapper type, a vector/record if available, or a gensym-style unique sentinel that cannot appear in user-written source. This is the highest-risk correctness issue in the PR.

Correctness: trampoline Self-Recursion is Subtle but Correct

trampoline is a seq-lisp closure whose tail-recursive call goes through apply-closure (which now returns a thunk). The outer trampoline loop in seval re-enters for each thunk, so the stack stays bounded. This is correct, but subtle enough to warrant a comment explaining why trampoline does not need to be a built-in primitive to avoid stack overflow.

Test Coverage

The single new test only covers the if tail-call path in a simple linear recursion. Worth adding tests for:

cond tail position (both a matched clause and the else branch)
begin with a tail call in the final expression
let body as a tail call
Mutual recursion if supported

Without these, regressions in do-cond, do-begin, or do-let won't be caught.

Minor: Removed Documentation Comment

In do-if, the original comment ; One-armed if: return '() when else branch is missing was dropped. That comment explained a non-obvious semantic choice and is worth keeping.

Minor: Macro Path Double-Thunking

(let expanded (trampoline ev (apply-closure ev op args))
  (if (eval-error? expanded) expanded
      (make-thunk env expanded)))

apply-closure always returns a thunk now, so trampoline unwraps it immediately, then we re-wrap the result in a new thunk. This is correct but one allocation is always transient. A brief comment — "trampoline resolves the macro body; then we tail-call into the expansion" — would clarify the intent.

Performance Note

Each tail call allocates a 3-element list (__thunk__ env expr). For tight loops this is O(n) short-lived allocations, which is expected for a trampoline. Worth watching if GC pressure shows up later.

Summary

Area	Status
Overall approach	Correct trampoline pattern
Tail positions covered	if, cond, begin, let, closure apply, macro expand
Non-tail wrapping	eval-args, condition evals correctly trampolined
Thunk sentinel safety	Potential user-data collision — needs attention
Test coverage	Needs more cases
Comments/docs	Minor gaps

Good foundational work. The thunk collision issue is the one I'd want addressed before merge.

## PR Review: Step 1 (TCO) via Trampoline Overall this is a clean, well-structured implementation of the trampoline pattern for TCO. The approach is sound and the key changes are easy to follow. --- **Correctness: Thunk Tag Collision Risk** The sentinel `__thunk__` is just a symbol in user-visible data. If any user program returns a list starting with that symbol, `thunk?` will silently misidentify it as a deferred thunk and corrupt the result. For example, `'(__thunk__ a b)` in user code would cause incorrect behavior. A more robust approach: use a tagged wrapper type, a vector/record if available, or a gensym-style unique sentinel that cannot appear in user-written source. This is the highest-risk correctness issue in the PR. --- **Correctness: `trampoline` Self-Recursion is Subtle but Correct** `trampoline` is a seq-lisp closure whose tail-recursive call goes through `apply-closure` (which now returns a thunk). The outer trampoline loop in `seval` re-enters for each thunk, so the stack stays bounded. This is correct, but subtle enough to warrant a comment explaining why `trampoline` does not need to be a built-in primitive to avoid stack overflow. --- **Test Coverage** The single new test only covers the `if` tail-call path in a simple linear recursion. Worth adding tests for: - `cond` tail position (both a matched clause and the `else` branch) - `begin` with a tail call in the final expression - `let` body as a tail call - Mutual recursion if supported Without these, regressions in `do-cond`, `do-begin`, or `do-let` won't be caught. --- **Minor: Removed Documentation Comment** In `do-if`, the original comment `; One-armed if: return '() when else branch is missing` was dropped. That comment explained a non-obvious semantic choice and is worth keeping. --- **Minor: Macro Path Double-Thunking** ```scheme (let expanded (trampoline ev (apply-closure ev op args)) (if (eval-error? expanded) expanded (make-thunk env expanded))) ``` `apply-closure` always returns a thunk now, so `trampoline` unwraps it immediately, then we re-wrap the result in a new thunk. This is correct but one allocation is always transient. A brief comment — "trampoline resolves the macro body; then we tail-call into the expansion" — would clarify the intent. --- **Performance Note** Each tail call allocates a 3-element list `(__thunk__ env expr)`. For tight loops this is O(n) short-lived allocations, which is expected for a trampoline. Worth watching if GC pressure shows up later. --- **Summary** | Area | Status | |---|---| | Overall approach | Correct trampoline pattern | | Tail positions covered | if, cond, begin, let, closure apply, macro expand | | Non-tail wrapping | eval-args, condition evals correctly trampolined | | Thunk sentinel safety | Potential user-data collision — needs attention | | Test coverage | Needs more cases | | Comments/docs | Minor gaps | Good foundational work. The thunk collision issue is the one I'd want addressed before merge.

claude[bot] commented

2026-03-28 15:08:37 +00:00

(Migrated from github.com)

PR Review: Step 1 (TCO) via Trampoline

The trampoline approach is a clean, idiomatic solution for adding TCO to a self-hosted evaluator. The design is sound and the implementation is mostly correct. A few things worth addressing:

Bug: `do-define` missing trampoline

do-define-all was updated, but its sibling do-define was not. The non-function branch on line ~257 still calls ev directly without trampolining:

; do-define (used by self-eval for single-expr seval)
(let val (ev ev env (car (cdr args)))   ; <-- no trampoline

vs. do-define-all (correctly updated):

(let val (trampoline ev (ev ev env (car (cdr args))))

This means (seval '(define x <tail-recursive-expr>)) could return a raw thunk instead of the resolved value. Example that would silently misbehave:

(seval '(define (count n) (if (= n 0) 'done (count (- n 1)))))
(seval '(define result (count 10000)))
; result would be a (__thunk__ ...) list, not 'done

`trampoline` is itself host-recursive

The trampoline bounces via self-recursion, which still consumes host (Rust) stack frames for each bounce. For the current tests (depth 10000) this should be fine, but if SeqLisp's host interpreter doesn't have TCO for trampoline's own tail call, extremely deep loops could still overflow.

This is an inherent limitation of the bootstrap approach and may be intentional as "Step 1" — just worth noting in a comment so it's not forgotten.

`bootstrap-eval` not trampolined (minor)

Lines ~585 and ~589 use raw (ev ev env expr). Bootstrap functions (map, filter, fold, not) are not tail-recursive in their current form so this is harmless now, but it creates a subtle inconsistency — if bootstrap functions are ever extended with tail-recursive patterns, they'd silently get thunks back.

Test coverage

The four new TCO tests are well-chosen and cover the main paths. A couple of gaps:

Mutual recursion — two functions bouncing between each other:

(define (even? n) (if (= n 0) #t (odd? (- n 1))))
(define (odd? n) (if (= n 0) #f (even? (- n 1))))
(even? 10000)

TCO through macro expansion — the macro path in do-apply now returns a thunk via make-thunk env expanded; a test verifying this works for a tail-call inside a macro body would be valuable.

Minor observations

thunk? can false-positive on user lists starting with '__thunk__. Already documented — just confirming the risk is understood.
thunk-env / thunk-expr have no guard; calling them on a non-thunk would return garbage silently. Low priority since thunk? gates all call sites.
The ; TAIL POSITION comments are helpful — consistent and appreciated.

Summary: The core trampoline design is correct and well-executed. The do-define missing trampoline is the one actual bug I'd want fixed before merging. Everything else is either a known limitation or a nice-to-have improvement.

## PR Review: Step 1 (TCO) via Trampoline The trampoline approach is a clean, idiomatic solution for adding TCO to a self-hosted evaluator. The design is sound and the implementation is mostly correct. A few things worth addressing: --- ### Bug: `do-define` missing trampoline `do-define-all` was updated, but its sibling `do-define` was not. The non-function branch on line ~257 still calls `ev` directly without trampolining: ```lisp ; do-define (used by self-eval for single-expr seval) (let val (ev ev env (car (cdr args))) ; <-- no trampoline ``` vs. `do-define-all` (correctly updated): ```lisp (let val (trampoline ev (ev ev env (car (cdr args)))) ``` This means `(seval '(define x <tail-recursive-expr>))` could return a raw thunk instead of the resolved value. Example that would silently misbehave: ```lisp (seval '(define (count n) (if (= n 0) 'done (count (- n 1))))) (seval '(define result (count 10000))) ; result would be a (__thunk__ ...) list, not 'done ``` --- ### `trampoline` is itself host-recursive The trampoline bounces via self-recursion, which still consumes host (Rust) stack frames for each bounce. For the current tests (depth 10000) this should be fine, but if SeqLisp's host interpreter doesn't have TCO for `trampoline`'s own tail call, extremely deep loops could still overflow. This is an inherent limitation of the bootstrap approach and may be intentional as "Step 1" — just worth noting in a comment so it's not forgotten. --- ### `bootstrap-eval` not trampolined (minor) Lines ~585 and ~589 use raw `(ev ev env expr)`. Bootstrap functions (`map`, `filter`, `fold`, `not`) are not tail-recursive in their current form so this is harmless now, but it creates a subtle inconsistency — if bootstrap functions are ever extended with tail-recursive patterns, they'd silently get thunks back. --- ### Test coverage The four new TCO tests are well-chosen and cover the main paths. A couple of gaps: 1. **Mutual recursion** — two functions bouncing between each other: ```lisp (define (even? n) (if (= n 0) #t (odd? (- n 1)))) (define (odd? n) (if (= n 0) #f (even? (- n 1)))) (even? 10000) ``` 2. **TCO through macro expansion** — the macro path in `do-apply` now returns a thunk via `make-thunk env expanded`; a test verifying this works for a tail-call inside a macro body would be valuable. --- ### Minor observations - `thunk?` can false-positive on user lists starting with `'__thunk__`. Already documented — just confirming the risk is understood. - `thunk-env` / `thunk-expr` have no guard; calling them on a non-thunk would return garbage silently. Low priority since `thunk?` gates all call sites. - The `; TAIL POSITION` comments are helpful — consistent and appreciated. --- **Summary**: The core trampoline design is correct and well-executed. The `do-define` missing trampoline is the one actual bug I'd want fixed before merging. Everything else is either a known limitation or a nice-to-have improvement.

claude[bot] commented

2026-03-28 17:22:34 +00:00

(Migrated from github.com)

PR Review: Step 1 (TCO) via Trampoline

Overall this is a clean, well-structured implementation of tail-call optimization. The trampoline pattern is the right choice for a self-hosting Lisp evaluator that can't rely on host-level TCO, and the execution follows through consistently.

Code Quality

Thunk representation — using a tagged list (__thunk__ env expr) is idiomatic and consistent with how closures and macros are already represented in this evaluator. Good.

thunk? predicate — the guard (if (list? x) (if (null? x) #f ...)) is correct and handles edge cases (non-list values, empty list). No issues.

Accessor names — thunk-env and thunk-expr are clear. Consider whether thunk-body would read more naturally than thunk-expr (to mirror closure-body), but this is minor.

; TAIL POSITION comments — very helpful for future maintainers trying to understand why a thunk is returned instead of a direct eval call.

Macro path — the double-trampoline pattern for macros is subtle but correct:

apply-closure returns a thunk for the macro body
trampoline drives that to completion → produces the expanded form
make-thunk env expanded defers evaluation of the expansion

The comment explaining this is appreciated.

Potential Issues

do-cond recursion — do-cond calls itself directly (not trampolined) when the test is false and there are more clauses. This is fine for reasonable cond depths, but worth noting as a host-stack consumer for pathologically long clause lists. Not a blocker.

thunk? name collision — acknowledged in the reserved names comment, but worth emphasizing: __thunk__ is only reserved by convention. Any user-created list starting with the __thunk__ symbol will be misidentified as a thunk. This is consistent with how __closure__ and __macro__ work today, so it's an acceptable tradeoff for a bootstrap evaluator.

trampoline takes ev as a parameter — this couples the trampoline to the evaluator's internal calling convention. It works correctly, but a future refactor might consider a zero-argument thunk wrapper to decouple them. Not urgent.

Tests

Good coverage of the four tail positions: if, cond, begin, let. The mutual-recursion limitation is clearly documented as a known constraint tied to immutable environments.

One suggestion: the sum test uses n=10000 with an accumulator — this is the canonical TCO test and 10,000 iterations is plenty to verify the trampoline fires. The test value 50005000 is correct (10000 * 10001 / 2). ✓

The macro TCO test (when → if) is a nice addition that exercises the double-trampoline path.

Summary

No blocking issues. The implementation is correct, well-commented, and consistent with the existing codebase style.
The known limitations (mutual recursion, __thunk__ name collision) are properly documented.
Test coverage for the new feature is solid.

Nice work getting TCO in — this is a meaningful correctness improvement for any seq-lisp program that relies on tail recursion.

## PR Review: Step 1 (TCO) via Trampoline Overall this is a clean, well-structured implementation of tail-call optimization. The trampoline pattern is the right choice for a self-hosting Lisp evaluator that can't rely on host-level TCO, and the execution follows through consistently. --- ### Code Quality **Thunk representation** — using a tagged list `(__thunk__ env expr)` is idiomatic and consistent with how closures and macros are already represented in this evaluator. Good. **`thunk?` predicate** — the guard `(if (list? x) (if (null? x) #f ...))` is correct and handles edge cases (non-list values, empty list). No issues. **Accessor names** — `thunk-env` and `thunk-expr` are clear. Consider whether `thunk-body` would read more naturally than `thunk-expr` (to mirror `closure-body`), but this is minor. **`; TAIL POSITION` comments** — very helpful for future maintainers trying to understand why a thunk is returned instead of a direct eval call. **Macro path** — the double-trampoline pattern for macros is subtle but correct: 1. `apply-closure` returns a thunk for the macro body 2. `trampoline` drives that to completion → produces the expanded form 3. `make-thunk env expanded` defers evaluation of the expansion The comment explaining this is appreciated. --- ### Potential Issues **`do-cond` recursion** — `do-cond` calls itself directly (not trampolined) when the test is false and there are more clauses. This is fine for reasonable `cond` depths, but worth noting as a host-stack consumer for pathologically long clause lists. Not a blocker. **`thunk?` name collision** — acknowledged in the reserved names comment, but worth emphasizing: `__thunk__` is only reserved by convention. Any user-created list starting with the `__thunk__` symbol will be misidentified as a thunk. This is consistent with how `__closure__` and `__macro__` work today, so it's an acceptable tradeoff for a bootstrap evaluator. **`trampoline` takes `ev` as a parameter** — this couples the trampoline to the evaluator's internal calling convention. It works correctly, but a future refactor might consider a zero-argument thunk wrapper to decouple them. Not urgent. --- ### Tests Good coverage of the four tail positions: `if`, `cond`, `begin`, `let`. The mutual-recursion limitation is clearly documented as a known constraint tied to immutable environments. One suggestion: the `sum` test uses `n=10000` with an accumulator — this is the canonical TCO test and 10,000 iterations is plenty to verify the trampoline fires. The test value `50005000` is correct (`10000 * 10001 / 2`). ✓ The macro TCO test (`when` → `if`) is a nice addition that exercises the double-trampoline path. --- ### Summary - **No blocking issues.** The implementation is correct, well-commented, and consistent with the existing codebase style. - The known limitations (mutual recursion, `__thunk__` name collision) are properly documented. - Test coverage for the new feature is solid. Nice work getting TCO in — this is a meaningful correctness improvement for any seq-lisp program that relies on tail recursion.