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

Refresh diagnostics on project changes #10964

Merged
merged 9 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -105,6 +105,7 @@ public static void AddDiagnosticServices(this IServiceCollection services)
services.AddHandlerWithCapabilities<DocumentPullDiagnosticsEndpoint>();
services.AddSingleton<RazorTranslateDiagnosticsService>();
services.AddSingleton(sp => new Lazy<RazorTranslateDiagnosticsService>(sp.GetRequiredService<RazorTranslateDiagnosticsService>));
services.AddSingleton<IRazorStartupService, WorkspaceDiagnosticsRefresher>();
}

public static void AddHoverServices(this IServiceCollection services)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.LanguageServer.Hosting;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Microsoft.CodeAnalysis.Razor.Protocol;
using Microsoft.CodeAnalysis.Razor.Workspaces;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Microsoft.VisualStudio.Threading;

namespace Microsoft.AspNetCore.Razor.LanguageServer;

internal sealed class WorkspaceDiagnosticsRefresher : IRazorStartupService, IDisposable
{
private readonly AsyncBatchingWorkQueue _queue;
private readonly IProjectSnapshotManager _projectSnapshotManager;
private readonly IClientCapabilitiesService _clientCapabilitiesService;
private readonly IClientConnection _clientConnection;
private bool? _supported;
private CancellationTokenSource _disposeTokenSource = new();

public WorkspaceDiagnosticsRefresher(
IProjectSnapshotManager projectSnapshotManager,
IClientCapabilitiesService clientCapabilitiesService,
IClientConnection clientConnection)
{
_clientConnection = clientConnection;
_projectSnapshotManager = projectSnapshotManager;
_clientCapabilitiesService = clientCapabilitiesService;
_queue = new(
TimeSpan.FromMilliseconds(200),
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
ProcessBatchAsync,
_disposeTokenSource.Token);
_projectSnapshotManager.Changed += ProjectSnapshotManager_Changed;
}

private ValueTask ProcessBatchAsync(CancellationToken token)
{
_clientConnection
.SendNotificationAsync(Methods.WorkspaceDiagnosticRefreshName, _disposeTokenSource.Token)
.Forget();

return new ValueTask(Task.CompletedTask);
}

private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventArgs e)
{
if (e.SolutionIsClosing)
{
return;
}

if ( _disposeTokenSource.IsCancellationRequested)
Copy link
Member

Choose a reason for hiding this comment

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

nit: extra space between ( and _. Or, if you decide to take my feedback to adjust the Dispose() method, this check isn't needed at all! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on a laptop today and it shows 😅

{
return;
}

_supported ??= GetSupported();

if (_supported != true)
{
return;
}

ryzngard marked this conversation as resolved.
Show resolved Hide resolved

if (e.Kind is not ProjectChangeKind.DocumentChanged)
{
_queue.AddWork();
}
}

private bool? GetSupported()
{
if (!_clientCapabilitiesService.CanGetClientCapabilities)
{
return null;
}

return _clientCapabilitiesService.ClientCapabilities.Workspace?.Diagnostics?.RefreshSupport;
}

internal TestAccessor GetTestAccessor()
=> new(this);

public void Dispose()
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
{
if (_disposeTokenSource.IsCancellationRequested)
{
return;
}

_disposeTokenSource.Cancel();
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
_projectSnapshotManager.Changed -= ProjectSnapshotManager_Changed;
}

internal sealed class TestAccessor
{
private readonly WorkspaceDiagnosticsRefresher _instance;

public TestAccessor(WorkspaceDiagnosticsRefresher instance)
{
_instance = instance;
}

public Task WaitForRefreshAsync()
{
return _instance._queue.WaitUntilCurrentBatchCompletesAsync();
Copy link
Member

@DustinCampbell DustinCampbell Oct 4, 2024

Choose a reason for hiding this comment

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

FWIW, there might be a hang in tests where WaitForRefreshAsync() is called after the WorkspaceDiagnosticRefresher is disposed. I'm not 100% sure though. I'm trying to "psychically debug" this. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to check disposal here and return a Task.CompletedTask?

Copy link
Member

Choose a reason for hiding this comment

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

I quick fix might be something like...

if (_instance._disposeTokenSource.IsCancellationRequested)
{
    return Task.CompletedTask;
}

Copy link
Member

Choose a reason for hiding this comment

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

Is it better to check disposal here and return a Task.CompletedTask?

LOL. I didn't refresh quickly enough to see your comment. 🤣

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,26 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.LanguageServer.Hosting;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis.Razor.Protocol;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Microsoft.VisualStudio.Threading;

namespace Microsoft.AspNetCore.Razor.LanguageServer;

internal class WorkspaceSemanticTokensRefreshNotifier : IWorkspaceSemanticTokensRefreshNotifier, IDisposable
internal sealed class WorkspaceSemanticTokensRefreshNotifier : IWorkspaceSemanticTokensRefreshNotifier, IDisposable
{
private static readonly TimeSpan s_delay = TimeSpan.FromMilliseconds(250);

Check failure on line 17 in src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

View check run for this annotation

Azure Pipelines / razor-tooling-ci (Build macOS debug)

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs#L17

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs(17,38): error IDE0052: (NETCORE_ENGINEERING_TELEMETRY=Build) Private member 'WorkspaceSemanticTokensRefreshNotifier.s_delay' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)

Check failure on line 17 in src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

View check run for this annotation

Azure Pipelines / razor-tooling-ci (Build macOS debug)

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs#L17

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs(17,38): error IDE0052: (NETCORE_ENGINEERING_TELEMETRY=Build) Private member 'WorkspaceSemanticTokensRefreshNotifier.s_delay' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)

Check failure on line 17 in src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

View check run for this annotation

Azure Pipelines / razor-tooling-ci (Build macOS release)

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs#L17

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs(17,38): error IDE0052: (NETCORE_ENGINEERING_TELEMETRY=Build) Private member 'WorkspaceSemanticTokensRefreshNotifier.s_delay' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)

Check failure on line 17 in src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

View check run for this annotation

Azure Pipelines / razor-tooling-ci (Build macOS release)

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs#L17

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs(17,38): error IDE0052: (NETCORE_ENGINEERING_TELEMETRY=Build) Private member 'WorkspaceSemanticTokensRefreshNotifier.s_delay' can be removed as the value assigned to it is never read (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0052)

private readonly IClientCapabilitiesService _clientCapabilitiesService;
private readonly IClientConnection _clientConnection;
private readonly CancellationTokenSource _disposeTokenSource;
private readonly IDisposable _optionsChangeListener;

private readonly object _gate = new();
private bool? _supportsRefresh;
private bool _waitingToRefresh;
private Task _refreshTask = Task.CompletedTask;
private readonly AsyncBatchingWorkQueue _queue;
Copy link
Member

Choose a reason for hiding this comment

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

Amusingly, I had used an AsyncBatchingWorkQueue in #10140 and removed it based on feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I'm bringing it back! :P


private bool _isColoringBackground;
private bool? _supportsRefresh;

public WorkspaceSemanticTokensRefreshNotifier(
IClientCapabilitiesService clientCapabilitiesService,
Expand All @@ -37,10 +36,24 @@

_disposeTokenSource = new();

_queue = new(
TimeSpan.FromMilliseconds(250),
ProcessBatchAsync,
_disposeTokenSource.Token);

_isColoringBackground = optionsMonitor.CurrentValue.ColorBackground;
_optionsChangeListener = optionsMonitor.OnChange(HandleOptionsChange);
}

private ValueTask ProcessBatchAsync(CancellationToken token)
{
_clientConnection
.SendNotificationAsync(Methods.WorkspaceSemanticTokensRefreshName, _disposeTokenSource.Token)
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
.Forget();

return new(Task.CompletedTask);
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
}

public void Dispose()
{
if (_disposeTokenSource.IsCancellationRequested)
Expand Down Expand Up @@ -70,58 +83,30 @@
return;
}

lock (_gate)
// We could have been called before the LSP server has even been initialized
if (!_clientCapabilitiesService.CanGetClientCapabilities)
{
if (_waitingToRefresh)
{
// We're going to refresh shortly.
return;
}

// We could have been called before the LSP server has even been initialized
if (!_clientCapabilitiesService.CanGetClientCapabilities)
{
return;
}

_supportsRefresh ??= _clientCapabilitiesService.ClientCapabilities.Workspace?.SemanticTokens?.RefreshSupport ?? false;

if (_supportsRefresh is false)
{
return;
}

_refreshTask = RefreshAfterDelayAsync();
_waitingToRefresh = true;
return;
}

async Task RefreshAfterDelayAsync()
{
await Task.Delay(s_delay, _disposeTokenSource.Token).ConfigureAwait(false);

_clientConnection
.SendNotificationAsync(Methods.WorkspaceSemanticTokensRefreshName, _disposeTokenSource.Token)
.Forget();
_supportsRefresh ??= _clientCapabilitiesService.ClientCapabilities.Workspace?.SemanticTokens?.RefreshSupport ?? false;

_waitingToRefresh = false;
if (_supportsRefresh is false)
{
return;
}

_queue.AddWork();
}

internal TestAccessor GetTestAccessor()
=> new(this);

internal class TestAccessor(WorkspaceSemanticTokensRefreshNotifier instance)
internal sealed class TestAccessor(WorkspaceSemanticTokensRefreshNotifier instance)
{
public async Task WaitForNotificationAsync()
public Task WaitForNotificationAsync()
{
Task refreshTask;

lock (instance._gate)
{
refreshTask = instance._refreshTask;
}

await refreshTask.ConfigureAwait(false);
return instance._queue.WaitUntilCurrentBatchCompletesAsync();
}
}
}
Loading
Loading