From 74e05373d925d8e9b2e575008ff2b4b16a308ca0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 27 Aug 2024 13:14:23 -0700 Subject: [PATCH 01/11] Handler in progress --- .../Extensions/ProtocolConversions.cs | 7 +- .../RelatedDocumentsHandler.cs | 157 ++++++++++++++++++ ...odeAnalysis.LanguageServer.Protocol.csproj | 4 + .../VSInternalDocumentSpellCheckableParams.cs | 4 +- .../Protocol/Internal/VSInternalMethods.cs | 5 + .../VSInternalRelatedDocumentParams.cs | 44 +++++ 6 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs create mode 100644 src/LanguageServer/Protocol/Protocol/Internal/VSInternalRelatedDocumentParams.cs diff --git a/src/LanguageServer/Protocol/Extensions/ProtocolConversions.cs b/src/LanguageServer/Protocol/Extensions/ProtocolConversions.cs index 5c6ac4651692a..13a3bffdbdaaf 100644 --- a/src/LanguageServer/Protocol/Extensions/ProtocolConversions.cs +++ b/src/LanguageServer/Protocol/Extensions/ProtocolConversions.cs @@ -309,13 +309,14 @@ public static LSP.TextDocumentPositionParams PositionToTextDocumentPositionParam } public static LSP.TextDocumentIdentifier DocumentToTextDocumentIdentifier(TextDocument document) - => new LSP.TextDocumentIdentifier { Uri = document.GetURI() }; + => new() { Uri = document.GetURI() }; public static LSP.VersionedTextDocumentIdentifier DocumentToVersionedTextDocumentIdentifier(Document document) - => new LSP.VersionedTextDocumentIdentifier { Uri = document.GetURI() }; + => new() { Uri = document.GetURI() }; public static LinePosition PositionToLinePosition(LSP.Position position) - => new LinePosition(position.Line, position.Character); + => new(position.Line, position.Character); + public static LinePositionSpan RangeToLinePositionSpan(LSP.Range range) => new(PositionToLinePosition(range.Start), PositionToLinePosition(range.End)); diff --git a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs new file mode 100644 index 0000000000000..de7744ebf1638 --- /dev/null +++ b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs @@ -0,0 +1,157 @@ +// 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.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.RelatedDocuments; +using Microsoft.CodeAnalysis.Serialization; +using Microsoft.CodeAnalysis.Shared.Extensions; +using Microsoft.CodeAnalysis.Text; +using Microsoft.CommonLanguageServerProtocol.Framework; +using Roslyn.LanguageServer.Protocol; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.LanguageServer.Handler.RelatedDocuments; + +[Method(VSInternalMethods.CopilotRelatedDocumentsName)] +internal sealed class RelatedDocumentsHandler + : ILspServiceRequestHandler, + ITextDocumentIdentifierHandler +{ + /// + /// Cache where we store the data produced by prior requests so that they can be returned if nothing of significance + /// changed. The version key is produced by combining the checksums for project options and + /// + private readonly VersionedPullCache<(Checksum parseOptionsChecksum, Checksum textChecksum)?> _versionedCache = new(nameof(RelatedDocumentsHandler)); + + public bool MutatesSolutionState => false; + public bool RequiresLSPSolution => true; + + private static async Task<(Checksum parseOptionsChecksum, Checksum textChecksum)> ComputeChecksumsAsync(Document document, CancellationToken cancellationToken) + { + var project = document.Project; + var parseOptionsChecksum = project.State.GetParseOptionsChecksum(); + + var documentChecksumState = await document.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); + var textChecksum = documentChecksumState.Text; + + return (parseOptionsChecksum, textChecksum); + } + + public TextDocumentIdentifier? GetTextDocumentIdentifier(VSInternalRelatedDocumentParams requestParams) + => requestParams.TextDocument; + + /// + /// Retrieve the previous results we reported. Used so we can avoid resending data for unchanged files. + /// + private static ImmutableArray? GetPreviousResults(VSInternalRelatedDocumentParams requestParams) + => requestParams.PreviousResultId != null && requestParams.TextDocument != null + ? [new PreviousPullResult(requestParams.PreviousResultId, requestParams.TextDocument)] + // The client didn't provide us with a previous result to look for, so we can't lookup anything. + : null; + + public async Task HandleRequestAsync( + VSInternalRelatedDocumentParams requestParams, RequestContext context, CancellationToken cancellationToken) + { + context.TraceInformation($"{this.GetType()} started getting related documents"); + + // The progress object we will stream reports to. + using var progress = BufferedProgress.Create(requestParams.PartialResultToken); + + // Get the set of results the request said were previously reported. We can use this to determine both + // what to skip, and what files we have to tell the client have been removed. + var previousResults = GetPreviousResults(requestParams) ?? []; + context.TraceInformation($"previousResults.Length={previousResults.Length}"); + + // Create a mapping from documents to the previous results the client says it has for them. That way as we + // process documents we know if we should tell the client it should stay the same, or we can tell it what + // the updated spans are. + var documentToPreviousParams = GetDocumentToPreviousParams(context, previousResults); + + var solution = context.Solution; + var document = context.Document; + Contract.ThrowIfNull(solution); + Contract.ThrowIfNull(document); + + context.TraceInformation($"Processing: {document.FilePath}"); + + var relatedDocumentsService = document.GetLanguageService(); + if (relatedDocumentsService == null) + { + context.TraceInformation($"Ignoring document '{document.FilePath}' because it does not support related documents"); + } + else + { + var newResultId = await _versionedCache.GetNewResultIdAsync( + documentToPreviousParams, + document, + computeVersionAsync: async () => await ComputeChecksumsAsync(document, cancellationToken).ConfigureAwait(false), + cancellationToken).ConfigureAwait(false); + if (newResultId != null) + { + context.TraceInformation($"Version was changed for document: {document.FilePath}"); + + var linePosition = requestParams.Position is null + ? new LinePosition(0, 0) + : ProtocolConversions.PositionToLinePosition(requestParams.Position); + + var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); + var position = text.Lines.GetPosition(linePosition); + + await relatedDocumentsService.GetRelatedDocumentIdsAsync( + document, + position, + (relatedDocumentIds, cancellationToken) => + { + progress.Report(new VSInternalRelatedDocumentReport + { + ResultId = newResultId, + FilePaths = relatedDocumentIds.Select(id => solution.GetRequiredDocument(id).FilePath).WhereNotNull().ToArray(), + }); + + return ValueTaskFactory.CompletedTask; + }, + cancellationToken).ConfigureAwait(false); + } + else + { + context.TraceInformation($"Version was unchanged for document: {document.FilePath}"); + + // Nothing changed between the last request and this one. Report a (null-file-paths, same-result-id) + // response to the client as that means they should just preserve the current related file paths they + // have for this file. + var previousParams = documentToPreviousParams[document]; + progress.Report(new VSInternalRelatedDocumentReport { ResultId = previousParams.PreviousResultId }); + } + } + + // If we had a progress object, then we will have been reporting to that. Otherwise, take what we've been + // collecting and return that. + context.TraceInformation($"{this.GetType()} finished getting related documents"); + return progress.GetFlattenedValues(); + } + + private static Dictionary GetDocumentToPreviousParams( + RequestContext context, ImmutableArray previousResults) + { + Contract.ThrowIfNull(context.Solution); + + var result = new Dictionary(); + foreach (var requestParams in previousResults) + { + if (requestParams.TextDocument != null) + { + var document = context.Solution.GetDocument(requestParams.TextDocument); + if (document != null) + result[document] = requestParams; + } + } + + return result; + } +} diff --git a/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj b/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj index 73945f4ec700e..0072a8bcae842 100644 --- a/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj +++ b/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj @@ -117,6 +117,10 @@ + + + + diff --git a/src/LanguageServer/Protocol/Protocol/Internal/VSInternalDocumentSpellCheckableParams.cs b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalDocumentSpellCheckableParams.cs index c1b456342f95e..cb5b5f5b550aa 100644 --- a/src/LanguageServer/Protocol/Protocol/Internal/VSInternalDocumentSpellCheckableParams.cs +++ b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalDocumentSpellCheckableParams.cs @@ -8,9 +8,9 @@ namespace Roslyn.LanguageServer.Protocol using System.Text.Json.Serialization; /// - /// Parameter for tD/_vs_spellCheckableRanges. + /// Parameter for textDocument/_vs_spellCheckableRanges. /// - internal class VSInternalDocumentSpellCheckableParams : VSInternalStreamingParams, IPartialResultParams + internal sealed class VSInternalDocumentSpellCheckableParams : VSInternalStreamingParams, IPartialResultParams { /// [JsonPropertyName(Methods.PartialResultTokenName)] diff --git a/src/LanguageServer/Protocol/Protocol/Internal/VSInternalMethods.cs b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalMethods.cs index 19902a762fd0b..1be519d95de57 100644 --- a/src/LanguageServer/Protocol/Protocol/Internal/VSInternalMethods.cs +++ b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalMethods.cs @@ -9,6 +9,11 @@ namespace Roslyn.LanguageServer.Protocol /// internal static class VSInternalMethods { + /// + /// Method name for 'copilot/_related_documents'. + /// + public const string CopilotRelatedDocumentsName = "copilot/_related_documents"; + /// /// Method name for 'textDocument/foldingRange/_vs_refresh'. /// diff --git a/src/LanguageServer/Protocol/Protocol/Internal/VSInternalRelatedDocumentParams.cs b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalRelatedDocumentParams.cs new file mode 100644 index 0000000000000..e52be3d89dcf5 --- /dev/null +++ b/src/LanguageServer/Protocol/Protocol/Internal/VSInternalRelatedDocumentParams.cs @@ -0,0 +1,44 @@ +// 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. + +namespace Roslyn.LanguageServer.Protocol +{ + using System; + using System.Text.Json.Serialization; + + /// + /// Parameter for copilot/_related_documents. + /// + internal sealed class VSInternalRelatedDocumentParams : VSInternalStreamingParams, IPartialResultParams + { + /// + /// Gets or sets the value which indicates the position within the document. + /// + [JsonPropertyName("position")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public Position? Position { get; set; } + + /// + [JsonPropertyName(Methods.PartialResultTokenName)] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public IProgress? PartialResultToken { get; set; } + } + + internal sealed class VSInternalRelatedDocumentReport + { + /// + /// Gets or sets the server-generated version number for the related documents result. This is treated as a + /// black box by the client: it is stored on the client for each textDocument and sent back to the server when + /// requesting related documents. The server can use this result ID to avoid resending results + /// that had previously been sent. + /// + [JsonPropertyName("_vs_resultId")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string? ResultId { get; set; } + + [JsonPropertyName("_vs_file_paths")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public string[]? FilePaths { get; set; } + } +} From 18cea13cadd6b23f4202b22c48d733c99948e205 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 27 Aug 2024 13:20:14 -0700 Subject: [PATCH 02/11] Flesh out --- .../RelatedDocumentsHandler.cs | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs index de7744ebf1638..f366dfe69cdcc 100644 --- a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs +++ b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs @@ -2,11 +2,14 @@ // 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.Collections.Generic; using System.Collections.Immutable; +using System.Composition; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Host.Mef; using Microsoft.CodeAnalysis.RelatedDocuments; using Microsoft.CodeAnalysis.Serialization; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -17,10 +20,19 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.RelatedDocuments; +[ExportCSharpVisualBasicLspServiceFactory(typeof(RelatedDocumentsHandler)), Shared] +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] +internal sealed class RelatedDocumentsHandlerFactory() : ILspServiceFactory +{ + public ILspService CreateILspService(LspServices lspServices, WellKnownLspServerKinds serverKind) + => new RelatedDocumentsHandler(); +} + [Method(VSInternalMethods.CopilotRelatedDocumentsName)] internal sealed class RelatedDocumentsHandler : ILspServiceRequestHandler, - ITextDocumentIdentifierHandler + ITextDocumentIdentifierHandler { /// /// Cache where we store the data produced by prior requests so that they can be returned if nothing of significance @@ -43,7 +55,7 @@ internal sealed class RelatedDocumentsHandler return (parseOptionsChecksum, textChecksum); } - public TextDocumentIdentifier? GetTextDocumentIdentifier(VSInternalRelatedDocumentParams requestParams) + public TextDocumentIdentifier GetTextDocumentIdentifier(VSInternalRelatedDocumentParams requestParams) => requestParams.TextDocument; /// @@ -63,15 +75,7 @@ internal sealed class RelatedDocumentsHandler // The progress object we will stream reports to. using var progress = BufferedProgress.Create(requestParams.PartialResultToken); - // Get the set of results the request said were previously reported. We can use this to determine both - // what to skip, and what files we have to tell the client have been removed. - var previousResults = GetPreviousResults(requestParams) ?? []; - context.TraceInformation($"previousResults.Length={previousResults.Length}"); - - // Create a mapping from documents to the previous results the client says it has for them. That way as we - // process documents we know if we should tell the client it should stay the same, or we can tell it what - // the updated spans are. - var documentToPreviousParams = GetDocumentToPreviousParams(context, previousResults); + context.TraceInformation($"PreviousResultId={requestParams.PreviousResultId}"); var solution = context.Solution; var document = context.Document; @@ -87,6 +91,10 @@ internal sealed class RelatedDocumentsHandler } else { + var documentToPreviousParams = new Dictionary(); + if (requestParams.PreviousResultId != null) + documentToPreviousParams.Add(document, new PreviousPullResult(requestParams.PreviousResultId, requestParams.TextDocument)); + var newResultId = await _versionedCache.GetNewResultIdAsync( documentToPreviousParams, document, @@ -108,6 +116,8 @@ await relatedDocumentsService.GetRelatedDocumentIdsAsync( position, (relatedDocumentIds, cancellationToken) => { + // As the related docs services reports document ids to us, stream those immediately through our + // progress reporter. progress.Report(new VSInternalRelatedDocumentReport { ResultId = newResultId, @@ -125,8 +135,7 @@ await relatedDocumentsService.GetRelatedDocumentIdsAsync( // Nothing changed between the last request and this one. Report a (null-file-paths, same-result-id) // response to the client as that means they should just preserve the current related file paths they // have for this file. - var previousParams = documentToPreviousParams[document]; - progress.Report(new VSInternalRelatedDocumentReport { ResultId = previousParams.PreviousResultId }); + progress.Report(new VSInternalRelatedDocumentReport { ResultId = requestParams.PreviousResultId }); } } @@ -135,23 +144,4 @@ await relatedDocumentsService.GetRelatedDocumentIdsAsync( context.TraceInformation($"{this.GetType()} finished getting related documents"); return progress.GetFlattenedValues(); } - - private static Dictionary GetDocumentToPreviousParams( - RequestContext context, ImmutableArray previousResults) - { - Contract.ThrowIfNull(context.Solution); - - var result = new Dictionary(); - foreach (var requestParams in previousResults) - { - if (requestParams.TextDocument != null) - { - var document = context.Solution.GetDocument(requestParams.TextDocument); - if (document != null) - result[document] = requestParams; - } - } - - return result; - } } From c05f9a591f42fbb230c7b038c747c33effc89e22 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 27 Aug 2024 14:00:13 -0700 Subject: [PATCH 03/11] Add tests --- .../RelatedDocuments/RelatedDocumentsTests.cs | 137 ++++++++++++++++++ .../SpellCheck/SpellCheckTests.cs | 6 +- 2 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs new file mode 100644 index 0000000000000..58b2836a10bac --- /dev/null +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -0,0 +1,137 @@ +// 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.LanguageServer.Handler; +using Roslyn.LanguageServer.Protocol; +using Roslyn.Test.Utilities; +using Roslyn.Utilities; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.RelatedDocuments; + +public sealed class RelatedDocumentsTests(ITestOutputHelper testOutputHelper) + : AbstractLanguageServerProtocolTests(testOutputHelper) +{ + private static async Task RunGetRelatedDocumentsAsync( + TestLspServer testLspServer, + Uri uri, + string? previousResultId = null, + bool useProgress = false) + { + BufferedProgress? progress = useProgress ? BufferedProgress.Create(null) : null; + var spans = await testLspServer.ExecuteRequestAsync( + VSInternalMethods.CopilotRelatedDocumentsName, + new VSInternalRelatedDocumentParams + { + TextDocument = new TextDocumentIdentifier { Uri = uri }, + PreviousResultId = previousResultId, + PartialResultToken = progress, + }, + CancellationToken.None).ConfigureAwait(false); + + if (useProgress) + { + Assert.Null(spans); + spans = progress!.Value.GetFlattenedValues(); + } + + AssertEx.NotNull(spans); + return spans; + } + + [Theory, CombinatorialData] + public async Task ReferenceNoDocuments(bool mutatingLspWorkspace, bool useProgress) + { + var markup1 = """ + class X + { + } + """; + + var markup2 = """ + class Y + { + } + """; + + await using var testLspServer = await CreateTestLspServerAsync([markup1, markup2], mutatingLspWorkspace); + + var project = testLspServer.TestWorkspace.CurrentSolution.Projects.Single(); + var results = await RunGetRelatedDocumentsAsync( + testLspServer, + project.Documents.First().GetURI(), + useProgress: useProgress); + + Assert.Equal(0, results.Length); + } + + [Theory, CombinatorialData] + public async Task ReferenceSingleOtherDocument(bool mutatingLspWorkspace, bool useProgress) + { + var markup1 = """ + class X + { + Y y; + } + """; + + var markup2 = """ + class Y + { + } + """; + + await using var testLspServer = await CreateTestLspServerAsync([markup1, markup2], mutatingLspWorkspace); + + var project = testLspServer.TestWorkspace.CurrentSolution.Projects.Single(); + var results = await RunGetRelatedDocumentsAsync( + testLspServer, + project.Documents.First().GetURI(), + useProgress: useProgress); + + Assert.Equal(1, results.Length); + Assert.Equal(project.Documents.Last().FilePath, results[0].FilePaths.Single()); + } + + [Theory, CombinatorialData] + public async Task ReferenceMultipleOtherDocument(bool mutatingLspWorkspace, bool useProgress) + { + var markup1 = """ + class X + { + Y y; + Z z; + } + """; + + var markup2 = """ + class Y + { + } + """; + + var markup3 = """ + class Z + { + } + """; + + await using var testLspServer = await CreateTestLspServerAsync([markup1, markup2, markup3], mutatingLspWorkspace); + + var project = testLspServer.TestWorkspace.CurrentSolution.Projects.Single(); + var results = await RunGetRelatedDocumentsAsync( + testLspServer, + project.Documents.First().GetURI(), + useProgress: useProgress); + + Assert.Equal(1, results.Length); + Assert.Equal(2, results[0].FilePaths!.Length); + AssertEx.SetEqual([.. project.Documents.Skip(1).Select(d => d.FilePath)], results[0].FilePaths); + } +} diff --git a/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs b/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs index 775d6669a6b43..aef13e160cc99 100644 --- a/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs @@ -18,11 +18,9 @@ namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.SpellCheck { - public class SpellCheckTests : AbstractLanguageServerProtocolTests + public sealed class SpellCheckTests(ITestOutputHelper testOutputHelper) + : AbstractLanguageServerProtocolTests(testOutputHelper) { - public SpellCheckTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) - { - } #region Document From 686d17a9c646d289a91e33984dbe280af7e7919d Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 27 Aug 2024 20:59:01 -0700 Subject: [PATCH 04/11] revert --- .../Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj b/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj index 0072a8bcae842..73945f4ec700e 100644 --- a/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj +++ b/src/LanguageServer/Protocol/Microsoft.CodeAnalysis.LanguageServer.Protocol.csproj @@ -117,10 +117,6 @@ - - - - From 74d2fa50bda765907e8eaebea6148af63ea89f64 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 27 Aug 2024 20:59:55 -0700 Subject: [PATCH 05/11] Abs --- .../RelatedDocuments/AbstractRelatedDocumentsService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Features/Core/Portable/RelatedDocuments/AbstractRelatedDocumentsService.cs b/src/Features/Core/Portable/RelatedDocuments/AbstractRelatedDocumentsService.cs index 507e063daeb64..42a8920a612fd 100644 --- a/src/Features/Core/Portable/RelatedDocuments/AbstractRelatedDocumentsService.cs +++ b/src/Features/Core/Portable/RelatedDocuments/AbstractRelatedDocumentsService.cs @@ -94,7 +94,7 @@ private async ValueTask GetRelatedDocumentIdsInCurrentProcessAsync( // results to whatever client is calling into us. await ProducerConsumer.RunParallelAsync( // Order the nodes by the distance from the requested position. - IteratePotentialTypeNodes(root).OrderBy(t => t.expression.SpanStart - position), + IteratePotentialTypeNodes(root).OrderBy(t => Math.Abs(t.expression.SpanStart - position)), produceItems: (tuple, callback, _, cancellationToken) => { ProduceItems(tuple.expression, tuple.nameToken, callback, cancellationToken); From 348d5a8b273100bdc5c8f5dade7fce9f35c79541 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:19:27 -0700 Subject: [PATCH 06/11] Switch to early return --- .../RelatedDocumentsHandler.cs | 85 +++++++++---------- 1 file changed, 42 insertions(+), 43 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs index f366dfe69cdcc..2608991129a53 100644 --- a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs +++ b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs @@ -88,55 +88,54 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(VSInternalRelatedDocumen if (relatedDocumentsService == null) { context.TraceInformation($"Ignoring document '{document.FilePath}' because it does not support related documents"); + return; } - else - { - var documentToPreviousParams = new Dictionary(); - if (requestParams.PreviousResultId != null) - documentToPreviousParams.Add(document, new PreviousPullResult(requestParams.PreviousResultId, requestParams.TextDocument)); - var newResultId = await _versionedCache.GetNewResultIdAsync( - documentToPreviousParams, - document, - computeVersionAsync: async () => await ComputeChecksumsAsync(document, cancellationToken).ConfigureAwait(false), - cancellationToken).ConfigureAwait(false); - if (newResultId != null) - { - context.TraceInformation($"Version was changed for document: {document.FilePath}"); + var documentToPreviousParams = new Dictionary(); + if (requestParams.PreviousResultId != null) + documentToPreviousParams.Add(document, new PreviousPullResult(requestParams.PreviousResultId, requestParams.TextDocument)); - var linePosition = requestParams.Position is null - ? new LinePosition(0, 0) - : ProtocolConversions.PositionToLinePosition(requestParams.Position); + var newResultId = await _versionedCache.GetNewResultIdAsync( + documentToPreviousParams, + document, + computeVersionAsync: async () => await ComputeChecksumsAsync(document, cancellationToken).ConfigureAwait(false), + cancellationToken).ConfigureAwait(false); + if (newResultId != null) + { + context.TraceInformation($"Version was changed for document: {document.FilePath}"); + + var linePosition = requestParams.Position is null + ? new LinePosition(0, 0) + : ProtocolConversions.PositionToLinePosition(requestParams.Position); - var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); - var position = text.Lines.GetPosition(linePosition); + var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); + var position = text.Lines.GetPosition(linePosition); - await relatedDocumentsService.GetRelatedDocumentIdsAsync( - document, - position, - (relatedDocumentIds, cancellationToken) => + await relatedDocumentsService.GetRelatedDocumentIdsAsync( + document, + position, + (relatedDocumentIds, cancellationToken) => + { + // As the related docs services reports document ids to us, stream those immediately through our + // progress reporter. + progress.Report(new VSInternalRelatedDocumentReport { - // As the related docs services reports document ids to us, stream those immediately through our - // progress reporter. - progress.Report(new VSInternalRelatedDocumentReport - { - ResultId = newResultId, - FilePaths = relatedDocumentIds.Select(id => solution.GetRequiredDocument(id).FilePath).WhereNotNull().ToArray(), - }); - - return ValueTaskFactory.CompletedTask; - }, - cancellationToken).ConfigureAwait(false); - } - else - { - context.TraceInformation($"Version was unchanged for document: {document.FilePath}"); - - // Nothing changed between the last request and this one. Report a (null-file-paths, same-result-id) - // response to the client as that means they should just preserve the current related file paths they - // have for this file. - progress.Report(new VSInternalRelatedDocumentReport { ResultId = requestParams.PreviousResultId }); - } + ResultId = newResultId, + FilePaths = relatedDocumentIds.Select(id => solution.GetRequiredDocument(id).FilePath).WhereNotNull().ToArray(), + }); + + return ValueTaskFactory.CompletedTask; + }, + cancellationToken).ConfigureAwait(false); + } + else + { + context.TraceInformation($"Version was unchanged for document: {document.FilePath}"); + + // Nothing changed between the last request and this one. Report a (null-file-paths, same-result-id) + // response to the client as that means they should just preserve the current related file paths they + // have for this file. + progress.Report(new VSInternalRelatedDocumentReport { ResultId = requestParams.PreviousResultId }); } // If we had a progress object, then we will have been reporting to that. Otherwise, take what we've been From 1c27d2a8cddb5b67aa05a9411fb84cb893713464 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:34:44 -0700 Subject: [PATCH 07/11] Switch to early return --- .../RelatedDocumentsHandler.cs | 9 +++---- .../RelatedDocuments/RelatedDocumentsTests.cs | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs index 2608991129a53..85261644b8d3b 100644 --- a/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs +++ b/src/LanguageServer/Protocol/Handler/RelatedDocuments/RelatedDocumentsHandler.cs @@ -71,10 +71,6 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(VSInternalRelatedDocumen VSInternalRelatedDocumentParams requestParams, RequestContext context, CancellationToken cancellationToken) { context.TraceInformation($"{this.GetType()} started getting related documents"); - - // The progress object we will stream reports to. - using var progress = BufferedProgress.Create(requestParams.PartialResultToken); - context.TraceInformation($"PreviousResultId={requestParams.PreviousResultId}"); var solution = context.Solution; @@ -88,9 +84,12 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(VSInternalRelatedDocumen if (relatedDocumentsService == null) { context.TraceInformation($"Ignoring document '{document.FilePath}' because it does not support related documents"); - return; + return []; } + // The progress object we will stream reports to. + using var progress = BufferedProgress.Create(requestParams.PartialResultToken); + var documentToPreviousParams = new Dictionary(); if (requestParams.PreviousResultId != null) documentToPreviousParams.Add(document, new PreviousPullResult(requestParams.PreviousResultId, requestParams.TextDocument)); diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs index 58b2836a10bac..05de46f2c146e 100644 --- a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -134,4 +134,30 @@ class Z Assert.Equal(2, results[0].FilePaths!.Length); AssertEx.SetEqual([.. project.Documents.Skip(1).Select(d => d.FilePath)], results[0].FilePaths); } + + [Theory, CombinatorialData] + public async Task TestResultIds(bool mutatingLspWorkspace, bool useProgress) + { + var markup1 = """ + class X + { + Y y; + } + """; + + var markup2 = """ + class Y + { + } + """; + + await using var testLspServer = await CreateTestLspServerAsync([markup1, markup2], mutatingLspWorkspace); + + var project = testLspServer.TestWorkspace.CurrentSolution.Projects.Single(); + var results = await RunGetRelatedDocumentsAsync( + testLspServer, + project.Documents.First().GetURI(), + useProgress: useProgress); + AssertJsonEquals(results, new VSInternalRelatedDocumentReport[0]); + } } From 1ce5f40838e4c43054da352938bcdc8cff903210 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:40:23 -0700 Subject: [PATCH 08/11] tests --- .../LanguageServer/AbstractLanguageServerProtocolTests.cs | 2 +- .../RelatedDocuments/RelatedDocumentsTests.cs | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs b/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs index 764938a38ec41..7231d074d546c 100644 --- a/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs +++ b/src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs @@ -133,7 +133,7 @@ protected static void AssertEqualIgnoringWhitespace(string expected, string actu { var expectedWithoutWhitespace = Regex.Replace(expected, @"\s+", string.Empty); var actualWithoutWhitespace = Regex.Replace(actual, @"\s+", string.Empty); - Assert.Equal(expectedWithoutWhitespace, actualWithoutWhitespace); + AssertEx.Equal(expectedWithoutWhitespace, actualWithoutWhitespace); } /// diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs index 05de46f2c146e..51f0139e65475 100644 --- a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -130,9 +130,8 @@ class Z project.Documents.First().GetURI(), useProgress: useProgress); - Assert.Equal(1, results.Length); - Assert.Equal(2, results[0].FilePaths!.Length); - AssertEx.SetEqual([.. project.Documents.Skip(1).Select(d => d.FilePath)], results[0].FilePaths); + Assert.Equal(2, results.SelectMany(r => r.FilePaths).Count()); + AssertEx.SetEqual([.. project.Documents.Skip(1).Select(d => d.FilePath)], results.SelectMany(r => r.FilePaths)); } [Theory, CombinatorialData] From b86b46469e64b1d2044f16d859abc302f3c0ab1c Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:50:23 -0700 Subject: [PATCH 09/11] tests --- .../RelatedDocuments/RelatedDocumentsTests.cs | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs index 51f0139e65475..162f268f28ad2 100644 --- a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -157,6 +157,30 @@ class Y testLspServer, project.Documents.First().GetURI(), useProgress: useProgress); - AssertJsonEquals(results, new VSInternalRelatedDocumentReport[0]); + + AssertJsonEquals(results, new VSInternalRelatedDocumentReport[] + { + new() + { + ResultId = "RelatedDocumentsHandler:0", + FilePaths = [project.Documents.Last().FilePath!], + } + }); + + // Calling again, without a change, should return the old result id and no filepaths. + var results1 = await RunGetRelatedDocumentsAsync( + testLspServer, + project.Documents.First().GetURI(), + previousResultId: results.Single().ResultId, + useProgress: useProgress); + + AssertJsonEquals(results, new VSInternalRelatedDocumentReport[] + { + new() + { + ResultId = "RelatedDocumentsHandler:0", + FilePaths = null, + } + }); } } From f0fed71e971b0ff4debe62dc9fb3038c16b2f4b0 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:51:23 -0700 Subject: [PATCH 10/11] tests --- .../RelatedDocuments/RelatedDocumentsTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs index 162f268f28ad2..636e12d63552e 100644 --- a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -153,12 +153,12 @@ class Y await using var testLspServer = await CreateTestLspServerAsync([markup1, markup2], mutatingLspWorkspace); var project = testLspServer.TestWorkspace.CurrentSolution.Projects.Single(); - var results = await RunGetRelatedDocumentsAsync( + var results1 = await RunGetRelatedDocumentsAsync( testLspServer, project.Documents.First().GetURI(), useProgress: useProgress); - AssertJsonEquals(results, new VSInternalRelatedDocumentReport[] + AssertJsonEquals(results1, new VSInternalRelatedDocumentReport[] { new() { @@ -168,13 +168,13 @@ class Y }); // Calling again, without a change, should return the old result id and no filepaths. - var results1 = await RunGetRelatedDocumentsAsync( + var results2 = await RunGetRelatedDocumentsAsync( testLspServer, project.Documents.First().GetURI(), previousResultId: results.Single().ResultId, useProgress: useProgress); - AssertJsonEquals(results, new VSInternalRelatedDocumentReport[] + AssertJsonEquals(results2, new VSInternalRelatedDocumentReport[] { new() { From aa5c2bcad3366ef7248098a3f33a79d788e8a0c8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 28 Aug 2024 13:51:47 -0700 Subject: [PATCH 11/11] tests --- .../ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs index 636e12d63552e..02502fef59e14 100644 --- a/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/RelatedDocuments/RelatedDocumentsTests.cs @@ -171,7 +171,7 @@ class Y var results2 = await RunGetRelatedDocumentsAsync( testLspServer, project.Documents.First().GetURI(), - previousResultId: results.Single().ResultId, + previousResultId: results1.Single().ResultId, useProgress: useProgress); AssertJsonEquals(results2, new VSInternalRelatedDocumentReport[]