Skip to content

Commit

Permalink
Merge pull request #9141 from davidwengier/FixGeneratedDocuments
Browse files Browse the repository at this point in the history 
Ensure we don't send duplicate changes when unique filenames aren't used
  • Loading branch information
@davidwengier
davidwengier authored Aug 21, 2023
2 parents 4aea767 + c4ec59a commit 7530bea
Show file tree
Hide file tree
Showing 7 changed files with 164 additions and 31 deletions.
56 changes: 27 additions & 29 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultGeneratedDocumentPublisher.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Microsoft.AspNetCore.Razor.LanguageServer.Common;
using Microsoft.AspNetCore.Razor.TextDifferencing;
Expand All @@ -19,7 +18,7 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer;
internal class DefaultGeneratedDocumentPublisher : GeneratedDocumentPublisher
{
private readonly Dictionary<DocumentKey, PublishData> _publishedCSharpData;
private readonly Dictionary<DocumentKey, PublishData> _publishedHtmlData;
private readonly Dictionary<string, PublishData> _publishedHtmlData;
private readonly ClientNotifierServiceBase _server;
private readonly LanguageServerFeatureOptions _languageServerFeatureOptions;
private readonly ILogger _logger;
Expand Down Expand Up @@ -57,7 +56,12 @@ public DefaultGeneratedDocumentPublisher(
_languageServerFeatureOptions = languageServerFeatureOptions;
_logger = loggerFactory.CreateLogger<DefaultGeneratedDocumentPublisher>();
_publishedCSharpData = new Dictionary<DocumentKey, PublishData>();
_publishedHtmlData = new Dictionary<DocumentKey, PublishData>();

// We don't generate individual Html documents per-project, so in order to ensure diffs are calculated correctly
// we don't use the project key for the key for this dictionary. This matches when we send edits to the client,
// as they are only tracking a single Html file for each Razor file path, thus edits need to be correct or we'll
// get out of sync.
_publishedHtmlData = new Dictionary<string, PublishData>(FilePathComparer.Instance);
}

public override void Initialize(ProjectSnapshotManagerBase projectManager)
Expand Down Expand Up @@ -85,6 +89,15 @@ public override void PublishCSharp(ProjectKey projectKey, string filePath, Sourc

_projectSnapshotManagerDispatcher.AssertDispatcherThread();

// If our generated documents don't have unique file paths, then using project key information is problematic for the client.
// For example, when a document moves from the Misc Project to a real project, we will update it here, and each version would
// have a different project key. On the receiving end however, there is only one file path, therefore one version of the contents,
// so we must ensure we only have a single document to compute diffs from, or things get out of sync.
// if (!_languageServerFeatureOptions.IncludeProjectKeyInGeneratedFilePath)
{
projectKey = default;
}

var key = new DocumentKey(projectKey, filePath);
if (!_publishedCSharpData.TryGetValue(key, out var previouslyPublishedData))
{
Expand Down Expand Up @@ -124,16 +137,6 @@ public override void PublishCSharp(ProjectKey projectKey, string filePath, Sourc
HostDocumentVersion = hostDocumentVersion,
};

// HACK: We know about a document being in multiple projects, but despite having ProjectKeyId in the request, currently the other end
// of this LSP message only knows about a single document file path. To prevent confusing them, we just send an update for the first project
// in the list.
if (_projectSnapshotManager is { } projectSnapshotManager &&
projectSnapshotManager.GetLoadedProject(projectKey) is { } projectSnapshot &&
projectSnapshotManager.GetAllProjectKeys(projectSnapshot.FilePath).First() != projectKey)
{
return;
}

_ = _server.SendNotificationAsync(CustomMessageNames.RazorUpdateCSharpBufferEndpoint, request, CancellationToken.None);
}

Expand All @@ -151,8 +154,7 @@ public override void PublishHtml(ProjectKey projectKey, string filePath, SourceT

_projectSnapshotManagerDispatcher.AssertDispatcherThread();

var key = new DocumentKey(projectKey, filePath);
if (!_publishedHtmlData.TryGetValue(key, out var previouslyPublishedData))
if (!_publishedHtmlData.TryGetValue(filePath, out var previouslyPublishedData))
{
previouslyPublishedData = PublishData.Default;
}
Expand All @@ -179,7 +181,7 @@ public override void PublishHtml(ProjectKey projectKey, string filePath, SourceT
textChanges.Count);
}

_publishedHtmlData[key] = new PublishData(sourceText, hostDocumentVersion);
_publishedHtmlData[filePath] = new PublishData(sourceText, hostDocumentVersion);

var request = new UpdateBufferRequest()
{
Expand All @@ -189,16 +191,6 @@ public override void PublishHtml(ProjectKey projectKey, string filePath, SourceT
HostDocumentVersion = hostDocumentVersion,
};

// HACK: We know about a document being in multiple projects, but despite having ProjectKeyId in the request, currently the other end
// of this LSP message only knows about a single document file path. To prevent confusing them, we just send an update for the first project
// in the list.
if (_projectSnapshotManager is { } projectSnapshotManager &&
projectSnapshotManager.GetLoadedProject(projectKey) is { } projectSnapshot &&
projectSnapshotManager.GetAllProjectKeys(projectSnapshot.FilePath).First() != projectKey)
{
return;
}

_ = _server.SendNotificationAsync(CustomMessageNames.RazorUpdateHtmlBufferEndpoint, request, CancellationToken.None);
}

Expand All @@ -220,7 +212,13 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
Assumes.NotNull(args.DocumentFilePath);
if (!_projectSnapshotManager.IsDocumentOpen(args.DocumentFilePath))
{
var key = new DocumentKey(args.ProjectKey, args.DocumentFilePath);
var projectKey = args.ProjectKey;
// if (!_languageServerFeatureOptions.IncludeProjectKeyInGeneratedFilePath)
{
projectKey = default;
}

var key = new DocumentKey(projectKey, args.DocumentFilePath);
// Document closed, evict published source text, unless the server doesn't want us to.
if (_languageServerFeatureOptions.UpdateBuffersForClosedDocuments)
{
Expand All @@ -239,9 +237,9 @@ private void ProjectSnapshotManager_Changed(object? sender, ProjectChangeEventAr
}
}

if (_publishedHtmlData.ContainsKey(key))
if (_publishedHtmlData.ContainsKey(args.DocumentFilePath))
{
var removed = _publishedHtmlData.Remove(key);
var removed = _publishedHtmlData.Remove(args.DocumentFilePath);
if (!removed)
{
_logger.LogError("Published data should be protected by the project snapshot manager's thread and should never fail to remove.");
Expand Down
2 changes: 1 addition & 1 deletion src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Folding/FoldingRangeEndpoint.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ private FoldingRange FixFoldingRangeStart(FoldingRange range, RazorCodeDocument
var sourceText = codeDocument.GetSourceText();
var startLine = range.StartLine;

if (startLine > sourceText.Lines.Count)
if (startLine >= sourceText.Lines.Count)
{
// Sometimes VS Code seems to send us wildly out-of-range folding ranges for Html, so log a warning,
// but prevent a toast from appearing from an exception.
Expand Down
2 changes: 1 addition & 1 deletion ...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/SemanticTokensRangeEndpoint.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(SemanticTokensRangeParam
var semanticTokens = await semanticTokensInfoService.GetSemanticTokensAsync(request.TextDocument, request.Range, documentContext, _razorSemanticTokensLegend.AssumeNotNull(), correlationId, cancellationToken).ConfigureAwait(false);
var amount = semanticTokens is null ? "no" : (semanticTokens.Data.Length / 5).ToString(Thread.CurrentThread.CurrentCulture);

requestContext.Logger.LogInformation("Returned {amount} semantic tokens for range {request.Range} in {request.TextDocument.Uri}.", amount, request.Range, request.TextDocument.Uri);
requestContext.Logger.LogInformation("Returned {amount} semantic tokens for range ({startLine},{startChar})-({endLine},{endChar}) in {request.TextDocument.Uri}.", amount, request.Range.Start.Line, request.Range.Start.Character, request.Range.End.Line, request.Range.End.Character, request.TextDocument.Uri);

if (semanticTokens is not null)
{
Expand Down
20 changes: 20 additions & 0 deletions src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/CSharpVirtualDocument.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
#if DEBUG
using System.Diagnostics;
#endif
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Microsoft.VisualStudio.LanguageServer.ContainedLanguage;
using Microsoft.VisualStudio.Text;
Expand All @@ -19,4 +23,20 @@ public CSharpVirtualDocument(ProjectKey projectKey, Uri uri, ITextBuffer textBuf
}

protected override CSharpVirtualDocumentSnapshot GetUpdatedSnapshot(object? state) => new(_projectKey, Uri, TextBuffer.CurrentSnapshot, HostDocumentVersion);

public override VirtualDocumentSnapshot Update(IReadOnlyList<ITextChange> changes, int hostDocumentVersion, object? state)
{
var result = base.Update(changes, hostDocumentVersion, state);

#if DEBUG
var text = TextBuffer.CurrentSnapshot.GetText();

var generatedFileStartIndex = text.IndexOf("#pragma warning disable 1591");
var secondGeneratedFileStartIndex = text.IndexOf("#pragma warning disable 1591", generatedFileStartIndex + 20);

Debug.Assert(secondGeneratedFileStartIndex == -1, "Generated C# file appears to have duplicated file contents. This could indicate a sync problem between language server and client.");
#endif

return result;
}
}
3 changes: 3 additions & 0 deletions src/Razor/src/rzls/Program.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ public static async Task Main(string[] args)
logger,
NoOpTelemetryReporter.Instance,
featureOptions: languageServerFeatureOptions);

logger.LogInformation("Razor Language Server started successfully.");

await server.WaitForExitAsync().ConfigureAwait(true);
}
}
38 changes: 38 additions & 0 deletions ...t/Microsoft.AspNetCore.Razor.LanguageServer.Test/DefaultGeneratedDocumentPublisherTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public class DefaultGeneratedDocumentPublisherTest : LanguageServerTestBase
private readonly TestClient _serverClient;
private readonly TestProjectSnapshotManager _projectManager;
private readonly HostProject _hostProject;
private readonly HostProject _hostProject2;
private readonly HostDocument _hostDocument;

public DefaultGeneratedDocumentPublisherTest(ITestOutputHelper testOutput)
Expand All @@ -25,6 +26,7 @@ public DefaultGeneratedDocumentPublisherTest(ITestOutputHelper testOutput)
_projectManager = TestProjectSnapshotManager.Create(ErrorReporter);
_projectManager.AllowNotifyListeners = true;
_hostProject = new HostProject("/path/to/project.csproj", "/path/to/obj", RazorConfiguration.Default, "TestRootNamespace");
_hostProject2 = new HostProject("/path/to/project2.csproj", "/path/to/obj2", RazorConfiguration.Default, "TestRootNamespace");
_projectManager.ProjectAdded(_hostProject);
_hostDocument = new HostDocument("/path/to/file.razor", "file.razor");
_projectManager.DocumentAdded(_hostProject.Key, _hostDocument, new EmptyTextLoader(_hostDocument.FilePath));
Expand Down Expand Up @@ -309,4 +311,40 @@ public void ProjectSnapshotManager_DocumentChanged_ClosedDocument_RepublishesTex
Assert.Equal(sourceTextContent, textChange.NewText);
Assert.Equal(123, updateRequest.HostDocumentVersion);
}

[Fact]
public void ProjectSnapshotManager_DocumentMoved_DoesntRepublishWholeDocument()
{
// Arrange
var generatedDocumentPublisher = new DefaultGeneratedDocumentPublisher(LegacyDispatcher, _serverClient, TestLanguageServerFeatureOptions.Instance, LoggerFactory);
generatedDocumentPublisher.Initialize(_projectManager);
var sourceTextContent = """
public void Method()
{
}
""";
var initialSourceText = SourceText.From(sourceTextContent);
var changedTextContent = """
public void Method()
{
// some new code here
}
""";
var changedSourceText = SourceText.From(changedTextContent);
generatedDocumentPublisher.PublishCSharp(_hostProject.Key, _hostDocument.FilePath, initialSourceText, 123);
_projectManager.DocumentOpened(_hostProject.Key, _hostDocument.FilePath, initialSourceText);

// Act
_projectManager.ProjectAdded(_hostProject2);
_projectManager.DocumentAdded(_hostProject2.Key, _hostDocument, new EmptyTextLoader(_hostDocument.FilePath));
generatedDocumentPublisher.PublishCSharp(_hostProject2.Key, _hostDocument.FilePath, changedSourceText, 124);

// Assert
Assert.Equal(2, _serverClient.UpdateRequests.Count);
var updateRequest = _serverClient.UpdateRequests.Last();
Assert.Equal(_hostDocument.FilePath, updateRequest.HostDocumentFilePath);
var textChange = Assert.Single(updateRequest.Changes);
Assert.Equal("// some new code here", textChange.NewText!.Trim());
Assert.Equal(124, updateRequest.HostDocumentVersion);
}
}
74 changes: 74 additions & 0 deletions src/Razor/test/Microsoft.VisualStudio.Razor.IntegrationTests/ProjectTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,78 @@ public async Task OpenExistingProject()
var actualProjectFileName = await TestServices.SolutionExplorer.GetAbsolutePathForProjectRelativeFilePathAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ProjectFile, ControlledHangMitigatingCancellationToken);
Assert.Equal(expectedProjectFileName, actualProjectFileName);
}

[IdeFact]
public async Task OpenExistingProject_WithReopenedFile()
{
var solutionPath = await TestServices.SolutionExplorer.GetDirectoryNameAsync(ControlledHangMitigatingCancellationToken);
var expectedProjectFileName = await TestServices.SolutionExplorer.GetAbsolutePathForProjectRelativeFilePathAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ProjectFile, ControlledHangMitigatingCancellationToken);

// Open SurveyPrompt and make sure its all up and running
await TestServices.SolutionExplorer.OpenFileAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ErrorCshtmlFile, ControlledHangMitigatingCancellationToken);
await TestServices.Editor.WaitForSemanticClassificationAsync("class name", ControlledHangMitigatingCancellationToken, count: 1);

