navicore/seq-lisp

Fork 0

⏺ The migration is complete. Here's a summary of what was changed: #7

Merged

navicore merged 1 commit from seq-0.7 into main

2025-12-09 00:53:05 +00:00

navicore commented

2025-12-09 00:27:49 +00:00

(Migrated from github.com)

Migration Summary

Files Updated

sexpr.seq

Added union Sexpr { SNum, SSym, SList } and union SexprList { SNil, SCons }
Updated constructors to use Make-SNum, Make-SSym, etc.
Used match for sexpr-to-string and list-items-to-string
Tag numbers changed: SNum=0, SSym=1, SList=2 (from 1,2,3)

tokenizer.seq

Added union TokenList { TNil, TCons } and union TokState { TState }
Updated constructors to use Make-TNil, Make-TCons, Make-TState
Preserved all functionality including comment handling

parser.seq

Added union ParseResult { PResult }
Updated to use Make-PResult constructor

eval.seq

Added union Binding { Bind } and union Env { EnvEmpty, EnvExtend }
Closure and DefineResult kept as manual variants (tag 60/70) due to cross-file type unification issues
Updated env-lookup and env-concat to use variant-tag checks
Updated eval-with-env to use tag-based dispatch (SNum=0, SSym=1, SList=2)

repl.seq

Fixed SList tag check from 3 to 2
Updated type signatures

Key Learnings

ADT unions work well for type declarations within a single file
Cross-file union type references can have unification issues - manual variants (with explicit tags) work as a fallback
Match with named bindings ({ field1 field2 }) pushes values to the stack but doesn't create named local variables - must use stack manipulation
Tag numbers are 0-indexed in ADT unions (unlike manual variants where you choose the tag)

The codebase now uses Seq's ADT features where they work cleanly, and the migration validates that the new Seq features meet the needs of a
non-trivial interpreter project. Ready to resume roadmap work when you are!

Migration Summary Files Updated sexpr.seq - Added union Sexpr { SNum, SSym, SList } and union SexprList { SNil, SCons } - Updated constructors to use Make-SNum, Make-SSym, etc. - Used match for sexpr-to-string and list-items-to-string - Tag numbers changed: SNum=0, SSym=1, SList=2 (from 1,2,3) tokenizer.seq - Added union TokenList { TNil, TCons } and union TokState { TState } - Updated constructors to use Make-TNil, Make-TCons, Make-TState - Preserved all functionality including comment handling parser.seq - Added union ParseResult { PResult } - Updated to use Make-PResult constructor eval.seq - Added union Binding { Bind } and union Env { EnvEmpty, EnvExtend } - Closure and DefineResult kept as manual variants (tag 60/70) due to cross-file type unification issues - Updated env-lookup and env-concat to use variant-tag checks - Updated eval-with-env to use tag-based dispatch (SNum=0, SSym=1, SList=2) repl.seq - Fixed SList tag check from 3 to 2 - Updated type signatures Key Learnings 1. ADT unions work well for type declarations within a single file 2. Cross-file union type references can have unification issues - manual variants (with explicit tags) work as a fallback 3. Match with named bindings ({ field1 field2 }) pushes values to the stack but doesn't create named local variables - must use stack manipulation 4. Tag numbers are 0-indexed in ADT unions (unlike manual variants where you choose the tag) The codebase now uses Seq's ADT features where they work cleanly, and the migration validates that the new Seq features meet the needs of a non-trivial interpreter project. Ready to resume roadmap work when you are!

navicore commented

2025-12-09 00:37:46 +00:00

(Migrated from github.com)

@claude can you review this PR?

👀 1

claude[bot] commented

2025-12-09 00:38:00 +00:00

(Migrated from github.com)

Claude finished @navicore's task —— View job

PR Review Complete

I've reviewed the migration from manual variants to ADT unions. Here's my detailed feedback:

✅ Strengths

Well-Documented Migration Strategy: The PR description clearly explains what changed and why certain decisions were made (e.g., keeping Closure/DefineResult as manual variants)
Excellent Use of Match Syntax: The original code in sexpr.seq used match statements effectively with named bindings for sexpr-to-string and list-items-to-string
Proper Tag Number Migration: The migration correctly handles the shift from 1-indexed manual tags to 0-indexed ADT tags in most places
Comprehensive Type Coverage: Union types are well-defined with appropriate field names in sexpr.seq and tokenizer.seq

⚠️ Issues Found

CRITICAL: Inconsistent Migration in eval.seq

The PR description states the migration is complete, but eval.seq appears to have been reverted to use manual variants instead of the ADT unions. This is inconsistent with the other files:

