map, filter, fold, apply, and not are now self-hosted SeqLisp — no sp… #68

Merged

navicore merged 5 commits from self-hosting-evaluator into main 2026-03-28 04:15:24 +00:00

Conversation 20 Commits 5 Files changed 5 +795 -4

navicore commented 2026-03-28 02:36:39 +00:00 (Migrated from github.com)

Copy link

…ecial casing, no host delegation for higher-order functions. The bootstrap

pattern means we can progressively move more builtins into self-hosted SeqLisp over time.

…ecial casing, no host delegation for higher-order functions. The bootstrap pattern means we can progressively move more builtins into self-hosted SeqLisp over time.

navicore commented 2026-03-28 02:42:32 +00:00 (Migrated from github.com)

Copy link

@claude where is our code review for this PR??!

@claude where is our code review for this PR??!

👀 1

claude[bot] commented 2026-03-28 02:42:47 +00:00 (Migrated from github.com)

Copy link

Claude finished @navicore's task in 1m 33s —— View job

---

Code Review for PR #68

• Read changed files
• Analyze and provide review feedback

---

Summary

This PR implements a self-hosted SeqLisp evaluator (lib/reader/eval.slisp) with 536 lines of SeqLisp, plus a 140-line test suite. The architecture is well-designed — passing ev explicitly through all forms is the right pattern for a language without forward references. The bootstrap pattern for map, filter, fold, apply, and not is clean. Overall the structure is solid, but there are several issues worth addressing.

---

Issues

🔴 Critical

apply silently truncates arguments beyond 4 (lib/reader/eval.slisp:488-493)

(define (apply f args)
  (if (null? args) (f)
    (if (null? (cdr args)) (f (car args))
      ...
        (f (car args) (car (cdr args)) (car (cdr (cdr args))) (car (cdr (cdr (cdr args)))))))))

