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.

Sign up for GitHub

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

Jump to bottom

[LSP] Modify semanticTokens isFinalized logic #60484

Merged
merged 14 commits into from
Apr 5, 2022
Merged

[LSP] Modify semanticTokens isFinalized logic #60484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c6d83aa
Modify isFinalized logic
allisonchou Mar 30, 2022
2f569b8
Add comment
allisonchou Mar 30, 2022
eae6fa3
Code review feedback
allisonchou Mar 30, 2022
1828ce3
Review feedback
allisonchou Mar 30, 2022
20cf491
Add options
allisonchou Mar 30, 2022
4740f18
Merge branch 'release/dev17.2' of https://github.com/dotnet/roslyn in…
allisonchou Mar 30, 2022
8606db0
Merge branch 'release/dev17.2' into AddIsFullyLoaded
allisonchou Mar 30, 2022
fe6307f
Update logic
allisonchou Mar 31, 2022
ef16c05
Merge branch 'release/dev17.2' of https://github.com/dotnet/roslyn in…
allisonchou Mar 31, 2022
d3274e2
Merge remote-tracking branch 'origin/AddIsFullyLoaded' into AddIsFull…
allisonchou Mar 31, 2022
3af6c20
Add null check
allisonchou Mar 31, 2022
f96cf89
Review feedback
allisonchou Apr 1, 2022
fb287ff
Add lock
allisonchou Apr 1, 2022
9bc049f
Merge branch 'release/dev17.2' into AddIsFullyLoaded
allisonchou Apr 1, 2022
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
10 changes: 4 additions & 6 deletions src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ static SemanticTokensHelpers()
/// <summary>
/// Returns the semantic tokens data for a given document with an optional range.
/// </summary>
internal static async Task<(int[], bool isFinalized)> ComputeSemanticTokensDataAsync(
internal static async Task<int[]> ComputeSemanticTokensDataAsync(
Document document,
Dictionary<string, int> tokenTypesToIndex,
LSP.Range? range,
Expand All @@ -147,10 +147,8 @@ static SemanticTokensHelpers()

// If the full compilation is not yet available, we'll try getting a partial one. It may contain inaccurate
// results but will speed up how quickly we can respond to the client's request.
var frozenDocument = document.WithFrozenPartialSemantics(cancellationToken);
var semanticModel = await frozenDocument.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
var isFinalized = document.Project.TryGetCompilation(out var compilation) && compilation == semanticModel.Compilation;
document = frozenDocument;
document = document.WithFrozenPartialSemantics(cancellationToken);
options = options with { ForceFrozenPartialSemanticsForCrossProcessOperations = true };
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 want to use frozen partial semantics with isFinalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I wouldn't think so? I believe we want to call TryGetCompilation on the complete document. The only other place the frozen document would be used with isFinalized is when retrieving the WorkspaceStatusService, and unless I'm mistaken it doesn't seem like frozen partial semantics would be useful for that.


var classifiedSpans = await GetClassifiedSpansForDocumentAsync(
document, textSpan, options, includeSyntacticClassifications, cancellationToken).ConfigureAwait(false);
Expand All @@ -161,7 +159,7 @@ static SemanticTokensHelpers()

// TO-DO: We should implement support for streaming if LSP adds support for it:
// https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1276300
return (ComputeTokens(text.Lines, updatedClassifiedSpans, tokenTypesToIndex), isFinalized);
return ComputeTokens(text.Lines, updatedClassifiedSpans, tokenTypesToIndex);
}

private static async Task<ClassifiedSpan[]> GetClassifiedSpansForDocumentAsync(
Expand Down
54 changes: 52 additions & 2 deletions src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensRangeHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Composition;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Classification;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.LanguageServer.Protocol;
Expand All @@ -24,6 +26,10 @@ internal class SemanticTokensRangeHandler : AbstractStatelessRequestHandler<LSP.
{
private readonly IGlobalOptionService _globalOptions;

// Lock to guard _projectToCompilation dictionary
private readonly object _lock = new();
private readonly Dictionary<ProjectId, Task<Compilation?>> _projectIdToCompilation = new();

public override bool MutatesSolutionState => false;
public override bool RequiresLSPSolution => true;

Expand All @@ -48,13 +54,20 @@ public SemanticTokensRangeHandler(IGlobalOptionService globalOptions)
Contract.ThrowIfNull(request.TextDocument, "TextDocument is null.");
Contract.ThrowIfNull(context.Document, "Document is null.");

var options = _globalOptions.GetClassificationOptions(context.Document.Project.Language);
var project = context.Document.Project;
var options = _globalOptions.GetClassificationOptions(project.Language);

// Razor uses isFinalized to determine whether to cache tokens. We should be able to
// remove it altogether once Roslyn implements workspace/semanticTokens/refresh:
// https://github.com/dotnet/roslyn/issues/60441
var isFinalized = !context.Document.IsRazorDocument() ||
await IsDataFinalizedAsync(project, cancellationToken).ConfigureAwait(false);

// The results from the range handler should not be cached since we don't want to cache
// partial token results. In addition, a range request is only ever called with a whole
// document request, so caching range results is unnecessary since the whole document
// handler will cache the results anyway.
var (tokensData, isFinalized) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var tokensData = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
context.Document,
SemanticTokensHelpers.TokenTypeToIndex,
request.Range,
Expand All @@ -64,5 +77,42 @@ public SemanticTokensRangeHandler(IGlobalOptionService globalOptions)

return new RoslynSemanticTokens { Data = tokensData, IsFinalized = isFinalized };
}

private async Task<bool> IsDataFinalizedAsync(Project project, CancellationToken cancellationToken)
{
// We use a combination of IsFullyLoaded + the completed project compilation as the metric
// for isFinalized. It may not be completely accurate but this is only a a temporary fix until
// workspace/semanticTokens/refresh is implemented.
lock (_lock)
{
if (_projectIdToCompilation.TryGetValue(project.Id, out var compilationTask) && compilationTask.IsCompleted)
{
// We don't want to hang on to the compilation since this can be very expensive,
// but we do want to mark the compilation as being successfully retrieved.
if (compilationTask.Result is not null)
{
_projectIdToCompilation[project.Id] = Task.FromResult<Compilation?>(null);
}

return true;
}
}

var workspaceStatusService = project.Solution.Workspace.Services.GetRequiredService<IWorkspaceStatusService>();
var isFullyLoaded = await workspaceStatusService.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false);

lock (_lock)
{
if (isFullyLoaded && !_projectIdToCompilation.ContainsKey(project.Id))
{
// If the project's compilation isn't yet available, kick off a task in the background to
// hopefully make it available faster since we'll need it later to compute tokens.
var newCompilationTask = project.GetCompilationAsync(cancellationToken);
_projectIdToCompilation.Add(project.Id, newCompilationTask);
}
}

return false;
}
}
}
10 changes: 5 additions & 5 deletions src/Features/LanguageServer/ProtocolUnitTests/SemanticTokens/SemanticTokensRangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ static class C { }
var range = new LSP.Range { Start = new Position(0, 0), End = new Position(2, 0) };
var options = ClassificationOptions.Default;

