001-lsp-server #197

aabs · 2026-01-23T04:08:52Z

PR Checklist (Fifth Language Engineering Constitution)

Please confirm each item before requesting review.

Builds the full solution without cancellation using documented commands
Tests added/updated first and now passing locally (unit/parser/integration as applicable)
No manual edits under src/ast-generated/; included metamodel/template changes and regeneration steps
For grammar changes: visitor updates and parser tests included
CLI behavior/contracts documented; outputs are deterministic and scriptable
Complexity increases are justified in the description
Versioning considered (SemVer); migration notes provided for breakages
Docs updated (README.md, AGENTS.md references) where behavior or commands changed

Summary

What changed and why?
User-visible impact (CLI, AST, grammar, codegen)?
Alternatives considered?

Validation

Provide commands you ran locally (paste exact output snippets as needed):

# Required sequence
DOTNET_CLI_TELEMETRY_OPTOUT=1 dotnet --info
java -version

dotnet restore fifthlang.sln
# Regenerate when changing AST metamodel/templates
# dotnet run --project src/ast_generator/ast_generator.csproj -- --folder src/ast-generated

dotnet build fifthlang.sln --no-restore

dotnet test test/ast-tests/ast_tests.csproj --no-build
# Optionally all tests
# dotnet test fifthlang.sln --no-build

Screenshots/Logs (optional)

Breaking Changes (if any)

Impacted users/scenarios:
Migration steps:

Related Issues/Links

…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.

… configuration

Copilot

Pull request overview

Implements the 001-lsp-server MVP by extending the .NET language server with semantic diagnostics + symbol-aware features, and adding a VS Code client extension (syntax highlighting + stdio LSP wiring), alongside supporting smoke tests and specs.

Changes:

Added semantic diagnostics in the parsing pipeline and published combined syntax+semantic diagnostics via LSP.
Implemented workspace symbol indexing to power hover, completion, and go-to-definition (including unopened files).
Added a VS Code client extension (TextMate grammar + language configuration + LSP client bootstrap) plus expanded smoke tests and spec artifacts.

Reviewed changes

Copilot reviewed 31 out of 34 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
test/language-server-smoke/packages.lock.json	Updates locked dependencies for smoke tests (new LSP/logging deps).
test/language-server-smoke/SmokeTests.cs	Adds smoke coverage for diagnostics updates, hover/definition across workspace files, and completions.
src/vscode-client/tsconfig.json	TypeScript build configuration for VS Code extension.
src/vscode-client/syntaxes/fifth.tmLanguage.json	Adds TextMate grammar for Fifth syntax highlighting.
src/vscode-client/src/extension.ts	Implements VS Code LSP client startup (stdio) and server path resolution.
src/vscode-client/package.json	Declares VS Code extension manifest, contributions, and dependencies.
src/vscode-client/package-lock.json	Locks extension npm dependencies.
src/vscode-client/language-configuration.json	Adds editor language configuration (comments/brackets/pairs).
src/vscode-client/dist/extension.js.map	Compiled source map output for extension.
src/vscode-client/dist/extension.js	Compiled JS output for extension.
src/vscode-client/README.md	Documents extension build/run/configuration steps.
src/language-server/packages.lock.json	Updates locked dependencies for language-server project.
src/language-server/SymbolService.cs	Implements workspace indexing + definition resolution and signature building.
src/language-server/Program.cs	Adds structured stderr logging + registers new handlers/services.
src/language-server/Parsing/ParsingService.cs	Adds compiler semantic analysis phase and returns semantic diagnostics.
src/language-server/Logging/StderrLoggerProvider.cs	Introduces stderr logger provider for structured server logs.
src/language-server/Handlers/HoverHandler.cs	Replaces placeholder hover with symbol-aware hover content.
src/language-server/Handlers/HandlerResults.cs	Adds helper for graceful empty definition results.
src/language-server/Handlers/DocumentSyncHandler.cs	Adds logging + publishes semantic diagnostics along with parser diagnostics.
src/language-server/Handlers/DefinitionHandler.cs	Adds go-to-definition handler backed by symbol index.
src/language-server/Handlers/CompletionHandler.cs	Enhances completion with keywords + symbol-driven items.
src/language-server/Fifth.LanguageServer.csproj	Adds explicit package references needed for LSP/logging usage.
src/language-server/DocumentStore.cs	Adds snapshot support for open document texts.
src/language-server/DocumentService.cs	Adds snapshot support for parsed documents.
specs/001-lsp-server/tasks.md	Task breakdown for phased delivery of LSP features.
specs/001-lsp-server/spec.md	Feature spec defining MVP stories/requirements for LSP support.
specs/001-lsp-server/research.md	Captures design decisions and alternatives considered.
specs/001-lsp-server/quickstart.md	Minimal quickstart for connecting an editor to the server.
specs/001-lsp-server/plan.md	Implementation plan and repo structure notes.
specs/001-lsp-server/data-model.md	Data model for documents/workspace/diagnostics/symbols.
specs/001-lsp-server/contracts/lsp.openapi.yaml	Planning contract (REST-style representation of key LSP interactions).
specs/001-lsp-server/checklists/requirements.md	Spec quality checklist for requirements completeness.
.github/copilot-instructions.md	Documents the new LSP server technology footprint in repo instructions.