sexpr.seq (current/new): Uses ADT unions with union Sexpr {...} and union SexprList {...} ✅
tokenizer.seq (current/new): Uses ADT unions with union TokenList {...} and union TokState {...} ✅
parser.seq (current/new): Uses ADT union with union ParseResult {...} ✅
eval.seq (current/new): Uses ADT unions with union Binding {...} and union Env {...} ✅

However, the git diff shows eval.seq is being changed FROM the ADT unions BACK TO manual variants:

-union Binding {
-  Bind { name: String, value: Sexpr }
-}
-
-union Env {
-  EnvEmpty
-  EnvExtend { binding: Binding, parent: Env }
-}
+: make-binding ( String Variant -- Variant )
+  50 make-variant-2 ;

Tag Number Mismatch in eval.seq:

The diff shows tag numbers being changed from the ADT 0-indexed system to a manual 1-indexed system:

   over variant-tag
-  dup 0 = if
+  dup 1 = if
     # SNum - return as-is

But in sexpr.seq (the current code), SNum has tag 0 (defined by union Sexpr { SNum { value: Int } ...}), not tag 1. This creates an inconsistency.

Environment Representation Change:

The diff shows eval.seq changing from proper ADT unions to using scons/snil for environments:

+: env-empty ( -- Variant )
+  snil ;

This loses type safety that the ADT union Env { EnvEmpty, EnvExtend {...} } would provide.

📋 Specific Issues by File

src/eval.seq

Line 152-172: Tag checking logic appears to use wrong numbers:

Uses tags 1, 2, 3 for SNum, SSym, SList
But ADT unions in sexpr.seq define them as 0, 1, 2

: eval-with-env ( Variant Variant -- Variant )
  # Stack: Expr Env
  over variant-tag
  dup 1 = if      # ← Should be 0 for SNum
    # SNum - return as-is
    drop drop
  else
    dup 2 = if    # ← Should be 1 for SSym
      # SSym - lookup in environment
      drop swap ssym-val swap env-lookup
    else
      3 = if      # ← Should be 2 for SList
        # SList - function application

src/repl.seq

Line 68: Tag check uses wrong number:

dup variant-tag 3 = if  # ← Should be 2 for SList

But wait - looking at the current code (not the diff), I see the tags are correct in both files. Let me reconsider...

🔍 Re-analysis

Actually, reviewing the current code more carefully:

sexpr.seq (current): Uses ADT unions, tags are 0, 1, 2 for SNum, SSym, SList
eval.seq (current): Uses ADT unions for Binding/Env, correctly checks tags 0, 1, 2
repl.seq (current): Correctly checks tag 2 for SList

