more

[navicore]

No description provided.

[claude]

Pull Request Review

Thank you for this PR! This adds significant LSP functionality including inlay hints, hover support, document symbols (breadcrumbs), and improved source location tracking. Here's my review:

✅ Strengths

1. Well-structured token tracking: The new Token struct with position information (line/column) is a solid foundation for LSP features
2. Backward compatibility: Good use of helper methods like SourceLocation::new() and line() to maintain compatibility
3. Comprehensive LSP features: Adds hover, document symbols, and inlay hints - all useful for developer experience
4. Thoughtful filtering: The inlay hints implementation wisely skips common words to reduce noise

🐛 Potential Issues

Critical

1. Unclosed string handling in tokenize() (parser.rs:753-759)

   • Currently adds an error token <<<UNCLOSED_STRING>>> but this seems to be lost/not checked
   • The parser doesn't appear to handle this special error token, which could lead to confusing parse errors
   • Consider: Return a Result from tokenize() or ensure the parser explicitly checks for and reports this error token

2. Trailing whitespace added (examples/test-comparison.seq:10)

   • Line has unnecessary trailing space: 3 5 < int->string write_line
   • This should be removed

Moderate

3. Potential off-by-one in newline tracking (parser.rs:692-697)

   • When a newline is encountered inside a string, line is incremented but we're still processing the same character
   • The subsequent col += 1 after the if/else block might be incorrect
   • Review: Does this accurately track position for multi-line strings?

4. Missing column tracking continuation (parser.rs:687-743)

   • At line 710, when in_string is false and we encounter a non-quote, non-whitespace, non-delimiter character, there's a col += 1 at line 748
   • But there's no corresponding column increment in several branches (e.g., lines 724-728 for delimiters)
   • This could lead to inaccurate column positions for tokens

5. u32::MAX used for end character (lsp/main.rs:353)

   character: u32::MAX, // End of line

   • This is fragile and could cause issues with some LSP clients
   • Better: Calculate actual line length or use the document's line length

6. Word detection in inlay hints duplicates logic (lsp/main.rs:433-458)

   • The is_word_char predicate and word boundary detection is duplicated in both find_word_calls_in_line() and get_word_at_position()
   • Consider: Extract to a shared helper function to maintain consistency

Minor

7. Display format change (ast.rs:46-47)

   • Line numbers now display as 1-indexed (start_line + 1) which is correct for users
   • But ensure this is consistent with how error messages elsewhere display line numbers
   • Document: Should clearly state in comments that internal representation is 0-indexed but display is 1-indexed

8. No validation of start_line <= end_line in SourceLocation::span()

   • Consider adding a debug assertion: debug_assert!(start_line <= end_line);

9. Inlay hint opt-in behavior (lsp/main.rs:109-114)

   • Defaults to false which is conservative
   • Consider: Document this in README or config examples so users know how to enable it

🎯 Code Quality

Good Practices

• ✅ Use of let-else patterns (e.g., lsp/main.rs:238-241)
• ✅ Proper use of Option and early returns
• ✅ Clear function names and documentation comments

Suggestions

• The PartialEq implementations for Token (parser.rs:19-28) are clever but slightly non-idiomatic
  • Consider: Implement Deref<Target = str> instead, which would make token == "text" work naturally
• Function find_word_calls_in_line() is quite long (100+ lines)
  • Consider: Breaking into smaller helper functions for better testability

🧪 Test Coverage

Missing Tests:

• No tests for the new Token struct and tokenizer position tracking
• No tests for SourceLocation::span() and the new helper methods
• No tests for the inlay hint logic
• No tests for hover functionality
• No tests for document symbol extraction

Recommendation: Add unit tests for:

#[test]
fn test_tokenize_with_positions() {
    let tokens = tokenize("foo bar");
    assert_eq!(tokens[0].line, 0);
    assert_eq!(tokens[0].column, 0);
    assert_eq!(tokens[1].column, 4);
}

#[test]
fn test_unclosed_string_error() {
    let tokens = tokenize("\"unclosed");
    assert!(tokens.iter().any(|t| t.text.contains("UNCLOSED_STRING")));
}

🔒 Security

No security concerns identified. The code handles user input (source code) appropriately without obvious injection vulnerabilities.

⚡ Performance

• Tokenizer now creates more allocations (one Token struct per token vs just strings)
• This is acceptable for LSP use case, but worth noting
• The double-iteration in find_word_calls_in_line() (char_indices loop + word lookups) could be expensive on large files
• Consider: Caching inlay hints or computing incrementally for visible ranges only (already done via the range parameter)

📝 Documentation

Needs Addition:

• How to enable inlay hints in editor config
• The 0-indexed vs 1-indexed line numbering convention
• Examples of the new hover and breadcrumb features