Files not reviewed (1)

src/vscode-client/package-lock.json: Language not supported

Comments suppressed due to low confidence (1)

src/language-server/SymbolService.cs:37

GetWordAtPosition splits on \n but doesn’t account for Windows \r\n line endings, so line can include a trailing \r and position.Character/ranges can drift. Consider normalizing text (e.g., trim trailing \r per line) or using a line-splitting approach that handles both newline styles consistently.

    public (string word, LspRange range)? GetWordAtPosition(string text, Position position)
    {
        var lines = text.Split('\n');
        if (position.Line < 0 || position.Line >= lines.Length)
        {
            return null;
        }

        var line = lines[position.Line];
        if (position.Character < 0 || position.Character > line.Length)

Copilot · 2026-01-23T04:25:30Z

src/language-server/Handlers/DefinitionHandler.cs

+        var workspaceRoot = SymbolService.ResolveWorkspaceRoot(request.TextDocument.Uri.ToUri());
+        var index = _symbols.BuildWorkspaceIndex(workspaceRoot, _store.Snapshot(), _documents.SnapshotParsed());
+        var location = _symbols.FindDefinition(index, word.Value.word, request.TextDocument.Uri.ToUri());


DefinitionHandler rebuilds the workspace index for each go-to-definition request. Since this may parse/analyze many files, it can be slow and also duplicate work already done for hover/completion. Consider caching a workspace index per root (and updating it incrementally) rather than rebuilding per request.

Copilot · 2026-01-23T04:25:31Z

src/language-server/Handlers/CompletionHandler.cs

+            var requestUri = request.TextDocument.Uri.ToUri();
+            var workspaceRoot = SymbolService.ResolveWorkspaceRoot(requestUri);
+            var index = _symbols.BuildWorkspaceIndex(workspaceRoot, _store.Snapshot(), _documents.SnapshotParsed());
+
+            var allDefinitions = index.Definitions.Values.SelectMany(d => d).ToList();
+            var scopedDefinitions = allDefinitions.Where(d => d.Uri == requestUri).ToList();
+            if (scopedDefinitions.Count == 0)
+            {
+                scopedDefinitions = allDefinitions;
+            }
+


CompletionHandler rebuilds the workspace index per completion request, then flattens all entries (SelectMany) and later groups them. Combined with the per-identifier-occurrence indexing in SymbolService, this can get very expensive during typing. Recommend caching the index, and shaping it for completion (e.g., store per-file unique names and/or only definition symbols) to avoid repeated large allocations.

Copilot · 2026-01-23T04:25:31Z

src/language-server/Logging/StderrLoggerProvider.cs

+            var message = formatter(state, exception);
+            var timestamp = DateTimeOffset.UtcNow.ToString("yyyy-MM-ddTHH:mm:ss.fffZ");
+            var line = $"{timestamp} [{logLevel}] {_category}: {message}";
+            if (exception is not null)
+            {
+                line += $" | {exception}";
+            }
+
+            Console.Error.WriteLine(line);


The stderr logger prepends a UTC timestamp to every log line. This makes logs non-deterministic across runs, which conflicts with the stated MVP requirement of deterministic stderr logs (and complicates golden-file testing). Consider removing the timestamp (or making it opt-in via configuration) while keeping the rest of the structured fields.

Copilot · 2026-01-23T04:25:31Z

src/vscode-client/package-lock.json

+            "version": "1.108.1",
+            "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.108.1.tgz",


package.json declares @types/vscode as ^1.85.0 (and the extension engine is ^1.85.0), but the lockfile resolved @types/vscode to 1.108.1. This increases the risk of accidentally using VS Code APIs/types that don’t exist in the minimum supported engine version. Consider pinning @types/vscode to the same version as engines.vscode (or tightening the range) and regenerating the lockfile accordingly.

Suggested change

"version": "1.108.1",

"resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.108.1.tgz",

"version": "1.85.0",

"resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.85.0.tgz",

Copilot · 2026-01-23T04:25:31Z

src/language-server/SymbolService.cs

+            var parsed = false;
+            try
+            {
+                var ast = FifthParserManager.ParseFile(file) as AssemblyDef;
+                if (ast is not null)
                {
-                    Uri = uri,
-                    Range = range
-                }));
+                    var diagnostics = new List<compiler.Diagnostic>();
+                    var analyzed = FifthParserManager.ApplyLanguageAnalysisPhases(ast, diagnostics);
+                    if (analyzed is AssemblyDef analyzedAssembly)
+                    {
+                        AddDefinitionsFromAst(analyzedAssembly, uri, definitions);
+                        parsed = true;
+                    }
+                }
+            }
+            catch
+            {
+                parsed = false;
+            }
+
+            if (!parsed)
+            {
+                var text = openDocuments.TryGetValue(uri, out var openText)
+                    ? openText
+                    : File.ReadAllText(file);
+                AddDefinitionsFromText(text, uri, definitions);
            }


