Fix project publish (or: Various project system fixes) (#10983)

Fixes #10970

Each commit in this PR essentially hardens the project system a little
bit more. Together they fix the publish issue, which is a particularly
difficult scenario, as it essentially removes and quickly re-adds a
project, but I think the changes are pretty good overall. No one commit
fixes the publish issue, and I wouldn't say there is no more work to do,
but forward progress is forward progress.

Each commit is isolated to a particular service and has a brief
description of what the change is and why, but I think it's perfectly
understood when looked at as a whole too. Review however you prefer :)

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentPublisher.cs

@@ -76,7 +76,7 @@ public void PublishCSharp(ProjectKey projectKey, string filePath, SourceText sou
             if (previouslyPublishedData.HostDocumentVersion > hostDocumentVersion)
             {
                 // We've already published a newer version of this document. No-op.
-                _logger.LogWarning($"Skipping publish of C# for {filePath} because we've already published version {previouslyPublishedData.HostDocumentVersion}, and this request is for {hostDocumentVersion}.");
+                _logger.LogWarning($"Skipping publish of C# for {documentKey.ProjectKey}/{filePath} because we've already published version {previouslyPublishedData.HostDocumentVersion}, and this request is for {hostDocumentVersion} (and {projectKey}).");
                 return;
             }
 
@@ -161,6 +161,27 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args)
 
         switch (args.Kind)
         {
+            case ProjectChangeKind.DocumentRemoved:
+                {
+                    if (!_options.IncludeProjectKeyInGeneratedFilePath)
+                    {
+                        break;
+                    }
+
+                    // When a C# document is removed we remove it from the publishing, because it could come back with the same name
+                    var key = new DocumentKey(args.ProjectKey, args.DocumentFilePath.AssumeNotNull());
+
+                    lock (_publishedCSharpData)
+                    {
+                        if (_publishedCSharpData.Remove(key))
+                        {
+                            _logger.LogDebug($"Removing previous C# publish data for {key.ProjectKey}/{key.DocumentFilePath}");
+                        }
+                    }
+
+                    break;
+                }
+
             case ProjectChangeKind.DocumentChanged:
                 var documentFilePath = args.DocumentFilePath.AssumeNotNull();
 
@@ -180,27 +201,17 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args)
 
                     lock (_publishedCSharpData)
                     {
-                        if (_publishedCSharpData.ContainsKey(documentKey))
+                        if (_publishedCSharpData.Remove(documentKey))
                         {
-                            var removed = _publishedCSharpData.Remove(documentKey);
-                            if (!removed)
-                            {
-                                _logger.LogError($"Published data should be protected by the project snapshot manager's thread and should never fail to remove.");
-                                Debug.Fail("Published data should be protected by the project snapshot manager's thread and should never fail to remove.");
-                            }
+                            _logger.LogDebug($"Removing previous C# publish data for {documentKey.ProjectKey}/{documentKey.DocumentFilePath}");
                         }
                     }
 
                     lock (_publishedHtmlData)
                     {
-                        if (_publishedHtmlData.ContainsKey(documentFilePath))
+                        if (_publishedHtmlData.Remove(documentFilePath))
                         {
-                            var removed = _publishedHtmlData.Remove(documentFilePath);
-                            if (!removed)
-                            {
-                                _logger.LogError($"Published data should be protected by the project snapshot manager's thread and should never fail to remove.");
-                                Debug.Fail("Published data should be protected by the project snapshot manager's thread and should never fail to remove.");
-                            }
+                            _logger.LogDebug($"Removing previous Html publish data for {documentKey.ProjectKey}/{documentKey.DocumentFilePath}");
                         }
                     }
                 }
@@ -231,7 +242,10 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs args)
 
                         foreach (var key in keysToRemove)
                         {
-                            _publishedCSharpData.Remove(key);
+                            if (_publishedCSharpData.Remove(key))
+                            {
+                                _logger.LogDebug($"Removing previous C# publish data for {key.ProjectKey}/{key.DocumentFilePath}");
+                            }
                         }
                     }
 

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentSynchronizer.cs

@@ -28,6 +28,13 @@ public void DocumentProcessed(RazorCodeDocument codeDocument, IDocumentSnapshot
             return;
         }
 