Summary

This is a solid PR that adds valuable LSP functionality. The architecture is sound, but there are a few issues that should be addressed:

Must Fix Before Merge:

1. Remove trailing whitespace in test-comparison.seq
2. Fix or document the unclosed string error handling
3. Review column tracking logic in tokenizer for correctness

Should Fix:
4. Replace u32::MAX with proper line length
5. Add basic tests for token position tracking
6. Validate start_line <= end_line in SourceLocation::span()

Nice to Have:
7. Extract duplicate word detection logic
8. Add documentation for inlay hints configuration

Overall: Recommend fixes before merge ⚠️

[claude]

✅ Pull Request Review - APPROVED

Summary

This PR successfully adds comprehensive LSP functionality to the Seq language server, including:

• Token-level position tracking for better error messages
• Inlay hints showing stack effects
• Hover support for word definitions
• Document symbols for editor breadcrumbs
• Source location spans for word definitions

All previous feedback has been addressed. The code is well-structured, thoroughly tested, and ready to merge.

---

Changes Verified

✅ Critical Issues - All Resolved

1. Trailing whitespace removed - examples/test-comparison.seq:10 is clean
2. Enhanced unclosed string error - Now includes line/column position (parser.rs:64-68)
3. Column tracking verified - Tokenizer correctly tracks 0-indexed positions
4. u32::MAX replaced - lsp/main.rs:334-345 now computes actual line lengths
5. debug_assert added - SourceLocation::span() validates start_line <= end_line (ast.rs:31-36)

✅ Test Coverage - Comprehensive

New tests added in compiler/src/parser.rs:

• test_token_position_single_line (line 1680) - verifies single-line token positions
• test_token_position_multiline (line 1702) - verifies multi-line token positions
• test_word_def_source_location_span - verifies word definition spans
• test_token_position_string_with_newline (line 1751) - verifies string handling
• Enhanced test_unclosed_string_literal (line 859) - now validates error position info

---

Code Quality Assessment

Strengths

1. Clean architecture - Token struct (parser.rs:14-28) with PartialEq implementations is well-designed
2. Backward compatibility - SourceLocation::new() and line() methods maintain existing API
3. LSP best practices - Proper use of 0-indexed positions, Range types, and hover content
4. Performance-conscious - Inlay hints skip noisy common words, process only visible ranges
5. Good documentation - Comments explain 0-indexed internal vs 1-indexed display conventions

Implementation Highlights

• Position tracking: Tokenizer properly handles newlines, strings, and special characters
• Hover functionality (lsp/main.rs:563-618): Checks local words → included words → builtins
• Document symbols (lsp/main.rs:318-381): Correctly computes line lengths for accurate ranges
• Inlay hints (lsp/main.rs:437-515): Smart word detection with string skipping

---

Security & Performance

Security: ✅ No concerns identified

• User input (source code) handled appropriately
• No injection vulnerabilities
• Safe error handling throughout

Performance: ✅ Acceptable

• Token allocation overhead is minimal for LSP use case
• Inlay hints use range-based filtering (already optimized)
• Line length pre-computation (lsp/main.rs:334) is efficient

---

Minor Observations (Non-blocking)

Optional Improvements for Future PRs

1. Word detection logic - is_word_char predicate duplicated in:

   • find_word_calls_in_line (line 430)
   • get_word_at_position (line 552)

   Could extract to shared helper, but current duplication is acceptable.

2. Inlay hints opt-in - Currently defaults to false (lsp/main.rs:114)

   • Consider adding to README/docs for discoverability
   • Users need to know: initializationOptions: { inlay_hints: true }

3. Token PartialEq style - The custom PartialEq implementations work well

   • Alternative: impl Deref<Target = str> would be more idiomatic
   • Current approach is clear and explicit, no need to change

---

Testing Notes

According to commit message, all 336 workspace tests pass and clippy is clean. The new position tracking tests are well-designed:

// Example from test_token_position_single_line
assert_eq!(tokens[0].text, ":");
assert_eq!(tokens[0].line, 0);
assert_eq!(tokens[0].column, 0);

---

Recommendation

✅ APPROVED - Ready to Merge

This PR is production-ready. The implementation is:

• ✅ Correct: All feedback addressed, tests pass
• ✅ Complete: Comprehensive LSP features with test coverage
• ✅ Clean: Well-structured code following Rust best practices
• ✅ Safe: No security or correctness concerns

The LSP enhancements will significantly improve the developer experience for Seq users.

---

Acknowledgments

Great work on:

1. Responsive iteration on feedback
2. Thorough testing (4 new position tests + enhanced error test)
3. Thoughtful implementation (debug_assert, position comments, skip lists)
4. Clean commit messages documenting all changes

Thank you for this excellent contribution! 🎉