Fix lsp

[navicore]

No description provided.

[claude]

PR Review: Fix LSP Line Number Reporting

Summary

This PR successfully addresses line number reporting issues in LSP diagnostics by threading span information through the AST and typechecker. The implementation is well-structured and includes comprehensive test coverage.

✅ Strengths

1. Systematic Approach

• Clean threading of span fields through AST nodes (Statement::If, Statement::Match, MatchArm)
• Consistent pattern: capture span during parsing, propagate through typechecker, use in error messages
• All affected code paths properly updated (parser, typechecker, codegen, linting, tests)

2. Excellent Test Coverage

The three new integration tests in diagnostics.rs are exemplary (lines 640-668, 700-734, 736-769). Each validates both error message content AND correct line numbers.

3. Documentation

• Clear inline comments explaining span semantics
• New unions.seq example (211 lines) is well-documented
• Updated examples/language/README.md

4. Non-Breaking Changes

• Using Option allows graceful degradation
• All existing tests updated to pass span: None
• No API changes to public interfaces

🔍 Code Quality Observations

Parser Changes (parser.rs:518-755)

Good: Captures line/column at the right moment. Span sizes are appropriate (2 for if, 5 for match).

Typechecker Changes (typechecker.rs:709-1236)

Good: Conditional line formatting, graceful fallbacks, informative error messages.

Minor Concern: Line info formatting is duplicated in 3 places. Consider extracting to a helper method (non-blocking).

Diagnostic Structure (diagnostics.rs:823-830)

Good: Improved structure ensures editor compatibility. The test_diagnostic_structure() test is excellent for debugging.

🐛 Potential Issues

1. Line Number Indexing

Verify that Span stores 0-indexed line numbers. The code uses s.line + 1 consistently, but this should be documented on the Span type definition.

2. Ignored Spans

Many locations use span: _ to ignore the field. Consider whether any might benefit from span info for future diagnostics.

🔐 Security & ⚡ Performance

• Security: No concerns
• Performance: Minimal overhead (pointer-sized Option), no runtime impact

📝 Suggestions (Non-blocking)

1. Document whether Span uses 0-indexed or 1-indexed lines/columns
2. Consider extracting repeated line formatting to a helper method
3. Add edge case tests (multi-line constructs, nested if/match, empty arms)
4. Verify the i./ drop comment in unions.seq line 73

🎯 Final Assessment

Recommendation: ✅ APPROVE with minor suggestions

This PR successfully fixes the line number reporting issue in a clean, maintainable way. Excellent test coverage gives confidence the fix works correctly. Minor suggestions above are optimizations, not blockers.

Particularly Good:

• Comprehensive test coverage with clear assertions
• Graceful fallback behavior
• Consistent application across codebase
• Well-documented changes

Great work improving developer experience with better error messages! 🎉

[claude]

PR Review: Fix LSP

Summary

This PR improves LSP error reporting by adding source span information to AST nodes (If, Match, MatchArm). This enables precise line number reporting for type errors in the LSP, addressing issues where errors were being reported at incorrect locations.

Code Quality: ✅ Strong

Strengths:

• Consistent implementation: The span field is added uniformly across all relevant AST nodes with clear documentation
• Backward compatibility: Uses Option<Span> allowing gradual rollout and handling legacy code paths
• Comprehensive coverage: All pattern matches updated with span: _ to avoid exhaustiveness warnings
• Clear intent: Documentation on each span field explains what it points to (e.g., "points to variant name", "points to 'if' keyword")

Code patterns align well with CONTRIBUTING.md principles:

• Changes are focused and systematic
• No surprises - every modification serves the stated purpose
• Consistent error handling throughout

Test Coverage: ✅ Excellent

The PR includes 4 new comprehensive tests in crates/lsp/src/diagnostics.rs:

1. test_if_else_branch_mismatch_reports_correct_line - Validates if/else errors point to the if keyword
2. test_diagnostic_structure - Ensures diagnostics have all required fields for LSP clients
3. test_stack_type_mismatch_reports_correct_line - Validates stack mismatch errors point to the problematic word call
4. test_match_arm_mismatch_reports_correct_line - Validates match errors point to the match keyword

Tests also include helpful assertions with debug output, making failures easy to diagnose.

Test fixes in REPL: Added history.clear() calls in 4 REPL tests to prevent loaded history from interfering - good defensive programming.

Potential Issues: ⚠️ Minor Concerns

1. Error message duplication (crates/compiler/src/typechecker.rs:772-792, 944-556, 1229-1246):

   • Error formatting logic is duplicated with conditional line number formatting
   • Consider extracting to a helper function:

   fn format_error_with_line(line: usize, base_msg: String) -> String {
       if line > 0 {
           format!("at line {}: {}", line, base_msg)
       } else {
           base_msg
       }
   }

2. Hardcoded string lengths (crates/compiler/src/parser.rs:273, 301):

   span: Some(Span::new(start_line, start_column, 2)), // "if" is 2 chars
   span: Some(Span::new(start_line, start_column, 5)), // "match" is 5 chars

   • While comments explain the magic numbers, consider using keyword.len() or constants to be more maintainable

3. LSP diagnostic source change (crates/lsp/src/diagnostics.rs:833):

   • Changed from "seq" to "seqc"
   • Is this intentional? Worth mentioning in PR description if this affects LSP client filtering

Performance: ✅ No Concerns

• Span storage adds minimal memory overhead (3 usize fields per node)
• Line number formatting only happens on error paths
• No algorithmic changes

