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 2 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,104 @@
// 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.Tasks;
using Microsoft.AspNetCore.Razor.LanguageServer.Hosting;
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 class WorkspaceDiagnosticsRefresher : IRazorStartupService
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly object _gate = new();
private readonly IClientCapabilitiesService _clientCapabilitiesService;
private readonly IClientConnection _clientConnection;
private bool _refreshQueued;
private Task? _refreshTask;

private static readonly TimeSpan s_delay = TimeSpan.FromMilliseconds(200);

public WorkspaceDiagnosticsRefresher(
IProjectSnapshotManager projectSnapshotManager,
IClientCapabilitiesService clientCapabilitiesService,
IClientConnection clientConnection)
{
projectSnapshotManager.Changed += ProjectSnapshotManager_Changed;
_clientCapabilitiesService = clientCapabilitiesService;
_clientConnection = clientConnection;
}

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

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

private void QueueRefresh()
{
lock(_gate)
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
{
if (_refreshQueued)
{
return;
}

if (!_clientCapabilitiesService.CanGetClientCapabilities)
{
return;
}

var supported = _clientCapabilitiesService.ClientCapabilities.Workspace?.Diagnostics?.RefreshSupport;
if (supported != true)
{
return;
}

_refreshQueued = true;
_refreshTask = RefreshAfterDelayAsync();
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
}
}

private async Task RefreshAfterDelayAsync()
{
await Task.Delay(s_delay).ConfigureAwait(false);

_clientConnection
.SendNotificationAsync(Methods.WorkspaceDiagnosticRefreshName, default)
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
.Forget();

_refreshQueued = false;
}

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

public class TestAccessor
{
private readonly WorkspaceDiagnosticsRefresher _instance;

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

public Task WaitForRefreshAsync()
{
return _instance._refreshTask ?? Task.CompletedTask;
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.LanguageServer.Hosting;
using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
using Microsoft.AspNetCore.Razor.Test.Common.ProjectSystem;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Moq;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.AspNetCore.Razor.LanguageServer.Test;

public class WorkspaceDiagnosticRefreshTest(ITestOutputHelper testOutputHelper) : LanguageServerTestBase(testOutputHelper)
{
[Fact]
public async Task WorkspaceRefreshSent()
{
var projectSnapshotManager = CreateProjectSnapshotManager();
var clientConnection = new Mock<IClientConnection>(MockBehavior.Strict);
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
clientConnection
.Setup(c => c.SendNotificationAsync(Methods.WorkspaceDiagnosticRefreshName, It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask)
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.

Does ReturnsAsync not work here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk tbh. What's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like not. ReturnsAsync doesn't have a way to do a Task, it requires Task<T>

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I'm remembering a Moq extension I had added in a branch to address that.

Copy link
Member

Choose a reason for hiding this comment

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

Idk tbh. What's the difference?

ReturnsAsync(...) is just convenient for returning Task<T> and ValueTask<T> in mocks.

.Verifiable();

var publisher = new WorkspaceDiagnosticsRefresher(
projectSnapshotManager,
new TestClientCapabilitiesService(new()
{
Workspace = new()
{
Diagnostics = new()
{
RefreshSupport = true
}
}
}),
clientConnection.Object);

var testAccessor = publisher.GetTestAccessor();

await projectSnapshotManager.UpdateAsync(
static updater =>
{
updater.CreateAndAddProject("C:/path/to/project.csproj");
});

await testAccessor.WaitForRefreshAsync();

clientConnection.Verify();
}

[Fact]
public async Task WorkspaceRefreshSent_MultipleTimes()
{
var projectSnapshotManager = CreateProjectSnapshotManager();
var clientConnection = new Mock<IClientConnection>(MockBehavior.Strict);
clientConnection
.Verify(c => c.SendNotificationAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()),
Times.Exactly(2));

var publisher = new WorkspaceDiagnosticsRefresher(
projectSnapshotManager,
new TestClientCapabilitiesService(new()
{
Workspace = new()
{
Diagnostics = new()
{
RefreshSupport = true
}
}
}),
clientConnection.Object);

var testAccessor = publisher.GetTestAccessor();

await projectSnapshotManager.UpdateAsync(
static updater =>
{
updater.CreateAndAddProject("C:/path/to/project.csproj");
});

await testAccessor.WaitForRefreshAsync();

await projectSnapshotManager.UpdateAsync(
static updater =>
{
var project = (ProjectSnapshot)updater.GetProjects().Single();
updater.CreateAndAddDocument(project, "C:/path/to/document.razor");
});

await testAccessor.WaitForRefreshAsync();

clientConnection.Verify();
}

[Fact]
public async Task WorkspaceRefreshNotSent_ClientDoesNotSupport()
{
var projectSnapshotManager = CreateProjectSnapshotManager();
var clientConnection = new Mock<IClientConnection>(MockBehavior.Strict);
clientConnection
.Verify(c => c.SendNotificationAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()),
Times.Never);

var publisher = new WorkspaceDiagnosticsRefresher(
projectSnapshotManager,
new TestClientCapabilitiesService(new()
{
Workspace = new()
{
Diagnostics = new()
{
RefreshSupport = false
}
}
}),
clientConnection.Object);

var testAccessor = publisher.GetTestAccessor();

await projectSnapshotManager.UpdateAsync(
static updater =>
{
updater.CreateAndAddProject("C:/path/to/project.csproj");
});

await testAccessor.WaitForRefreshAsync();
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.

Since publisher is disposed above, will this ever complete?

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 believe so. The underlying ABWQ method doesn't seem to check for cancellation, it just returns the task. That task is completed when this hits


clientConnection.Verify();
}
}
Loading