Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recalculate if LSP inlay hint cache miss #75709

Merged
merged 4 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,31 @@
// 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 System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeLens;
using Roslyn.Utilities;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Shared.Extensions;
using LSP = Roslyn.LanguageServer.Protocol;
using System.Text.Json;
using Roslyn.Utilities;
using StreamJsonRpc;
using LSP = Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.CodeLens;

[ExportCSharpVisualBasicStatelessLspService(typeof(CodeLensResolveHandler)), Shared]
[Method(LSP.Methods.CodeLensResolveName)]
internal sealed class CodeLensResolveHandler : ILspServiceDocumentRequestHandler<LSP.CodeLens, LSP.CodeLens>
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class CodeLensResolveHandler() : ILspServiceDocumentRequestHandler<LSP.CodeLens, LSP.CodeLens>
{
/// <summary>
/// Command name implemented by the client and invoked when the references code lens is selected.
/// </summary>
private const string ClientReferencesCommand = "roslyn.client.peekReferences";

public CodeLensResolveHandler()
{
}

public bool MutatesSolutionState => false;

public bool RequiresLSPSolution => true;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ public InlayHintCache() : base(maxCacheSize: 3)
/// <summary>
/// Cached data need to resolve a specific inlay hint item.
/// </summary>
internal record InlayHintCacheEntry(ImmutableArray<InlineHint> InlayHintMembers, VersionStamp SyntaxVersion);
internal record InlayHintCacheEntry(ImmutableArray<InlineHint> InlayHintMembers);
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,12 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(InlayHintParams request)
internal static async Task<LSP.InlayHint[]?> GetInlayHintsAsync(Document document, TextDocumentIdentifier textDocumentIdentifier, LSP.Range range, InlineHintsOptions options, bool displayAllOverride, InlayHintCache inlayHintCache, CancellationToken cancellationToken)
{
var text = await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
var textSpan = ProtocolConversions.RangeToTextSpan(range, text);

var inlineHintService = document.GetRequiredLanguageService<IInlineHintsService>();
var hints = await inlineHintService.GetInlineHintsAsync(document, textSpan, options, displayAllOverride, cancellationToken).ConfigureAwait(false);

var hints = await CalculateInlayHintsAsync(document, range, options, displayAllOverride, cancellationToken).ConfigureAwait(false);
var syntaxVersion = await document.GetSyntaxVersionAsync(cancellationToken).ConfigureAwait(false);

// Store the members in the resolve cache so that when we get a resolve request for a particular
// member we can re-use the inline hint.
var resultId = inlayHintCache.UpdateCache(new InlayHintCache.InlayHintCacheEntry(hints, syntaxVersion));
var resultId = inlayHintCache.UpdateCache(new InlayHintCache.InlayHintCacheEntry(hints));

var inlayHints = new LSP.InlayHint[hints.Length];
for (var i = 0; i < hints.Length; i++)
Expand Down Expand Up @@ -89,7 +85,7 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(InlayHintParams request)
ToolTip = null,
PaddingLeft = leftPadding,
PaddingRight = rightPadding,
Data = new InlayHintResolveData(resultId, i, textDocumentIdentifier)
Data = new InlayHintResolveData(resultId, i, textDocumentIdentifier, syntaxVersion.ToString(), range, displayAllOverride)
};

inlayHints[i] = inlayHint;
Expand All @@ -98,6 +94,16 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(InlayHintParams request)
return inlayHints;
}

internal static async Task<ImmutableArray<InlineHint>> CalculateInlayHintsAsync(Document document, LSP.Range range, InlineHintsOptions options, bool displayAllOverride, CancellationToken cancellationToken)
{
var text = await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
var textSpan = ProtocolConversions.RangeToTextSpan(range, text);

var inlineHintService = document.GetRequiredLanguageService<IInlineHintsService>();
var hints = await inlineHintService.GetInlineHintsAsync(document, textSpan, options, displayAllOverride, cancellationToken).ConfigureAwait(false);
return hints;
}