+        // If the document has been removed from the project, then don't do anything, or version numbers will be thrown off
+        if (!_projectManager.TryGetLoadedProject(document.Project.Key, out var project) ||
+            !project.ContainsDocument(document.FilePath))
+        {
+            return;
+        }
+
         // If cohosting is on, then it is responsible for updating the Html buffer
         if (!_languageServerFeatureOptions.UseRazorCohostServer)
         {

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/OpenDocumentGenerator.Comparer.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using Microsoft.CodeAnalysis.Razor;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
+using Microsoft.Extensions.Internal;
 
 namespace Microsoft.AspNetCore.Razor.LanguageServer;
 
@@ -28,12 +29,16 @@ public bool Equals(IDocumentSnapshot? x, IDocumentSnapshot? y)
                 return false;
             }
 
-            return FilePathComparer.Instance.Equals(x.FilePath, y.FilePath);
+            return x.Project.Key.Equals(y.Project.Key) &&
+                FilePathComparer.Instance.Equals(x.FilePath, y.FilePath);
         }
 
         public int GetHashCode(IDocumentSnapshot obj)
         {
-            return FilePathComparer.Instance.GetHashCode(obj);
+            var hash = HashCodeCombiner.Start();
+            hash.Add(obj.Project.Key.Id, FilePathComparer.Instance);
+            hash.Add(obj.FileKind.AssumeNotNull(), FilePathComparer.Instance);
+            return hash.CombinedHash;
         }
     }
 }

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/OpenDocumentGenerator.cs

@@ -175,7 +175,7 @@ void EnqueueIfNecessary(IDocumentSnapshot document)
                 return;
             }
 
-            _logger.LogDebug($"Enqueuing generation of {document.FilePath} in {document.Project.Key.Id}");
+            _logger.LogDebug($"Enqueuing generation of {document.FilePath} in {document.Project.Key.Id} at version {document.Version}");
 
             _workQueue.AddWork(document);
         }

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/ProjectSystem/RazorProjectService.cs

@@ -341,8 +341,6 @@ private ProjectKey AddProjectCore(ProjectSnapshotManager.Updater updater, string
 
         _logger.LogInformation($"Added project '{filePath}' with key {hostProject.Key} to project system.");
 
-        TryMigrateMiscellaneousDocumentsToProject(updater);
-
         return hostProject.Key;
     }
 
@@ -611,46 +609,4 @@ private static string EnsureFullPath(string filePath, string projectDirectory)
 
         return normalizedFilePath;
     }
-
-    private void TryMigrateMiscellaneousDocumentsToProject(ProjectSnapshotManager.Updater updater)
-    {
-        var miscellaneousProject = _projectManager.GetMiscellaneousProject();
-
-        foreach (var documentFilePath in miscellaneousProject.DocumentFilePaths)
-        {
-            var projectSnapshot = _projectManager.FindPotentialProjects(documentFilePath).FirstOrDefault();
-            if (projectSnapshot is null)
-            {
-                continue;
-            }
-
-            if (miscellaneousProject.GetDocument(documentFilePath) is not DocumentSnapshot documentSnapshot)
-            {
-                continue;
-            }
-
-            // Remove from miscellaneous project
-            updater.DocumentRemoved(miscellaneousProject.Key, documentSnapshot.State.HostDocument);
-
-            // Add to new project
-
-            var textLoader = new DocumentSnapshotTextLoader(documentSnapshot);
-
-            // If we're moving from the misc files project to a real project, then target path will be the full path to the file
-            // and the next update to the project will update it to be a relative path. To save a bunch of busy work if that is
-            // the only change necessary, we can proactively do that work here. This also means that when we later find out about
-            // this document the "real" way, it will be equal to the one we already know about, and we won't lose content
-            var projectDirectory = FilePathNormalizer.GetNormalizedDirectoryName(projectSnapshot.FilePath);
-            var newTargetPath = documentSnapshot.TargetPath;
-            if (FilePathNormalizer.Normalize(newTargetPath).StartsWith(projectDirectory))
-            {
-                newTargetPath = newTargetPath[projectDirectory.Length..];
-            }
-
-            var newHostDocument = new HostDocument(documentSnapshot.FilePath, newTargetPath, documentSnapshot.FileKind);
-            _logger.LogInformation($"Migrating '{documentFilePath}' from the '{miscellaneousProject.Key}' project to '{projectSnapshot.Key}' project.");
-
-            updater.DocumentAdded(projectSnapshot.Key, newHostDocument, textLoader);
-        }
-    }
 }

src/Razor/src/Microsoft.VisualStudio.LanguageServices.Razor/LanguageClient/CSharpVirtualDocumentManager.cs

@@ -1,27 +1,60 @@
 // 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.ComponentModel.Composition;