await TestServices.SolutionExplorer.CloseSolutionAsync(ControlledHangMitigatingCancellationToken);

var solutionFileName = Path.Combine(solutionPath, RazorProjectConstants.BlazorSolutionName + ".sln");
await TestServices.SolutionExplorer.OpenSolutionAsync(solutionFileName, ControlledHangMitigatingCancellationToken);

await TestServices.Workspace.WaitForProjectSystemAsync(ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.LanguageServer, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.Workspace, ControlledHangMitigatingCancellationToken);

// Razor extension doesn't launch until a razor file is opened, so wait for it to equalize
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.LanguageServer, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.Workspace, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForProjectSystemAsync(ControlledHangMitigatingCancellationToken);

await TestServices.Editor.WaitForSemanticClassificationAsync("class name", ControlledHangMitigatingCancellationToken, count: 1);

TestServices.Input.Send("1");

// Make sure the test framework didn't do something weird and create new project
var actualProjectFileName = await TestServices.SolutionExplorer.GetAbsolutePathForProjectRelativeFilePathAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ProjectFile, ControlledHangMitigatingCancellationToken);
Assert.Equal(expectedProjectFileName, actualProjectFileName);

await TestServices.Editor.CloseCodeFileAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ErrorCshtmlFile, saveFile: false, ControlledHangMitigatingCancellationToken);
}