/// <summary>
/// Goes through the tagged text of the hint and trims off leading and trailing spaces.
/// If there is leading or trailing space, then we want to add padding to the left and right accordingly.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 Microsoft.CodeAnalysis.Text;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.InlayHint;
Expand All @@ -12,4 +13,4 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.InlayHint;
/// <param name="ResultId">the resultId associated with the inlay hint created on original request.</param>
/// <param name="ListIndex">the index of the specific inlay hint item in the original list.</param>
/// <param name="TextDocument">the text document associated with the inlay hint to resolve.</param>
internal sealed record InlayHintResolveData(long ResultId, int ListIndex, TextDocumentIdentifier TextDocument) : DocumentResolveData(TextDocument);
internal sealed record InlayHintResolveData(long ResultId, int ListIndex, TextDocumentIdentifier TextDocument, string SyntaxVersion, Range Range, bool DisplayAllOverride) : DocumentResolveData(TextDocument);
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,27 @@
// 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 System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.InlineHints;
using Microsoft.CodeAnalysis.Options;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;
using StreamJsonRpc;
using LSP = Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.InlayHint
{
[ExportCSharpVisualBasicStatelessLspService(typeof(InlayHintResolveHandler)), Shared]
[Method(Methods.InlayHintResolveName)]
internal sealed class InlayHintResolveHandler : ILspServiceDocumentRequestHandler<LSP.InlayHint, LSP.InlayHint>
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class InlayHintResolveHandler(IGlobalOptionService globalOptions) : ILspServiceDocumentRequestHandler<LSP.InlayHint, LSP.InlayHint>
{
private readonly InlayHintCache _inlayHintCache;

public InlayHintResolveHandler(InlayHintCache inlayHintCache)
{
_inlayHintCache = inlayHintCache;
}

public bool MutatesSolutionState => false;

public bool RequiresLSPSolution => true;
Expand All @@ -33,39 +33,53 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(LSP.InlayHint request)
public Task<LSP.InlayHint> HandleRequestAsync(LSP.InlayHint request, RequestContext context, CancellationToken cancellationToken)
{
var document = context.GetRequiredDocument();
return ResolveInlayHintAsync(document, request, _inlayHintCache, cancellationToken);
var options = globalOptions.GetInlineHintsOptions(document.Project.Language);
var inlayHintCache = context.GetRequiredService<InlayHintCache>();
var resolveData = GetInlayHintResolveData(request);
return ResolveInlayHintAsync(document, request, inlayHintCache, resolveData, options, cancellationToken);
}

internal static async Task<LSP.InlayHint> ResolveInlayHintAsync(Document document, LSP.InlayHint request, InlayHintCache inlayHintCache, CancellationToken cancellationToken)
internal static async Task<LSP.InlayHint> ResolveInlayHintAsync(
Document document,
LSP.InlayHint request,
InlayHintCache inlayHintCache,
InlayHintResolveData resolveData,
InlineHintsOptions options,
CancellationToken cancellationToken)
{
var resolveData = GetInlayHintResolveData(request);
var (cacheEntry, inlineHintToResolve) = GetCacheEntry(resolveData, inlayHintCache);

var currentSyntaxVersion = await document.GetSyntaxVersionAsync(cancellationToken).ConfigureAwait(false);
var cachedSyntaxVersion = cacheEntry.SyntaxVersion;
var resolveSyntaxVersion = resolveData.SyntaxVersion;

if (currentSyntaxVersion != cachedSyntaxVersion)
if (currentSyntaxVersion.ToString() != resolveSyntaxVersion)
{
throw new LocalRpcException($"Cached resolve version {cachedSyntaxVersion} does not match current version {currentSyntaxVersion}")
throw new LocalRpcException($"Request resolve version {resolveSyntaxVersion} does not match current version {currentSyntaxVersion}")
{
ErrorCode = LspErrorCodes.ContentModified
};
}

var taggedText = await inlineHintToResolve.GetDescriptionAsync(document, cancellationToken).ConfigureAwait(false);
var inlineHintToResolve = GetCacheEntry(resolveData, inlayHintCache);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to audit any other cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you're talking about other caches in different handlers, I believe we're good. We only also use a cache in completion, which doesn't have this same issue - switching editor focus will always dismiss the previous list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switching editor focus will always dismiss the previous list

Perhaps. Though that seemsl ike a host-specific behavior. Would we have an issue if the host chnaged and decided to not do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: i'm not saying we need to do anything here. Just wanted to have this occupy a few brain cycles thinking about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the client ever allowed multiple completion sessions at once, then completion will function, but at a slightly degraded experience - the items could be committed, but would not have documentation. Additionally items with additional text edits (for example unimported types), or complex edits (for example override completion) would only apply the word edit, and not the additional ones.

It may be technically possible to recalculate in completion - but that would slow down the resolve request a fair amount. IMHO if we want to support the multiple lists at once scenario I'd much prefer adding something to the spec similar to microsoft/language-server-protocol#1802 (comment) that allows the client to tell us which completion items to hold onto (and for how long).

if (inlineHintToResolve is null)
{
// It is very possible that the cache no longer contains the hint being resolved (for example, multiple documents open side by side).
// Instead of throwing, we can recompute the hints since we've already verified above that the version has not changed.
dibarbet marked this conversation as resolved.
Show resolved Hide resolved
var hints = await InlayHintHandler.CalculateInlayHintsAsync(document, resolveData.Range, options, resolveData.DisplayAllOverride, cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you :)

inlineHintToResolve = hints[resolveData.ListIndex];
}

var taggedText = await inlineHintToResolve.Value.GetDescriptionAsync(document, cancellationToken).ConfigureAwait(false);

request.ToolTip = ProtocolConversions.GetDocumentationMarkupContent(taggedText, document, true);
return request;
}

private static (InlayHintCache.InlayHintCacheEntry CacheEntry, InlineHint InlineHintToResolve) GetCacheEntry(InlayHintResolveData resolveData, InlayHintCache inlayHintCache)
private static InlineHint? GetCacheEntry(InlayHintResolveData resolveData, InlayHintCache inlayHintCache)
{
var cacheEntry = inlayHintCache.GetCachedEntry(resolveData.ResultId);
Contract.ThrowIfNull(cacheEntry, "Missing cache entry for inlay hint resolve request");
return (cacheEntry, cacheEntry.InlayHintMembers[resolveData.ListIndex]);
return cacheEntry?.InlayHintMembers[resolveData.ListIndex];
}

private static InlayHintResolveData GetInlayHintResolveData(LSP.InlayHint inlayHint)
internal static InlayHintResolveData GetInlayHintResolveData(LSP.InlayHint inlayHint)
{
Contract.ThrowIfNull(inlayHint.Data);
var resolveData = JsonSerializer.Deserialize<InlayHintResolveData>((JsonElement)inlayHint.Data, ProtocolConversions.LspJsonSerializerOptions);
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ void X((int, bool) d)
}

[Theory, CombinatorialData]
public async Task TestDoesNotShutdownServerIfCacheEntryMissing(bool mutatingLspWorkspace)
public async Task TestReturnsInlayHintsEvenIfCacheMisses(bool mutatingLspWorkspace)
{
var markup =
@"class A
Expand Down Expand Up @@ -148,12 +148,9 @@ void M()
// Assert that the first result id is no longer in the cache.
Assert.Null(cache.GetCachedEntry(firstResultId));

// Assert that the request throws because the item no longer exists in the cache.
await Assert.ThrowsAsync<RemoteInvocationException>(async () => await testLspServer.ExecuteRequestAsync<LSP.InlayHint, LSP.InlayHint>(LSP.Methods.InlayHintResolveName, firstInlayHint, CancellationToken.None));

// Assert that the server did not shutdown and that we can resolve the latest inlay hint request we made.
var lastInlayHint = await testLspServer.ExecuteRequestAsync<LSP.InlayHint, LSP.InlayHint>(LSP.Methods.InlayHintResolveName, lastInlayHints.First(), CancellationToken.None);
Assert.NotNull(lastInlayHint?.ToolTip);
// Assert that the resolve request returns the inlay hint even if not in the cache.
var firstResolvedHint = await testLspServer.ExecuteRequestAsync<LSP.InlayHint, LSP.InlayHint>(LSP.Methods.InlayHintResolveName, firstInlayHint, CancellationToken.None);
Assert.NotNull(firstResolvedHint?.ToolTip);
}

private async Task RunVerifyInlayHintAsync(string markup, bool mutatingLspWorkspace, bool hasTextEdits = true)
Expand Down
24 changes: 16 additions & 8 deletions src/Tools/ExternalAccess/Razor/Cohost/Handlers/InlayHints.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,21 @@ internal static class InlayHints
// always just result in the defaults, which for inline hints are to not show anything. However, the editor has a
// setting for LSP inlay hints, so we can assume that if we get a request from the client, the user wants hints.
// When overriding however, Roslyn does a nicer job if type hints are off.
var options = GetOptions(displayAllOverride);

return InlayHintHandler.GetInlayHintsAsync(document, textDocumentIdentifier, range, options, displayAllOverride, s_resolveCache, cancellationToken);
}

public static Task<InlayHint> ResolveInlayHintAsync(Document document, InlayHint request, CancellationToken cancellationToken)
{
Contract.ThrowIfNull(s_resolveCache, "Cache should never be null for resolve, since it should have been created by the original request");
var data = InlayHintResolveHandler.GetInlayHintResolveData(request);
var options = GetOptions(data.DisplayAllOverride);
return InlayHintResolveHandler.ResolveInlayHintAsync(document, request, s_resolveCache, data, options, cancellationToken);
}

private static InlineHintsOptions GetOptions(bool displayAllOverride)
{
var options = InlineHintsOptions.Default;
if (!displayAllOverride)
{
Expand All @@ -36,14 +51,7 @@ internal static class InlayHints
};
}

return InlayHintHandler.GetInlayHintsAsync(document, textDocumentIdentifier, range, options, displayAllOverride, s_resolveCache, cancellationToken);
}

public static Task<InlayHint> ResolveInlayHintAsync(Document document, InlayHint request, CancellationToken cancellationToken)
{
Contract.ThrowIfNull(s_resolveCache, "Cache should never be null for resolve, since it should have been created by the original request");

return InlayHintResolveHandler.ResolveInlayHintAsync(document, request, s_resolveCache, cancellationToken);
return options;
}
}
}
Loading