PR feedback on recent work to sync solutions consistently

src/EditorFeatures/Core/EditorConfigSettings/Aggregator/SettingsAggregator.cs

@@ -3,31 +3,48 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
-using System.Collections.Generic;
-using System.Linq;
+using System.Diagnostics.CodeAnalysis;
+using System.Threading;
+using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Editor.EditorConfigSettings.Data;
 using Microsoft.CodeAnalysis.Editor.EditorConfigSettings.DataProvider;
+using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
 using Microsoft.CodeAnalysis.Shared.Collections;
-using Microsoft.CodeAnalysis.Shared.Extensions;
+using Microsoft.CodeAnalysis.Shared.TestHooks;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.Editor.EditorConfigSettings;
 
 internal partial class SettingsAggregator : ISettingsAggregator
 {
     private readonly Workspace _workspace;
     private readonly ISettingsProviderFactory<AnalyzerSetting> _analyzerProvider;
+    private readonly AsyncBatchingWorkQueue _workQueue;
+
     private ISettingsProviderFactory<Setting> _whitespaceProvider;
     private ISettingsProviderFactory<NamingStyleSetting> _namingStyleProvider;
     private ISettingsProviderFactory<CodeStyleSetting> _codeStyleProvider;
 
-    public SettingsAggregator(Workspace workspace)
+    public SettingsAggregator(
+        Workspace workspace,
+        IThreadingContext threadingContext,
+        IAsynchronousOperationListener listener)
     {
         _workspace = workspace;
         _workspace.WorkspaceChanged += UpdateProviders;
-        _whitespaceProvider = GetOptionsProviderFactory<Setting>(_workspace);
-        _codeStyleProvider = GetOptionsProviderFactory<CodeStyleSetting>(_workspace);
-        _namingStyleProvider = GetOptionsProviderFactory<NamingStyleSetting>(_workspace);
-        _analyzerProvider = GetOptionsProviderFactory<AnalyzerSetting>(_workspace);
+
+        var currentSolution = _workspace.CurrentSolution.SolutionState;
+        UpdateProviders(currentSolution);
+
+        // TODO(cyrusn): Why do we not update this as well inside UpdateProviders when we hear about a workspace event?
+        _analyzerProvider = GetOptionsProviderFactory<AnalyzerSetting>(currentSolution);
+
+        // Batch these up so that we don't do a lot of expensive work when hearing a flurry of workspace events.
+        _workQueue = new AsyncBatchingWorkQueue(
+            TimeSpan.FromSeconds(1),
+            UpdateProvidersAsync,
+            listener,
+            threadingContext.DisposalToken);
     }
 
     private void UpdateProviders(object? sender, WorkspaceChangeEventArgs e)
@@ -42,11 +59,7 @@ private void UpdateProviders(object? sender, WorkspaceChangeEventArgs e)
             case WorkspaceChangeKind.ProjectAdded:
             case WorkspaceChangeKind.ProjectRemoved:
             case WorkspaceChangeKind.ProjectChanged:
-                _whitespaceProvider = GetOptionsProviderFactory<Setting>(_workspace);
-                _codeStyleProvider = GetOptionsProviderFactory<CodeStyleSetting>(_workspace);
-                _namingStyleProvider = GetOptionsProviderFactory<NamingStyleSetting>(_workspace);
-                break;
-            default:
+                _workQueue.AddWork();
                 break;
         }
     }
@@ -76,14 +89,30 @@ private void UpdateProviders(object? sender, WorkspaceChangeEventArgs e)
         return null;
     }
 