This hardcoded dispatch only handles 0–4 args. A call like (apply + '(1 2 3 4 5)) will silently ignore the 5th argument and return 10 instead of 15. There's no error or comment documenting the limit. Either add a variadic path or emit an error when args has more than 4 elements.

Fix this →

---

do-if crashes on missing else branch (lib/reader/eval.slisp:172-177)

(define (do-if ev env args)
  (let test-val (ev ev env (car args))
    (if (eval-error? test-val) test-val
        (if (equal? test-val #f)
            (ev ev env (car (cdr (cdr args))))  ; blindly indexes 3rd arg
            (ev ev env (car (cdr args)))))))

(if #f 1) — no else branch — will call (car (cdr (cdr args))) on a 2-element list, crashing with a car of nil error rather than a meaningful message. One-armed if is common in Lisp. Should guard with a null check and return '() or (make-eval-error "if: missing else branch").

Fix this →

---

🟡 Moderate

__not-found__ sentinel leaks into user namespace (lib/reader/eval.slisp:21-22)

(define (frame-lookup frame name)
  (if (null? frame)
      '__not-found__

If user code ever binds or compares against the symbol __not-found__, env-lookup will return it as a found binding instead of the user's value. A tagged list like (list '__not-found__) would be unambiguous (users can't construct this without knowing to), at the cost of a slightly more complex equality check.

---

bootstrap-eval silently ignores errors (lib/reader/eval.slisp:504-513)

(let result (do-define-all ev env (cdr expr))
  (if (eval-error? result) env    ; <-- silently continues with old env
      (bootstrap-eval ev (car result) (cdr exprs))))

If a bootstrap definition fails, execution continues with the old environment and the error is swallowed. A bug in map/filter/fold/apply/not would produce a mysteriously broken evaluator with no indication of what went wrong.

---

do-define discards the new environment (lib/reader/eval.slisp:213-218)

(begin
  (env-define env name val)  ; returns new env, discarded
  val)

env-define returns a new environment (immutable-style), but begin returns val and the updated env is lost. This means do-define within self-eval (single-expression path) does not actually persist variable bindings. This is worked around correctly by do-define-all in do-eval-all, but using seval directly for a define expression will silently not define anything.

---

🟢 Minor / Polish

Missing test coverage for key edge cases (tests/lisp/reader/eval.slisp)

Tests that are absent but would catch the above issues:

• apply with 5+ args
• (if #f 1) — one-armed if
• (map (lambda (x) (* x x)) '(1 2 3)) — lambda (not named fn) with map
• (not 42) — truthy non-boolean
• Calling a non-function: (seval-string "(1 2 3)") should return an error
• Too-few-args: (seval-string "((lambda (x y) x) 1)") should error

---

val->str silently serializes unknowns as ?? (lib/reader/eval.slisp:329-339)

When a symbol isn't in builtin-name-map (e.g., a user-defined name), it becomes "??" in the serialized form. This is unavoidable for the string-round-trip delegation strategy, but there's no comment explaining why. Also, passing a closure to a builtin (e.g. (length some-closure)) would serialize incorrectly. A code comment would prevent future confusion.

---

do-cond returns '() on no match (lib/reader/eval.slisp:179)

(define (do-cond ev env clauses)
  (if (null? clauses) '()

When no clause matches and there's no else, the result is '() (empty list). In standard Scheme this is "unspecified," but '() is a real value that could be mistaken for a meaningful result. Returning (make-eval-error "cond: no matching clause") would be safer.

---

build-name-map calls eval-with-errors/ok?/ok-value — undocumented host dependencies (lib/reader/eval.slisp:281-285)

These are host-provided functions not in builtin-names. A comment listing what host functions this module depends on (beyond the registered builtins) would help future maintainers.

---

Justfile

The new reader-test block (justfile:131-145) is correct — it chains all four reader components properly and reuses the shared $tmp file with proper cleanup. The pattern is consistent with the existing reader tests. No issues.

---

Verdict

The architecture and overall approach are excellent — this is a well-structured, readable self-hosted evaluator. The two critical issues (apply arg truncation and one-armed if crash) should be fixed before merge. The silent error swallowing in bootstrap-eval and the discarded environment in do-define are worth addressing too. The test gaps are the easiest wins since the framework is already in place.

**Claude finished @navicore's task in 1m 33s** —— [View job](https://github.com/navicore/seq-lisp/actions/runs/23675713151) --- ### Code Review for PR #68 - [x] Read changed files - [x] Analyze and provide review feedback --- ### Summary This PR implements a self-hosted SeqLisp evaluator (`lib/reader/eval.slisp`) with 536 lines of SeqLisp, plus a 140-line test suite. The architecture is well-designed — passing `ev` explicitly through all forms is the right pattern for a language without forward references. The bootstrap pattern for `map`, `filter`, `fold`, `apply`, and `not` is clean. Overall the structure is solid, but there are several issues worth addressing. --- ### Issues #### 🔴 Critical **`apply` silently truncates arguments beyond 4** (`lib/reader/eval.slisp:488-493`) ```lisp (define (apply f args) (if (null? args) (f) (if (null? (cdr args)) (f (car args)) ... (f (car args) (car (cdr args)) (car (cdr (cdr args))) (car (cdr (cdr (cdr args))))))))) ``` This hardcoded dispatch only handles 0–4 args. A call like `(apply + '(1 2 3 4 5))` will silently ignore the 5th argument and return 10 instead of 15. There's no error or comment documenting the limit. Either add a variadic path or emit an error when `args` has more than 4 elements. [Fix this →](https://claude.ai/code?q=In%20lib%2Freader%2Feval.slisp%20in%20branch%20self-hosting-evaluator%20of%20navicore%2Fseq-lisp%2C%20the%20bootstrap%20apply%20function%20%28around%20line%20488%29%20silently%20drops%20arguments%20beyond%204.%20Add%20an%20error%20path%20when%20args%20has%20more%20than%204%20elements%2C%20or%20add%20a%20comment%20clearly%20documenting%20the%20max%20arity%20limitation.) --- **`do-if` crashes on missing else branch** (`lib/reader/eval.slisp:172-177`) ```lisp (define (do-if ev env args) (let test-val (ev ev env (car args)) (if (eval-error? test-val) test-val (if (equal? test-val #f) (ev ev env (car (cdr (cdr args)))) ; blindly indexes 3rd arg (ev ev env (car (cdr args))))))) ``` `(if #f 1)` — no else branch — will call `(car (cdr (cdr args)))` on a 2-element list, crashing with a `car of nil` error rather than a meaningful message. One-armed `if` is common in Lisp. Should guard with a null check and return `'()` or `(make-eval-error "if: missing else branch")`. [Fix this →](https://claude.ai/code?q=In%20lib%2Freader%2Feval.slisp%20in%20branch%20self-hosting-evaluator%20of%20navicore%2Fseq-lisp%2C%20the%20do-if%20function%20%28around%20line%20172%29%20crashes%20when%20the%20else%20branch%20is%20missing%20%28one-armed%20if%29.%20Add%20a%20null%20check%20on%20%28cdr%20%28cdr%20args%29%29%20before%20calling%20car%20on%20it%2C%20returning%20%27%28%29%20or%20an%20eval-error%20when%20no%20else%20branch%20is%20provided.) --- #### 🟡 Moderate **`__not-found__` sentinel leaks into user namespace** (`lib/reader/eval.slisp:21-22`) ```lisp (define (frame-lookup frame name) (if (null? frame) '__not-found__ ``` If user code ever binds or compares against the symbol `__not-found__`, `env-lookup` will return it as a found binding instead of the user's value. A tagged list like `(list '__not-found__)` would be unambiguous (users can't construct this without knowing to), at the cost of a slightly more complex equality check. --- **`bootstrap-eval` silently ignores errors** (`lib/reader/eval.slisp:504-513`) ```lisp (let result (do-define-all ev env (cdr expr)) (if (eval-error? result) env ; <-- silently continues with old env (bootstrap-eval ev (car result) (cdr exprs)))) ``` If a bootstrap definition fails, execution continues with the old environment and the error is swallowed. A bug in `map`/`filter`/`fold`/`apply`/`not` would produce a mysteriously broken evaluator with no indication of what went wrong. --- **`do-define` discards the new environment** (`lib/reader/eval.slisp:213-218`) ```lisp (begin (env-define env name val) ; returns new env, discarded val) ``` `env-define` returns a new environment (immutable-style), but `begin` returns `val` and the updated env is lost. This means `do-define` within `self-eval` (single-expression path) does not actually persist variable bindings. This is worked around correctly by `do-define-all` in `do-eval-all`, but using `seval` directly for a `define` expression will silently not define anything. --- #### 🟢 Minor / Polish **Missing test coverage for key edge cases** (`tests/lisp/reader/eval.slisp`) Tests that are absent but would catch the above issues: - `apply` with 5+ args - `(if #f 1)` — one-armed if - `(map (lambda (x) (* x x)) '(1 2 3))` — lambda (not named fn) with map - `(not 42)` — truthy non-boolean - Calling a non-function: `(seval-string "(1 2 3)")` should return an error - Too-few-args: `(seval-string "((lambda (x y) x) 1)")` should error --- **`val->str` silently serializes unknowns as `??`** (`lib/reader/eval.slisp:329-339`) When a symbol isn't in `builtin-name-map` (e.g., a user-defined name), it becomes `"??"` in the serialized form. This is unavoidable for the string-round-trip delegation strategy, but there's no comment explaining why. Also, passing a closure to a builtin (e.g. `(length some-closure)`) would serialize incorrectly. A code comment would prevent future confusion. --- **`do-cond` returns `'()` on no match** (`lib/reader/eval.slisp:179`) ```lisp (define (do-cond ev env clauses) (if (null? clauses) '() ``` When no clause matches and there's no `else`, the result is `'()` (empty list). In standard Scheme this is "unspecified," but `'()` is a real value that could be mistaken for a meaningful result. Returning `(make-eval-error "cond: no matching clause")` would be safer. --- **`build-name-map` calls `eval-with-errors`/`ok?`/`ok-value` — undocumented host dependencies** (`lib/reader/eval.slisp:281-285`) These are host-provided functions not in `builtin-names`. A comment listing what host functions this module depends on (beyond the registered builtins) would help future maintainers. --- ### Justfile The new `reader-test` block (`justfile:131-145`) is correct — it chains all four reader components properly and reuses the shared `$tmp` file with proper cleanup. The pattern is consistent with the existing reader tests. No issues. --- ### Verdict The architecture and overall approach are excellent — this is a well-structured, readable self-hosted evaluator. The two critical issues (`apply` arg truncation and one-armed `if` crash) should be fixed before merge. The silent error swallowing in `bootstrap-eval` and the discarded environment in `do-define` are worth addressing too. The test gaps are the easiest wins since the framework is already in place.

claude[bot] commented 2026-03-28 03:14:32 +00:00 (Migrated from github.com)

Copy link

Test comment - please ignore

Test comment - please ignore

claude[bot] commented 2026-03-28 03:15:00 +00:00 (Migrated from github.com)

Copy link

SeqLisp

A Lisp interpreter written in Seq, a stack-based concatenative programming language.

Requirements

• seqc - The Seq compiler (install from patch-seq)
• just - Command runner (optional, but recommended)

Installation

# Clone and build
git clone https://github.com/navicore/seq-lisp
cd seq-lisp
just build

# Install to ~/.local/bin (includes LSP server)
just install

# Or install to custom prefix
PREFIX=/usr/local just install

This installs:

• seqlisp - The interpreter/REPL
• seqlisp-lsp - Language server for editor integration
• Library files to $PREFIX/share/seqlisp/

Learning SeqLisp

New to Lisp? Try Lisplings — interactive exercises that teach Lisp concepts through hands-on practice with instant feedback.

cargo install lisplings
lisplings init my-lisplings
cd my-lisplings
lisplings

Quick Start

# Run the REPL
just repl

# Or after installing:
seqlisp

Project Structure

seq-lisp/
├── src/              # Seq source code (the interpreter)
│   ├── tokenizer.seq # Lexical analysis
│   ├── parser.seq    # S-expression parser
│   ├── sexpr.seq     # S-expression data types
│   ├── eval.seq      # Evaluator with environments
│   └── repl.seq      # Interactive REPL
├── tests/
│   ├── seq/          # Seq unit tests (test_*.seq)
│   ├── lisp/         # Lisp test suites
│   │   ├── core/           # Arithmetic, types, strings, floats, JSON
│   │   ├── functions/      # Closures, higher-order, recursion
│   │   ├── special_forms/  # if, let, begin, quote, cond
│   │   ├── macros/         # defmacro, quasiquote, gensym
│   │   └── edge_cases/     # Parser edge cases, error suggestions
│   └── lsp/          # LSP integration tests
├── lib/
│   └── test.slisp     # Lisp test framework
├── examples/         # Lisp programs
├── justfile          # Build commands
└── README.md

Commands

just build        # Build the REPL
just install      # Install seqlisp and seqlisp-lsp to ~/.local
just uninstall    # Remove installed files
just repl         # Run the REPL
just test         # Run Seq unit tests
just lisp-test    # Run Lisp test suite
just lsp-test     # Run LSP integration tests
just examples     # Run all Lisp examples
just clean        # Remove build artifacts
just ci           # Run all tests and build

Lisp Features

SeqLisp supports:

• Arithmetic: +, -, *, /, abs, min, max, modulo
• Comparisons: <, >, <=, >=, =
• Booleans: #t, #f
• Definitions: define, lambda (with variadic support and TCO), let
• Conditionals: if, cond
• Lists: cons, car, cdr, list, quote ('), append, reverse, length, nth, last, take, drop
• Higher-order: map, filter, fold, apply
• Predicates: null?, number?, symbol?, list?, boolean?, equal?
• Macros: defmacro, quasiquote (`), unquote (,), splice (,@), gensym
• Sequencing: begin
• Error Handling: try (returns (ok value) or (error message)), symbol suggestions for typos
• I/O: print, exit

Example

;; Define factorial
(define factorial
  (lambda (n)
    (if (<= n 1)
        1
        (* n (factorial (- n 1))))))

(print (factorial 5))  ;; 120

List Utilities

;; Length of a list
(length '(1 2 3 4 5))    ;; => 5

;; Get nth element (0-indexed)
(nth 2 '(a b c d e))     ;; => c

;; Get last element
(last '(1 2 3 4 5))      ;; => 5

;; Take first n elements
(take 3 '(a b c d e))    ;; => (a b c)

;; Drop first n elements
(drop 2 '(a b c d e))    ;; => (c d e)

Higher-Order Functions

;; Map: transform each element
(map (lambda (x) (* x x)) '(1 2 3 4 5))
;; => (1 4 9 16 25)

;; Filter: keep elements matching predicate
(filter (lambda (x) (> x 2)) '(1 2 3 4 5))
;; => (3 4 5)

;; Fold: reduce list to single value
(fold (lambda (acc x) (+ acc x)) 0 '(1 2 3 4 5))
;; => 15

Macros

;; Define a simple macro
(defmacro (when cond body)
  `(if ,cond ,body '()))

(when #t 'yes)           ;; => yes
(when #f 'yes)           ;; => ()

;; Unless macro (opposite of when)
(defmacro (unless cond body)
  `(if ,cond '() ,body))

(unless #f 42)           ;; => 42

;; Short-circuit and/or
(defmacro (and2 a b) `(if ,a ,b #f))
(defmacro (or2 a b) `(if ,a #t ,b))

;; Use gensym for hygiene (unique symbols)
(gensym 0)               ;; => g0
(gensym 'temp 1)         ;; => temp1

Editor Integration

Neovim

Install seq-lisp.nvim for syntax highlighting and LSP diagnostics:

-- lazy.nvim
{
  "navicore/seq-lisp.nvim",
  ft = "seqlisp",
  opts = {},
}

The plugin expects seqlisp-lsp to be in your PATH (installed via just install).

Documentation

• Architecture - How the interpreter works
• Roadmap - Future plans and vision

License

MIT

[](https://github.com/navicore/seq-lisp/actions/workflows/ci.yml) [](https://github.com/navicore/seq-lisp/actions/workflows/release.yml) # SeqLisp A Lisp interpreter written in [Seq](https://github.com/navicore/patch-seq), a stack-based concatenative programming language. ## Requirements - `seqc` - The Seq compiler (install from [patch-seq](https://github.com/navicore/patch-seq)) - `just` - Command runner (optional, but recommended) ## Installation ```bash # Clone and build git clone https://github.com/navicore/seq-lisp cd seq-lisp just build # Install to ~/.local/bin (includes LSP server) just install # Or install to custom prefix PREFIX=/usr/local just install ``` This installs: - `seqlisp` - The interpreter/REPL - `seqlisp-lsp` - Language server for editor integration - Library files to `$PREFIX/share/seqlisp/` ## Learning SeqLisp New to Lisp? Try [Lisplings](https://github.com/navicore/lisplings) — interactive exercises that teach Lisp concepts through hands-on practice with instant feedback. ```bash cargo install lisplings lisplings init my-lisplings cd my-lisplings lisplings ``` ## Quick Start ```bash # Run the REPL just repl # Or after installing: seqlisp ``` ## Project Structure ``` seq-lisp/ ├── src/ # Seq source code (the interpreter) │ ├── tokenizer.seq # Lexical analysis │ ├── parser.seq # S-expression parser │ ├── sexpr.seq # S-expression data types │ ├── eval.seq # Evaluator with environments │ └── repl.seq # Interactive REPL ├── tests/ │ ├── seq/ # Seq unit tests (test_*.seq) │ ├── lisp/ # Lisp test suites │ │ ├── core/ # Arithmetic, types, strings, floats, JSON │ │ ├── functions/ # Closures, higher-order, recursion │ │ ├── special_forms/ # if, let, begin, quote, cond │ │ ├── macros/ # defmacro, quasiquote, gensym │ │ └── edge_cases/ # Parser edge cases, error suggestions │ └── lsp/ # LSP integration tests ├── lib/ │ └── test.slisp # Lisp test framework ├── examples/ # Lisp programs ├── justfile # Build commands └── README.md ``` ## Commands ```bash just build # Build the REPL just install # Install seqlisp and seqlisp-lsp to ~/.local just uninstall # Remove installed files just repl # Run the REPL just test # Run Seq unit tests just lisp-test # Run Lisp test suite just lsp-test # Run LSP integration tests just examples # Run all Lisp examples just clean # Remove build artifacts just ci # Run all tests and build ``` ## Lisp Features SeqLisp supports: - **Arithmetic**: `+`, `-`, `*`, `/`, `abs`, `min`, `max`, `modulo` - **Comparisons**: `<`, `>`, `<=`, `>=`, `=` - **Booleans**: `#t`, `#f` - **Definitions**: `define`, `lambda` (with variadic support and TCO), `let` - **Conditionals**: `if`, `cond` - **Lists**: `cons`, `car`, `cdr`, `list`, `quote` (`'`), `append`, `reverse`, `length`, `nth`, `last`, `take`, `drop` - **Higher-order**: `map`, `filter`, `fold`, `apply` - **Predicates**: `null?`, `number?`, `symbol?`, `list?`, `boolean?`, `equal?` - **Macros**: `defmacro`, quasiquote (`` ` ``), unquote (`,`), splice (`,@`), `gensym` - **Sequencing**: `begin` - **Error Handling**: `try` (returns `(ok value)` or `(error message)`), symbol suggestions for typos - **I/O**: `print`, `exit` ### Example ```lisp ;; Define factorial (define factorial (lambda (n) (if (<= n 1) 1 (* n (factorial (- n 1)))))) (print (factorial 5)) ;; 120 ``` ### List Utilities ```lisp ;; Length of a list (length '(1 2 3 4 5)) ;; => 5 ;; Get nth element (0-indexed) (nth 2 '(a b c d e)) ;; => c ;; Get last element (last '(1 2 3 4 5)) ;; => 5 ;; Take first n elements (take 3 '(a b c d e)) ;; => (a b c) ;; Drop first n elements (drop 2 '(a b c d e)) ;; => (c d e) ``` ### Higher-Order Functions ```lisp ;; Map: transform each element (map (lambda (x) (* x x)) '(1 2 3 4 5)) ;; => (1 4 9 16 25) ;; Filter: keep elements matching predicate (filter (lambda (x) (> x 2)) '(1 2 3 4 5)) ;; => (3 4 5) ;; Fold: reduce list to single value (fold (lambda (acc x) (+ acc x)) 0 '(1 2 3 4 5)) ;; => 15 ``` ### Macros ```lisp ;; Define a simple macro (defmacro (when cond body) `(if ,cond ,body '())) (when #t 'yes) ;; => yes (when #f 'yes) ;; => () ;; Unless macro (opposite of when) (defmacro (unless cond body) `(if ,cond '() ,body)) (unless #f 42) ;; => 42 ;; Short-circuit and/or (defmacro (and2 a b) `(if ,a ,b #f)) (defmacro (or2 a b) `(if ,a #t ,b)) ;; Use gensym for hygiene (unique symbols) (gensym 0) ;; => g0 (gensym 'temp 1) ;; => temp1 ``` ## Editor Integration ### Neovim Install [seq-lisp.nvim](https://github.com/navicore/seq-lisp.nvim) for syntax highlighting and LSP diagnostics: ```lua -- lazy.nvim { "navicore/seq-lisp.nvim", ft = "seqlisp", opts = {}, } ``` The plugin expects `seqlisp-lsp` to be in your PATH (installed via `just install`). ## Documentation - [Architecture](docs/ARCHITECTURE.md) - How the interpreter works - [Roadmap](docs/ROADMAP.md) - Future plans and vision ## License MIT

claude[bot] commented 2026-03-28 03:15:51 +00:00 (Migrated from github.com)

Copy link

test comment 2 - will delete

test comment 2 - will delete

claude[bot] commented 2026-03-28 03:16:54 +00:00 (Migrated from github.com)

Copy link

Part 1 of PR Review: Self-Hosted Evaluator. This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky, and the ev-threading pattern is a clean solution. Overall the code is well-structured and well-commented. A few issues worth addressing: (1) Bug - String escaping in val->str: Strings are serialized by wrapping in double-quotes without escaping internal quotes or backslashes. A string-escape helper is needed. (2) call-builtin serialization round-trip: Every builtin call incurs a parse+eval cycle on the host (performance), and val->str returns ?? for unrepresentable values - an explicit make-eval-error would be safer. (3) apply hard-coded arity limit 0-4: Returns a plain error list for 5+ args that does not propagate like a real evaluator error - should go through make-eval-error. (4) do-define discards updated environment: env-define return value is discarded in scalar branch - bare seval with define silently has no effect. (5) Sentinel collision risk: user code defining not-found, closure, or macro variables would collide with internals. (6) Missing tests: defmacro has zero tests, bind-params varargs untested, no test for apply 5+ args error, no test for strings with special chars, /tmp test files will race under parallel runs. The string-escaping bug and do-define env issue are the most important things to address before merging.

Part 1 of PR Review: Self-Hosted Evaluator. This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky, and the ev-threading pattern is a clean solution. Overall the code is well-structured and well-commented. A few issues worth addressing: (1) Bug - String escaping in val->str: Strings are serialized by wrapping in double-quotes without escaping internal quotes or backslashes. A string-escape helper is needed. (2) call-builtin serialization round-trip: Every builtin call incurs a parse+eval cycle on the host (performance), and val->str returns ?? for unrepresentable values - an explicit make-eval-error would be safer. (3) apply hard-coded arity limit 0-4: Returns a plain error list for 5+ args that does not propagate like a real evaluator error - should go through make-eval-error. (4) do-define discards updated environment: env-define return value is discarded in scalar branch - bare seval with define silently has no effect. (5) Sentinel collision risk: user code defining __not-found__, __closure__, or __macro__ variables would collide with internals. (6) Missing tests: defmacro has zero tests, bind-params varargs untested, no test for apply 5+ args error, no test for strings with special chars, /tmp test files will race under parallel runs. The string-escaping bug and do-define env issue are the most important things to address before merging.

claude[bot] commented 2026-03-28 03:17:12 +00:00 (Migrated from github.com)

Copy link

PR Review

## PR Review

claude[bot] commented 2026-03-28 03:17:18 +00:00 (Migrated from github.com)

Copy link

PR Review: Self-Hosted Evaluator

## PR Review: Self-Hosted Evaluator

claude[bot] commented 2026-03-28 03:17:30 +00:00 (Migrated from github.com)

Copy link

PR Review: Self-Hosted Evaluator

This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky.

## PR Review: Self-Hosted Evaluator This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky.

claude[bot] commented 2026-03-28 03:19:13 +00:00 (Migrated from github.com)

Copy link

PR Review: Self-Hosted Evaluator

This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky, and the ev-threading pattern is a clean solution. Overall the code is well-structured and well-commented. A few issues worth addressing below.

---

Bug: String escaping in val->str / list-items->str

Strings are serialized by wrapping in double-quotes without escaping internal quotes or backslashes. If a string argument contains a double-quote or backslash, the serialized expression will be syntactically malformed, causing eval-with-errors to fail. A string-escape helper is needed before values can safely round-trip through the host evaluator.

Design concern: call-builtin serialization round-trip

• Performance - Every builtin call incurs a parse + eval cycle on the host. For map over large lists, this compounds quickly.
• ?? fallback - val->str returns ?? for values it cannot represent. If a closure or user-defined symbol sneaks through to a builtin call, the host receives a malformed expression. An explicit make-eval-error here would be safer than silently emitting ??.

apply hard-coded arity limit (0-4)

The bootstrap apply returns a plain (list 'error ...) for 5+ args. This does not propagate like a real evaluator error - the caller receives it as a normal list value. It should go through make-eval-error so the standard error propagation path handles it.

do-define discards the updated environment

In the scalar do-define branch, env-define is called but its return value (the new environment) is discarded. define in the single-expression self-eval path silently has no effect. The do-eval-all / do-define-all path correctly threads the environment, so seval-string works fine, but bare seval with a define expression will silently not define anything. At minimum a comment documenting this asymmetry would prevent confusion.

Sentinel collision risk

If user code defines a variable named not-found, frame-lookup will treat it as the sentinel and skip it. Same risk for lists starting with closure or macro. Acceptable for a bootstrapping evaluator, but worth a comment noting these names are reserved.

Missing test coverage

• Macros - defmacro is fully implemented but has zero tests
• Varargs - bind-params handles (. rest) syntax but it is not tested
• apply with 5+ args - the documented arity limit should have a test confirming the error case
• Strings with special characters - a test with embedded quotes would catch the escaping bug
• /tmp test files - fixed filenames in /tmp will race under parallel test runs; consider gensym-derived names

---

Summary

Area	Status
Architecture / design	Solid - ev-threading cleanly solves the forward-reference constraint
String serialization escaping	Bug - strings with embedded quotes produce malformed host calls
apply error propagation	Issue - error value not integrated with the evaluator error path
do-define env threading	Subtle bug - bare seval with define silently does nothing
Test coverage	Good breadth; missing macros, varargs, and edge cases noted above
Performance	String round-trip per builtin call is expensive; fine for bootstrap, worth a TODO

The core idea and execution are solid. The string-escaping bug and do-define env issue are the most important things to address before merging.

**PR Review: Self-Hosted Evaluator** This is a significant milestone. Implementing a self-hosted evaluator in a language that lacks forward references is genuinely tricky, and the ev-threading pattern is a clean solution. Overall the code is well-structured and well-commented. A few issues worth addressing below. --- **Bug: String escaping in val->str / list-items->str** Strings are serialized by wrapping in double-quotes without escaping internal quotes or backslashes. If a string argument contains a double-quote or backslash, the serialized expression will be syntactically malformed, causing eval-with-errors to fail. A string-escape helper is needed before values can safely round-trip through the host evaluator. **Design concern: call-builtin serialization round-trip** - **Performance** - Every builtin call incurs a parse + eval cycle on the host. For map over large lists, this compounds quickly. - **?? fallback** - val->str returns ?? for values it cannot represent. If a closure or user-defined symbol sneaks through to a builtin call, the host receives a malformed expression. An explicit make-eval-error here would be safer than silently emitting ??. **apply hard-coded arity limit (0-4)** The bootstrap apply returns a plain (list 'error ...) for 5+ args. This does not propagate like a real evaluator error - the caller receives it as a normal list value. It should go through make-eval-error so the standard error propagation path handles it. **do-define discards the updated environment** In the scalar do-define branch, env-define is called but its return value (the new environment) is discarded. define in the single-expression self-eval path silently has no effect. The do-eval-all / do-define-all path correctly threads the environment, so seval-string works fine, but bare seval with a define expression will silently not define anything. At minimum a comment documenting this asymmetry would prevent confusion. **Sentinel collision risk** If user code defines a variable named __not-found__, frame-lookup will treat it as the sentinel and skip it. Same risk for lists starting with __closure__ or __macro__. Acceptable for a bootstrapping evaluator, but worth a comment noting these names are reserved. **Missing test coverage** - **Macros** - defmacro is fully implemented but has zero tests - **Varargs** - bind-params handles (. rest) syntax but it is not tested - **apply with 5+ args** - the documented arity limit should have a test confirming the error case - **Strings with special characters** - a test with embedded quotes would catch the escaping bug - **/tmp test files** - fixed filenames in /tmp will race under parallel test runs; consider gensym-derived names --- **Summary** | Area | Status | |------|--------| | Architecture / design | Solid - ev-threading cleanly solves the forward-reference constraint | | String serialization escaping | **Bug** - strings with embedded quotes produce malformed host calls | | apply error propagation | **Issue** - error value not integrated with the evaluator error path | | do-define env threading | **Subtle bug** - bare seval with define silently does nothing | | Test coverage | Good breadth; missing macros, varargs, and edge cases noted above | | Performance | String round-trip per builtin call is expensive; fine for bootstrap, worth a TODO | The core idea and execution are solid. The string-escaping bug and do-define env issue are the most important things to address before merging.

claude[bot] commented 2026-03-28 03:39:05 +00:00 (Migrated from github.com)

Copy link

PR Review: Self-hosted SeqLisp Evaluator - The PR is a well-structured meta-circular evaluator. The bootstrap pattern is elegant. BUGS: 1. Incomplete string escaping in escape-string-char (High) - Only backslash and double-quote are escaped. Strings with newline/CR/tab/null will be embedded raw into eval-with-errors calls, causing host parser failures. 2. do-define in self-eval silently discards env update (Low, documented). 3. list-items->str unconditionally quotes nested lists (Low). DESIGN: 4. Self-hosted apply capped at 4 arguments (Medium) - 5+ args error. 5. Forgeable sentinel symbols not-found, closure, macro (Low). 6. build-name-map uses eval-with-errors at load time (Low). MISSING TESTS: 7. Strings with control chars through call-builtin. 8. Mutual recursion. 9. apply valid arities 0-4. 10. Error propagation through map/filter/fold. MINOR: bootstrap-eval and do-eval-all nearly identical; /tmp/seqlisp-seval-test.slisp hardcoded causing parallel test races; make-seval-context stateful API would help. SUMMARY: String escaping (High), apply 4-arg cap (Medium), missing tests (Medium), rest Low. Architecture is sound. Fixing string escaping and apply arity limit are the two priorities before merging.

PR Review: Self-hosted SeqLisp Evaluator - The PR is a well-structured meta-circular evaluator. The bootstrap pattern is elegant. BUGS: 1. Incomplete string escaping in escape-string-char (High) - Only backslash and double-quote are escaped. Strings with newline/CR/tab/null will be embedded raw into eval-with-errors calls, causing host parser failures. 2. do-define in self-eval silently discards env update (Low, documented). 3. list-items->str unconditionally quotes nested lists (Low). DESIGN: 4. Self-hosted apply capped at 4 arguments (Medium) - 5+ args error. 5. Forgeable sentinel symbols __not-found__, __closure__, __macro__ (Low). 6. build-name-map uses eval-with-errors at load time (Low). MISSING TESTS: 7. Strings with control chars through call-builtin. 8. Mutual recursion. 9. apply valid arities 0-4. 10. Error propagation through map/filter/fold. MINOR: bootstrap-eval and do-eval-all nearly identical; /tmp/seqlisp-seval-test.slisp hardcoded causing parallel test races; make-seval-context stateful API would help. SUMMARY: String escaping (High), apply 4-arg cap (Medium), missing tests (Medium), rest Low. Architecture is sound. Fixing string escaping and apply arity limit are the two priorities before merging.

claude[bot] commented 2026-03-28 03:56:55 +00:00 (Migrated from github.com)

Copy link

test

test

claude[bot] commented 2026-03-28 03:57:34 +00:00 (Migrated from github.com)

Copy link

SeqLisp

A Lisp interpreter written in Seq, a stack-based concatenative programming language.

Requirements

• seqc - The Seq compiler (install from patch-seq)
• just - Command runner (optional, but recommended)

Installation

# Clone and build
git clone https://github.com/navicore/seq-lisp
cd seq-lisp
just build

# Install to ~/.local/bin (includes LSP server)
just install

# Or install to custom prefix
PREFIX=/usr/local just install

This installs:

• seqlisp - The interpreter/REPL
• seqlisp-lsp - Language server for editor integration
• Library files to $PREFIX/share/seqlisp/

Learning SeqLisp

New to Lisp? Try Lisplings — interactive exercises that teach Lisp concepts through hands-on practice with instant feedback.

cargo install lisplings
lisplings init my-lisplings
cd my-lisplings
lisplings

Quick Start

# Run the REPL
just repl

# Or after installing:
seqlisp

Project Structure

seq-lisp/
├── src/              # Seq source code (the interpreter)
│   ├── tokenizer.seq # Lexical analysis
│   ├── parser.seq    # S-expression parser
│   ├── sexpr.seq     # S-expression data types
│   ├── eval.seq      # Evaluator with environments
│   └── repl.seq      # Interactive REPL
├── tests/
│   ├── seq/          # Seq unit tests (test_*.seq)
│   ├── lisp/         # Lisp test suites
│   │   ├── core/           # Arithmetic, types, strings, floats, JSON
│   │   ├── functions/      # Closures, higher-order, recursion
│   │   ├── special_forms/  # if, let, begin, quote, cond
│   │   ├── macros/         # defmacro, quasiquote, gensym
│   │   └── edge_cases/     # Parser edge cases, error suggestions
│   └── lsp/          # LSP integration tests
├── lib/
│   └── test.slisp     # Lisp test framework
├── examples/         # Lisp programs
├── justfile          # Build commands
└── README.md

Commands

just build        # Build the REPL
just install      # Install seqlisp and seqlisp-lsp to ~/.local
just uninstall    # Remove installed files
just repl         # Run the REPL
just test         # Run Seq unit tests
just lisp-test    # Run Lisp test suite
just lsp-test     # Run LSP integration tests
just examples     # Run all Lisp examples
just clean        # Remove build artifacts
just ci           # Run all tests and build

Lisp Features

SeqLisp supports:

• Arithmetic: +, -, *, /, abs, min, max, modulo
• Comparisons: <, >, <=, >=, =
• Booleans: #t, #f
• Definitions: define, lambda (with variadic support and TCO), let
• Conditionals: if, cond
• Lists: cons, car, cdr, list, quote ('), append, reverse, length, nth, last, take, drop
• Higher-order: map, filter, fold, apply
• Predicates: null?, number?, symbol?, list?, boolean?, equal?
• Macros: defmacro, quasiquote (`), unquote (,), splice (,@), gensym
• Sequencing: begin
• Error Handling: try (returns (ok value) or (error message)), symbol suggestions for typos
• I/O: print, exit

Example

;; Define factorial
(define factorial
  (lambda (n)
    (if (<= n 1)
        1
        (* n (factorial (- n 1))))))

(print (factorial 5))  ;; 120

List Utilities

;; Length of a list
(length '(1 2 3 4 5))    ;; => 5

;; Get nth element (0-indexed)
(nth 2 '(a b c d e))     ;; => c

;; Get last element
(last '(1 2 3 4 5))      ;; => 5

;; Take first n elements
(take 3 '(a b c d e))    ;; => (a b c)

;; Drop first n elements
(drop 2 '(a b c d e))    ;; => (c d e)

Higher-Order Functions

;; Map: transform each element
(map (lambda (x) (* x x)) '(1 2 3 4 5))
;; => (1 4 9 16 25)

;; Filter: keep elements matching predicate
(filter (lambda (x) (> x 2)) '(1 2 3 4 5))
;; => (3 4 5)

;; Fold: reduce list to single value
(fold (lambda (acc x) (+ acc x)) 0 '(1 2 3 4 5))
;; => 15

Macros

;; Define a simple macro
(defmacro (when cond body)
  `(if ,cond ,body '()))

(when #t 'yes)           ;; => yes
(when #f 'yes)           ;; => ()

;; Unless macro (opposite of when)
(defmacro (unless cond body)
  `(if ,cond '() ,body))

(unless #f 42)           ;; => 42

;; Short-circuit and/or
(defmacro (and2 a b) `(if ,a ,b #f))
(defmacro (or2 a b) `(if ,a #t ,b))

;; Use gensym for hygiene (unique symbols)
(gensym 0)               ;; => g0
(gensym 'temp 1)         ;; => temp1

Editor Integration

Neovim

Install seq-lisp.nvim for syntax highlighting and LSP diagnostics:

-- lazy.nvim
{
  "navicore/seq-lisp.nvim",
  ft = "seqlisp",
  opts = {},
}

The plugin expects seqlisp-lsp to be in your PATH (installed via just install).

Documentation

• Architecture - How the interpreter works
• Roadmap - Future plans and vision

License

MIT

[](https://github.com/navicore/seq-lisp/actions/workflows/ci.yml) [](https://github.com/navicore/seq-lisp/actions/workflows/release.yml) # SeqLisp A Lisp interpreter written in [Seq](https://github.com/navicore/patch-seq), a stack-based concatenative programming language. ## Requirements - `seqc` - The Seq compiler (install from [patch-seq](https://github.com/navicore/patch-seq)) - `just` - Command runner (optional, but recommended) ## Installation ```bash # Clone and build git clone https://github.com/navicore/seq-lisp cd seq-lisp just build # Install to ~/.local/bin (includes LSP server) just install # Or install to custom prefix PREFIX=/usr/local just install ``` This installs: - `seqlisp` - The interpreter/REPL - `seqlisp-lsp` - Language server for editor integration - Library files to `$PREFIX/share/seqlisp/` ## Learning SeqLisp New to Lisp? Try [Lisplings](https://github.com/navicore/lisplings) — interactive exercises that teach Lisp concepts through hands-on practice with instant feedback. ```bash cargo install lisplings lisplings init my-lisplings cd my-lisplings lisplings ``` ## Quick Start ```bash # Run the REPL just repl # Or after installing: seqlisp ``` ## Project Structure ``` seq-lisp/ ├── src/ # Seq source code (the interpreter) │ ├── tokenizer.seq # Lexical analysis │ ├── parser.seq # S-expression parser │ ├── sexpr.seq # S-expression data types │ ├── eval.seq # Evaluator with environments │ └── repl.seq # Interactive REPL ├── tests/ │ ├── seq/ # Seq unit tests (test_*.seq) │ ├── lisp/ # Lisp test suites │ │ ├── core/ # Arithmetic, types, strings, floats, JSON │ │ ├── functions/ # Closures, higher-order, recursion │ │ ├── special_forms/ # if, let, begin, quote, cond │ │ ├── macros/ # defmacro, quasiquote, gensym │ │ └── edge_cases/ # Parser edge cases, error suggestions │ └── lsp/ # LSP integration tests ├── lib/ │ └── test.slisp # Lisp test framework ├── examples/ # Lisp programs ├── justfile # Build commands └── README.md ``` ## Commands ```bash just build # Build the REPL just install # Install seqlisp and seqlisp-lsp to ~/.local just uninstall # Remove installed files just repl # Run the REPL just test # Run Seq unit tests just lisp-test # Run Lisp test suite just lsp-test # Run LSP integration tests just examples # Run all Lisp examples just clean # Remove build artifacts just ci # Run all tests and build ``` ## Lisp Features SeqLisp supports: - **Arithmetic**: `+`, `-`, `*`, `/`, `abs`, `min`, `max`, `modulo` - **Comparisons**: `<`, `>`, `<=`, `>=`, `=` - **Booleans**: `#t`, `#f` - **Definitions**: `define`, `lambda` (with variadic support and TCO), `let` - **Conditionals**: `if`, `cond` - **Lists**: `cons`, `car`, `cdr`, `list`, `quote` (`'`), `append`, `reverse`, `length`, `nth`, `last`, `take`, `drop` - **Higher-order**: `map`, `filter`, `fold`, `apply` - **Predicates**: `null?`, `number?`, `symbol?`, `list?`, `boolean?`, `equal?` - **Macros**: `defmacro`, quasiquote (`` ` ``), unquote (`,`), splice (`,@`), `gensym` - **Sequencing**: `begin` - **Error Handling**: `try` (returns `(ok value)` or `(error message)`), symbol suggestions for typos - **I/O**: `print`, `exit` ### Example ```lisp ;; Define factorial (define factorial (lambda (n) (if (<= n 1) 1 (* n (factorial (- n 1)))))) (print (factorial 5)) ;; 120 ``` ### List Utilities ```lisp ;; Length of a list (length '(1 2 3 4 5)) ;; => 5 ;; Get nth element (0-indexed) (nth 2 '(a b c d e)) ;; => c ;; Get last element (last '(1 2 3 4 5)) ;; => 5 ;; Take first n elements (take 3 '(a b c d e)) ;; => (a b c) ;; Drop first n elements (drop 2 '(a b c d e)) ;; => (c d e) ``` ### Higher-Order Functions ```lisp ;; Map: transform each element (map (lambda (x) (* x x)) '(1 2 3 4 5)) ;; => (1 4 9 16 25) ;; Filter: keep elements matching predicate (filter (lambda (x) (> x 2)) '(1 2 3 4 5)) ;; => (3 4 5) ;; Fold: reduce list to single value (fold (lambda (acc x) (+ acc x)) 0 '(1 2 3 4 5)) ;; => 15 ``` ### Macros ```lisp ;; Define a simple macro (defmacro (when cond body) `(if ,cond ,body '())) (when #t 'yes) ;; => yes (when #f 'yes) ;; => () ;; Unless macro (opposite of when) (defmacro (unless cond body) `(if ,cond '() ,body)) (unless #f 42) ;; => 42 ;; Short-circuit and/or (defmacro (and2 a b) `(if ,a ,b #f)) (defmacro (or2 a b) `(if ,a #t ,b)) ;; Use gensym for hygiene (unique symbols) (gensym 0) ;; => g0 (gensym 'temp 1) ;; => temp1 ``` ## Editor Integration ### Neovim Install [seq-lisp.nvim](https://github.com/navicore/seq-lisp.nvim) for syntax highlighting and LSP diagnostics: ```lua -- lazy.nvim { "navicore/seq-lisp.nvim", ft = "seqlisp", opts = {}, } ``` The plugin expects `seqlisp-lsp` to be in your PATH (installed via `just install`). ## Documentation - [Architecture](docs/ARCHITECTURE.md) - How the interpreter works - [Roadmap](docs/ROADMAP.md) - Future plans and vision ## License MIT

claude[bot] commented 2026-03-28 03:57:39 +00:00 (Migrated from github.com)

Copy link

PLACEHOLDER_REVIEW_BODY

PLACEHOLDER_REVIEW_BODY

claude[bot] commented 2026-03-28 04:03:37 +00:00 (Migrated from github.com)

Copy link

Note: Full review comment posting was attempted but the sandbox is restricting multi-line heredocs and file writes containing markdown headings. Please see the full review text provided by the user for the complete PR review.

Note: Full review comment posting was attempted but the sandbox is restricting multi-line heredocs and file writes containing markdown headings. Please see the full review text provided by the user for the complete PR review.

claude[bot] commented 2026-03-28 04:05:36 +00:00 (Migrated from github.com)

Copy link

PR Review: Self-hosted SeqLisp evaluator

This is a substantial and well-structured piece of work. The ev parameter threading pattern to break mutual recursion without forward references is clever and consistent throughout. The comments documenting limitations (reserved names, seval env-discard, apply arity cap) are appreciated. Test coverage is comprehensive. Feedback below, ordered by severity.

BUG: Builtins bypass environment lookup

In do-apply, (builtin? head) fires before env lookup whenever the head symbol is in builtin-names. Any local rebinding of a builtin is silently ignored. For example, (let car ...) shadowing the builtin is ignored -- the host builtin fires instead. Standard Lisp semantics require that env lookup always wins. The fix is to look up head in the env first and fall back to the builtin path only on a miss. This is a semantic correctness bug worth fixing before merge.

Dead code in do-define (value case, around line 251)

(begin (env-define env name val) val)

env-define has no side effects -- it returns a new env. The call above discards the return value and does nothing. The comment documenting that seval with define does not persist is correct, but the dead call is misleading. Remove it or replace with a comment.

seval-string and sload always start from initial-env

Every call gets a fresh initial-env, so definitions from one call do not carry over to the next. Callers who load a library then eval user code as separate calls silently lose the library. This limitation should be documented on both functions, or they should accept an optional env parameter.

do-cond only evaluates the second element of each clause

A standard cond clause body supports multiple expressions evaluated in sequence. The current (ev ev env (car (cdr clause))) silently truncates any extras. Either document the restriction or delegate to do-begin.

map and filter in bootstrap are not tail-recursive

The cons after the recursive call in map and filter means they are not TCO-eligible and can stack-overflow on deep lists. fold is already tail-recursive. Worth noting as a known limitation even if not fixed now.

Minor: unserializable emitted inline into host eval expressions

In list-items->str, an unserializable value emits the bare string unserializable into the expression being host-eval'd. The host evaluator treats it as a symbol reference and reports a confusing undefined-symbol error rather than a clear serialization failure. Propagating an error value through call-builtin would give a cleaner diagnostic.

Nit: build-name-map runs eval-with-errors 37 times at load time

Both builtin-names and builtin-name-strings are already in parallel order, so a simple zip avoids all the eval-with-errors calls at load time. No dynamic evaluation needed at module load.

Positive notes:

• The ev threading pattern is the right solution for the no-forward-references constraint and is applied consistently.
• eval-error? bubble-up is handled uniformly across all helpers.
• Bootstrap printing (bootstrap-error ...) instead of swallowing failures is correct.
• escape-string covering all five escape sequences is necessary and correct.
• Test coverage is solid across atoms, special forms, closures, recursion, quasiquote, macros, varargs, and error cases.
• Good callout explaining why apply tests use self-hosted closures rather than host builtins.

The builtins-bypass-env issue is the only semantic correctness bug worth fixing before merge; the rest are improvements or documentation gaps.

PR Review: Self-hosted SeqLisp evaluator This is a substantial and well-structured piece of work. The ev parameter threading pattern to break mutual recursion without forward references is clever and consistent throughout. The comments documenting limitations (reserved names, seval env-discard, apply arity cap) are appreciated. Test coverage is comprehensive. Feedback below, ordered by severity. BUG: Builtins bypass environment lookup In do-apply, (builtin? head) fires before env lookup whenever the head symbol is in builtin-names. Any local rebinding of a builtin is silently ignored. For example, (let car ...) shadowing the builtin is ignored -- the host builtin fires instead. Standard Lisp semantics require that env lookup always wins. The fix is to look up head in the env first and fall back to the builtin path only on a miss. This is a semantic correctness bug worth fixing before merge. Dead code in do-define (value case, around line 251) (begin (env-define env name val) val) env-define has no side effects -- it returns a new env. The call above discards the return value and does nothing. The comment documenting that seval with define does not persist is correct, but the dead call is misleading. Remove it or replace with a comment. seval-string and sload always start from initial-env Every call gets a fresh initial-env, so definitions from one call do not carry over to the next. Callers who load a library then eval user code as separate calls silently lose the library. This limitation should be documented on both functions, or they should accept an optional env parameter. do-cond only evaluates the second element of each clause A standard cond clause body supports multiple expressions evaluated in sequence. The current (ev ev env (car (cdr clause))) silently truncates any extras. Either document the restriction or delegate to do-begin. map and filter in bootstrap are not tail-recursive The cons after the recursive call in map and filter means they are not TCO-eligible and can stack-overflow on deep lists. fold is already tail-recursive. Worth noting as a known limitation even if not fixed now. Minor: __unserializable__ emitted inline into host eval expressions In list-items->str, an unserializable value emits the bare string __unserializable__ into the expression being host-eval'd. The host evaluator treats it as a symbol reference and reports a confusing undefined-symbol error rather than a clear serialization failure. Propagating an error value through call-builtin would give a cleaner diagnostic. Nit: build-name-map runs eval-with-errors 37 times at load time Both builtin-names and builtin-name-strings are already in parallel order, so a simple zip avoids all the eval-with-errors calls at load time. No dynamic evaluation needed at module load. Positive notes: - The ev threading pattern is the right solution for the no-forward-references constraint and is applied consistently. - eval-error? bubble-up is handled uniformly across all helpers. - Bootstrap printing (bootstrap-error ...) instead of swallowing failures is correct. - escape-string covering all five escape sequences is necessary and correct. - Test coverage is solid across atoms, special forms, closures, recursion, quasiquote, macros, varargs, and error cases. - Good callout explaining why apply tests use self-hosted closures rather than host builtins. The builtins-bypass-env issue is the only semantic correctness bug worth fixing before merge; the rest are improvements or documentation gaps.

claude[bot] commented 2026-03-28 04:22:00 +00:00 (Migrated from github.com)

Copy link

Code Review - Self-hosted SeqLisp Evaluator: This is a substantial and well-executed PR. Full review details posted below in a follow-up comment.

Code Review - Self-hosted SeqLisp Evaluator: This is a substantial and well-executed PR. Full review details posted below in a follow-up comment.

claude[bot] commented 2026-03-28 04:22:12 +00:00 (Migrated from github.com)

Copy link

Correctness Issues

1. Builtins shadow user bindings in do-apply

builtin? checks the symbol name before looking it up in the environment. This means user code can never shadow a builtin -- (define (cons a b) ...) would be silently ignored whenever cons is called. Standard Lisp semantics allow shadowing builtins. Since this is a bootstrap evaluator it may be intentional, but it should be documented if so.

2. unserializable silently corrupts builtin calls

In list-items->str and val->str, when a value cannot be serialized (e.g. a closure passed to a builtin), the sentinel unserializable is emitted and the call proceeds. This means (length my-closure) silently becomes (length unserializable) rather than returning an error. Returning (make-eval-error ...) at the call site would be safer.

3. build-name-map uses eval-with-errors to intern strings as symbols

Fragile: relies on the host parser to round-trip a string as a quoted symbol. If any builtin name had special characters, it would be silently dropped from the map. A parallel list of symbol+string pairs without going through the host evaluator would be more robust.

### Correctness Issues **1. Builtins shadow user bindings in do-apply** builtin? checks the symbol name before looking it up in the environment. This means user code can never shadow a builtin -- (define (cons a b) ...) would be silently ignored whenever cons is called. Standard Lisp semantics allow shadowing builtins. Since this is a bootstrap evaluator it may be intentional, but it should be documented if so. **2. __unserializable__ silently corrupts builtin calls** In list-items->str and val->str, when a value cannot be serialized (e.g. a closure passed to a builtin), the sentinel __unserializable__ is emitted and the call proceeds. This means (length my-closure) silently becomes (length __unserializable__) rather than returning an error. Returning (make-eval-error ...) at the call site would be safer. **3. build-name-map uses eval-with-errors to intern strings as symbols** Fragile: relies on the host parser to round-trip a string as a quoted symbol. If any builtin name had special characters, it would be silently dropped from the map. A parallel list of symbol+string pairs without going through the host evaluator would be more robust.

claude[bot] commented 2026-03-28 04:22:31 +00:00 (Migrated from github.com)

Copy link

Design Limitations: (4) seval-string does not carry environment across calls -- always evaluates against initial-env, so definitions from one call do not persist into the next. A note in the file header would help users. (5) apply arity cap of 6 -- consider noting the preferred extension path (varargs + recursion in the self-hosted apply). Minor: (6) Sentinel names not-found / closure / macro should be in a LIMITATIONS section header, not just inline comments. (7) Hardcoded /tmp paths in tests could race on parallel runs -- use gensym or timestamp suffix. (8) bootstrap-eval continues with stale env after errors -- cascade failures are confusing; consider exit 1 on bootstrap failure.

Design Limitations: (4) seval-string does not carry environment across calls -- always evaluates against initial-env, so definitions from one call do not persist into the next. A note in the file header would help users. (5) apply arity cap of 6 -- consider noting the preferred extension path (varargs + recursion in the self-hosted apply). Minor: (6) Sentinel names __not-found__ / __closure__ / __macro__ should be in a LIMITATIONS section header, not just inline comments. (7) Hardcoded /tmp paths in tests could race on parallel runs -- use gensym or timestamp suffix. (8) bootstrap-eval continues with stale env after errors -- cascade failures are confusing; consider exit 1 on bootstrap failure.

claude[bot] commented 2026-03-28 04:22:42 +00:00 (Migrated from github.com)

Copy link

Test coverage is solid -- atoms, arithmetic, quote, if/cond/let/begin, closures, recursion, quasiquote, map/filter/fold/apply/not, error cases, macros, varargs, string escaping, and file loading are all covered. Missing cases worth adding: (a) a test confirming user-defined cons is superseded by the builtin, or a comment documenting that shadowing is intentionally blocked; (b) a test verifying that passing a closure to a host builtin produces an error rather than unserializable propagating silently. GitHub Actions: Pinning to v1.0.78 is good. Permission upgrades (read to write) for pull-requests and issues are required for the review action to post comments -- looks correct. Overall: well-thought-out addition. Main actionable items are documenting (or fixing) the builtin-shadowing behavior and replacing unserializable with an explicit error.

Test coverage is solid -- atoms, arithmetic, quote, if/cond/let/begin, closures, recursion, quasiquote, map/filter/fold/apply/not, error cases, macros, varargs, string escaping, and file loading are all covered. Missing cases worth adding: (a) a test confirming user-defined cons is superseded by the builtin, or a comment documenting that shadowing is intentionally blocked; (b) a test verifying that passing a closure to a host builtin produces an error rather than __unserializable__ propagating silently. GitHub Actions: Pinning to v1.0.78 is good. Permission upgrades (read to write) for pull-requests and issues are required for the review action to post comments -- looks correct. Overall: well-thought-out addition. Main actionable items are documenting (or fixing) the builtin-shadowing behavior and replacing __unserializable__ with an explicit error.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

seq-lisp-2-backport

Ideas from the seq-lisp-2 experiment

  

wontfix

This will not be worked on

No labels

bug

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

seq-lisp-2-backport

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/seq-lisp!68

No description provided.