BuildWorkspaceIndex uses a bare catch and silently falls back to regex parsing. This hides real failures (e.g., IO issues, bugs in the parser/analysis) and makes debugging hard. Also, if parsing fails, the fallback path can still throw (e.g., File.ReadAllText when a file disappears mid-enumeration), potentially failing the entire LSP request. Suggest: (1) catch narrower exception types, (2) log the exception via the injected logger, and (3) guard file reads with try/catch and skip unreadable files rather than propagating.

Copilot · 2026-01-23T04:25:32Z

src/language-server/SymbolService.cs

+    private static void AddDefinitionsFromText(string text, Uri uri, Dictionary<string, List<SymbolDefinition>> definitions)
+    {
+        foreach (Match match in FunctionDefinitionRegex.Matches(text))
+        {
+            var name = match.Groups["name"].Value;
+            var parameters = match.Groups["params"].Value.Trim();
+            var returnType = match.Groups["return"].Value.Trim();
+            var signature = $"{name}({parameters}): {returnType}";
+
+            var (startLine, startCol) = CountLines(text, match.Index);
+            var (endLine, endCol) = CountLines(text, match.Index + name.Length);
+            var range = new LspRange(
+                new Position(startLine, startCol),
+                new Position(endLine, endCol));
+
+            var definition = new SymbolDefinition(name, uri, range, null, ast.SymbolKind.FunctionDef, signature);
+            if (!definitions.TryGetValue(name, out var functionList))
+            {
+                functionList = new List<SymbolDefinition>();
+                definitions[name] = functionList;
+            }
+            if (functionList.All(existing => existing.Uri != uri || existing.Range != range))
+            {
+                functionList.Add(definition);
+            }
+        }
+
+        var identifierMatch = IdentifierRegex.Match(text);
+        while (identifierMatch.Success)
+        {
+            var (startLine, startCol) = CountLines(text, identifierMatch.Index);
+            var (endLine, endCol) = CountLines(text, identifierMatch.Index + identifierMatch.Length);
+            var range = new LspRange(
+                new Position(startLine, startCol),
+                new Position(endLine, endCol));
+
+            var definition = new SymbolDefinition(identifierMatch.Value, uri, range, null, ast.SymbolKind.VoidSymbol, null);
+            if (!definitions.TryGetValue(identifierMatch.Value, out var list))
+            {
+                list = new List<SymbolDefinition>();
+                definitions[identifierMatch.Value] = list;
+            }
+            if (list.All(existing => existing.Uri != uri || existing.Range != range))
+            {
+                list.Add(definition);
+            }
+
+            identifierMatch = identifierMatch.NextMatch();
+        }


AddDefinitionsFromText records a VoidSymbol entry for every identifier occurrence (unique by range). For common identifiers this can explode memory and CPU (large per-name lists, then later SelectMany/GroupBy in completions). Consider indexing only real definitions (functions/types/etc.) and/or de-duplicating identifiers at the file level (store a single entry per name per file) instead of per occurrence.

Copilot · 2026-01-23T04:25:32Z

src/vscode-client/src/extension.ts

+    const serverDllPath = config.get<string>(
+        "serverDllPath",
+        "src/language-server/bin/Debug/net8.0/Fifth.LanguageServer.dll"
+    );


serverDllPath default here ("src/language-server/bin/Debug/net8.0/...") doesn’t match the configuration default declared in package.json ("../language-server/bin/Debug/net8.0/..."). This can lead to confusing behavior depending on whether the setting is defined. Suggest removing the second argument to config.get (so VS Code’s contributed default is used) or aligning both defaults to the same value.

Suggested change

const serverDllPath = config.get<string>(

"serverDllPath",

"src/language-server/bin/Debug/net8.0/Fifth.LanguageServer.dll"

);

const serverDllPath =

config.get<string>("serverDllPath") ??

"../language-server/bin/Debug/net8.0/Fifth.LanguageServer.dll";

Copilot · 2026-01-23T04:25:32Z

src/language-server/Fifth.LanguageServer.csproj

  <ItemGroup>
    <PackageReference Include="OmniSharp.Extensions.LanguageServer" Version="0.19.9" />
+    <PackageReference Include="OmniSharp.Extensions.LanguageProtocol" Version="0.19.9" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="8.0.0" />


Microsoft.Extensions.Logging.Console is referenced, but the code uses a custom StderrLoggerProvider and never calls AddConsole. If the intent is only to get LoggerFactory/Logger<> types, a more minimal reference (or relying on the transitive logging packages already brought in by OmniSharp) would reduce dependency surface and avoid forcing Microsoft.Extensions.* to unify to 8.0. Consider removing this package or switching to Microsoft.Extensions.Logging/Microsoft.Extensions.Logging.Abstractions as appropriate.

Suggested change

<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="8.0.0" />

Copilot · 2026-01-23T04:25:33Z

src/language-server/DocumentService.cs

    public bool TryGetParsed(Uri uri, out ParsedDocument document) => _parsedDocuments.TryGetValue(uri, out document!);
+
+    public IReadOnlyDictionary<Uri, ParsedDocument> SnapshotParsed() => new Dictionary<Uri, ParsedDocument>(_parsedDocuments);
 }


