navicore/seq-lisp

Fork 0

Lsp #58

Merged

navicore merged 3 commits from lsp into main

2026-02-07 16:43:55 +00:00

navicore commented

2026-02-07 15:15:02 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-07 15:16:54 +00:00

(Migrated from github.com)

PR Review: LSP Document Symbols Implementation

Overview

This PR adds document symbols support to the SeqLisp LSP server, enabling outline/navigation features (e.g., Ctrl+Shift+O in VSCode). The implementation finds all define and defmacro statements and reports their locations to the editor.

✅ Strengths

Good test coverage: Both new features (diagnostics documentation and document symbols) have dedicated test files that verify correct LSP protocol responses.
Follows LSP specification: The textDocument/documentSymbol response correctly returns SymbolInformation objects with proper structure (name, kind, location, range).
Code reuse: Cleverly extends existing find-definitions logic to track positions, avoiding duplication.
Documentation: Updated comments and roadmap appropriately to reflect new capabilities.
Consistent style: The implementation follows the existing SeqLisp coding patterns (e.g., recursive helper functions with -loop suffix).

🔍 Issues & Recommendations

1. Correctness: Range calculation bug

Location: lib/lsp.slisp:450

The range calculation for document symbols is incorrect:

(make-range line col line (+ col (string-length name)))