-    private static ISettingsProviderFactory<T> GetOptionsProviderFactory<T>(Workspace workspace)
+    private ValueTask UpdateProvidersAsync(CancellationToken cancellationToken)
+    {
+        UpdateProviders(_workspace.CurrentSolution.SolutionState);
+        return ValueTaskFactory.CompletedTask;
+    }
+
+    [MemberNotNull(nameof(_whitespaceProvider))]
+    [MemberNotNull(nameof(_codeStyleProvider))]
+    [MemberNotNull(nameof(_namingStyleProvider))]
+    private void UpdateProviders(SolutionState solution)
+    {
+        _whitespaceProvider = GetOptionsProviderFactory<Setting>(solution);
+        _codeStyleProvider = GetOptionsProviderFactory<CodeStyleSetting>(solution);
+        _namingStyleProvider = GetOptionsProviderFactory<NamingStyleSetting>(solution);
+    }
+
+    private static ISettingsProviderFactory<T> GetOptionsProviderFactory<T>(SolutionState solution)
     {
         using var providers = TemporaryArray<ISettingsProviderFactory<T>>.Empty;
 
-        var commonProvider = workspace.Services.GetRequiredService<IWorkspaceSettingsProviderFactory<T>>();
+        var commonProvider = solution.Services.GetRequiredService<IWorkspaceSettingsProviderFactory<T>>();
         providers.Add(commonProvider);
 
-        var projectCountByLanguage = workspace.CurrentSolution.SolutionState.ProjectCountByLanguage;
+        var projectCountByLanguage = solution.ProjectCountByLanguage;
 
         TryAddProviderForLanguage(LanguageNames.CSharp);
         TryAddProviderForLanguage(LanguageNames.VisualBasic);
@@ -94,7 +123,7 @@ void TryAddProviderForLanguage(string language)
         {
             if (projectCountByLanguage.ContainsKey(language))
             {
-                var provider = workspace.Services.GetLanguageServices(language).GetService<ILanguageSettingsProviderFactory<T>>();
+                var provider = solution.Services.GetLanguageServices(language).GetService<ILanguageSettingsProviderFactory<T>>();
                 if (provider != null)
                     providers.Add(provider);
             }

src/EditorFeatures/Core/EditorConfigSettings/Aggregator/SettingsAggregatorFactory.cs

@@ -4,20 +4,23 @@
 
 using System;
 using System.Composition;
+using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
 using Microsoft.CodeAnalysis.Host;
 using Microsoft.CodeAnalysis.Host.Mef;
+using Microsoft.CodeAnalysis.Shared.TestHooks;
 
 namespace Microsoft.CodeAnalysis.Editor.EditorConfigSettings;
 
 [ExportWorkspaceServiceFactory(typeof(ISettingsAggregator), ServiceLayer.Default), Shared]
-internal class SettingsAggregatorFactory : IWorkspaceServiceFactory
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
+internal sealed class SettingsAggregatorFactory(
+    IThreadingContext threadingContext,
+    IAsynchronousOperationListenerProvider listenerProvider) : IWorkspaceServiceFactory
 {
-    [ImportingConstructor]
-    [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
-    public SettingsAggregatorFactory()
-    {
-    }
+    private readonly IThreadingContext _threadingContext = threadingContext;
+    private readonly IAsynchronousOperationListener _listener = listenerProvider.GetListener(FeatureAttribute.RuleSetEditor);
 
     public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
-        => new SettingsAggregator(workspaceServices.Workspace);
+        => new SettingsAggregator(workspaceServices.Workspace, _threadingContext, _listener);
 }

src/Workspaces/Core/Portable/SourceGeneration/IRemoteSourceGenerationService.cs

@@ -13,7 +13,7 @@
 namespace Microsoft.CodeAnalysis.SourceGeneration;
 
 [DataContract]
-internal readonly record struct RegularCompilationTrackerSourceGenerationInfo(
+internal readonly record struct SourceGeneratedDocmentInfo(
     [property: DataMember(Order = 0)] SourceGeneratedDocumentIdentity DocumentIdentity,
     [property: DataMember(Order = 1)] SourceGeneratedDocumentContentIdentity ContentIdentity,
     [property: DataMember(Order = 2)] DateTime GenerationDateTime);
@@ -27,24 +27,22 @@ internal interface IRemoteSourceGenerationService
     /// compare that to the prior generated documents it has to see if it can reuse those directly, or if it needs to
     /// remove any documents no longer around, add any new documents, or change the contents of any existing documents.
     /// </summary>
-    /// <remarks>
-    /// Should only be called by the "RegularCompilationTracker", and should only return data from its view of the
-    /// world.  Not from the view of a "GeneratedFileReplacingCompilationTracker".
-    /// </remarks>
-    ValueTask<ImmutableArray<RegularCompilationTrackerSourceGenerationInfo>> GetRegularCompilationTrackerSourceGenerationInfoAsync(
-        Checksum solutionChecksum, ProjectId projectId, CancellationToken cancellationToken);
+    /// <param name="withFrozenSourceGeneratedDocuments">Controls if the caller wants frozen source generator documents
+    /// included in the result, or if only the most underlying generated documents (produced by the real compiler <see
+    /// cref="GeneratorDriver"/> should be included.</param>
+    ValueTask<ImmutableArray<SourceGeneratedDocmentInfo>> GetSourceGeneratedDocumentInfoAsync(
+        Checksum solutionChecksum, ProjectId projectId, bool withFrozenSourceGeneratedDocuments, CancellationToken cancellationToken);
 
     /// <summary>
     /// Given a particular set of generated document ids, returns the fully generated content for those documents.
     /// Should only be called by the host for documents it does not know about, or documents whose checksum contents are
     /// different than the last time the document was queried.
     /// </summary>
-    /// <remarks>
-    /// Should only be called by the "RegularCompilationTracker", and should only return data from its view of the
-    /// world.  Not from the view of a "GeneratedFileReplacingCompilationTracker".
-    /// </remarks>
-    ValueTask<ImmutableArray<string>> GetRegularCompilationTrackerContentsAsync(
-        Checksum solutionChecksum, ProjectId projectId, ImmutableArray<DocumentId> documentIds, CancellationToken cancellationToken);
+    /// <param name="withFrozenSourceGeneratedDocuments">Controls if the caller wants frozen source generator documents
+    /// included in the result, or if only the most underlying generated documents (produced by the real compiler <see
+    /// cref="GeneratorDriver"/> should be included.</param>
+    ValueTask<ImmutableArray<string>> GetContentsAsync(
+        Checksum solutionChecksum, ProjectId projectId, ImmutableArray<DocumentId> documentIds, bool withFrozenSourceGeneratedDocuments, CancellationToken cancellationToken);
 
     /// <summary>
     /// Whether or not the specified analyzer references have source generators or not.

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.ICompilationTracker.cs

@@ -56,21 +56,15 @@ bool ContainsAssemblyOrModuleOrDynamic(
         Task<Checksum> GetDependentChecksumAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken);
 
         /// <summary>
-        /// Gets the *final* view of the generated documents for this tracker.  If this is a <see
-        /// cref="RegularCompilationTracker"/> this will be the true generated documents for this tracker (generated by
-        /// our underlying <see cref="GeneratorDriver"/>).  If this is a <see
-        /// cref="GeneratedFileReplacingCompilationTracker"/>, then this will be the generated documents of its <see
-        /// cref="GeneratedFileReplacingCompilationTracker.UnderlyingTracker"/>, along with all of its replacement
-        /// frozen documents overlaid on top.
+        /// Gets the source generator files generated by this <see cref="ICompilationTracker"/>. <paramref
+        /// name="withFrozenSourceGeneratedDocuments"/>Controls whether frozen source generated documents are included
+        /// in the result.  If <see langword="false"/> this will call all the way through to the most underlying <see
+        /// cref="RegularCompilationTracker"/> to get its generated documents.  If this is <see langword="true"/> then
+        /// this will be those same generated documents, along with all the generated documents from all wrapping <see
+        /// cref="WithFrozenSourceGeneratedDocumentsCompilationTracker"/>'s frozen documents overlaid on top.
         /// </summary>
-        ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetSourceGeneratedDocumentStatesAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken);
-
-        /// <summary>
-        /// Equivalent to <see cref="GetSourceGeneratedDocumentStatesAsync"/>, but only returning from the innermost
-        /// underlying <see cref="RegularCompilationTracker"/>.  Any frozen generated documents in a <see
-        /// cref="GeneratedFileReplacingCompilationTracker"/> will *not* be overlaid on top of this.
-        /// </summary>
-        ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetRegularCompilationTrackerSourceGeneratedDocumentStatesAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken);
+        ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetSourceGeneratedDocumentStatesAsync(
+            SolutionCompilationState compilationState, bool withFrozenSourceGeneratedDocuments, CancellationToken cancellationToken);
 
         ValueTask<ImmutableArray<Diagnostic>> GetSourceGeneratorDiagnosticsAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken);
         ValueTask<GeneratorDriverRunResult?> GetSourceGeneratorRunResultAsync(SolutionCompilationState solution, CancellationToken cancellationToken);

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker.cs → src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker.cs

@@ -837,13 +837,13 @@ public ICompilationTracker WithDoNotCreateCreationPolicy(CancellationToken cance
                 }
             }
 