var (results, _) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var results = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
document, SemanticTokensHelpers.TokenTypeToIndex, range, options, includeSyntacticClassifications: true, CancellationToken.None);

var expectedResults = new LSP.SemanticTokens
Expand Down Expand Up @@ -87,7 +87,7 @@ static class C { }
var document = testLspServer.GetCurrentSolution().Projects.First().Documents.First();
var range = new LSP.Range { Start = new Position(1, 0), End = new Position(2, 0) };
var options = ClassificationOptions.Default;
var (results, _) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var results = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
document, SemanticTokensHelpers.TokenTypeToIndex, range, options, includeSyntacticClassifications: true, CancellationToken.None);

var expectedResults = new LSP.SemanticTokens
Expand Down Expand Up @@ -122,7 +122,7 @@ public async Task TestGetSemanticTokensRange_MultiLineComment_RazorAsync()
var document = testLspServer.GetCurrentSolution().Projects.First().Documents.First();
var range = new LSP.Range { Start = new Position(0, 0), End = new Position(4, 0) };
var options = ClassificationOptions.Default;
var (results, _) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var results = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
document, SemanticTokensHelpers.TokenTypeToIndex, range, options, includeSyntacticClassifications: true, CancellationToken.None);

var expectedResults = new LSP.SemanticTokens
Expand Down Expand Up @@ -197,7 +197,7 @@ void M()
var document = testLspServer.GetCurrentSolution().Projects.First().Documents.First();
var range = new LSP.Range { Start = new Position(0, 0), End = new Position(9, 0) };
var options = ClassificationOptions.Default;
var (results, _) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var results = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
document, SemanticTokensHelpers.TokenTypeToIndex, range, options, includeSyntacticClassifications: true, CancellationToken.None);

var expectedResults = new LSP.SemanticTokens
Expand Down Expand Up @@ -250,7 +250,7 @@ void M()
var document = testLspServer.GetCurrentSolution().Projects.First().Documents.First();
var range = new LSP.Range { Start = new Position(0, 0), End = new Position(9, 0) };
var options = ClassificationOptions.Default;
var (results, _) = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
var results = await SemanticTokensHelpers.ComputeSemanticTokensDataAsync(
document, SemanticTokensHelpers.TokenTypeToIndex, range, options, includeSyntacticClassifications: true, CancellationToken.None);

var expectedResults = new LSP.SemanticTokens
Expand Down