This uses the start of the (define keyword as the symbol location, but col points to the opening paren, not the symbol name. The range should span only the symbol name itself, not from the ( character.

Impact: In editors, the highlighted range will be off by several characters, making navigation confusing.

Suggested fix: Calculate the actual position where the symbol name starts (after (define or (defmacro ), or pass this information from extract-defined-symbol.

2. Code organization: Function ordering inconsistency

Location: lib/lsp.slisp:342-374

In the "Completion Support" section, helper functions are reordered so that string-index-of-loop appears before string-index-of, and extract-symbol-loop before extract-symbol. While this follows a "declare before use" pattern, it's inconsistent with other sections of the file where wrapper functions come first.

Example of existing pattern (lines 64-65):

(define (paren-balance str)
  (paren-balance-loop str 0 0))

(define (paren-balance-loop str pos balance)
  ...)

Recommendation: Consider keeping the existing pattern (wrapper first, then loop implementation) for consistency, unless there's a technical requirement for forward declaration in SeqLisp.

3. Maintainability: Repeated boolean chaining pattern

Location: lib/lsp.slisp:359, 369

The code uses deeply nested if expressions to check multiple whitespace characters:

(if (if (equal? ch " ") #t (if (equal? ch "\n") #t (if (equal? ch "\t") #t (equal? ch "\r"))))
    ...)

Issues:

Hard to read and verify
Error-prone if more conditions need to be added
Not idiomatic Lisp style

Suggested refactoring: Define a helper predicate:

(define (whitespace? ch)
  (if (equal? ch " ") #t
    (if (equal? ch "\n") #t
      (if (equal? ch "\t") #t
        (equal? ch "\r")))))

Then use:

(if (whitespace? ch)
    (skip-whitespace text (+ pos 1))
    pos)

Alternatively, once SeqLisp supports or, this could be:

(if (or (equal? ch " ") (equal? ch "\n") (equal? ch "\t") (equal? ch "\r"))
    ...)

4. Performance: Redundant position calculations

Location: lib/lsp.slisp:432-434

The offset-to-position function is called for every found definition, scanning from the beginning of the document each time. For a file with many definitions, this creates O(n²) complexity.

Example: In a 1000-line file with 50 definitions, this scans ~25,000 lines unnecessarily.

Suggested optimization: Thread the current position through the loop, similar to how other functions maintain position state:

; Track (line, col) as we scan, update when we pass newlines
(define (find-definitions-pos-loop text pattern pattern-len pos line col acc)
  ...)

This would make the algorithm O(n), scanning the document only once.

5. Testing: Missing edge cases

Location: tests/lsp/test_documentsymbols.txt

The test only covers the happy path with two simple definitions. Consider adding tests for:

Multiple definitions on same line: (define x 1) (define y 2)
Nested definitions: (define (outer) (define inner 42)) - should inner be reported?
Define with function signature: (define (bar x) x) - currently tested but could verify position accuracy
Defmacro: Test file only has define, should also test defmacro
Empty document: Verify empty array returned
Document with no definitions: Just regular code

6. Semantic correctness: Symbol kind classification

Location: lib/lsp.slisp:445

All symbols are marked with kind 12 (Function), even when they're variables:

(make-symbol-info (car sym) 12 uri (car (cdr sym)) (car (cdr (cdr sym))))

Issue: (define x 42) creates a variable, not a function, but it's marked as a function in the outline.

LSP SymbolKind values:

12 = Function
13 = Variable

Recommendation: Detect whether the definition is a function or variable:

(define (symbol-kind text start-pos)
  (let pos (skip-whitespace text start-pos)
    (if (equal? (char-at text pos) "(")
        12  ; Function form: (define (name args) body)
        13  ; Variable form: (define name value)
    )))

7. Documentation: Missing limitation note

Location: lib/lsp.slisp:17-19

The limitations section should mention that document symbols:

Only find top-level definitions (no nested scopes)
Don't distinguish between functions and variables (if not fixed per #6)
Report positions of the define keyword, not the symbol name (if not fixed per #1)

🎯 Priority Summary

Priority	Issue	Impact
High	#1 Range calculation bug	Incorrect editor navigation
Medium	#6 Symbol kind classification	Confusing outline (everything looks like a function)
Medium	#3 Boolean chaining readability	Maintenance burden, easy to introduce bugs
Low	#4 Performance (O(n²))	Only matters for very large files
Low	#2 Function ordering	Consistency preference
Low	#5 Test coverage	Nice-to-have edge cases
Low	#7 Documentation	Helps users understand limitations

✅ Approval

Despite the issues noted above, this PR:

Implements the feature correctly at the protocol level
Has working tests that verify the integration
Follows existing code patterns
Adds useful functionality

Recommendation: ✅ Approve with suggestions - The issues are mostly minor improvements and can be addressed in follow-up PRs if desired. The range calculation bug (#1) is the most user-facing issue and would be good to fix before merging.

Great work on extending the LSP server! The document symbols feature will significantly improve the editor experience for SeqLisp users. 🎉

## PR Review: LSP Document Symbols Implementation ### Overview This PR adds document symbols support to the SeqLisp LSP server, enabling outline/navigation features (e.g., Ctrl+Shift+O in VSCode). The implementation finds all `define` and `defmacro` statements and reports their locations to the editor. ### ✅ Strengths 1. **Good test coverage**: Both new features (diagnostics documentation and document symbols) have dedicated test files that verify correct LSP protocol responses. 2. **Follows LSP specification**: The `textDocument/documentSymbol` response correctly returns `SymbolInformation` objects with proper structure (name, kind, location, range). 3. **Code reuse**: Cleverly extends existing `find-definitions` logic to track positions, avoiding duplication. 4. **Documentation**: Updated comments and roadmap appropriately to reflect new capabilities. 5. **Consistent style**: The implementation follows the existing SeqLisp coding patterns (e.g., recursive helper functions with `-loop` suffix). --- ### 🔍 Issues & Recommendations #### **1. Correctness: Range calculation bug** **Location**: `lib/lsp.slisp:450` The range calculation for document symbols is incorrect: ```lisp (make-range line col line (+ col (string-length name))) ``` This uses the start of the `(define` keyword as the symbol location, but `col` points to the opening paren, not the symbol name. The range should span only the symbol name itself, not from the `(` character. **Impact**: In editors, the highlighted range will be off by several characters, making navigation confusing. **Suggested fix**: Calculate the actual position where the symbol name starts (after `(define ` or `(defmacro `), or pass this information from `extract-defined-symbol`. --- #### **2. Code organization: Function ordering inconsistency** **Location**: `lib/lsp.slisp:342-374` In the "Completion Support" section, helper functions are reordered so that `string-index-of-loop` appears before `string-index-of`, and `extract-symbol-loop` before `extract-symbol`. While this follows a "declare before use" pattern, it's inconsistent with other sections of the file where wrapper functions come first. **Example of existing pattern** (lines 64-65): ```lisp (define (paren-balance str) (paren-balance-loop str 0 0)) (define (paren-balance-loop str pos balance) ...) ``` **Recommendation**: Consider keeping the existing pattern (wrapper first, then loop implementation) for consistency, unless there's a technical requirement for forward declaration in SeqLisp. --- #### **3. Maintainability: Repeated boolean chaining pattern** **Location**: `lib/lsp.slisp:359, 369` The code uses deeply nested `if` expressions to check multiple whitespace characters: ```lisp (if (if (equal? ch " ") #t (if (equal? ch "\n") #t (if (equal? ch "\t") #t (equal? ch "\r")))) ...) ``` **Issues**: - Hard to read and verify - Error-prone if more conditions need to be added - Not idiomatic Lisp style **Suggested refactoring**: Define a helper predicate: ```lisp (define (whitespace? ch) (if (equal? ch " ") #t (if (equal? ch "\n") #t (if (equal? ch "\t") #t (equal? ch "\r"))))) ``` Then use: ```lisp (if (whitespace? ch) (skip-whitespace text (+ pos 1)) pos) ``` Alternatively, once SeqLisp supports `or`, this could be: ```lisp (if (or (equal? ch " ") (equal? ch "\n") (equal? ch "\t") (equal? ch "\r")) ...) ``` --- #### **4. Performance: Redundant position calculations** **Location**: `lib/lsp.slisp:432-434` The `offset-to-position` function is called for every found definition, scanning from the beginning of the document each time. For a file with many definitions, this creates O(n²) complexity. **Example**: In a 1000-line file with 50 definitions, this scans ~25,000 lines unnecessarily. **Suggested optimization**: Thread the current position through the loop, similar to how other functions maintain position state: ```lisp ; Track (line, col) as we scan, update when we pass newlines (define (find-definitions-pos-loop text pattern pattern-len pos line col acc) ...) ``` This would make the algorithm O(n), scanning the document only once. --- #### **5. Testing: Missing edge cases** **Location**: `tests/lsp/test_documentsymbols.txt` The test only covers the happy path with two simple definitions. Consider adding tests for: - **Multiple definitions on same line**: `(define x 1) (define y 2)` - **Nested definitions**: `(define (outer) (define inner 42))` - should inner be reported? - **Define with function signature**: `(define (bar x) x)` - currently tested but could verify position accuracy - **Defmacro**: Test file only has `define`, should also test `defmacro` - **Empty document**: Verify empty array returned - **Document with no definitions**: Just regular code --- #### **6. Semantic correctness: Symbol kind classification** **Location**: `lib/lsp.slisp:445` All symbols are marked with kind `12` (Function), even when they're variables: ```lisp (make-symbol-info (car sym) 12 uri (car (cdr sym)) (car (cdr (cdr sym)))) ``` **Issue**: `(define x 42)` creates a variable, not a function, but it's marked as a function in the outline. **LSP SymbolKind values**: - 12 = Function - 13 = Variable **Recommendation**: Detect whether the definition is a function or variable: ```lisp (define (symbol-kind text start-pos) (let pos (skip-whitespace text start-pos) (if (equal? (char-at text pos) "(") 12 ; Function form: (define (name args) body) 13 ; Variable form: (define name value) ))) ``` --- #### **7. Documentation: Missing limitation note** **Location**: `lib/lsp.slisp:17-19` The limitations section should mention that document symbols: - Only find top-level definitions (no nested scopes) - Don't distinguish between functions and variables (if not fixed per #6) - Report positions of the `define` keyword, not the symbol name (if not fixed per #1) --- ### 🎯 Priority Summary | Priority | Issue | Impact | |----------|-------|--------| | **High** | #1 Range calculation bug | Incorrect editor navigation | | **Medium** | #6 Symbol kind classification | Confusing outline (everything looks like a function) | | **Medium** | #3 Boolean chaining readability | Maintenance burden, easy to introduce bugs | | **Low** | #4 Performance (O(n²)) | Only matters for very large files | | **Low** | #2 Function ordering | Consistency preference | | **Low** | #5 Test coverage | Nice-to-have edge cases | | **Low** | #7 Documentation | Helps users understand limitations | --- ### ✅ Approval Despite the issues noted above, this PR: - Implements the feature correctly at the protocol level - Has working tests that verify the integration - Follows existing code patterns - Adds useful functionality **Recommendation**: ✅ **Approve with suggestions** - The issues are mostly minor improvements and can be addressed in follow-up PRs if desired. The range calculation bug (#1) is the most user-facing issue and would be good to fix before merging. --- Great work on extending the LSP server\! The document symbols feature will significantly improve the editor experience for SeqLisp users. 🎉

claude[bot] commented

2026-02-07 15:40:04 +00:00

(Migrated from github.com)

Code Review: LSP Document Symbols Implementation

Summary

This PR adds document symbol support to the SeqLisp LSP server, enabling outline/navigation features (Ctrl+Shift+O in VSCode). The implementation finds all define and defmacro declarations and reports their positions with proper LSP SymbolKind classifications.

✅ Strengths

Good code organization: The new document symbols functionality is well-separated into its own section with clear comments
Proper LSP compliance: Uses correct SymbolKind values (12=Function, 13=Variable) and returns proper SymbolInformation objects
Comprehensive test coverage: Added two new tests covering diagnostics details and document symbols
Code reuse: Leverages existing helper functions (string-index-of, skip-whitespace, etc.) and extends them thoughtfully
Documentation: Updated ROADMAP.md to mark diagnostics as completed, and added inline comments explaining the implementation

🔍 Code Quality Observations

Good Practices

Function decomposition: The new extract-defined-symbol-info function cleanly extends extract-defined-symbol by adding position and kind tracking
Consistent naming: Functions follow the existing naming conventions (e.g., find-definitions-with-pos, offset-to-position)
Helper functions: The addition of whitespace? and delimiter? improves code readability vs. the previous inline boolean checks

Minor Refactoring

The code moves string-index-of definition (lines 342-352) from its previous location (lines 340-348). This appears to be a reorganization to place the loop helper before the main function, which is a reasonable style choice for readability.

🐛 Potential Issues

1. Character Encoding Assumption (Minor)

The offset-to-position function in lib/lsp.slisp:441-451 assumes byte offsets equal character offsets. This works for ASCII but may produce incorrect positions with multi-byte UTF-8 characters.

(define (offset-to-position-loop text pos line col target)
  (if (>= pos target)
      (list line col)
      (if (>= pos (string-length text))
          (list line col)
          (if (equal? (char-at text pos) "\n")
              (offset-to-position-loop text (+ pos 1) (+ line 1) 0 target)
              (offset-to-position-loop text (+ pos 1) line (+ col 1) target)))))

Impact: For SeqLisp files with ASCII-only identifiers, this is fine. If users include Unicode symbols or comments, positions may be off.

Recommendation: Document this limitation or consider whether SeqLisp supports Unicode identifiers.

2. Return Type Inconsistency (Minor)

The extract-symbol function changed its return type from (symbol . end-pos) (cons pair) to (symbol end-pos) (list). The comment was updated correctly on line 376, but verify all callers handle this change.

Looking at the code, both extract-defined-symbol (line 397) and extract-defined-symbol-info (line 411, 414) use car to extract the symbol name, which works correctly for lists. This change appears intentional and safe.

3. Test Assertions Could Be More Specific (Minor)

The new test in justfile:193-209 uses grep to check for both symbol names:

if echo "$output" | grep -q '"name":"foo"' && echo "$output" | grep -q '"name":"bar"'; then

While functional, this doesn't verify:

That the symbols are returned in the correct structure
That the positions are accurate
That kinds are correct (12 for function bar, 13 for variable foo)

Recommendation: Consider adding more specific assertions for symbol kinds and positions in future improvements.

🔒 Security Considerations

No security concerns identified. The implementation:

Processes user document text in a read-only manner
Does not execute code in the document symbols handler
Uses the existing safe string manipulation functions
Returns properly structured LSP responses

⚡ Performance Considerations

Linear search pattern: find-definitions-pos-loop (line 455) searches the entire document linearly for each pattern. For large files with many definitions, this could be slow.

However, this is acceptable because:
- Document symbols requests are user-initiated (not on every keystroke)
- SeqLisp files are typically small
- The implementation matches the existing find-definitions pattern
Offset conversion: offset-to-position scans from the beginning of the document each time. With the current code structure, this is called once per definition found.

Current complexity: O(n × m) where n = document size, m = number of definitions

This is reasonable for typical use cases.

📝 Test Coverage

Excellent test coverage:

✅ Test 7 (test_diagnostics.txt): Verifies diagnostics include error details with severity
✅ Test 8 (test_documentsymbols.txt): Verifies document symbols returns both variable and function definitions

The test file content shows proper LSP message formatting and lifecycle (initialize → initialized → request → shutdown → exit).

📋 Recommendations

Documentation: Consider adding a comment in offset-to-position noting the ASCII assumption
Future enhancement: The comment on line 19 mentions "Completion only includes builtins (not user-defined symbols)" - the new find-defined-symbols-with-pos infrastructure could enable user-defined symbol completion in a future PR
Edge case testing: Consider adding tests for:
- Empty file (no definitions)
- File with only defmacro definitions
- Definitions with unusual formatting/whitespace

✨ Conclusion

This is a well-implemented, focused PR that adds valuable LSP functionality. The code quality is good, follows existing patterns, and includes appropriate test coverage. The few minor issues noted are not blockers and could be addressed in future refinements if needed.

Recommendation: Approve and merge ✅

Great work on implementing this feature! The document symbols support will significantly improve the developer experience when working with SeqLisp files in modern editors.

## Code Review: LSP Document Symbols Implementation ### Summary This PR adds document symbol support to the SeqLisp LSP server, enabling outline/navigation features (Ctrl+Shift+O in VSCode). The implementation finds all `define` and `defmacro` declarations and reports their positions with proper LSP SymbolKind classifications. --- ### ✅ Strengths 1. **Good code organization**: The new document symbols functionality is well-separated into its own section with clear comments 2. **Proper LSP compliance**: Uses correct SymbolKind values (12=Function, 13=Variable) and returns proper SymbolInformation objects 3. **Comprehensive test coverage**: Added two new tests covering diagnostics details and document symbols 4. **Code reuse**: Leverages existing helper functions (`string-index-of`, `skip-whitespace`, etc.) and extends them thoughtfully 5. **Documentation**: Updated ROADMAP.md to mark diagnostics as completed, and added inline comments explaining the implementation --- ### 🔍 Code Quality Observations #### Good Practices - **Function decomposition**: The new `extract-defined-symbol-info` function cleanly extends `extract-defined-symbol` by adding position and kind tracking - **Consistent naming**: Functions follow the existing naming conventions (e.g., `find-definitions-with-pos`, `offset-to-position`) - **Helper functions**: The addition of `whitespace?` and `delimiter?` improves code readability vs. the previous inline boolean checks #### Minor Refactoring The code moves `string-index-of` definition (lines 342-352) from its previous location (lines 340-348). This appears to be a reorganization to place the loop helper before the main function, which is a reasonable style choice for readability. --- ### 🐛 Potential Issues #### 1. **Character Encoding Assumption** (Minor) The `offset-to-position` function in **lib/lsp.slisp:441-451** assumes byte offsets equal character offsets. This works for ASCII but may produce incorrect positions with multi-byte UTF-8 characters. ```lisp (define (offset-to-position-loop text pos line col target) (if (>= pos target) (list line col) (if (>= pos (string-length text)) (list line col) (if (equal? (char-at text pos) "\n") (offset-to-position-loop text (+ pos 1) (+ line 1) 0 target) (offset-to-position-loop text (+ pos 1) line (+ col 1) target))))) ``` **Impact**: For SeqLisp files with ASCII-only identifiers, this is fine. If users include Unicode symbols or comments, positions may be off. **Recommendation**: Document this limitation or consider whether SeqLisp supports Unicode identifiers. #### 2. **Return Type Inconsistency** (Minor) The `extract-symbol` function changed its return type from `(symbol . end-pos)` (cons pair) to `(symbol end-pos)` (list). The comment was updated correctly on **line 376**, but verify all callers handle this change. Looking at the code, both `extract-defined-symbol` (line 397) and `extract-defined-symbol-info` (line 411, 414) use `car` to extract the symbol name, which works correctly for lists. This change appears intentional and safe. #### 3. **Test Assertions Could Be More Specific** (Minor) The new test in **justfile:193-209** uses `grep` to check for both symbol names: ```bash if echo "$output" | grep -q '"name":"foo"' && echo "$output" | grep -q '"name":"bar"'; then ``` While functional, this doesn't verify: - That the symbols are returned in the correct structure - That the positions are accurate - That kinds are correct (12 for function `bar`, 13 for variable `foo`) **Recommendation**: Consider adding more specific assertions for symbol kinds and positions in future improvements. --- ### 🔒 Security Considerations **No security concerns identified.** The implementation: - Processes user document text in a read-only manner - Does not execute code in the document symbols handler - Uses the existing safe string manipulation functions - Returns properly structured LSP responses --- ### ⚡ Performance Considerations 1. **Linear search pattern**: `find-definitions-pos-loop` (line 455) searches the entire document linearly for each pattern. For large files with many definitions, this could be slow. However, this is **acceptable** because: - Document symbols requests are user-initiated (not on every keystroke) - SeqLisp files are typically small - The implementation matches the existing `find-definitions` pattern 2. **Offset conversion**: `offset-to-position` scans from the beginning of the document each time. With the current code structure, this is called once per definition found. **Current complexity**: O(n × m) where n = document size, m = number of definitions This is reasonable for typical use cases. --- ### 📝 Test Coverage **Excellent test coverage:** - ✅ Test 7 (`test_diagnostics.txt`): Verifies diagnostics include error details with severity - ✅ Test 8 (`test_documentsymbols.txt`): Verifies document symbols returns both variable and function definitions The test file content shows proper LSP message formatting and lifecycle (initialize → initialized → request → shutdown → exit). --- ### 📋 Recommendations 1. **Documentation**: Consider adding a comment in `offset-to-position` noting the ASCII assumption 2. **Future enhancement**: The comment on line 19 mentions "Completion only includes builtins (not user-defined symbols)" - the new `find-defined-symbols-with-pos` infrastructure could enable user-defined symbol completion in a future PR 3. **Edge case testing**: Consider adding tests for: - Empty file (no definitions) - File with only `defmacro` definitions - Definitions with unusual formatting/whitespace --- ### ✨ Conclusion This is a **well-implemented, focused PR** that adds valuable LSP functionality. The code quality is good, follows existing patterns, and includes appropriate test coverage. The few minor issues noted are not blockers and could be addressed in future refinements if needed. **Recommendation: Approve and merge** ✅ Great work on implementing this feature\! The document symbols support will significantly improve the developer experience when working with SeqLisp files in modern editors.