SnapshotParsed() copies _parsedDocuments, but _parsedDocuments is a non-thread-safe Dictionary. Since LSP requests/notifications can be handled concurrently, enumerating/copying while Update() is writing can throw or corrupt results. Consider switching _parsedDocuments to ConcurrentDictionary (matching DocumentStore) or guarding all accesses/snapshots with a lock.

Copilot · 2026-01-23T04:25:33Z

src/language-server/Handlers/HoverHandler.cs

+        var workspaceRoot = SymbolService.ResolveWorkspaceRoot(request.TextDocument.Uri.ToUri());
+        var index = _symbols.BuildWorkspaceIndex(workspaceRoot, _store.Snapshot(), _documents.SnapshotParsed());
+        var definition = _symbols.FindDefinition(index, word.Value.word, request.TextDocument.Uri.ToUri());


HoverHandler rebuilds a full workspace index on every hover request. This does filesystem enumeration + parse/analyze of unopened files, which is likely to violate the <500ms hover target on non-trivial workspaces. Recommend caching the workspace index (with invalidation on file changes / open-doc updates) and reusing it across hover/definition/completion requests.

aabs added 4 commits January 23, 2026 10:17

spec complete

add8239

Enhance LSP server documentation to clarify symbol resolution across …

8d8edd3

…workspace files, including unopened files, and update acceptance criteria and tasks accordingly.

feat: Add Fifth Language Client for VS Code with language support and…

fcd8e79

… configuration

Copilot AI review requested due to automatic review settings January 23, 2026 04:08

Copilot started reviewing on behalf of aabs January 23, 2026 04:09 View session

aabs merged commit baac48e into copilot/implement-lsp-for-ide-integration Jan 23, 2026
5 checks passed

aabs deleted the 001-lsp-server branch January 23, 2026 04:11

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

001-lsp-server #197

001-lsp-server #197

Uh oh!

aabs commented Jan 23, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		"version": "1.108.1",
		"resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.108.1.tgz",

001-lsp-server #197

001-lsp-server #197

Uh oh!

Conversation

aabs commented Jan 23, 2026

PR Checklist (Fifth Language Engineering Constitution)

Summary

Validation

Screenshots/Logs (optional)

Breaking Changes (if any)

Related Issues/Links

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants