Add metadata documents to the MAS workspace upfront #78886

dibarbet · 2025-06-10T01:51:35Z

Precursor to changes to allow LSP to use virtual files for MAS. This modifies the MAS providers to put the generated document into the workspace upfront, instead of waiting for someone to attempt to open it. This does mean that we may keep files in the workspace for longer, but generally we were keeping the project and file info around anyway in order to service requests when the document is re-opened. We can remove those separate stores and just use the workspace as the document holder.

Additionally, this simplifies the current LSP MAS implementation (now the MAS workspace is just a normal workspace which we pull documents from), and will simplify the virtual file implementation (virtual files will be stored as documents in the workspace that LSP can query as it normally does). Will be working on that in a followup

Resolves #78421

Clean RPS - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/644402

dibarbet · 2025-06-10T18:54:48Z

src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs

+        _keyToInformation.Clear();
+    }
+
+    private static (ProjectInfo, DocumentId) GenerateProjectAndDocumentInfo(


moved from metadataassourcegeneratedfileinfo

CyrusNajmabadi · 2025-06-10T21:32:21Z

src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs

@@ -50,15 +46,10 @@ internal sealed class DecompilationMetadataAsSourceFileProvider(IImplementationA
    /// generally run concurrently.  However, to be safe, we make this a concurrent dictionary to be safe to that
    /// potentially happening.
    /// </summary>
-    private readonly ConcurrentDictionary<string, MetadataAsSourceGeneratedFileInfo> _generatedFilenameToInformation = new(StringComparer.OrdinalIgnoreCase);
+    private readonly ConcurrentDictionary<string, (MetadataAsSourceGeneratedFileInfo Metadata, DocumentId DocumentId)> _generatedFilenameToInformation = new(StringComparer.OrdinalIgnoreCase);


might it make sense for MetadataAsSourceGeneratedFileInfo to contain the DocId? or is that not possible?

It isn't immediately straightforward as the MetadataAsSourceGeneratedFileInfo is created before we create the document (and stored in a separate map). However, I am changing all this a bit more for the virtual file part - so I'll include this feedback in that PR if it ends up working.

src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs

jasonmalinowski · 2025-06-19T23:46:06Z

src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs

@@ -383,50 +371,6 @@ public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string fil
        return _fileToDocumentInfoMap.TryGetValue(filePath, out _) && blockStructureOptions.CollapseMetadataImplementationsWhenFirstOpened;


I'm beginning to wonder why _fileToDocumentInfoMap needs to exist at all -- couldn't that now just be centrally held?

jasonmalinowski · 2025-06-19T23:50:46Z

src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs

@@ -51,15 +50,6 @@ internal sealed class LspMiscellaneousFilesWorkspaceProvider(ILspServices lspSer
            documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri.ParsedUri);
        }

-        var container = new StaticSourceTextContainer(documentText);


Can we delete StaticSourceTextContainer too?

jasonmalinowski · 2025-06-19T23:52:25Z

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

-        workspace.GetService<LspWorkspaceRegistrationService>().Register(workspace);
+        // By default, 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.
+        var lspWorkspaceRegistrationListener = (LspWorkspaceRegistrationEventListener)workspace.ExportProvider.GetExports<IEventListener>().Single(e => e.Value is LspWorkspaceRegistrationEventListener).Value;


Suggested change

var lspWorkspaceRegistrationListener = (LspWorkspaceRegistrationEventListener)workspace.ExportProvider.GetExports<IEventListener>().Single(e => e.Value is LspWorkspaceRegistrationEventListener).Value;

var lspWorkspaceRegistrationListener = (LspWorkspaceRegistrationEventListener)workspace.ExportProvider.GetExportedValues<IEventListener>().Single(e => e is LspWorkspaceRegistrationEventListener);

Since there's not any metadata you're checking before creating everything anyways.

jasonmalinowski · 2025-06-19T23:53:39Z

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

@@ -364,7 +365,11 @@ internal LspTestWorkspace CreateWorkspace(
            composition ?? Composition, workspaceKind, configurationOptions: new WorkspaceConfigurationOptions(ValidateCompilationTrackerStates: true), supportsLspMutation: mutatingLspWorkspace);
        options?.OptionUpdater?.Invoke(workspace.GetService<IGlobalOptionService>());

-        workspace.GetService<LspWorkspaceRegistrationService>().Register(workspace);
+        // By default, workspace event listeners are disabled in tests.  Explicitly enable the LSP workspace registration event listener


This feels a bit fishy that we're not enabling the listeners generally, but I appreciate this makes the code behave a bit more like the product rather than the explicit registration going on. Should we have a tracking bug to investigate this further though?

jasonmalinowski · 2025-06-19T23:55:10Z

src/EditorFeatures/CSharpTest/PdbSourceDocument/PdbSourceDocumentTests.cs

@@ -1022,7 +1022,7 @@ await RunTestAsync(async path =>
            var file = await service.GetGeneratedFileAsync(project.Solution.Workspace, project, symbol, signaturesOnly: false, options: MetadataAsSourceOptions.Default, cancellationToken: CancellationToken.None);

            var result = service.TryRemoveDocumentFromWorkspace(file.FilePath);
-            Assert.False(result);
+            Assert.True(result);


Should this still be returning false? I'd assume we don't say we removed something if we didn't add it in the first place? It seems like this is missing a different return for the "if not open" case.

this is deleted in the followup (open doesn't always make sense here)
#79023

jasonmalinowski · 2025-06-19T23:57:39Z

src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs

-
-                // We do own the file, so let's open it up in our workspace
-                (var projectInfo, documentId) = fileInfo.GetProjectInfoAndDocumentId(workspace.Services.SolutionServices, loadFileFromDisk: true);
+            workspace.OnDocumentClosed(documentIdInfo.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, fileInfo.TemporaryFilePath, MetadataAsSourceGeneratedFileInfo.Encoding));


Do we have to do that check for open files here?

jasonmalinowski · 2025-06-20T00:02:03Z

src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs

                .GetRequiredDocument(temporaryDocumentId);

-            if (useDecompiler)
+            // Generate the file if it doesn't exist (we may still have it if there was a previous request for it that was then closed).


I admit it's not clear to me the scenario for this comment...

jasonmalinowski · 2025-06-20T00:03:28Z

src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs

                }

-                // Mark read-only
-                new FileInfo(fileInfo.TemporaryFilePath).IsReadOnly = true;
+                if (!skipWritingFile && !File.Exists(fileInfo.TemporaryFilePath))


We're still under the !File.Exists() check you added on line 100 in the new version; isn't that part always true?

I think this is guarding against the file going away in between (though there is technically a mutex held there so not sure if it could)? I changed this in the followup though

github-actions bot added the Area-IDE label Jun 10, 2025

dotnet-policy-service bot added the VSCode label Jun 10, 2025

Add metadata documents to the MAS workspace upfront

a7e6b19

dibarbet force-pushed the metadata_virtual branch from 758b375 to a7e6b19 Compare June 10, 2025 18:45

dibarbet commented Jun 10, 2025

View reviewed changes

dibarbet marked this pull request as ready for review June 10, 2025 19:16

dibarbet requested a review from a team as a code owner June 10, 2025 19:16

dibarbet requested a review from jasonmalinowski June 10, 2025 19:16

CyrusNajmabadi reviewed Jun 10, 2025

View reviewed changes

src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs Show resolved Hide resolved

CyrusNajmabadi approved these changes Jun 10, 2025

View reviewed changes

Use singleordefault as there should only be one documentId per path

91702f6

build-analysis bot mentioned this pull request Jun 11, 2025

RestApiException`1: The response contained an invalid status code 500 Internal Server Error dotnet/dnceng#2298

Open

3 tasks

jasonmalinowski approved these changes Jun 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add metadata documents to the MAS workspace upfront #78886

Add metadata documents to the MAS workspace upfront #78886

Uh oh!

dibarbet commented Jun 10, 2025 •

edited

Loading

Uh oh!

dibarbet Jun 10, 2025

Uh oh!

CyrusNajmabadi Jun 10, 2025

Uh oh!

dibarbet Jun 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

dibarbet Jun 20, 2025

Uh oh!

jasonmalinowski Jun 19, 2025

Uh oh!

jasonmalinowski Jun 20, 2025

Uh oh!

jasonmalinowski Jun 20, 2025

Uh oh!

dibarbet Jun 20, 2025

Uh oh!

Uh oh!

		@@ -383,50 +371,6 @@ public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string fil
		return _fileToDocumentInfoMap.TryGetValue(filePath, out _) && blockStructureOptions.CollapseMetadataImplementationsWhenFirstOpened;

	var lspWorkspaceRegistrationListener = (LspWorkspaceRegistrationEventListener)workspace.ExportProvider.GetExports<IEventListener>().Single(e => e.Value is LspWorkspaceRegistrationEventListener).Value;
	var lspWorkspaceRegistrationListener = (LspWorkspaceRegistrationEventListener)workspace.ExportProvider.GetExportedValues<IEventListener>().Single(e => e is LspWorkspaceRegistrationEventListener);

Add metadata documents to the MAS workspace upfront #78886

Are you sure you want to change the base?

Add metadata documents to the MAS workspace upfront #78886

Uh oh!

Conversation

dibarbet commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dibarbet commented Jun 10, 2025 •

edited

Loading

dibarbet Jun 11, 2025 •

edited

Loading