-
Notifications
You must be signed in to change notification settings - Fork 0
Add Fifth LSP server with document sync, parsing, diagnostics, and basic hover/completion stubs #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: aabs <157775+aabs@users.noreply.github.com>
|
@copilot continue with the work |
Implemented the requested updates: added DI wiring and cancellation handling, registered document sync/hover/completion handlers, fixed handler implementations to compile against OmniSharp 0.19.9, and ensured the language server builds alongside the smoke test. |
Co-authored-by: aabs <157775+aabs@users.noreply.github.com>
|
@copilot continue with the implementation |
Will continue extending the LSP implementation now (handlers, capabilities, and document/diagnostic wiring). |
|
@copilot continue |
Continuing the implementation work now. |
aabs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please now implement the next phases in the issue's implementation plan (2.3 and 2.4 for parser integration)
Implemented phases 2.3/2.4 parser integration: added parsing and document services with AST/diagnostic caching, wired document sync to reparse on open/change/save and publish diagnostics, and added a smoke test for parsing diagnostics (commit e3b1b12). |
…lus smoke tests Co-authored-by: aabs <157775+aabs@users.noreply.github.com>
|
@copilot Proceed with the next stage |
Continuing to the next stage now. |
|
@copilot Now implement phase 3 from the implementation plan in the issue (core features) |
|
@copilot continue with the work |
Continuing with the core feature implementation now. |
…workspace files, including unopened files, and update acceptance criteria and tasks accordingly.
- Upgraded Microsoft.Extensions.* packages to version 8.0.0 in language server and smoke test projects. - Added StderrLoggerProvider for logging to standard error. - Implemented DefinitionHandler for resolving definitions in the language server. - Created HandlerResults utility for handling empty definition responses. - Added smoke tests for document service diagnostics, definition resolution, and completion handling. - Introduced Getting Started guide for running the Fifth Language Server in VS Code Insiders.
001-lsp-server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a Fifth Language Server Protocol (LSP) implementation to enable IDE integration. The server provides stdio-based communication, document synchronization, real-time diagnostics (syntax and semantic), hover information, symbol-based completion, and go-to-definition across workspace files.
Changes:
- Added LSP server executable with OmniSharp.Extensions.LanguageServer integration
- Implemented document management with in-memory parsing and AST caching
- Added workspace-wide symbol indexing that parses both open and unopened files
- Created VS Code extension client with TypeScript configuration
Reviewed changes
Copilot reviewed 34 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/language-server/Program.cs | LSP server entry point with DI and handler registration |
| src/language-server/Parsing/ParsingService.cs | Parser integration with syntax and semantic diagnostic extraction |
| src/language-server/DocumentService.cs | Document state management and parse result caching |
| src/language-server/SymbolService.cs | Workspace symbol indexing and definition resolution |
| src/language-server/Handlers/*.cs | LSP protocol handlers for sync, hover, completion, and definition |
| src/vscode-client/* | VS Code extension for connecting to the language server |
| test/language-server-smoke/*.cs | Smoke tests for LSP functionality |
| specs/001-lsp-server/*.md | Feature specifications and planning documents |
| fifthlang.sln | Solution file updates for new projects |
Files not reviewed (1)
- src/vscode-client/package-lock.json: Language not supported
| { | ||
| tree = parser.fifth(); | ||
| } | ||
| catch |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch-all exception handler on line 30 swallows all exceptions without logging them. While the comment states "Syntax errors are captured by the listener," this will also suppress parser crashes, runtime errors, or other unexpected exceptions that could indicate bugs. Consider at least logging unexpected exceptions before swallowing them, or being more specific about which exceptions are expected.
| catch | |
| catch (RecognitionException) |
| catch | ||
| { |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch-all exception handler on line 124 silently suppresses all parsing errors for workspace files. This makes it impossible to diagnose issues when files fail to parse. At minimum, failed parses should be logged with the file path and exception details so that developers can understand why symbols are missing from the workspace index.
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| Console.Error.WriteLine($"[SymbolService] Failed to parse workspace file '{file}': {ex}"); |
| using Microsoft.Extensions.Logging.Abstractions; | ||
| using OmniSharp.Extensions.LanguageServer.Protocol; | ||
| using OmniSharp.Extensions.LanguageServer.Protocol.Models; | ||
| using Xunit; |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test project uses xUnit (imported on line 7) rather than TUnit, which is inconsistent with the project's stated testing framework preference. According to the coding guidelines and other test projects in the codebase, TUnit is the preferred test framework. Consider migrating these tests to TUnit for consistency.
| } | ||
|
|
||
| return Directory.EnumerateFiles(workspaceRoot, "*.5th", SearchOption.AllDirectories) | ||
| .Where(path => !WorkspaceIgnoreSegments.Any(segment => path.Split(Path.DirectorySeparatorChar).Contains(segment))); |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path filtering logic on line 294 uses Contains() to check if any path segment matches ignore patterns, but this will incorrectly filter paths that contain these strings as substrings. For example, a file in "my-bin-utils/" would be filtered because "bin" appears as a substring. Use Path.GetFileName on each directory segment or ensure exact segment matching to avoid false positives.
| .Where(path => !WorkspaceIgnoreSegments.Any(segment => path.Split(Path.DirectorySeparatorChar).Contains(segment))); | |
| .Where(path => | |
| { | |
| var segments = path.Split( | |
| new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, | |
| StringSplitOptions.RemoveEmptyEntries); | |
| return !WorkspaceIgnoreSegments.Any(ignore => | |
| segments.Any(segment => | |
| string.Equals(segment, ignore, StringComparison.OrdinalIgnoreCase))); | |
| }); |
| .Select(p => $"{p.Name}: {p.TypeName.Value ?? p.TypeName.ToString()}") | ||
| .ToList(); | ||
| var returnTypeName = returnType.Name.Value ?? returnType.Name.ToString(); | ||
| var signature = $"{name}({string.Join(", ", parameterList)})"; | ||
| if (!string.IsNullOrWhiteSpace(returnTypeName)) | ||
| { | ||
| signature += $": {returnTypeName}"; | ||
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default 'ToString()': TypeName inherits 'ToString()' from 'Object', and so is not suitable for printing.
| .Select(p => $"{p.Name}: {p.TypeName.Value ?? p.TypeName.ToString()}") | |
| .ToList(); | |
| var returnTypeName = returnType.Name.Value ?? returnType.Name.ToString(); | |
| var signature = $"{name}({string.Join(", ", parameterList)})"; | |
| if (!string.IsNullOrWhiteSpace(returnTypeName)) | |
| { | |
| signature += $": {returnTypeName}"; | |
| } | |
| .Select(p => | |
| { | |
| var typeName = p.TypeName.Value; | |
| if (string.IsNullOrWhiteSpace(typeName)) | |
| { | |
| typeName = "unknown"; | |
| } | |
| return $"{p.Name}: {typeName}"; | |
| }) | |
| .ToList(); | |
| var returnTypeName = returnType.Name.Value; | |
| if (string.IsNullOrWhiteSpace(returnTypeName)) | |
| { | |
| returnTypeName = "unknown"; | |
| } | |
| var signature = $"{name}({string.Join(", ", parameterList)})"; | |
| if (!string.IsNullOrWhiteSpace(returnTypeName)) | |
| { | |
| signature += $": {returnTypeName}"; | |
| } |
| .Select(p => $"{p.Name}: {p.TypeName.Value ?? p.TypeName.ToString()}") | ||
| .ToList(); | ||
| var returnTypeName = returnType.Name.Value ?? returnType.Name.ToString(); |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default 'ToString()': TypeName inherits 'ToString()' from 'Object', and so is not suitable for printing.
| .Select(p => $"{p.Name}: {p.TypeName.Value ?? p.TypeName.ToString()}") | |
| .ToList(); | |
| var returnTypeName = returnType.Name.Value ?? returnType.Name.ToString(); | |
| .Select(p => $"{p.Name}: {p.TypeName.Value ?? string.Empty}") | |
| .ToList(); | |
| var returnTypeName = returnType.Name.Value ?? string.Empty; |
Introduces a Fifth Language Server Protocol executable with stdio wiring, document synchronization, parsing, and basic user-facing features. The server now reparses on open/change/save, publishes syntax diagnostics, offers a placeholder hover (line echo), and provides keyword-only completion as a safe default.
Changes Made
Testing
Original prompt
This section details on the original issue you should resolve
<issue_title>Implement Language Server Protocol (LSP) for IDE Integration</issue_title>
<issue_description># Implement Language Server Protocol (LSP) for IDE Integration
Labels:
arch-review,ide-support,lsp,criticalPriority: P0
Severity: CRITICAL
Epic: Architectural Improvements Q2 2026
Problem Summary
The compiler has no Language Server Protocol implementation, preventing integration with modern editors (VS Code, Neovim, Emacs, etc.). This severely limits the language's adoption potential and developer experience.
Current State
Impact
Adoption Barrier
Development Velocity
Feature Gap
Cannot implement standard IDE features:
Requirements
Core LSP Server
Document Management
Essential Features (MVP)
Diagnostics (textDocument/publishDiagnostics)
Hover (textDocument/hover)
Completion (textDocument/completion)
Go to Definition (textDocument/definition)
Advanced Features (Post-MVP)
Architecture
Project Structure
Key Components
WorkspaceService:
DocumentService:
ParsingService:
SymbolService:
Implementation Plan
Phase 1: Infrastructure (Weeks 1-4)
Phase 2: Document Synchronization (Weeks 5-8)
Phase 3: Core Features (Weeks 9-12)
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.