+using System.Threading;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Razor.Utilities;
 using Microsoft.CodeAnalysis.Razor.ProjectSystem;
 using Microsoft.CodeAnalysis.Razor.Workspaces;
 using Microsoft.VisualStudio.LanguageServer.ContainedLanguage;
 
 namespace Microsoft.VisualStudio.Razor.LanguageClient;
 
 [Export(typeof(IRazorStartupService))]
-internal class CSharpVirtualDocumentManager : IRazorStartupService
+internal class CSharpVirtualDocumentManager : IRazorStartupService, IDisposable
 {
     private readonly LSPDocumentManager _lspDocumentManager;
 
+    private static readonly TimeSpan s_defaultDelay = TimeSpan.FromMilliseconds(200);
+    private readonly CancellationTokenSource _disposeTokenSource;
+    private readonly AsyncBatchingWorkQueue _workQueue;
+
     [ImportingConstructor]
     public CSharpVirtualDocumentManager(
         LSPDocumentManager lspDocumentManager,
         IProjectSnapshotManager projectManager)
     {
         _lspDocumentManager = lspDocumentManager;
+
+        _disposeTokenSource = new();
+        _workQueue = new AsyncBatchingWorkQueue(s_defaultDelay, ProcessBatchAsync, _disposeTokenSource.Token);
+
         projectManager.Changed += ProjectManager_Changed;
     }
 
+    public void Dispose()
+    {
+        if (_disposeTokenSource.IsCancellationRequested)
+        {
+            return;
+        }
+
+        _disposeTokenSource.Cancel();
+        _disposeTokenSource.Dispose();
+    }
+
+    private ValueTask ProcessBatchAsync(CancellationToken token)
+    {
+        if (!token.IsCancellationRequested)
+        {
+            _lspDocumentManager.RefreshVirtualDocuments();
+        }
+
+        return default;
+    }
+
     private void ProjectManager_Changed(object sender, ProjectChangeEventArgs e)
     {
         if (e.SolutionIsClosing)
@@ -36,7 +69,7 @@ private void ProjectManager_Changed(object sender, ProjectChangeEventArgs e)
             case ProjectChangeKind.ProjectChanged:
             case ProjectChangeKind.ProjectAdded:
             case ProjectChangeKind.ProjectRemoved:
-                _lspDocumentManager.RefreshVirtualDocuments();
+                _workQueue.AddWork();
                 break;
         }
     }

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/GeneratedDocumentPublisherTest.cs

@@ -387,6 +387,31 @@ await _projectManager.UpdateAsync(updater =>
         Assert.Equal(124, updateRequest.HostDocumentVersion);
     }
 
+    [Fact]
+    public async Task ProjectSnapshotManager_DocumentRemoved_ClearsContent()
+    {
+        // Arrange
+        var options = new TestLanguageServerFeatureOptions(includeProjectKeyInGeneratedFilePath: true);
+        var publisher = new GeneratedDocumentPublisher(_projectManager, _serverClient, options, LoggerFactory);
+        var sourceTextContent = "// The content";
+        var initialSourceText = SourceText.From(sourceTextContent);
+
+        publisher.PublishCSharp(s_hostProject.Key, s_hostDocument.FilePath, initialSourceText, 123);
+
+        await _projectManager.UpdateAsync(updater =>
+        {
+            updater.DocumentOpened(s_hostProject.Key, s_hostDocument.FilePath, initialSourceText);
+        });
+
+        // Act
+        await _projectManager.UpdateAsync(updater =>
+        {
+            updater.DocumentRemoved(s_hostProject.Key, s_hostDocument);
+        });
+
+        Assert.Equal(0, publisher.GetTestAccessor().PublishedCSharpDataCount);
+    }
+
     [Fact]
     public async Task ProjectSnapshotManager_ProjectRemoved_ClearsContent()
     {

src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/GeneratedDocumentSynchronizerTest.cs

@@ -86,6 +86,19 @@ public void DocumentProcessed_CloseDocument_DoesntPublish()
         Assert.False(_publisher.PublishedHtml);
     }
 
+    [Fact]
+    public void DocumentProcessed_RemovedDocument_DoesntPublish()
+    {
+        var document = TestDocumentSnapshot.Create("/path/to/non.existent.file.razor");
+
+        // Act
+        _synchronizer.DocumentProcessed(_codeDocument, document);
+
+        // Assert
+        Assert.False(_publisher.PublishedCSharp);
+        Assert.False(_publisher.PublishedHtml);
+    }
+
     private class TestGeneratedDocumentPublisher : IGeneratedDocumentPublisher
     {
         public bool PublishedCSharp { get; private set; }