Fix file-based programs getting stuck in the host workspace #79730

jasonmalinowski · 2025-08-01T01:29:44Z

Fix file-based programs getting stuck in the host workspace

If a file is loaded that can be treated as a file-based program, and then a real project is loaded that contains that file, we should
unload the file-based program since we're treating that as a miscellaneous file. But we had a bug here: the code in LspWorkspaceManager that loops through workspaces assumes that the miscellaneous files workspace would be found last, so if we find a document in any other workspace first, we're good to use it. This assumption was broken once we started putting file-based programs in the host workspace, since we might find both the file-based program and the 'real' project in the same workspace.

The fix is straightforward: if when we're finding a file context we end up with a miscellaneous file, check if we have any other files too, and if so, prefer those. This will then allow us to unload the miscellaneous file as normal.

The majority of this PR is trying to reuse our existing LSP miscellaneous files workspace tests to also apply to the file-based programs. I was expecting that to also flag this issue, but it required a few extra tweaks in the end. I'd highly recommend doing this commit-at-a-time, and or at least look at the last commit first (which has the actual fix) so you can tell the difference between the fix and all the other work.

Fixes dotnet/vscode-csharp#8471

...iagnosticsTestUtilities/CodeActionsLegacy/AbstractCodeActionOrUserDiagnosticTest_NoEditor.cs

src/LanguageServer/ProtocolUnitTests/Options/SolutionAnalyzerConfigOptionsUpdaterTests.cs

src/LanguageServer/Protocol.TestUtilities/AbstractLspMiscellaneousFilesWorkspaceTests.cs

RikkiGibson · 2025-08-04T17:01:57Z

Taking a look this morning.

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