[IdeFact]
public async Task OpenExistingProject_WithReopenedFile_NoProjectRazorJson()
{
var solutionPath = await TestServices.SolutionExplorer.GetDirectoryNameAsync(ControlledHangMitigatingCancellationToken);
var expectedProjectFileName = await TestServices.SolutionExplorer.GetAbsolutePathForProjectRelativeFilePathAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ProjectFile, ControlledHangMitigatingCancellationToken);

// Open SurveyPrompt and make sure its all up and running
await TestServices.SolutionExplorer.OpenFileAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ErrorCshtmlFile, ControlledHangMitigatingCancellationToken);
await TestServices.Editor.WaitForSemanticClassificationAsync("class name", ControlledHangMitigatingCancellationToken, count: 1);

await TestServices.SolutionExplorer.CloseSolutionAsync(ControlledHangMitigatingCancellationToken);

// Clear out the project.razor.json file which ensures our restored file will have to be in the Misc Project
var projectRazorJsonFileName = Directory.EnumerateFiles(solutionPath, "project.razor.*.json", SearchOption.AllDirectories).First();
File.Delete(projectRazorJsonFileName);

var solutionFileName = Path.Combine(solutionPath, RazorProjectConstants.BlazorSolutionName + ".sln");
await TestServices.SolutionExplorer.OpenSolutionAsync(solutionFileName, ControlledHangMitigatingCancellationToken);

await TestServices.Workspace.WaitForProjectSystemAsync(ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.LanguageServer, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.Workspace, ControlledHangMitigatingCancellationToken);

// Razor extension doesn't launch until a razor file is opened, so wait for it to equalize
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.LanguageServer, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForAsyncOperationsAsync(FeatureAttribute.Workspace, ControlledHangMitigatingCancellationToken);
await TestServices.Workspace.WaitForProjectSystemAsync(ControlledHangMitigatingCancellationToken);

await TestServices.Editor.WaitForSemanticClassificationAsync("class name", ControlledHangMitigatingCancellationToken, count: 1);

TestServices.Input.Send("1");

// Make sure the test framework didn't do something weird and create new project
var actualProjectFileName = await TestServices.SolutionExplorer.GetAbsolutePathForProjectRelativeFilePathAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ProjectFile, ControlledHangMitigatingCancellationToken);
Assert.Equal(expectedProjectFileName, actualProjectFileName);

await TestServices.Editor.CloseCodeFileAsync(RazorProjectConstants.BlazorProjectName, RazorProjectConstants.ErrorCshtmlFile, saveFile: false, ControlledHangMitigatingCancellationToken);
}
}

0 comments on commit 7530bea

Please sign in to comment.