Security: ✅ No Concerns

No security-sensitive changes. The PR is purely about improving error reporting.

Documentation & Examples: ✅ Excellent

The PR includes a comprehensive new example file examples/language/unions.seq (211 lines) demonstrating:

• Option types
• Result types
• Message passing patterns
• Recursive tree structures

This is well-commented and educational, going beyond the scope of the bug fix. Great addition to the examples!

Recommendations

1. Extract error formatting logic to reduce duplication in typechecker.rs
2. Consider using constants for keyword lengths instead of magic numbers
3. Clarify the source name change ("seq" → "seqc") in the PR description if intentional
4. Consider adding span info to more statement types in future work (e.g., Quotation, literals) for even better error messages

Verdict: ✅ Approve with Minor Suggestions

This is a solid PR that meaningfully improves the developer experience. The systematic approach, excellent test coverage, and bonus example file demonstrate care and thoroughness. The minor duplication issues are nitpicks that don't block merging.

Great work! 🎉

[claude]

PR Review: Fix LSP

Summary

This PR enhances error reporting in the LSP by adding source span tracking to AST nodes (Statement::If, Statement::Match, MatchArm). This enables more precise diagnostic line numbers for type errors.

Code Quality ✅

Strengths:

• Systematic approach: The change is well-structured, adding span: Option<Span> fields to relevant AST nodes
• Consistent pattern matching updates: All pattern matches have been updated to handle the new span field (using span: _ where not needed)
• Good documentation: Added clear doc comments explaining the purpose of each span field
• Helper function: The format_line_prefix helper (typechecker.rs:16-20) is clean and reusable

Adherence to project conventions:

• Code follows the established patterns in the codebase
• Proper use of Option<Span> for optional metadata
• Tests updated to include span: None where appropriate

Test Coverage ✅

Excellent test additions in crates/lsp/src/diagnostics.rs:

• test_if_else_branch_mismatch_reports_correct_line (line 877)
• test_diagnostic_structure (line 908) - Great addition for verifying neovim compatibility
• test_stack_type_mismatch_reports_correct_line (line 941)
• test_match_arm_mismatch_reports_correct_line (line 977)

These tests verify that:

1. Line numbers are reported correctly for different error types
2. Diagnostic structure contains all required fields for editor integration
3. Error messages include proper line context

Test maintenance:

• REPL tests updated to clear history (crates/repl/src/app.rs:1919, 2008, 2036, 2083) - good cleanup
• All existing tests updated with span: None to maintain compatibility

Potential Issues & Suggestions

1. Parser span collection (crates/compiler/src/parser.rs)

The parser now captures span information:

fn parse_if(&mut self, start_line: usize, start_column: usize)
fn parse_match(&mut self, start_line: usize, start_column: usize)
fn parse_match_arm(&mut self) -> Result<MatchArm, String>

✅ This looks correct. The span points to the keyword (if, match) or variant name, which is exactly what users need for error location.

2. Error message formatting (crates/compiler/src/typechecker.rs)

The enhanced error messages conditionally include line numbers:

if if_line > 0 {
    format!("at line {}: if/else branches...", if_line, ...)
} else {
    format!("if/else branches...", ...)
}

Question: Why the conditional formatting? If span is None, if_line would be 0, but this could be confusing since "line 0" doesn't exist. Consider:

• Always including line info when available
• Or documenting why some AST nodes might not have spans (e.g., programmatically generated code)

3. Example file addition (examples/language/unions.seq)

• 211 lines of comprehensive union type examples
• Great documentation and clear progression from simple to complex
• Covers Option, Result, Message passing, and recursive trees
• ✅ Well-commented and follows Seq style

4. LSP diagnostic improvements (crates/lsp/src/diagnostics.rs:862-867)

code: Some(tower_lsp::lsp_types::NumberOrString::String(
    "type-error".to_string(),
)),
source: Some("seqc".to_string()),  // Changed from "seq"

Note: The source changed from "seq" to "seqc". This might affect existing editor configurations. Is this intentional? If so, worth mentioning in the PR description or CHANGELOG.

Security Concerns ✅

No security issues identified. The changes are purely about improving error reporting metadata.

Performance Considerations ✅

Minimal overhead:

• Option<Span> fields add negligible memory overhead
• Span data is small (line, column, length)
• No runtime performance impact on the compiled code
• Parser changes are straightforward position tracking

Minor Observations

1. Consistency in exhaustive matching: The pattern span: _ is used consistently throughout to ignore spans in pattern matches where they're not needed. This is good practice.

2. No breaking changes for existing code: All span fields are Option<Span>, making them backward compatible with tests and programmatically generated AST nodes.

3. Documentation update (examples/language/README.md:55-79): The union types section was added to document the new example. Nice touch!

Recommendations

1. ✅ Run full CI locally: Before merging, ensure just ci passes (as per CONTRIBUTING.md)
2. 📝 Consider documenting: Why some spans might be None (e.g., programmatically generated AST vs. parsed code)
3. 🤔 Clarify: Is the change from source: "seq" to source: "seqc" intentional? Document if it affects editor integrations

Overall Assessment

Strong PR 🎉

This is a well-executed enhancement that significantly improves the developer experience by providing precise error locations. The implementation is:

• Systematic and consistent
• Well-tested with meaningful test cases
• Minimally invasive (no breaking changes)
• Properly documented

The addition of the comprehensive unions.seq example is a bonus that will help users understand union types better.

Recommendation: Approve with minor clarifications requested above.

---

Review generated with Claude Code