From e9a7720d1dd85b60ef99367dd575d8c6add3a696 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Mon, 22 Jul 2024 14:09:18 -0700 Subject: [PATCH 1/4] Add support for LSP language features in metadata --- .../NullResultMetadataAsSourceFileProvider.cs | 4 +- ...compilationMetadataAsSourceFileProvider.cs | 16 +++---- .../IMetadataAsSourceFileProvider.cs | 7 ++- .../IMetadataAsSourceFileService.cs | 11 ++++- .../MetadataAsSourceFileService.cs | 13 +++-- ...rceDocumentMetadataAsSourceFileProvider.cs | 11 +++-- .../Protocol/RoslynLanguageServer.cs | 2 +- .../LspMiscellaneousFilesWorkspace.cs | 47 ++++++++++++++----- .../LspMiscellaneousFilesWorkspaceProvider.cs | 29 ++++++++++++ .../Workspaces/LspWorkspaceManager.cs | 7 +-- 10 files changed, 108 insertions(+), 39 deletions(-) create mode 100644 src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs diff --git a/src/EditorFeatures/CSharpTest/PdbSourceDocument/NullResultMetadataAsSourceFileProvider.cs b/src/EditorFeatures/CSharpTest/PdbSourceDocument/NullResultMetadataAsSourceFileProvider.cs index 65dc2ed4d5d44..ddc805beef25f 100644 --- a/src/EditorFeatures/CSharpTest/PdbSourceDocument/NullResultMetadataAsSourceFileProvider.cs +++ b/src/EditorFeatures/CSharpTest/PdbSourceDocument/NullResultMetadataAsSourceFileProvider.cs @@ -4,6 +4,7 @@ using System; using System.Composition; +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Formatting; @@ -47,8 +48,9 @@ public void CleanupGeneratedFiles(MetadataAsSourceWorkspace workspace) return null; } - public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, Text.SourceTextContainer sourceTextContainer) + public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, Text.SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { + documentId = null!; return true; } diff --git a/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs index b410e66ecaa13..9e1e667a09142 100644 --- a/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs @@ -6,6 +6,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Composition; +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; using System.Linq; @@ -292,32 +293,30 @@ public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string fil return false; } - public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer) + public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { - AssertIsMainThread(workspace); - + // Serial access is guaranteed by the caller. if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) { Contract.ThrowIfTrue(_openedDocumentIds.ContainsKey(fileInfo)); // We do own the file, so let's open it up in our workspace - var (projectInfo, documentId) = fileInfo.GetProjectInfoAndDocumentId(workspace.Services.SolutionServices, loadFileFromDisk: true); + (var projectInfo, documentId) = fileInfo.GetProjectInfoAndDocumentId(workspace.Services.SolutionServices, loadFileFromDisk: true); workspace.OnProjectAdded(projectInfo); workspace.OnDocumentOpened(documentId, sourceTextContainer); _openedDocumentIds = _openedDocumentIds.Add(fileInfo, documentId); - return true; } + documentId = null; return false; } public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath) { - AssertIsMainThread(workspace); - + // Serial access is guaranteed by the caller. if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) { if (_openedDocumentIds.ContainsKey(fileInfo)) @@ -329,8 +328,7 @@ public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, private bool RemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, MetadataAsSourceGeneratedFileInfo fileInfo) { - AssertIsMainThread(workspace); - + // Serial access is guaranteed by the caller. var documentId = _openedDocumentIds.GetValueOrDefault(fileInfo); Contract.ThrowIfNull(documentId); diff --git a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs index efedd6fef1753..fc7545fa1733f 100644 --- a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Formatting; @@ -35,12 +36,16 @@ internal interface IMetadataAsSourceFileProvider /// /// Called when the file returned from needs to be added to the workspace, /// to be opened. Will be called on the main thread of the workspace host. + /// + /// Callers of this must guarantee serial access. /// - bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer); + bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId); /// /// Called when the file is being closed, and so needs to be removed from the workspace. Will be called on the /// main thread of the workspace host. + /// + /// Callers of this must guarantee serial access. /// bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath); diff --git a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileService.cs b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileService.cs index 4f0986e03ce2f..bf42a4c73fc68 100644 --- a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileService.cs +++ b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileService.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Diagnostics.CodeAnalysis; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Formatting; @@ -31,8 +32,16 @@ Task GetGeneratedFileAsync( MetadataAsSourceOptions options, CancellationToken cancellationToken); - bool TryAddDocumentToWorkspace(string filePath, SourceTextContainer buffer); + /// + /// Checks if the given file path is a metadata as source file and adds to the metadata workspace if it is. + /// Callers must ensure this is only called serially. + /// + bool TryAddDocumentToWorkspace(string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId); + /// + /// Checks if the given file path is a metadata as source file and removes from the metadata workspace if it is. + /// Callers must ensure this is only called serially. + /// bool TryRemoveDocumentFromWorkspace(string filePath); bool IsNavigableMetadataSymbol(ISymbol symbol); diff --git a/src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs b/src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs index 3db7aad7bca35..e21821dd8852a 100644 --- a/src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs +++ b/src/Features/Core/Portable/MetadataAsSource/MetadataAsSourceFileService.cs @@ -6,12 +6,12 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.ErrorReporting; -using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Shared.Utilities; @@ -163,7 +163,7 @@ private static void AssertIsMainThread(MetadataAsSourceWorkspace workspace) Contract.ThrowIfFalse(threadingService.IsOnMainThread); } - public bool TryAddDocumentToWorkspace(string filePath, SourceTextContainer sourceTextContainer) + public bool TryAddDocumentToWorkspace(string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { // If we haven't even created a MetadataAsSource workspace yet, then this file definitely cannot be added to // it. This happens when the MiscWorkspace calls in to just see if it can attach this document to the @@ -171,18 +171,19 @@ public bool TryAddDocumentToWorkspace(string filePath, SourceTextContainer sourc var workspace = _workspace; if (workspace != null) { - AssertIsMainThread(workspace); - foreach (var provider in _providers.Value) { if (!provider.IsValueCreated) continue; - if (provider.Value.TryAddDocumentToWorkspace(workspace, filePath, sourceTextContainer)) + if (provider.Value.TryAddDocumentToWorkspace(workspace, filePath, sourceTextContainer, out documentId)) + { return true; + } } } + documentId = null; return false; } @@ -194,8 +195,6 @@ public bool TryRemoveDocumentFromWorkspace(string filePath) var workspace = _workspace; if (workspace != null) { - AssertIsMainThread(workspace); - foreach (var provider in _providers.Value) { if (!provider.IsValueCreated) diff --git a/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs index 4e51fab6dd942..148817d46eef8 100644 --- a/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs @@ -7,6 +7,7 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; using System.Reflection.Metadata.Ecma335; @@ -355,23 +356,23 @@ public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string fil return _fileToDocumentInfoMap.TryGetValue(filePath, out _) && blockStructureOptions.CollapseMetadataImplementationsWhenFirstOpened; } - public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer) + public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { - AssertIsMainThread(workspace); - + // Serial access is guaranteed by the caller. if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) { workspace.OnDocumentOpened(info.DocumentId, sourceTextContainer); + documentId = info.DocumentId; return true; } + documentId = null; return false; } public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath) { - AssertIsMainThread(workspace); - + // Serial access is guaranteed by the caller. if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) { workspace.OnDocumentClosed(info.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, filePath, info.Encoding)); diff --git a/src/LanguageServer/Protocol/RoslynLanguageServer.cs b/src/LanguageServer/Protocol/RoslynLanguageServer.cs index 6aa1623551766..37c845e4e7578 100644 --- a/src/LanguageServer/Protocol/RoslynLanguageServer.cs +++ b/src/LanguageServer/Protocol/RoslynLanguageServer.cs @@ -99,7 +99,7 @@ private FrozenDictionary> GetBaseServices( // those cases, we do not need to add an additional workspace to manage new files we hear about. So only // add the LspMiscellaneousFilesWorkspace for hosts that have not already brought their own. if (serverKind == WellKnownLspServerKinds.CSharpVisualBasicLspServer) - AddLazyService(lspServices => new LspMiscellaneousFilesWorkspace(lspServices, hostServices)); + AddLazyService(lspServices => lspServices.GetRequiredService().CreateLspMiscellaneousFilesWorkspace(lspServices, hostServices)); return baseServiceMap.ToFrozenDictionary( keySelector: kvp => kvp.Key, diff --git a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs index bb5490d3220ea..ed9f599828eda 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs @@ -8,6 +8,7 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis.Features.Workspaces; using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.MetadataAsSource; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.Text; using Microsoft.CommonLanguageServerProtocol.Framework; @@ -24,27 +25,31 @@ namespace Microsoft.CodeAnalysis.LanguageServer /// Future work for this workspace includes supporting basic metadata references (mscorlib, System dlls, etc), /// but that is dependent on having a x-plat mechanism for retrieving those references from the framework / sdk. /// - internal sealed class LspMiscellaneousFilesWorkspace : Workspace, ILspService, ILspWorkspace + internal sealed class LspMiscellaneousFilesWorkspace(ILspServices lspServices, IMetadataAsSourceFileService metadataAsSourceFileService, HostServices hostServices) + : Workspace(hostServices, WorkspaceKind.MiscellaneousFiles), ILspService, ILspWorkspace { - private readonly ILspServices _lspServices; - - public LspMiscellaneousFilesWorkspace(ILspServices lspServices, HostServices hostServices) : base(hostServices, WorkspaceKind.MiscellaneousFiles) - { - _lspServices = lspServices; - } - public bool SupportsMutation => true; /// /// Takes in a file URI and text and creates a misc project and document for the file. /// - /// Calls to this method and are made + /// Calls to this method and are made /// from LSP text sync request handling which do not run concurrently. /// public Document? AddMiscellaneousDocument(Uri uri, SourceText documentText, string languageId, ILspLogger logger) { var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri); - var languageInfoProvider = _lspServices.GetRequiredService(); + + var container = new StaticSourceTextContainer(documentText); + if (metadataAsSourceFileService.TryAddDocumentToWorkspace(documentFilePath, container, out var documentId)) + { + var metadataWorkspace = metadataAsSourceFileService.TryGetWorkspace(); + Contract.ThrowIfNull(metadataWorkspace); + var document = metadataWorkspace.CurrentSolution.GetRequiredDocument(documentId); + return document; + } + + var languageInfoProvider = lspServices.GetRequiredService(); var languageInformation = languageInfoProvider.GetLanguageInformation(documentFilePath, languageId); if (languageInformation == null) { @@ -69,8 +74,14 @@ public LspMiscellaneousFilesWorkspace(ILspServices lspServices, HostServices hos /// Calls to this method and are made /// from LSP text sync request handling which do not run concurrently. /// - public void TryRemoveMiscellaneousDocument(Uri uri) + public void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadata) { + var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri); + if (removeFromMetadata && metadataAsSourceFileService.TryRemoveDocumentFromWorkspace(documentFilePath)) + { + return; + } + // We'll only ever have a single document matching this URI in the misc solution. var matchingDocument = CurrentSolution.GetDocumentIds(uri).SingleOrDefault(); if (matchingDocument != null) @@ -96,5 +107,19 @@ public ValueTask UpdateTextIfPresentAsync(DocumentId documentId, SourceText sour this.OnDocumentTextChanged(documentId, sourceText, PreservationMode.PreserveIdentity, requireDocumentPresent: false); return ValueTaskFactory.CompletedTask; } + + private class StaticSourceTextContainer(SourceText text) : SourceTextContainer + { + public override SourceText CurrentText => text; + + /// + /// Text changes are handled by LSP forking the document, we don't need to actually update anything here. + /// + public override event EventHandler TextChanged + { + add { } + remove { } + } + } } } diff --git a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs new file mode 100644 index 0000000000000..97ffe57bf2a45 --- /dev/null +++ b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs @@ -0,0 +1,29 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Composition; +using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.LanguageServer.Handler; +using Microsoft.CodeAnalysis.MetadataAsSource; +using Microsoft.CommonLanguageServerProtocol.Framework; + +namespace Microsoft.CodeAnalysis.LanguageServer; + +/// +/// Service to create instances. +/// This is not exported as a as it requires +/// special base language server dependencies such as the +/// +[ExportCSharpVisualBasicStatelessLspService(typeof(LspMiscellaneousFilesWorkspaceProvider)), Shared] +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] +internal class LspMiscellaneousFilesWorkspaceProvider(IMetadataAsSourceFileService metadataAsSourceFileService) : ILspService +{ + public LspMiscellaneousFilesWorkspace CreateLspMiscellaneousFilesWorkspace(ILspServices lspServices, HostServices hostServices) + { + return new LspMiscellaneousFilesWorkspace(lspServices, metadataAsSourceFileService, hostServices); + } +} diff --git a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs index 5a7b6a6a69a09..496460ba68510 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs @@ -145,8 +145,8 @@ public async ValueTask StopTrackingAsync(Uri uri, CancellationToken cancellation // If LSP changed, we need to compare against the workspace again to get the updated solution. _cachedLspSolutions.Clear(); - // Also remove it from our loose files workspace if it is still there. - _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri); + // Also remove it from our loose files or metadata workspace if it is still there. + _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadata: true); LspTextChanged?.Invoke(this, EventArgs.Empty); @@ -238,7 +238,8 @@ public void UpdateTrackedDocument(Uri uri, SourceText newSourceText) // As we found the document in a non-misc workspace, also attempt to remove it from the misc workspace // if it happens to be in there as well. if (workspace != _lspMiscellaneousFilesWorkspace) - _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri); + // Do not attempt to remove the file from the metadata workspace (the document is still open). + _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadata: false); return (workspace, document.Project.Solution, document); } From 8b79ee3a22b69ebddff7d0fdb11642f878e04959 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Mon, 22 Jul 2024 15:37:37 -0700 Subject: [PATCH 2/4] Add unit tests for LSP metadata --- .../Workspaces/LspWorkspaceManager.cs | 2 +- .../LspMetadataAsSourceWorkspaceTests.cs | 139 ++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs diff --git a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs index 496460ba68510..83b19cce439a2 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs @@ -256,7 +256,7 @@ public void UpdateTrackedDocument(Uri uri, SourceText newSourceText) { var miscDocument = _lspMiscellaneousFilesWorkspace?.AddMiscellaneousDocument(uri, trackedDocument.Text, trackedDocument.LanguageId, _logger); if (miscDocument is not null) - return (_lspMiscellaneousFilesWorkspace, miscDocument.Project.Solution, miscDocument); + return (miscDocument.Project.Solution.Workspace, miscDocument.Project.Solution, miscDocument); } return default; diff --git a/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs b/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs new file mode 100644 index 0000000000000..a2dcf43aa9b65 --- /dev/null +++ b/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs @@ -0,0 +1,139 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.MetadataAsSource; +using Roslyn.Test.Utilities; +using Roslyn.Utilities; +using Xunit; +using Xunit.Abstractions; +using LSP = Roslyn.LanguageServer.Protocol; + +namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.Metadata; + +public class LspMetadataAsSourceWorkspaceTests : AbstractLanguageServerProtocolTests +{ + public LspMetadataAsSourceWorkspaceTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) + { + } + + [Theory, CombinatorialData] + public async Task TestMetadataFile_OpenClosed(bool mutatingLspWorkspace) + { + var source = + """ + using System; + class A + { + void M() + { + Console.{|definition:WriteLine|}("Hello, World!"); + } + } + """; + + // Create a server with LSP misc file workspace and metadata service. + await using var testLspServer = await CreateTestLspServerAsync(source, mutatingLspWorkspace, new InitializationOptions { ServerKind = WellKnownLspServerKinds.CSharpVisualBasicLspServer }); + + // Get the metadata definition. + var location = testLspServer.GetLocations("definition").Single(); + var definition = await testLspServer.ExecuteRequestAsync(LSP.Methods.TextDocumentDefinitionName, + CreateTextDocumentPositionParams(location), CancellationToken.None); + + // Open the metadata file and verify it gets added to the metadata workspace. + await testLspServer.OpenDocumentAsync(definition.Single().Uri, text: string.Empty).ConfigureAwait(false); + + Assert.Equal(WorkspaceKind.MetadataAsSource, (await GetWorkspaceForDocument(testLspServer, definition.Single().Uri)).Kind); + AssertMiscFileWorkspaceEmpty(testLspServer); + + // Close the metadata file and verify it gets removed from the metadata workspace. + await testLspServer.CloseDocumentAsync(definition.Single().Uri).ConfigureAwait(false); + + AssertMetadataFileWorkspaceEmpty(testLspServer); + } + + [Theory, CombinatorialData] + public async Task TestMetadataFile_LanguageFeatures(bool mutatingLspWorkspace) + { + var source = + """ + using System; + class A + { + void M() + { + Console.{|definition:WriteLine|}("Hello, World!"); + } + } + """; + + var metadataSource = + """ + namespace System + { + public class Console + { + public static void WriteLine(string value) {} + } + } + """; + + // Create a server with LSP misc file workspace and metadata service. + await using var testLspServer = await CreateTestLspServerAsync(source, mutatingLspWorkspace, new InitializationOptions { ServerKind = WellKnownLspServerKinds.CSharpVisualBasicLspServer }); + + // Get the metadata definition. + var location = testLspServer.GetLocations("definition").Single(); + var definition = await testLspServer.ExecuteRequestAsync(LSP.Methods.TextDocumentDefinitionName, + CreateTextDocumentPositionParams(location), CancellationToken.None); + + // Open the metadata file and verify it gets added to the metadata workspace. + // We don't have the real metadata source, so just populate it with our fake metadata source. + await testLspServer.OpenDocumentAsync(definition.Single().Uri, text: metadataSource).ConfigureAwait(false); + var workspaceForDocument = await GetWorkspaceForDocument(testLspServer, definition.Single().Uri); + Assert.Equal(WorkspaceKind.MetadataAsSource, workspaceForDocument.Kind); + AssertMiscFileWorkspaceEmpty(testLspServer); + + // Manually register the workspace for followup requests - the workspace event listener that + // normally registers it on creation is not running in test code. + testLspServer.TestWorkspace.ExportProvider.GetExportedValue().Register(workspaceForDocument); + + var locationOfStringKeyword = new LSP.Location + { + Uri = definition.Single().Uri, + Range = new LSP.Range + { + Start = new LSP.Position { Line = 4, Character = 40 }, + End = new LSP.Position { Line = 4, Character = 40 } + } + }; + + var definitionFromMetadata = await testLspServer.ExecuteRequestAsync(LSP.Methods.TextDocumentDefinitionName, + CreateTextDocumentPositionParams(locationOfStringKeyword), CancellationToken.None); + + Assert.NotEmpty(definitionFromMetadata); + Assert.Contains("String.cs", definitionFromMetadata.Single().Uri.LocalPath); + } + + private static async Task GetWorkspaceForDocument(TestLspServer testLspServer, Uri fileUri) + { + var (lspWorkspace, _, _) = await testLspServer.GetManager().GetLspDocumentInfoAsync(new LSP.TextDocumentIdentifier { Uri = fileUri }, CancellationToken.None); + return lspWorkspace!; + } + + private static void AssertMiscFileWorkspaceEmpty(TestLspServer testLspServer) + { + var doc = testLspServer.GetManagerAccessor().GetLspMiscellaneousFilesWorkspace()!.CurrentSolution.Projects.SingleOrDefault()?.Documents.SingleOrDefault(); + Assert.Null(doc); + } + + private static void AssertMetadataFileWorkspaceEmpty(TestLspServer testLspServer) + { + var provider = testLspServer.TestWorkspace.ExportProvider.GetExportedValue(); + var metadataDocument = provider.TryGetWorkspace()?.CurrentSolution.Projects.SingleOrDefault()?.Documents.SingleOrDefault(); + Assert.Null(metadataDocument); + } +} From 4a64d1d895684ef9b6945ecc4a4abf95d094cfad Mon Sep 17 00:00:00 2001 From: David Barbet Date: Mon, 22 Jul 2024 15:50:59 -0700 Subject: [PATCH 3/4] Fix VS side --- .../AbstractMetadataAsSourceTests.TestContext.cs | 2 +- .../Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/EditorFeatures/Test/MetadataAsSource/AbstractMetadataAsSourceTests.TestContext.cs b/src/EditorFeatures/Test/MetadataAsSource/AbstractMetadataAsSourceTests.TestContext.cs index a1689f906e834..e462801ef3299 100644 --- a/src/EditorFeatures/Test/MetadataAsSource/AbstractMetadataAsSourceTests.TestContext.cs +++ b/src/EditorFeatures/Test/MetadataAsSource/AbstractMetadataAsSourceTests.TestContext.cs @@ -300,7 +300,7 @@ internal Document GetDocument(MetadataAsSourceFile file) using var reader = File.OpenRead(file.FilePath); var stringText = EncodedStringText.Create(reader); - Assert.True(_metadataAsSourceService.TryAddDocumentToWorkspace(file.FilePath, stringText.Container)); + Assert.True(_metadataAsSourceService.TryAddDocumentToWorkspace(file.FilePath, stringText.Container, out var _)); return stringText.Container.GetRelatedDocuments().Single(); } diff --git a/src/VisualStudio/Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs b/src/VisualStudio/Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs index ace82412c8ab2..1718b643f1d75 100644 --- a/src/VisualStudio/Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs +++ b/src/VisualStudio/Core/Def/ProjectSystem/MiscellaneousFilesWorkspace.cs @@ -259,7 +259,7 @@ private void AttachToDocument(string moniker, ITextBuffer textBuffer) { _threadingContext.ThrowIfNotOnUIThread(); - if (_fileTrackingMetadataAsSourceService.TryAddDocumentToWorkspace(moniker, textBuffer.AsTextContainer())) + if (_fileTrackingMetadataAsSourceService.TryAddDocumentToWorkspace(moniker, textBuffer.AsTextContainer(), out var _)) { // We already added it, so we will keep it excluded from the misc files workspace return; From 3940a4feafc16d0fef28516d1acfdda00fbc3860 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 23 Jul 2024 12:54:13 -0700 Subject: [PATCH 4/4] Review feedback --- ...compilationMetadataAsSourceFileProvider.cs | 75 ++++++++++--------- .../IMetadataAsSourceFileProvider.cs | 4 - ...rceDocumentMetadataAsSourceFileProvider.cs | 45 +++++------ .../LspMiscellaneousFilesWorkspace.cs | 4 +- .../LspMiscellaneousFilesWorkspaceProvider.cs | 2 +- .../Workspaces/LspWorkspaceManager.cs | 6 +- .../LspMetadataAsSourceWorkspaceTests.cs | 2 +- 7 files changed, 70 insertions(+), 68 deletions(-) diff --git a/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs index 9e1e667a09142..c9354eb78ae23 100644 --- a/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/MetadataAsSource/DecompilationMetadataAsSourceFileProvider.cs @@ -34,6 +34,11 @@ internal class DecompilationMetadataAsSourceFileProvider(IImplementationAssembly { internal const string ProviderName = "Decompilation"; + /// + /// Guards access to and workspace updates when opening / closing documents. + /// + private readonly object _gate = new(); + /// /// Accessed only in and , both of which /// are called under a lock in . So this is safe as a plain @@ -272,17 +277,8 @@ private async Task RelocateSymbol_NoLockAsync(Solution solution, Metad return await MetadataAsSourceHelpers.GetLocationInGeneratedSourceAsync(symbolId, temporaryDocument, cancellationToken).ConfigureAwait(false); } - private static void AssertIsMainThread(MetadataAsSourceWorkspace workspace) - { - Contract.ThrowIfNull(workspace); - var threadingService = workspace.Services.GetRequiredService().Service; - Contract.ThrowIfFalse(threadingService.IsOnMainThread); - } - public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string filePath, BlockStructureOptions blockStructureOptions) { - AssertIsMainThread(workspace); - if (_generatedFilenameToInformation.TryGetValue(filePath, out var info)) { return info.SignaturesOnly @@ -295,38 +291,42 @@ public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string fil public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { - // Serial access is guaranteed by the caller. - if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) + lock (_gate) { - Contract.ThrowIfTrue(_openedDocumentIds.ContainsKey(fileInfo)); + if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) + { + Contract.ThrowIfTrue(_openedDocumentIds.ContainsKey(fileInfo)); - // We do own the file, so let's open it up in our workspace - (var projectInfo, documentId) = fileInfo.GetProjectInfoAndDocumentId(workspace.Services.SolutionServices, loadFileFromDisk: true); + // 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.OnProjectAdded(projectInfo); - workspace.OnDocumentOpened(documentId, sourceTextContainer); + workspace.OnProjectAdded(projectInfo); + workspace.OnDocumentOpened(documentId, sourceTextContainer); - _openedDocumentIds = _openedDocumentIds.Add(fileInfo, documentId); - return true; - } + _openedDocumentIds = _openedDocumentIds.Add(fileInfo, documentId); + return true; + } - documentId = null; - return false; + documentId = null; + return false; + } } public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath) { - // Serial access is guaranteed by the caller. - if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) + lock (_gate) { - if (_openedDocumentIds.ContainsKey(fileInfo)) - return RemoveDocumentFromWorkspace(workspace, fileInfo); - } + if (_generatedFilenameToInformation.TryGetValue(filePath, out var fileInfo)) + { + if (_openedDocumentIds.ContainsKey(fileInfo)) + return RemoveDocumentFromWorkspace_NoLock(workspace, fileInfo); + } - return false; + return false; + } } - private bool RemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, MetadataAsSourceGeneratedFileInfo fileInfo) + private bool RemoveDocumentFromWorkspace_NoLock(MetadataAsSourceWorkspace workspace, MetadataAsSourceGeneratedFileInfo fileInfo) { // Serial access is guaranteed by the caller. var documentId = _openedDocumentIds.GetValueOrDefault(fileInfo); @@ -357,16 +357,19 @@ private bool RemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, Me public void CleanupGeneratedFiles(MetadataAsSourceWorkspace workspace) { - // Clone the list so we don't break our own enumeration - foreach (var generatedFileInfo in _generatedFilenameToInformation.Values.ToList()) + lock (_gate) { - if (_openedDocumentIds.ContainsKey(generatedFileInfo)) - RemoveDocumentFromWorkspace(workspace, generatedFileInfo); - } + // Clone the list so we don't break our own enumeration + foreach (var generatedFileInfo in _generatedFilenameToInformation.Values.ToList()) + { + if (_openedDocumentIds.ContainsKey(generatedFileInfo)) + RemoveDocumentFromWorkspace_NoLock(workspace, generatedFileInfo); + } - _generatedFilenameToInformation.Clear(); - _keyToInformation.Clear(); - Contract.ThrowIfFalse(_openedDocumentIds.IsEmpty); + _generatedFilenameToInformation.Clear(); + _keyToInformation.Clear(); + Contract.ThrowIfFalse(_openedDocumentIds.IsEmpty); + } } private static async Task GetUniqueDocumentKeyAsync(Project project, INamedTypeSymbol topLevelNamedType, bool signaturesOnly, CancellationToken cancellationToken) diff --git a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs index fc7545fa1733f..c06d750ebcc7b 100644 --- a/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/MetadataAsSource/IMetadataAsSourceFileProvider.cs @@ -36,16 +36,12 @@ internal interface IMetadataAsSourceFileProvider /// /// Called when the file returned from needs to be added to the workspace, /// to be opened. Will be called on the main thread of the workspace host. - /// - /// Callers of this must guarantee serial access. /// bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId); /// /// Called when the file is being closed, and so needs to be removed from the workspace. Will be called on the /// main thread of the workspace host. - /// - /// Callers of this must guarantee serial access. /// bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath); diff --git a/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs b/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs index 148817d46eef8..80244c24928b8 100644 --- a/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs +++ b/src/Features/Core/Portable/PdbSourceDocument/PdbSourceDocumentMetadataAsSourceFileProvider.cs @@ -44,6 +44,11 @@ internal sealed class PdbSourceDocumentMetadataAsSourceFileProvider( private readonly IImplementationAssemblyLookupService _implementationAssemblyLookupService = implementationAssemblyLookupService; private readonly IPdbSourceDocumentLogger? _logger = logger; + /// + /// Lock to guard access to workspace updates when opening / closing documents. + /// + private readonly object _gate = new(); + /// /// Accessed only in and , both of which /// are called under a lock in . So this is safe as a plain @@ -343,43 +348,39 @@ private ImmutableArray CreateDocumentInfos( return documents.ToImmutableAndClear(); } - private static void AssertIsMainThread(MetadataAsSourceWorkspace workspace) - { - Contract.ThrowIfNull(workspace); - var threadingService = workspace.Services.GetRequiredService().Service; - Contract.ThrowIfFalse(threadingService.IsOnMainThread); - } - public bool ShouldCollapseOnOpen(MetadataAsSourceWorkspace workspace, string filePath, BlockStructureOptions blockStructureOptions) { - AssertIsMainThread(workspace); return _fileToDocumentInfoMap.TryGetValue(filePath, out _) && blockStructureOptions.CollapseMetadataImplementationsWhenFirstOpened; } public bool TryAddDocumentToWorkspace(MetadataAsSourceWorkspace workspace, string filePath, SourceTextContainer sourceTextContainer, [NotNullWhen(true)] out DocumentId? documentId) { - // Serial access is guaranteed by the caller. - if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) + lock (_gate) { - workspace.OnDocumentOpened(info.DocumentId, sourceTextContainer); - documentId = info.DocumentId; - return true; - } + if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) + { + workspace.OnDocumentOpened(info.DocumentId, sourceTextContainer); + documentId = info.DocumentId; + return true; + } - documentId = null; - return false; + documentId = null; + return false; + } } public bool TryRemoveDocumentFromWorkspace(MetadataAsSourceWorkspace workspace, string filePath) { - // Serial access is guaranteed by the caller. - if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) + lock (_gate) { - workspace.OnDocumentClosed(info.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, filePath, info.Encoding)); - return true; - } + if (_fileToDocumentInfoMap.TryGetValue(filePath, out var info)) + { + workspace.OnDocumentClosed(info.DocumentId, new WorkspaceFileTextLoader(workspace.Services.SolutionServices, filePath, info.Encoding)); + return true; + } - return false; + return false; + } } public Project? MapDocument(Document document) diff --git a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs index ed9f599828eda..0dbbfe966ee2c 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspace.cs @@ -74,10 +74,10 @@ internal sealed class LspMiscellaneousFilesWorkspace(ILspServices lspServices, I /// Calls to this method and are made /// from LSP text sync request handling which do not run concurrently. /// - public void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadata) + public void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadataWorkspace) { var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri); - if (removeFromMetadata && metadataAsSourceFileService.TryRemoveDocumentFromWorkspace(documentFilePath)) + if (removeFromMetadataWorkspace && metadataAsSourceFileService.TryRemoveDocumentFromWorkspace(documentFilePath)) { return; } diff --git a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs index 97ffe57bf2a45..61dd7b2ae8dc3 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspMiscellaneousFilesWorkspaceProvider.cs @@ -20,7 +20,7 @@ namespace Microsoft.CodeAnalysis.LanguageServer; [ExportCSharpVisualBasicStatelessLspService(typeof(LspMiscellaneousFilesWorkspaceProvider)), Shared] [method: ImportingConstructor] [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] -internal class LspMiscellaneousFilesWorkspaceProvider(IMetadataAsSourceFileService metadataAsSourceFileService) : ILspService +internal sealed class LspMiscellaneousFilesWorkspaceProvider(IMetadataAsSourceFileService metadataAsSourceFileService) : ILspService { public LspMiscellaneousFilesWorkspace CreateLspMiscellaneousFilesWorkspace(ILspServices lspServices, HostServices hostServices) { diff --git a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs index 83b19cce439a2..d872296e74639 100644 --- a/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs +++ b/src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs @@ -146,7 +146,7 @@ public async ValueTask StopTrackingAsync(Uri uri, CancellationToken cancellation _cachedLspSolutions.Clear(); // Also remove it from our loose files or metadata workspace if it is still there. - _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadata: true); + _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadataWorkspace: true); LspTextChanged?.Invoke(this, EventArgs.Empty); @@ -238,8 +238,10 @@ public void UpdateTrackedDocument(Uri uri, SourceText newSourceText) // As we found the document in a non-misc workspace, also attempt to remove it from the misc workspace // if it happens to be in there as well. if (workspace != _lspMiscellaneousFilesWorkspace) + { // Do not attempt to remove the file from the metadata workspace (the document is still open). - _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadata: false); + _lspMiscellaneousFilesWorkspace?.TryRemoveMiscellaneousDocument(uri, removeFromMetadataWorkspace: false); + } return (workspace, document.Project.Solution, document); } diff --git a/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs b/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs index a2dcf43aa9b65..be4d17c5e185e 100644 --- a/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Metadata/LspMetadataAsSourceWorkspaceTests.cs @@ -15,7 +15,7 @@ namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.Metadata; -public class LspMetadataAsSourceWorkspaceTests : AbstractLanguageServerProtocolTests +public sealed class LspMetadataAsSourceWorkspaceTests : AbstractLanguageServerProtocolTests { public LspMetadataAsSourceWorkspaceTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) {