-            public ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetSourceGeneratedDocumentStatesAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken)
-                // Just defer to the core function that creates these.  They are the right values for both of these calls.
-                => GetRegularCompilationTrackerSourceGeneratedDocumentStatesAsync(compilationState, cancellationToken);
-
-            public async ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetRegularCompilationTrackerSourceGeneratedDocumentStatesAsync(
-                SolutionCompilationState compilationState, CancellationToken cancellationToken)
+            public async ValueTask<TextDocumentStates<SourceGeneratedDocumentState>> GetSourceGeneratedDocumentStatesAsync(
+                SolutionCompilationState compilationState, bool withFrozenSourceGeneratedDocuments, CancellationToken cancellationToken)
             {
+                // Note: withFrozenSourceGeneratedDocuments has no impact on is.  We're always returning real generated
+                // docs, not frozen docs.  Frozen docs are only involved with a
+                // WithFrozenSourceGeneratedDocumentsCompilationTracker
+
                 // If we don't have any generators, then we know we have no generated files, so we can skip the computation entirely.
                 if (!await compilationState.HasSourceGeneratorsAsync(this.ProjectState.Id, cancellationToken).ConfigureAwait(false))
                     return TextDocumentStates<SourceGeneratedDocumentState>.Empty;

src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.CompilationTracker_Generators.cs → src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs

@@ -93,14 +93,14 @@ private partial class RegularCompilationTracker : ICompilationTracker
 
             // First, grab the info from our external host about the generated documents it has for this project.  Note:
             // we ourselves are the innermost "RegularCompilationTracker" responsible for actually running generators.
-            // As such, our call to the oop side reflects that by asking for only *its* innermost
-            // RegularCompilationTracker to do the same.
+            // As such, our call to the oop side reflects that by asking for the real source generated docs, and *not*
+            // any overlaid 'frozen' source generated documents.
             var projectId = this.ProjectState.Id;
             var infosOpt = await connection.TryInvokeAsync(
                 compilationState,
                 projectId,
-                (service, solutionChecksum, cancellationToken) => service.GetRegularCompilationTrackerSourceGenerationInfoAsync(
-                    solutionChecksum, projectId, cancellationToken),
+                (service, solutionChecksum, cancellationToken) => service.GetSourceGeneratedDocumentInfoAsync(
+                    solutionChecksum, projectId, withFrozenSourceGeneratedDocuments: false, cancellationToken),
                 cancellationToken).ConfigureAwait(false);
 
             if (!infosOpt.HasValue)
@@ -160,13 +160,13 @@ private partial class RegularCompilationTracker : ICompilationTracker
             // Either we generated a different number of files, and/or we had contents of files that changed. Ensure we
             // know the contents of any new/changed files.  Note: we ourselves are the innermost
             // "RegularCompilationTracker" responsible for actually running generators. As such, our call to the oop
-            // side reflects that by asking for the source gen contents produced by *its* innermost
-            // RegularCompilationTracker.
+            // side reflects that by asking for the real source generated docs, and *not* any overlaid 'frozen' source
+            // generated documents.
             var generatedSourcesOpt = await connection.TryInvokeAsync(
                 compilationState,
                 projectId,
-                (service, solutionChecksum, cancellationToken) => service.GetRegularCompilationTrackerContentsAsync(
-                    solutionChecksum, projectId, documentsToAddOrUpdate.ToImmutable(), cancellationToken),
+                (service, solutionChecksum, cancellationToken) => service.GetContentsAsync(
+                    solutionChecksum, projectId, documentsToAddOrUpdate.ToImmutable(), withFrozenSourceGeneratedDocuments: false, cancellationToken),
                 cancellationToken).ConfigureAwait(false);
 
             if (!generatedSourcesOpt.HasValue)