Centralize Language Server workspace folder tracking - #85105
Conversation
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
6e9de4b to
9145fff
Compare
9145fff to
b6fb32a
Compare
b6fb32a to
c9ec94a
Compare
c9ec94a to
9523c9d
Compare
9523c9d to
d613ce0
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Workspace folder path normalization currently doesn’t handle trailing directory separators, which can lead to duplicate/unremovable entries with PathUtilities.Comparer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — NormalizePath uses Path.GetFullPath but does not normalize trailing directory separators. Since… |
|
src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs — Add a regression test that covers workspace-folder URIs that differ only by a trailing directory… |
What changed in this PR
This PR introduces a new Language Server service (IWorkspaceFolderTracker) to centralize tracking of local workspace-folder paths, initializes it during initialize, and updates it on workspace/didChangeWorkspaceFolders notifications. It also migrates existing file-based-program components away from IInitializeManager’s previous folder-state.
Changes:
- Add
IWorkspaceFolderTracker/WorkspaceFolderTrackerand register it as a server service. - Populate initial workspace folders during
initializeand update them duringworkspace/didChangeWorkspaceFolders. - Remove workspace-folder tracking from
IInitializeManagerand update file-based-program consumers to use the new tracker.
| File | Description |
|---|---|
| src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs | Adds unit tests for tracker behavior (path semantics, non-file URIs). |
| src/LanguageServer/Protocol/RoslynLanguageServer.cs | Registers IWorkspaceFolderTracker as a base service. |
| src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs | Implements synchronized add/remove/query of normalized workspace-folder paths. |
| src/LanguageServer/Protocol/Handler/ServerLifetime/InitializeHandler.cs | Seeds the tracker with InitializeParams.WorkspaceFolders. |
| src/LanguageServer/Protocol/Handler/ServerLifetime/DidChangeWorkspaceFoldersNotificationHandler.cs | Handles workspace-folder change notifications by removing/adding entries in the tracker. |
| src/LanguageServer/Protocol/Handler/IWorkspaceFolderTracker.cs | Defines the tracker service contract used across handlers/components. |
| src/LanguageServer/Protocol/Handler/InitializeManager.cs | Removes workspace-folder state; keeps only initialize params/capabilities. |
| src/LanguageServer/Protocol/Handler/IInitializeManager.cs | Drops the workspace-folder API from the initialize manager interface. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsEntryPointDiscovery.cs | Switches to reading workspace folders from IWorkspaceFolderTracker. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CsprojInConeChecker.cs | Switches to reading workspace folders from IWorkspaceFolderTracker. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Daemon/ClientProcessMonitorTests.cs | Mechanical file header/BOM update only. |
d613ce0 to
a244fa3
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Workspace folder “normalization” currently preserves trailing directory separators, which can lead to duplicate entries and failed removals unless canonicalized (and ideally covered by a regression test).
Review tier: Lite
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — NormalizePath uses Path.GetFullPath but does not normalize trailing directory separators. Since… View comment |
|
src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs — Add a regression test that covers workspace-folder URIs that differ only by a trailing directory… View comment |
Suppressed comments (2)
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs:81
- NormalizePath only calls Path.GetFullPath, which preserves trailing directory separators. That means the tracker can treat the same workspace folder as distinct entries (e.g. "C:\Repo" vs "C:\Repo\" or "/repo" vs "/repo/") and Remove may fail to match if the client switches representations. Since this type is intended to normalize/compare workspace folders with platform path semantics, it should canonicalize directory paths by trimming trailing separators (except for the root).
private static string NormalizePath(string path)
=> Path.GetFullPath(path);
src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs:67
- The current tests cover case-sensitivity differences and non-file URIs, but they don’t cover normalization of trailing directory separators. Without trimming trailing separators, the tracker can retain duplicate entries for the same folder or fail to remove a folder when the client changes URI formatting (with/without a trailing '/'). Adding a regression test here would lock in the intended normalization behavior.
private static WorkspaceFolder CreateWorkspaceFolder(DocumentUri uri)
=> new() { DocumentUri = uri, Name = "Workspace" };
}
a244fa3 to
24dc278
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new workspace-folder notification handler has an incorrect MutatesSolutionState setting (unnecessarily serializing work) and needs additional wiring-level coverage to prevent composition/dispatch regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/ServerLifetime/DidChangeWorkspaceFoldersNotificationHandler.cs — MutatesSolutionState is set to true, but this handler only updates the workspace-folder tracker and… |
|
src/LanguageServer/Protocol/Handler/ServerLifetime/DidChangeWorkspaceFoldersNotificationHandler.cs — This new notification handler introduces observable behavior (updating the authoritative… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — NormalizePath uses Path.GetFullPath but does not normalize trailing directory separators. Since… View comment |
|
src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs — Add a regression test that covers workspace-folder URIs that differ only by a trailing directory… View comment |
24dc278 to
844b90b
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The behavioral change is well-scoped, includes targeted tests, and only has minor follow-up nits noted in review comments.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — using System; is unnecessary here and will trigger unused-using analyzers (the file doesn’t… |
|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Remove allocates a builder and re-materializes the immutable array even when the input is… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — NormalizePath uses Path.GetFullPath but does not normalize trailing directory separators. Since… View comment |
|
src/LanguageServer/Protocol/Handler/ServerLifetime/DidChangeWorkspaceFoldersNotificationHandler.cs — This new notification handler introduces observable behavior (updating the authoritative… View comment |
|
src/LanguageServer/ProtocolUnitTests/WorkspaceFolderTrackerTests.cs — Add a regression test that covers workspace-folder URIs that differ only by a trailing directory… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/ServerLifetime/DidChangeWorkspaceFoldersNotificationHandler.cs — MutatesSolutionState is set to true, but this handler only updates the workspace-folder tracker and… View resolved comment |
844b90b to
11c5f2b
Compare
505e91b to
c30997d
Compare
c30997d to
9cb43f6
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A small but concrete test-utility robustness issue was found (missing null guard for the new response-factory delegate) that should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs — ConfigureHandler accepts a responseFactory delegate but doesn’t guard against a null value. If… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Path.GetFullPath can throw (e.g., invalid/unsupported/too-long paths). Since workspace folders… View comment |
| public void ConfigureHandler(bool mutatesSolutionState, bool requiresLspSolution, Func<RequestContext, CancellationToken, Task<TestConfigurableResponse>> responseFactory) | ||
| { | ||
| if (_mutatesSolutionState is not null || _requiresLSPSolution is not null || _response is not null) | ||
| if (_mutatesSolutionState is not null || _requiresLSPSolution is not null || _responseFactory is not null) | ||
| { | ||
| throw new InvalidOperationException($"{nameof(ConfigureHandler)} has already been called"); |
9cb43f6 to
949d262
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
WorkspaceFolderTracker.Update can still raise WorkspaceFoldersChanged for net-no-op remove+add updates, which can trigger unnecessary downstream work and should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Update uses ReferenceEquals to detect no-op updates, but a remove+add sequence that results in… |
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Path.GetFullPath can throw (e.g., invalid/unsupported/too-long paths). Since workspace folders… View comment |
|
src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs — ConfigureHandler accepts a responseFactory delegate but doesn’t guard against a null value. If… View comment |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Workspace-folder path normalization can throw on malformed client-provided file URIs (via Path.GetFullPath), risking a language-server crash during initialize or folder-change handling.
Review tier: Lite
Findings: 1
Pre-existing issues (3)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Path.GetFullPath can throw (e.g., invalid/unsupported/too-long paths). Since workspace folders… View comment |
|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Update uses ReferenceEquals to detect no-op updates, but a remove+add sequence that results in… View comment |
|
src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs — ConfigureHandler accepts a responseFactory delegate but doesn’t guard against a null value. If… View comment |
Suppressed comments (1)
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs:69
NormalizePathusesPath.GetFullPath, which can throw (e.g., malformed/empty file URI -> invalid path). Since workspace folders come from the client, a single bad workspace-folder URI could crash the language server during initialize or didChangeWorkspaceFolders. Consider treating path-normalization failures as non-fatal and ignoring that folder (return null) instead of throwing.
private static string? GetNormalizedFilePath(WorkspaceFolder workspaceFolder)
=> workspaceFolder.DocumentUri.ParsedDocumentUri?.IsFile == true
? NormalizePath(workspaceFolder.DocumentUri.GetDocumentFilePathFromUri())
: null;
f3d4ccb to
931c338
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes core LSP lifetime/workspace-folder state flow and introduces new cross-component update notifications that warrant maintainer validation in broader scenarios beyond the added tests.
Review tier: Lite
Findings: 1
Pre-existing issues (2)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Path.GetFullPath can throw (e.g., invalid/unsupported/too-long paths). Since workspace folders… View comment |
|
src/LanguageServer/ProtocolUnitTests/TestConfigurableDocumentHandler.cs — ConfigureHandler accepts a responseFactory delegate but doesn’t guard against a null value. If… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
src/LanguageServer/Protocol/Handler/WorkspaceFolderTracker.cs — Update uses ReferenceEquals to detect no-op updates, but a remove+add sequence that results in… View resolved comment |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Some small comments but could be done in a mop-up since everything else is stacked on this.
|
|
||
| internal sealed class WorkspaceFolderTracker : IWorkspaceFolderTracker | ||
| { | ||
| // Mutations are serialized by the request queue, but non-mutating requests may read the current folders concurrently. |
There was a problem hiding this comment.
Nit: use an XML doc comment here to mention the other field, so less of a chance this gets out of sync.
| if (removedFolders is not null) | ||
| { | ||
| foreach (var workspaceFolder in removedFolders) |
There was a problem hiding this comment.
Sometimes doing in removedFolders ?? [] is a nice shortcut...
| lock (_gate) | ||
| { |
There was a problem hiding this comment.
No reason to have the lock, especially since the comment said it wasn't needed.
| } | ||
|
|
||
| internal async Task FindAndLoadEntryPointsAsync() | ||
| private void OnWorkspaceFoldersChanged(ImmutableHashSet<string> _) |
There was a problem hiding this comment.
It's a bit strange the event handler is giving us the set and we're ignoring it. That's fine since we'll look at the "final" output, but maybe we just don't need the argument at all?
| internal ValueTask FindAndLoadEntryPointsAsync(CancellationToken cancellationToken) | ||
| => FindAndLoadEntryPointsAsync(globalOptionService, fileBasedProgramService, workspaceFolderTracker, lspServices, logger, cancellationToken); | ||
|
|
||
| private static async ValueTask FindAndLoadEntryPointsAsync( |
There was a problem hiding this comment.
Wasn't sure why this got made static.
There was a problem hiding this comment.
Other than to make the AsyncBatchingWorkQueue code duplicate the logic of the non-static overload.
|
/azp run roslyn-CI |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |



Summary
Introduce a synchronized Language Server service as the authoritative source of current local workspace folders.
workspace/didChangeWorkspaceFoldersnotifications.IInitializeManagerfolder state.Validation
WorkspaceFolderTrackerTests(2 passed)Microsoft Reviewers: Open in CodeFlow