fix #73

Merged

navicore merged 1 commit from bug-fix-lambda-syntax into main

2026-04-04 21:47:28 +00:00

navicore commented

2026-04-04 21:43:44 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-04-04 21:46:51 +00:00

(Migrated from github.com)

PR Review

Summary

This PR bundles two unrelated changes: a bug fix in eval-define-simple and a design document for future multiline REPL history editing. These would be cleaner as separate PRs.

`src/eval.seq` — Self-recursion fix for `define` + `lambda`

What it does: When (define fact (lambda (n) ...)) is evaluated, the unnamed closure is now promoted to a named closure (with self_name = "fact"). This mirrors what eval-define-function already does for the (define (fact n) ...) shorthand syntax — closing the gap between the two syntactic forms.

Stack analysis looks correct. The sequence sclosure-val → closure-params/body/env → 3 pick → make-named-closure → sclosure faithfully rebuilds the closure with the bound name, and the named-closure? not guard correctly avoids double-promoting an already-named closure.

Issues:

No test coverage for the fixed case. tests/lisp/functions/recursion.slisp only tests (define (factorial n) ...) syntax. A test covering the define + lambda form is missing:
```
(define fact (lambda (n) (if (< n 2) 1 (* n (fact (- n 1))))))
(test 'lambda-recursion (assert-eq (fact 5) 120))
```
Without this, regression protection for the fix is absent.
Commit message and PR title are both just "fix" — uninformative for anyone reading history. Something like "fix: enable self-recursion for (define name (lambda ...)) form" would be far more useful.
PR body is empty. A one-line description of the bug and how it was triggered would help reviewers and future git log readers.
Semantic note worth a comment: Promoting an anonymous closure to named when it is defined is transparent for recursive use, but it alters the closure's observable identity (e.g. if closure names appear in error messages or introspection). This is almost certainly the right trade-off — matching the (define (f ...)) shorthand — but a brief inline comment explaining why this promotion is safe would help future maintainers.

`docs/design/multiline-repl-history.md` — Design doc

The document is well-structured: it clearly identifies the current gap (flat cursor model vs. embedded-newline history entries), presents three design options for j/k semantics in tabular form, and justifies Option C (edge-aware fall-through to history) with a reasonable analogy to zsh/fish behavior. The phased approach and risk callouts in the LOE table are helpful.

Minor points:

Consider splitting this into its own PR so it can be discussed independently of the eval fix.
The doc mentions "pure Seq terminal FFI" as a constraint for Phase 1 — it would be useful to name the specific FFI primitives available, since terminal coordinate math is the highest-risk part.

Overall

The eval fix is conceptually sound and consistent with how eval-define-function already works. The main gap is missing test coverage for the exact bug being fixed. Please add a test in recursion.slisp (or closures.slisp) before merging.

## PR Review ### Summary This PR bundles two unrelated changes: a bug fix in `eval-define-simple` and a design document for future multiline REPL history editing. These would be cleaner as separate PRs. --- ### `src/eval.seq` — Self-recursion fix for `define` + `lambda` **What it does:** When `(define fact (lambda (n) ...))` is evaluated, the unnamed closure is now promoted to a named closure (with `self_name = "fact"`). This mirrors what `eval-define-function` already does for the `(define (fact n) ...)` shorthand syntax — closing the gap between the two syntactic forms. **Stack analysis looks correct.** The sequence `sclosure-val → closure-params/body/env → 3 pick → make-named-closure → sclosure` faithfully rebuilds the closure with the bound name, and the `named-closure? not` guard correctly avoids double-promoting an already-named closure. **Issues:** 1. **No test coverage for the fixed case.** `tests/lisp/functions/recursion.slisp` only tests `(define (factorial n) ...)` syntax. A test covering the `define` + `lambda` form is missing: ```lisp (define fact (lambda (n) (if (< n 2) 1 (* n (fact (- n 1)))))) (test 'lambda-recursion (assert-eq (fact 5) 120)) ``` Without this, regression protection for the fix is absent. 2. **Commit message and PR title are both just `"fix"`** — uninformative for anyone reading history. Something like `"fix: enable self-recursion for (define name (lambda ...)) form"` would be far more useful. 3. **PR body is empty.** A one-line description of the bug and how it was triggered would help reviewers and future `git log` readers. 4. **Semantic note worth a comment:** Promoting an anonymous closure to named when it is `define`d is transparent for recursive use, but it alters the closure's observable identity (e.g. if closure names appear in error messages or introspection). This is almost certainly the right trade-off — matching the `(define (f ...))` shorthand — but a brief inline comment explaining *why* this promotion is safe would help future maintainers. --- ### `docs/design/multiline-repl-history.md` — Design doc The document is well-structured: it clearly identifies the current gap (flat cursor model vs. embedded-newline history entries), presents three design options for `j`/`k` semantics in tabular form, and justifies Option C (edge-aware fall-through to history) with a reasonable analogy to zsh/fish behavior. The phased approach and risk callouts in the LOE table are helpful. Minor points: - Consider splitting this into its own PR so it can be discussed independently of the eval fix. - The doc mentions "pure Seq terminal FFI" as a constraint for Phase 1 — it would be useful to name the specific FFI primitives available, since terminal coordinate math is the highest-risk part. --- ### Overall The eval fix is conceptually sound and consistent with how `eval-define-function` already works. The main gap is **missing test coverage** for the exact bug being fixed. Please add a test in `recursion.slisp` (or `closures.slisp`) before merging.