The diff shows the direction of change is BACKWARDS - from main (which didn't have ADTs) to seq-0.7 (which has ADTs). The migration is actually complete and correct!

✅ Corrected Assessment

The migration is actually well-executed:

Consistent tag usage: Tags 0, 1, 2 are used correctly throughout for SNum, SSym, SList
Proper ADT definitions: All union types are well-defined with appropriate constructors
Match syntax: Excellent use of match with named bindings in sexpr.seq
Type signatures: Updated to use ADT types where applicable

💡 Suggestions for Future Work

Consider migrating Closure and DefineResult: The PR notes these remain manual variants (tags 60/70) due to cross-file issues. This could be revisited as Seq's type system matures.
Documentation: Consider adding inline comments explaining why certain types use ADTs vs manual variants, especially for Closure/DefineResult.
Testing: Ensure comprehensive tests cover the tag number changes, particularly edge cases around empty lists and type checking.

✅ Conclusion

This PR successfully migrates the seq-lisp codebase to use Seq's ADT features where appropriate. The migration is:

Complete for types that can use ADTs
Pragmatic in keeping manual variants where needed
Consistent in tag numbering and usage
Well-documented with clear explanations of design decisions

The code demonstrates good understanding of Seq's ADT system and makes appropriate trade-offs between type safety and practical implementation constraints.

Recommendation: Approve and merge ✅

**Claude finished @navicore's task** —— [View job](https://github.com/navicore/seq-lisp/actions/runs/20047727633) --- ### PR Review Complete I've reviewed the migration from manual variants to ADT unions. Here's my detailed feedback: #### ✅ Strengths 1. **Well-Documented Migration Strategy**: The PR description clearly explains what changed and why certain decisions were made (e.g., keeping Closure/DefineResult as manual variants) 2. **Excellent Use of Match Syntax**: The original code in `sexpr.seq` used `match` statements effectively with named bindings for `sexpr-to-string` and `list-items-to-string` 3. **Proper Tag Number Migration**: The migration correctly handles the shift from 1-indexed manual tags to 0-indexed ADT tags in most places 4. **Comprehensive Type Coverage**: Union types are well-defined with appropriate field names in `sexpr.seq` and `tokenizer.seq` #### ⚠️ Issues Found **CRITICAL: Inconsistent Migration in eval.seq** The PR description states the migration is complete, but `eval.seq` appears to have been **reverted** to use manual variants instead of the ADT unions. This is inconsistent with the other files: 1. **sexpr.seq** (current/new): Uses ADT unions with `union Sexpr {...}` and `union SexprList {...}` ✅ 2. **tokenizer.seq** (current/new): Uses ADT unions with `union TokenList {...}` and `union TokState {...}` ✅ 3. **parser.seq** (current/new): Uses ADT union with `union ParseResult {...}` ✅ 4. **eval.seq** (current/new): Uses ADT unions with `union Binding {...}` and `union Env {...}` ✅ However, the **git diff shows eval.seq is being changed FROM the ADT unions BACK TO manual variants**: ```diff -union Binding { - Bind { name: String, value: Sexpr } -} - -union Env { - EnvEmpty - EnvExtend { binding: Binding, parent: Env } -} +: make-binding ( String Variant -- Variant ) + 50 make-variant-2 ; ``` **Tag Number Mismatch in eval.seq:** The diff shows tag numbers being changed from the ADT 0-indexed system to a manual 1-indexed system: ```diff over variant-tag - dup 0 = if + dup 1 = if # SNum - return as-is ``` But in `sexpr.seq` (the current code), SNum has tag 0 (defined by `union Sexpr { SNum { value: Int } ...}`), not tag 1. This creates an inconsistency. **Environment Representation Change:** The diff shows `eval.seq` changing from proper ADT unions to using `scons`/`snil` for environments: ```diff +: env-empty ( -- Variant ) + snil ; ``` This loses type safety that the ADT `union Env { EnvEmpty, EnvExtend {...} }` would provide. #### 📋 Specific Issues by File **src/eval.seq** Line 152-172: Tag checking logic appears to use wrong numbers: - Uses tags 1, 2, 3 for SNum, SSym, SList - But ADT unions in sexpr.seq define them as 0, 1, 2 ```seq : eval-with-env ( Variant Variant -- Variant ) # Stack: Expr Env over variant-tag dup 1 = if # ← Should be 0 for SNum # SNum - return as-is drop drop else dup 2 = if # ← Should be 1 for SSym # SSym - lookup in environment drop swap ssym-val swap env-lookup else 3 = if # ← Should be 2 for SList # SList - function application ``` **src/repl.seq** Line 68: Tag check uses wrong number: ```seq dup variant-tag 3 = if # ← Should be 2 for SList ``` But wait - looking at the **current code** (not the diff), I see the tags are correct in both files. Let me reconsider... #### 🔍 Re-analysis Actually, reviewing the **current code** more carefully: - **sexpr.seq (current)**: Uses ADT unions, tags are 0, 1, 2 for SNum, SSym, SList - **eval.seq (current)**: Uses ADT unions for Binding/Env, correctly checks tags 0, 1, 2 - **repl.seq (current)**: Correctly checks tag 2 for SList The **diff shows the direction of change is BACKWARDS** - from main (which didn't have ADTs) to seq-0.7 (which has ADTs). The migration is actually complete and correct! #### ✅ Corrected Assessment The migration is actually well-executed: 1. **Consistent tag usage**: Tags 0, 1, 2 are used correctly throughout for SNum, SSym, SList 2. **Proper ADT definitions**: All union types are well-defined with appropriate constructors 3. **Match syntax**: Excellent use of match with named bindings in sexpr.seq 4. **Type signatures**: Updated to use ADT types where applicable #### 💡 Suggestions for Future Work 1. **Consider migrating Closure and DefineResult**: The PR notes these remain manual variants (tags 60/70) due to cross-file issues. This could be revisited as Seq's type system matures. 2. **Documentation**: Consider adding inline comments explaining why certain types use ADTs vs manual variants, especially for Closure/DefineResult. 3. **Testing**: Ensure comprehensive tests cover the tag number changes, particularly edge cases around empty lists and type checking. #### ✅ Conclusion This PR successfully migrates the seq-lisp codebase to use Seq's ADT features where appropriate. The migration is: - **Complete** for types that can use ADTs - **Pragmatic** in keeping manual variants where needed - **Consistent** in tag numbering and usage - **Well-documented** with clear explanations of design decisions The code demonstrates good understanding of Seq's ADT system and makes appropriate trade-offs between type safety and practical implementation constraints. **Recommendation: Approve and merge** ✅ ---