RikkiGibson · 2025-08-04T17:14:21Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

        {
-            var document = await lspSolution.GetTextDocumentAsync(textDocumentIdentifier, cancellationToken).ConfigureAwait(false);
-            if (document != null)
+            var documents = await lspSolution.GetTextDocumentsAsync(textDocumentIdentifier.DocumentUri, cancellationToken).ConfigureAwait(false);


I am curious about the decision to the check in this method, is there a reason the other callers would not want to "prioritize" matching non-misc over misc files?

That's a good question. This logic depends on knowing that there's a misc files provider service, so we'd have to fetch that in the extension method. @dibarbet any thoughts?

Sounds like the plan here is to do a larger refactoring.

src/LanguageServer/Protocol.TestUtilities/AbstractLspMiscellaneousFilesWorkspaceTests.cs

...iagnosticsTestUtilities/CodeActionsLegacy/AbstractCodeActionOrUserDiagnosticTest_NoEditor.cs

src/LanguageServer/ProtocolUnitTests/Workspaces/LspWorkspaceManagerTests.cs

jasonmalinowski · 2025-08-02T01:10:09Z

src/LanguageServer/Protocol.TestUtilities/AbstractLspMiscellaneousFilesWorkspaceTests.cs


-    private static async Task AssertFileInMiscWorkspaceAsync(TestLspServer testLspServer, DocumentUri fileUri)
+    [Theory, CombinatorialData]
+    public async Task TestLspTransfersFromMiscellaneousFilesToHostWorkspaceAsync(bool mutatingLspWorkspace, bool waitForWorkspace, bool fileBasedProgramContent)


This test moved from the LspWorkspaceManagerTests, and gets a few upgrades to also work for file-based programs too.

jasonmalinowski · 2025-08-02T01:12:13Z

src/LanguageServer/Protocol.TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

        InitializationOptions? options, string? workspaceKind, bool mutatingLspWorkspace, TestComposition? composition = null)
    {
        var workspace = new LspTestWorkspace(
-            composition ?? Composition, workspaceKind, configurationOptions: new WorkspaceConfigurationOptions(ValidateCompilationTrackerStates: true), supportsLspMutation: mutatingLspWorkspace);


I'm dropping the ValidateCompliationTrackerStates here; for this to work in the "real" MEF composition we need to inject a test part, which isn't in the product. I did go down the path of allowing test parts (since at the time I had a second part to also add), but it didn't seem worth the trouble since this can be added at lower-level solution tests anyways.

If we do want to keep it around, I can have this be conditioned on the underlying mock being present, so we at least test it in the TestComposition-based tests.

jasonmalinowski · 2025-08-02T01:12:52Z

src/LanguageServer/Protocol.TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

+        // By default in most MEF containers, workspace event listeners are disabled in tests.  Explicitly enable the LSP workspace registration event listener
+        // to ensure that the lsp workspace registration service sees all workspaces. If we're running tests against our full LSP server
+        // composition, we don't expect the mock to exist and the real thing is running.


This is a bit foul in that we shouldn't be creating this test workspace at all in the file-based tests, since there we have the "real" host workspace. But that'll require more decoupling.

jasonmalinowski · 2025-08-02T01:15:13Z

src/Workspaces/CoreTestUtilities/Logging/TestOutputLoggerProvider.cs

+    /// <summary>
+    /// The <see cref="ITestOutputHelper" /> given to us from xUnit. This is nulled out once we dispose this logger provider;
+    /// xUnit will abort a test run if something writes to this when a test isn't running; that's helpful for debugging,
+    /// but if a test fails we might still have asynchronous work logging in the background that didn't cleanly shut down. We don't want
+    /// an entire test run failing for that.
+    /// </summary>
+    private ITestOutputHelper? _testOutputHelper = testOutputHelper;


Not sure how I feel about this.

I guess this comment is just warning about what may be going on, in the event that the logger throws an exception?

Ah. It's really about whether to keep it non-null so that subsequent logging attempts would throw. I am comfortable with the current way, thanks. I would suggest to either name the constructor parameter exactly the same or use an ordinary constructor so that the testOutputHelper cannot be referenced and does not pollute the completion list. No strong need to make a change though.

src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs

src/LanguageServer/Protocol.TestUtilities/AbstractLspMiscellaneousFilesWorkspaceTests.cs

dibarbet · 2025-08-04T17:25:16Z

src/LanguageServer/Protocol/Handler/ServerLifetime/LspServiceLifeCycleManager.cs

        var cancellationToken = CancellationToken.None;

-        var hostWorkspace = _lspWorkspaceRegistrationService.GetAllRegistrations().SingleOrDefault(w => w.Kind == WorkspaceKind.Host);
+        var hostWorkspace = _lspWorkspaceRegistrationService.GetAllRegistrations().FirstOrDefault(w => w.Kind == WorkspaceKind.Host);


What needed this change? I would not generally expect there to be two host workspaces ever.

Unfortunately with this refactoring as it is, there are two host workspaces being created. There's the real one, and then the one that CreateTestLspServerAsync is creating from the markups that are being passed into the various helpers. It creates a TestWorkspace which then gets passed around. There is a deeper cleanup needed to make that go away -- a lot of the use of the TestWorkspace is to get workspace services or the MEF export provider. There's also a lot of places where the workspace gets handed around to give it back to a static helper method which is a bit strange in places too.

I don't see any fundamental blockers other than it'll take a bit of time. Unless you are particularly concerned I'll add a comment here just to clarify what's going on.

This is also a case where it's a bit strange we're getting a host workspace just to get this other service -- like why is this a workspace service in the first place? What does it mean to be a workspace service?

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

dibarbet

approving with followup requested

TestWorkspace only had constructors that took TestCompositions, but this reintroduces a constructor that takes an ExportProvider directly if the MEF composition requires too much setup -- specifically our LSP server tests that already have their own system for creating MEF compositions. A few things got cleaned up along the way: 1. The Composition property (which was oddly nullable) gets removed entirely; a few tests that were creating a second TestWorkspace to mirror a first one are able to just use the ExportProvider directly. 2. We had a hack where we'd inject a TestWorkspaceConfigurationService into the MEF container if the call to create the TestWorkspace passed along custom options. This just adds that type directly into the base configuration so we can reuse the TestCompositions better. Any tests trying to use that feature and creating custom MEF compositons are just expected to inject that type.

The problem here was that adding that dependency carries along many things we don't want to put into the MEF composition, but we now have a subfolder we can use for MEF discovery that contains just the product binaries we expect.

We have a set of tests for miscellaneous files tests, and this now runs that against both our base implementation and also the file-based application implementation in our language server. This required a few subtle changes: 1. Making AbstractLanguageServerProtocolTests deal with test workspaces that aren't our usual MEF TestCompositions. This is because these tests against the real server implementation are using the code that creates our real MEF container. Our code assumed we had some test mocks available, but in the real product those aren't there. We can gracefully skip those since we don't need them to properly test the end-to-end product. 2. Switching the tests away from assuming there is a misc files workspace but rather that a given documenet may or may not be a miscellaneous files document. This gets rid of a TODO we had referring to this, and although required a lot of changes it's a mechanical change.

This moves one test into the abstract base, and also fixes the tests to avoid using TestLspServer.TestWorkspace when in the real server tests we will be using our other host workspace implementation.

If a file is loaded that can be treated as a file-based program, and then a real project is loaded that contains that file, we should unload the file-based program since we're treating that as a miscellaneous file. But we had a bug here: the code in LspWorkspaceManager that loops through workspaces assumes that the miscellaneous files workspace would be found last, so if we find a document in any other workspace first, we're good to use it. This assumption was broken once we started putting file-based programs in the host workspace, since we might find both the file-based program and the 'real' project in the same workspace. The fix is straightforward: if when we're finding a file context we amd we end up with a miscellaneous file, check if we have any other files too, and if so, prefer those. This will then allow us to unload the miscellaneous file as normal. Fixes dotnet/vscode-csharp#8471

Preparing for prerelease patch for #79730

dibarbet · 2025-08-05T00:21:42Z

/backport to release/vscode

github-actions · 2025-08-05T00:21:49Z

Started backporting to release/vscode: https://github.com/dotnet/roslyn/actions/runs/16737265389

github-actions · 2025-08-05T00:24:17Z

@dibarbet backporting to "release/vscode" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Refactor how TestWorkspaces interact with TestCompositions
Using index info to reconstruct a base tree...
M	src/Features/DiagnosticsTestUtilities/CodeActionsLegacy/AbstractCodeActionOrUserDiagnosticTest_NoEditor.cs
M	src/LanguageServer/Protocol.TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
M	src/LanguageServer/Protocol.TestUtilities/Workspaces/LspTestWorkspace.cs
M	src/LanguageServer/ProtocolUnitTests/Workspaces/LspWorkspaceManagerTests.cs
M	src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs
M	src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs
M	src/Workspaces/CoreTestUtilities/Workspaces/TestWorkspace`1.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Features/DiagnosticsTestUtilities/CodeActionsLegacy/AbstractCodeActionOrUserDiagnosticTest_NoEditor.cs
Auto-merging src/LanguageServer/Protocol.TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
Auto-merging src/LanguageServer/Protocol.TestUtilities/Workspaces/LspTestWorkspace.cs
Auto-merging src/LanguageServer/ProtocolUnitTests/Workspaces/LspWorkspaceManagerTests.cs
Auto-merging src/VisualStudio/Core/Test.Next/Services/ServiceHubServicesTests.cs
Auto-merging src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs
CONFLICT (content): Merge conflict in src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs
Auto-merging src/Workspaces/CoreTestUtilities/Workspaces/TestWorkspace`1.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Refactor how TestWorkspaces interact with TestCompositions
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

jasonmalinowski self-assigned this Aug 1, 2025

github-actions bot added the Area-IDE label Aug 1, 2025

dotnet-policy-service bot added the VSCode label Aug 1, 2025

jasonmalinowski force-pushed the fix-file-based-programs-getting-stuck-in-workspace branch from 648f6de to d3d4c3e Compare August 1, 2025 05:02

JoeRobich reviewed Aug 1, 2025

View reviewed changes

...iagnosticsTestUtilities/CodeActionsLegacy/AbstractCodeActionOrUserDiagnosticTest_NoEditor.cs Show resolved Hide resolved

src/LanguageServer/ProtocolUnitTests/Options/SolutionAnalyzerConfigOptionsUpdaterTests.cs Outdated Show resolved Hide resolved

jasonmalinowski force-pushed the fix-file-based-programs-getting-stuck-in-workspace branch from d3d4c3e to 7518987 Compare August 2, 2025 01:04

jasonmalinowski marked this pull request as ready for review August 2, 2025 01:05

jasonmalinowski requested a review from a team as a code owner August 2, 2025 01:05

jasonmalinowski commented Aug 2, 2025

View reviewed changes

src/LanguageServer/Protocol.TestUtilities/AbstractLspMiscellaneousFilesWorkspaceTests.cs Outdated Show resolved Hide resolved

RikkiGibson self-assigned this Aug 4, 2025