From 8c5710b8ec1cada89c455c4c29e1da7ba084a121 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Thu, 30 Sep 2021 16:32:59 -0700 Subject: [PATCH 1/8] Fix spelling error --- .../CSharpTest/Completion/CompletionServiceTests.cs | 2 +- src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs | 2 +- .../SolutionTests/SolutionWithSourceGeneratorTests.cs | 6 +++--- src/Workspaces/CoreTestUtilities/WorkspaceTestUtilities.cs | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/Completion/CompletionServiceTests.cs b/src/EditorFeatures/CSharpTest/Completion/CompletionServiceTests.cs index e684569debd6f..a9cb2c69b273d 100644 --- a/src/EditorFeatures/CSharpTest/Completion/CompletionServiceTests.cs +++ b/src/EditorFeatures/CSharpTest/Completion/CompletionServiceTests.cs @@ -52,7 +52,7 @@ public class C1 var generatorRanCount = 0; var generator = new CallbackGenerator(onInit: _ => { }, onExecute: _ => Interlocked.Increment(ref generatorRanCount)); - using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartalSemantics(); + using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); var analyzerReference = new TestGeneratorReference(generator); var project = SolutionUtilities.AddEmptyProject(workspace.CurrentSolution) .AddAnalyzerReference(analyzerReference) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index 0c8c804d1eb10..8c33312289dd9 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -2569,7 +2569,7 @@ public void TestProjectWithBrokenCrossLanguageReferenceHasIncompleteReferences() [Fact] public async Task TestFrozenPartialProjectHasDifferentSemanticVersions() { - using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartalSemantics(); + using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); var project = workspace.CurrentSolution.AddProject("CSharpProject", "CSharpProject", LanguageNames.CSharp); project = project.AddDocument("Extra.cs", SourceText.From("class Extra { }")).Project; diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index e0bb0b2d87059..ad4be96477ed3 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -168,7 +168,7 @@ static async Task AssertCompilationContainsGeneratedFile(Project project, string [Fact] public async Task PartialCompilationsIncludeGeneratedFilesAfterFullGeneration() { - using var workspace = CreateWorkspaceWithPartalSemantics(); + using var workspace = CreateWorkspaceWithPartialSemantics(); var analyzerReference = new TestGeneratorReference(new GenerateFileForEachAdditionalFileWithContentsCommented()); var project = WithPreviewLanguageVersion(AddEmptyProject(workspace.CurrentSolution)) .AddAnalyzerReference(analyzerReference) @@ -304,7 +304,7 @@ public async Task GetDocumentWithGeneratedTreeReturnsGeneratedDocument() [Fact] public async Task GetDocumentWithGeneratedTreeForInProgressReturnsGeneratedDocument() { - using var workspace = CreateWorkspaceWithPartalSemantics(); + using var workspace = CreateWorkspaceWithPartialSemantics(); var analyzerReference = new TestGeneratorReference(new GenerateFileForEachAdditionalFileWithContentsCommented()); var project = WithPreviewLanguageVersion(AddEmptyProject(workspace.CurrentSolution)) .AddAnalyzerReference(analyzerReference) @@ -566,7 +566,7 @@ public async Task FreezingSolutionEnsuresGeneratorsDoNotRun(bool forkBeforeFreez var generatorRan = false; var generator = new CallbackGenerator(onInit: _ => { }, onExecute: _ => { generatorRan = true; }); - using var workspace = CreateWorkspaceWithPartalSemantics(); + using var workspace = CreateWorkspaceWithPartialSemantics(); var analyzerReference = new TestGeneratorReference(generator); var project = AddEmptyProject(workspace.CurrentSolution) .AddAnalyzerReference(analyzerReference) diff --git a/src/Workspaces/CoreTestUtilities/WorkspaceTestUtilities.cs b/src/Workspaces/CoreTestUtilities/WorkspaceTestUtilities.cs index d23f771b3148d..1a7dd0718af72 100644 --- a/src/Workspaces/CoreTestUtilities/WorkspaceTestUtilities.cs +++ b/src/Workspaces/CoreTestUtilities/WorkspaceTestUtilities.cs @@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis.UnitTests { public static class WorkspaceTestUtilities { - public static Workspace CreateWorkspaceWithPartalSemantics(Type[]? additionalParts = null) + public static Workspace CreateWorkspaceWithPartialSemantics(Type[]? additionalParts = null) => new WorkspaceWithPartialSemantics(FeaturesTestCompositions.Features.AddParts(additionalParts).GetHostServices()); private class WorkspaceWithPartialSemantics : Workspace From 1651a33377109959bc261702db91934ad8332894 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Thu, 30 Sep 2021 17:49:09 -0700 Subject: [PATCH 2/8] Don't break if we fork a snapshot after freezing partial semantics The FinalState in the compilation tracker holds onto two compilations: the compilation with generated files (the "real" compilation) and the compilation without generated files, should we need to later re-run generators if any further forking were to happen. When we froze a snapshot with partial semantics, the code was assuming that no further forking would happen, so it incorrectly passed the compilation with generated files into the field that should hold the compilation without generated files; if we later forked, we'd run generators again and that would result in us trying to add generated trees that already existed. The fix is to correctly track both compilations through that path like anything else. To start making that process simpler, I introduce a CompilationPair type that just holds onto both and lets you mutate both at once. There's also an optimization to ensure that if we don't have any generated files we aren't actually forking Compilations twice for no reason, which was a previous guarantee we held. Fixes https://github.com/dotnet/roslyn/issues/56702 --- .../Solution/SolutionState.CompilationPair.cs | 68 +++++++++++++++++++ ...pilationTracker.CompilationTrackerState.cs | 17 +++++ .../SolutionState.CompilationTracker.cs | 46 ++++++------- .../SolutionWithSourceGeneratorTests.cs | 22 ++++++ 4 files changed, 130 insertions(+), 23 deletions(-) create mode 100644 src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationPair.cs diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationPair.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationPair.cs new file mode 100644 index 0000000000000..08d010013e74d --- /dev/null +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationPair.cs @@ -0,0 +1,68 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; + +namespace Microsoft.CodeAnalysis +{ + internal partial class SolutionState + { + private partial class CompilationTracker + { + /// + /// When we're working with compilations, we often have two: a compilation that does not contain generated files + /// (which we might need later to run generators again), and one that has the stale generated files that we might + /// be able to reuse as well. In those cases we have to do the same transformations to both, and this gives us + /// a handy way to do precisely that while not forking compilations twice if there are no generated files anywhere. + /// + internal readonly struct CompilationPair + { + public CompilationPair(Compilation withoutGeneratedDocuments, Compilation withGeneratedDocuments) : this() + { + CompilationWithoutGeneratedDocuments = withoutGeneratedDocuments; + CompilationWithGeneratedDocuments = withGeneratedDocuments; + } + + public Compilation CompilationWithoutGeneratedDocuments { get; } + public Compilation CompilationWithGeneratedDocuments { get; } + + public CompilationPair ReplaceSyntaxTree(SyntaxTree oldTree, SyntaxTree newTree) + { + return WithChange(static (compilation, trees) => compilation.ReplaceSyntaxTree(trees.oldTree, trees.newTree), (oldTree, newTree)); + } + + public CompilationPair AddSyntaxTree(SyntaxTree newTree) + { + return WithChange(static (compilation, t) => compilation.AddSyntaxTrees(t), newTree); + } + + public CompilationPair WithPreviousScriptCompilation(Compilation previousScriptCompilation) + { + return WithChange(static (compilation, priorCompilation) => compilation.WithScriptCompilationInfo(compilation.ScriptCompilationInfo!.WithPreviousScriptCompilation(priorCompilation)), previousScriptCompilation); + } + + public CompilationPair WithReferences(IReadOnlyCollection metadataReferences) + { + return WithChange(static (c, r) => c.WithReferences(r), metadataReferences); + } + + private CompilationPair WithChange(Func change, TArg arg) + { + var changedWithoutGeneratedDocuments = change(CompilationWithoutGeneratedDocuments, arg); + + if (CompilationWithoutGeneratedDocuments == CompilationWithGeneratedDocuments) + { + // If we didn't have any generated files, then no reason to transform twice + return new CompilationPair(changedWithoutGeneratedDocuments, changedWithoutGeneratedDocuments); + } + + var changedWithGeneratedDocuments = change(CompilationWithGeneratedDocuments, arg); + + return new CompilationPair(changedWithoutGeneratedDocuments, changedWithGeneratedDocuments); + } + } + } + } +} diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs index ada903c8d65b5..a9e38398b864b 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs @@ -6,6 +6,8 @@ using System.Collections.Generic; using System.Collections.Immutable; using System.Diagnostics; +using System.Linq; +using System.Threading; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis @@ -101,6 +103,21 @@ protected CompilationTrackerState( { CompilationWithoutGeneratedDocuments = compilationWithoutGeneratedDocuments; GeneratorInfo = generatorInfo; + +#if DEBUG + + // As a sanity check, we should never see the generated trees inside of the compilation that should not + // have generated trees. + var compilation = compilationWithoutGeneratedDocuments?.GetValueOrNull(); + + if (compilation != null) + { + foreach (var generatedDocument in generatorInfo.Documents.States.Values) + { + Contract.ThrowIfTrue(compilation.SyntaxTrees.Contains(generatedDocument.GetSyntaxTree(CancellationToken.None))); + } + } +#endif } public static CompilationTrackerState Create( diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs index 0dff6246cad41..a417bba587742 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs @@ -156,22 +156,23 @@ public ICompilationTracker FreezePartialStateWithTree(SolutionState solution, Do GetPartialCompilationState( solution, docState.Id, out var inProgressProject, - out var inProgressCompilation, + out var compilationPair, out var generatorInfo, out var metadataReferenceToProjectId, cancellationToken); - if (!inProgressCompilation.SyntaxTrees.Contains(tree)) + // Ensure we actually have the tree we need in there + if (!compilationPair.CompilationWithoutGeneratedDocuments.SyntaxTrees.Contains(tree)) { - var existingTree = inProgressCompilation.SyntaxTrees.FirstOrDefault(t => t.FilePath == tree.FilePath); + var existingTree = compilationPair.CompilationWithoutGeneratedDocuments.SyntaxTrees.FirstOrDefault(t => t.FilePath == tree.FilePath); if (existingTree != null) { - inProgressCompilation = inProgressCompilation.ReplaceSyntaxTree(existingTree, tree); + compilationPair = compilationPair.ReplaceSyntaxTree(existingTree, tree); inProgressProject = inProgressProject.UpdateDocument(docState, textChanged: false, recalculateDependentVersions: false); } else { - inProgressCompilation = inProgressCompilation.AddSyntaxTrees(tree); + compilationPair = compilationPair.AddSyntaxTree(tree); Debug.Assert(!inProgressProject.DocumentStates.Contains(docState.Id)); inProgressProject = inProgressProject.AddDocuments(ImmutableArray.Create(docState)); } @@ -181,12 +182,12 @@ public ICompilationTracker FreezePartialStateWithTree(SolutionState solution, Do // have the compilation immediately disappear. So we force it to stay around with a ConstantValueSource. // As a policy, all partial-state projects are said to have incomplete references, since the state has no guarantees. var finalState = FinalState.Create( - new ConstantValueSource>(inProgressCompilation), - new ConstantValueSource>(inProgressCompilation), - inProgressCompilation, + finalCompilationSource: new ConstantValueSource>(compilationPair.CompilationWithGeneratedDocuments), + compilationWithoutGeneratedFilesSource: new ConstantValueSource>(compilationPair.CompilationWithoutGeneratedDocuments), + compilationWithoutGeneratedFiles: compilationPair.CompilationWithoutGeneratedDocuments, hasSuccessfullyLoaded: false, generatorInfo, - inProgressCompilation, + finalCompilation: compilationPair.CompilationWithGeneratedDocuments, this.ProjectState.Id, metadataReferenceToProjectId); @@ -202,12 +203,11 @@ public ICompilationTracker FreezePartialStateWithTree(SolutionState solution, Do /// The compilation state that is returned will have a compilation that is retained so /// that it cannot disappear. /// - /// The compilation to return. Contains any source generated documents that were available already added. private void GetPartialCompilationState( SolutionState solution, DocumentId id, out ProjectState inProgressProject, - out Compilation inProgressCompilation, + out CompilationPair compilations, out CompilationTrackerGeneratorInfo generatorInfo, out Dictionary? metadataReferenceToProjectId, CancellationToken cancellationToken) @@ -230,7 +230,9 @@ private void GetPartialCompilationState( // We'll add in whatever generated documents we do have; these may be from a prior run prior to some changes // being made to the project, but it's the best we have so we'll use it. - inProgressCompilation = compilationWithoutGeneratedDocuments.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken))); + compilations = new CompilationPair( + compilationWithoutGeneratedDocuments, + compilationWithoutGeneratedDocuments.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken)))); // This is likely a bug. It seems possible to pass out a partial compilation state that we don't // properly record assembly symbols for. @@ -248,7 +250,7 @@ private void GetPartialCompilationState( if (finalCompilation != null) { - inProgressCompilation = finalCompilation; + compilations = new CompilationPair(compilationWithoutGeneratedDocuments, finalCompilation); // This should hopefully be safe to return as null. Because we already reached the 'FinalState' // before, we should have already recorded the assembly symbols for it. So not recording them @@ -266,14 +268,12 @@ private void GetPartialCompilationState( if (compilationWithoutGeneratedDocuments == null) { inProgressProject = inProgressProject.RemoveAllDocuments(); - inProgressCompilation = CreateEmptyCompilation(); - } - else - { - inProgressCompilation = compilationWithoutGeneratedDocuments; + compilationWithoutGeneratedDocuments = CreateEmptyCompilation(); } - inProgressCompilation = inProgressCompilation.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken))); + compilations = new CompilationPair( + compilationWithoutGeneratedDocuments, + compilationWithoutGeneratedDocuments.AddSyntaxTrees(generatorInfo.Documents.States.Values.Select(state => state.GetSyntaxTree(cancellationToken)))); // Now add in back a consistent set of project references. For project references // try to get either a CompilationReference or a SkeletonReference. This ensures @@ -297,7 +297,7 @@ private void GetPartialCompilationState( // previous submission project must support compilation: RoslynDebug.Assert(previousScriptCompilation != null); - inProgressCompilation = inProgressCompilation.WithScriptCompilationInfo(inProgressCompilation.ScriptCompilationInfo!.WithPreviousScriptCompilation(previousScriptCompilation)); + compilations = compilations.WithPreviousScriptCompilation(previousScriptCompilation); } else { @@ -307,7 +307,7 @@ private void GetPartialCompilationState( if (metadata == null) { // if we failed to get the metadata, check to see if we previously had existing metadata and reuse it instead. - var inProgressCompilationNotRef = inProgressCompilation; + var inProgressCompilationNotRef = compilations.CompilationWithGeneratedDocuments; metadata = inProgressCompilationNotRef.ExternalReferences.FirstOrDefault( r => solution.GetProjectState(inProgressCompilationNotRef.GetAssemblyOrModuleSymbol(r) as IAssemblySymbol)?.Id == projectReference.ProjectId); } @@ -324,9 +324,9 @@ private void GetPartialCompilationState( inProgressProject = inProgressProject.WithProjectReferences(newProjectReferences); - if (!Enumerable.SequenceEqual(inProgressCompilation.ExternalReferences, metadataReferences)) + if (!Enumerable.SequenceEqual(compilations.CompilationWithoutGeneratedDocuments.ExternalReferences, metadataReferences)) { - inProgressCompilation = inProgressCompilation.WithReferences(metadataReferences); + compilations = compilations.WithReferences(metadataReferences); } SolutionLogger.CreatePartialProjectState(); diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index ad4be96477ed3..9951394a4bea0 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -592,5 +592,27 @@ public async Task FreezingSolutionEnsuresGeneratorsDoNotRun(bool forkBeforeFreez Assert.False(generatorRan); } + + [Fact] + [WorkItem(56702, "https://github.com/dotnet/roslyn/issues/56702")] + public async Task ForkAfterFreezeWorks() + { + using var workspace = CreateWorkspaceWithPartialSemantics(); + var analyzerReference = new TestGeneratorReference(new SingleFileTestGenerator("// Hello, Source Generators!")); + var project = AddEmptyProject(workspace.CurrentSolution) + .AddAnalyzerReference(analyzerReference) + .AddDocument("RegularDocument.cs", "// Source File", filePath: "RegularDocument.cs").Project; + + // Ensure generators are ran + _ = await project.GetCompilationAsync(); + + var document = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None); + + // And fork with new contents; this will force generators to run again + document = document.WithText(SourceText.From("// Something else")); + + var compilation = await document.Project.GetRequiredCompilationAsync(CancellationToken.None); + Assert.Equal(2, compilation.SyntaxTrees.Count()); + } } } From 36861dcdd843a505a0fdc4cf15d4a081b2b9247b Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Tue, 5 Oct 2021 16:51:13 -0700 Subject: [PATCH 3/8] Remove TODO that was actually completed --- .../Workspace/Solution/SolutionState.CompilationTracker.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs index a417bba587742..fb160dd04f5c8 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs @@ -798,8 +798,6 @@ private async Task FinalizeCompilationAsync( } // We will finalize the compilation by adding full contents here. - // TODO: allow finalize compilation to incrementally update a prior version - // https://github.com/dotnet/roslyn/issues/46418 Compilation compilationWithGenerators; TextDocumentStates generatedDocuments; From d198e8280a5a8fcb145f4ac3dff8080af17bba41 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Tue, 5 Oct 2021 17:15:33 -0700 Subject: [PATCH 4/8] Pass around CompilationTrackerGeneratorInfo more We still had a few places where we were decomposing the GeneratorInfo type to pass it into the parameters for FinalizeCompilationAsync; it's easier if we just pass it around directly and process the values in FinalizeCompilationAsync. --- .../SolutionState.CompilationTracker.cs | 89 +++++++------------ 1 file changed, 31 insertions(+), 58 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs index fb160dd04f5c8..63da8f5a595f3 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs @@ -218,7 +218,7 @@ private void GetPartialCompilationState( // check whether we can bail out quickly for typing case var inProgressState = state as InProgressState; - generatorInfo = state.GeneratorInfo; + generatorInfo = state.GeneratorInfo.WithDocumentsAreFinal(true); // all changes left for this document is modifying the given document. // we can use current state as it is since we will replace the document with latest document anyway. @@ -485,21 +485,12 @@ private async Task BuildCompilationInfoAsync( compilation = state.CompilationWithoutGeneratedDocuments?.GetValueOrNull(cancellationToken); - // If we have already reached FinalState in the past but the compilation was garbage collected, we still have the generated documents - // so we can pass those to FinalizeCompilationAsync to avoid the recomputation. This is necessary for correctness as otherwise - // we'd be reparsing trees which could result in generated documents changing identity. - var authoritativeGeneratedDocuments = state.GeneratorInfo.DocumentsAreFinal ? state.GeneratorInfo.Documents : (TextDocumentStates?)null; - var nonAuthoritativeGeneratedDocuments = state.GeneratorInfo.Documents; - var generatorDriver = state.GeneratorInfo.Driver; - if (compilation == null) { // We've got nothing. Build it from scratch :( return await BuildCompilationInfoFromScratchAsync( solution, - authoritativeGeneratedDocuments, - nonAuthoritativeGeneratedDocuments, - generatorDriver, + state.GeneratorInfo, cancellationToken).ConfigureAwait(false); } @@ -509,10 +500,8 @@ private async Task BuildCompilationInfoAsync( return await FinalizeCompilationAsync( solution, compilation, - authoritativeGeneratedDocuments, - nonAuthoritativeGeneratedDocuments, + state.GeneratorInfo, compilationWithStaleGeneratedTrees: null, - generatorDriver, cancellationToken).ConfigureAwait(false); } else @@ -525,28 +514,21 @@ private async Task BuildCompilationInfoAsync( private async Task BuildCompilationInfoFromScratchAsync( SolutionState solution, - TextDocumentStates? authoritativeGeneratedDocuments, - TextDocumentStates nonAuthoritativeGeneratedDocuments, - GeneratorDriver? generatorDriver, + CompilationTrackerGeneratorInfo generatorInfo, CancellationToken cancellationToken) { try { var compilation = await BuildDeclarationCompilationFromScratchAsync( solution.Services, - new CompilationTrackerGeneratorInfo( - nonAuthoritativeGeneratedDocuments, - generatorDriver, - documentsAreFinal: false), + generatorInfo, cancellationToken).ConfigureAwait(false); return await FinalizeCompilationAsync( solution, compilation, - authoritativeGeneratedDocuments, - nonAuthoritativeGeneratedDocuments, + generatorInfo, compilationWithStaleGeneratedTrees: null, - generatorDriver, cancellationToken).ConfigureAwait(false); } catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken)) @@ -613,10 +595,8 @@ private async Task BuildFinalStateFromInProgressStateAsync( return await FinalizeCompilationAsync( solution, compilationWithoutGenerators, - authoritativeGeneratedDocuments: null, - nonAuthoritativeGeneratedDocuments: state.GeneratorInfo.Documents, + state.GeneratorInfo.WithDriver(generatorDriver), compilationWithGenerators, - generatorDriver, cancellationToken).ConfigureAwait(false); } catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken)) @@ -707,26 +687,18 @@ public CompilationInfo(Compilation compilation, bool hasSuccessfullyLoaded, Text /// Add all appropriate references to the compilation and set it as our final compilation /// state. /// - /// The generated documents that can be used since they are already - /// known to be correct for the given state. This would be non-null in cases where we had computed everything and - /// ran generators, but then the compilation was garbage collected and are re-creating a compilation but we - /// still had the prior generated result available. - /// The generated documents from a previous pass which may - /// or may not be correct for the current compilation. These states may be used to access cached results, if - /// and when applicable for the current compilation. + /// The generator info that contains the last run of the documents, if any exists, as + /// well as the driver that can be used to run if need to. /// The compilation from a prior run that contains generated trees, which - /// match the states included in . If a generator run here produces - /// the same set of generated documents as are in , and we don't need to make any other + /// match the states included in . If a generator run here produces + /// the same set of generated documents as are in , and we don't need to make any other /// changes to references, we can then use this compilation instead of re-adding source generated files again to the /// . - /// The generator driver that can be reused for this finalization. private async Task FinalizeCompilationAsync( SolutionState solution, Compilation compilationWithoutGenerators, - TextDocumentStates? authoritativeGeneratedDocuments, - TextDocumentStates nonAuthoritativeGeneratedDocuments, + CompilationTrackerGeneratorInfo generatorInfo, Compilation? compilationWithStaleGeneratedTrees, - GeneratorDriver? generatorDriver, CancellationToken cancellationToken) { try @@ -800,12 +772,14 @@ private async Task FinalizeCompilationAsync( // We will finalize the compilation by adding full contents here. Compilation compilationWithGenerators; - TextDocumentStates generatedDocuments; - if (authoritativeGeneratedDocuments.HasValue) + if (generatorInfo.DocumentsAreFinal) { - generatedDocuments = authoritativeGeneratedDocuments.Value; + // We must have ran generators before, but for some reason had to remake the compilation from scratch. + // This could happen if the trees were strongly held, but the compilation was entirely garbage collected. + // Just add in the trees we already have. We don't want to rerun since the consumer of this Solution + // snapshot has already seen the trees and thus needs to ensure identity of them. compilationWithGenerators = compilationWithoutGenerators.AddSyntaxTrees( - await generatedDocuments.States.Values.SelectAsArrayAsync(state => state.GetSyntaxTreeAsync(cancellationToken)).ConfigureAwait(false)); + await generatorInfo.Documents.States.Values.SelectAsArrayAsync(state => state.GetSyntaxTreeAsync(cancellationToken)).ConfigureAwait(false)); } else { @@ -814,20 +788,20 @@ private async Task FinalizeCompilationAsync( if (ProjectState.SourceGenerators.Any()) { // If we don't already have a generator driver, we'll have to create one from scratch - if (generatorDriver == null) + if (generatorInfo.Driver == null) { var additionalTexts = this.ProjectState.AdditionalDocumentStates.SelectAsArray(static documentState => documentState.AdditionalText); var compilationFactory = this.ProjectState.LanguageServices.GetRequiredService(); - generatorDriver = compilationFactory.CreateGeneratorDriver( + generatorInfo = generatorInfo.WithDriver(compilationFactory.CreateGeneratorDriver( this.ProjectState.ParseOptions!, ProjectState.SourceGenerators, this.ProjectState.AnalyzerOptions.AnalyzerConfigOptionsProvider, - additionalTexts); + additionalTexts)); } - generatorDriver = generatorDriver.RunGenerators(compilationWithoutGenerators, cancellationToken); - var runResult = generatorDriver.GetRunResult(); + generatorInfo = generatorInfo.WithDriver(generatorInfo.Driver!.RunGenerators(compilationWithoutGenerators, cancellationToken)); + var runResult = generatorInfo.Driver!.GetRunResult(); // We may be able to reuse compilationWithStaleGeneratedTrees if the generated trees are identical. We will assign null // to compilationWithStaleGeneratedTrees if we at any point realize it can't be used. We'll first check the count of trees @@ -836,7 +810,7 @@ private async Task FinalizeCompilationAsync( // and the prior generated trees are identical. if (compilationWithStaleGeneratedTrees != null) { - if (nonAuthoritativeGeneratedDocuments.Count != runResult.Results.Sum(r => r.GeneratedSources.Length)) + if (generatorInfo.Documents.Count != runResult.Results.Sum(r => r.GeneratedSources.Length)) { compilationWithStaleGeneratedTrees = null; } @@ -847,7 +821,7 @@ private async Task FinalizeCompilationAsync( foreach (var generatedSource in generatorResult.GeneratedSources) { var existing = FindExistingGeneratedDocumentState( - nonAuthoritativeGeneratedDocuments, + generatorInfo.Documents, generatorResult.Generator, generatedSource.HintName); @@ -892,14 +866,16 @@ private async Task FinalizeCompilationAsync( // If we didn't null out this compilation, it means we can actually use it if (compilationWithStaleGeneratedTrees != null) { - generatedDocuments = nonAuthoritativeGeneratedDocuments; compilationWithGenerators = compilationWithStaleGeneratedTrees; + generatorInfo = generatorInfo.WithDocumentsAreFinal(true); } else { - generatedDocuments = new TextDocumentStates(generatedDocumentsBuilder.ToImmutableAndClear()); + // We produced new documents, so time to create new state for it + var generatedDocuments = new TextDocumentStates(generatedDocumentsBuilder.ToImmutableAndClear()); compilationWithGenerators = compilationWithoutGenerators.AddSyntaxTrees( await generatedDocuments.States.Values.SelectAsArrayAsync(state => state.GetSyntaxTreeAsync(cancellationToken)).ConfigureAwait(false)); + generatorInfo = new CompilationTrackerGeneratorInfo(generatedDocuments, generatorInfo.Driver, documentsAreFinal: true); } } @@ -908,17 +884,14 @@ private async Task FinalizeCompilationAsync( CompilationTrackerState.CreateValueSource(compilationWithoutGenerators, solution.Services), compilationWithoutGenerators, hasSuccessfullyLoaded, - new CompilationTrackerGeneratorInfo( - generatedDocuments, - generatorDriver, - documentsAreFinal: true), + generatorInfo, compilationWithGenerators, this.ProjectState.Id, metadataReferenceToProjectId); this.WriteState(finalState, solution.Services); - return new CompilationInfo(compilationWithGenerators, hasSuccessfullyLoaded, generatedDocuments); + return new CompilationInfo(compilationWithGenerators, hasSuccessfullyLoaded, generatorInfo.Documents); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { From eab3467822ee6186190c85661b32d07726b6fa82 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Tue, 5 Oct 2021 17:25:16 -0700 Subject: [PATCH 5/8] When we freeze partial semantics, never run generators again This implements a policy change that once we freeze semantics for a document, we won't ever run geneators again, even if we further fork and replace that original document. This implemented by adding another boolean to ur GeneratorInfo: besides having a flag which indicates whether we don't need to rerun generators for this specific state, we also add a flag to say never run even if we do change. --- ...pilationTracker.CompilationTrackerState.cs | 37 +++++++++++++++++-- .../SolutionState.CompilationTracker.cs | 2 +- .../SolutionWithSourceGeneratorTests.cs | 13 +++++-- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs index a9e38398b864b..3d23be15e1489 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs @@ -41,21 +41,52 @@ private readonly struct CompilationTrackerGeneratorInfo /// public readonly bool DocumentsAreFinal; + /// + /// Whether the generated documents are frozen and generators should never be ran again, ever, even if a document + /// is later changed. This is used to ensure that when we produce a frozen solution for partial semantics, + /// further downstream forking of that solution won't rerun generators. This is because of two reasons: + /// + /// Generally once we've produced a frozen solution with partial semantics, we now want speed rather + /// than accuracy; a generator running in a later path will still cause issues there. + /// The frozen solution with partial semantics makes no guarantee that other syntax trees exist or + /// whether we even have references -- it's pretty likely that running a generator might produce worse results + /// than what we originally had. + /// + /// + public readonly bool DocumentsAreFinalAndFrozen; + public CompilationTrackerGeneratorInfo( TextDocumentStates documents, GeneratorDriver? driver, - bool documentsAreFinal) + bool documentsAreFinal, + bool documentsAreFinalAndFrozen = false) { Documents = documents; Driver = driver; DocumentsAreFinal = documentsAreFinal; + DocumentsAreFinalAndFrozen = documentsAreFinalAndFrozen; + + // If we're frozen, that implies final as well + Contract.ThrowIfTrue(documentsAreFinalAndFrozen && !documentsAreFinal); } public CompilationTrackerGeneratorInfo WithDocumentsAreFinal(bool documentsAreFinal) - => DocumentsAreFinal == documentsAreFinal ? this : new(Documents, Driver, documentsAreFinal); + { + // If we're already frozen, then we don't allow any further changes to whether we need to be regenerated; + // otherwise we can return the same copy if nothing changed. + if (DocumentsAreFinalAndFrozen || DocumentsAreFinal == documentsAreFinal) + return this; + else + return new(Documents, Driver, documentsAreFinal); + } + + public CompilationTrackerGeneratorInfo WithDocumentsAreFinalAndFrozen() + { + return DocumentsAreFinalAndFrozen ? this : new(Documents, Driver, documentsAreFinal: true, documentsAreFinalAndFrozen: true); + } public CompilationTrackerGeneratorInfo WithDriver(GeneratorDriver? driver) - => Driver == driver ? this : new(Documents, driver, DocumentsAreFinal); + => Driver == driver ? this : new(Documents, driver, DocumentsAreFinal, DocumentsAreFinalAndFrozen); } /// diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs index 63da8f5a595f3..6ffc3e21b43fb 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs @@ -218,7 +218,7 @@ private void GetPartialCompilationState( // check whether we can bail out quickly for typing case var inProgressState = state as InProgressState; - generatorInfo = state.GeneratorInfo.WithDocumentsAreFinal(true); + generatorInfo = state.GeneratorInfo.WithDocumentsAreFinalAndFrozen(); // all changes left for this document is modifying the given document. // we can use current state as it is since we will replace the document with latest document anyway. diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index 9951394a4bea0..65798a698002f 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -595,10 +595,11 @@ public async Task FreezingSolutionEnsuresGeneratorsDoNotRun(bool forkBeforeFreez [Fact] [WorkItem(56702, "https://github.com/dotnet/roslyn/issues/56702")] - public async Task ForkAfterFreezeWorks() + public async Task ForkAfterFreezeNoLongerRunsGenerators() { using var workspace = CreateWorkspaceWithPartialSemantics(); - var analyzerReference = new TestGeneratorReference(new SingleFileTestGenerator("// Hello, Source Generators!")); + var generatorRan = false; + var analyzerReference = new TestGeneratorReference(new CallbackGenerator(_ => { }, onExecute: _ => { generatorRan = true; }, source: "// Hello World!")); var project = AddEmptyProject(workspace.CurrentSolution) .AddAnalyzerReference(analyzerReference) .AddDocument("RegularDocument.cs", "// Source File", filePath: "RegularDocument.cs").Project; @@ -606,13 +607,19 @@ public async Task ForkAfterFreezeWorks() // Ensure generators are ran _ = await project.GetCompilationAsync(); + Assert.True(generatorRan); + generatorRan = false; + var document = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None); - // And fork with new contents; this will force generators to run again + // And fork with new contents; we'll ensure the contents of this tree are different, but the generator will still not be ran document = document.WithText(SourceText.From("// Something else")); var compilation = await document.Project.GetRequiredCompilationAsync(CancellationToken.None); Assert.Equal(2, compilation.SyntaxTrees.Count()); + Assert.False(generatorRan); + + Assert.Equal("// Something else", (await document.GetRequiredSyntaxRootAsync(CancellationToken.None)).ToFullString()); } } } From a5b60dcd461fd76ae6709374141742e823d105e9 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Wed, 6 Oct 2021 15:21:05 -0700 Subject: [PATCH 6/8] Update test to also assert generators don't run Since we can reuse trees from prior runs, that might hide the fact we ran generators but then reused the prior tree anyways, when we really wanted to ensure generators didn't run at all. --- .../SolutionTests/SolutionWithSourceGeneratorTests.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index 65798a698002f..71692b51d4f7d 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; using System.Collections.Immutable; using System.Linq; using System.Text; @@ -267,7 +268,9 @@ public async Task CompilationsInCompilationReferencesIncludeGeneratedSourceFiles public async Task RequestingGeneratedDocumentsTwiceGivesSameInstance() { using var workspace = CreateWorkspaceWithRecoverableSyntaxTreesAndWeakCompilations(); - var analyzerReference = new TestGeneratorReference(new GenerateFileForEachAdditionalFileWithContentsCommented()); + + var generatorRan = false; + var analyzerReference = new TestGeneratorReference(new CallbackGenerator(_ => { }, onExecute: _ => { generatorRan = true; }, source: "// Hello World!")); var project = WithPreviewLanguageVersion(AddEmptyProject(workspace.CurrentSolution)) .AddAnalyzerReference(analyzerReference) .AddAdditionalDocument("Test.txt", "Hello, world!").Project; @@ -276,7 +279,8 @@ public async Task RequestingGeneratedDocumentsTwiceGivesSameInstance() var tree = await generatedDocumentFirstTime.GetSyntaxTreeAsync(); // Fetch the compilation, and then wait for it to be GC'ed, then fetch it again. This ensures that - // finalizing a compilation more than once doesn't recreate things incorrectly. + // finalizing a compilation more than once doesn't recreate things incorrectly or run the generator more than once. + generatorRan = false; var compilationReference = ObjectReference.CreateFromFactory(() => project.GetCompilationAsync().Result); compilationReference.AssertReleased(); var secondCompilation = await project.GetRequiredCompilationAsync(CancellationToken.None); @@ -285,6 +289,7 @@ public async Task RequestingGeneratedDocumentsTwiceGivesSameInstance() Assert.Same(generatedDocumentFirstTime, generatedDocumentSecondTime); Assert.Same(tree, secondCompilation.SyntaxTrees.Single()); + Assert.False(generatorRan); } [Fact] From f0314f9e4403187730742242e45e1a8b3c57cce3 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Wed, 6 Oct 2021 15:26:49 -0700 Subject: [PATCH 7/8] Clarify the subtle logic in WithDocumentsAreFinal --- ...tionState.CompilationTracker.CompilationTrackerState.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs index 3d23be15e1489..1328557666fdf 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.CompilationTrackerState.cs @@ -72,8 +72,11 @@ public CompilationTrackerGeneratorInfo( public CompilationTrackerGeneratorInfo WithDocumentsAreFinal(bool documentsAreFinal) { - // If we're already frozen, then we don't allow any further changes to whether we need to be regenerated; - // otherwise we can return the same copy if nothing changed. + // If we're already frozen, then we won't do anything even if somebody calls WithDocumentsAreFinal(false); + // this for example would happen if we had a frozen snapshot, and then we fork it further with additional changes. + // In that case we would be calling WithDocumentsAreFinal(false) to force generators to run again, but if we've + // frozen in partial semantics, we're done running them period. So we'll just keep treating them as final, + // no matter the wishes of the caller. if (DocumentsAreFinalAndFrozen || DocumentsAreFinal == documentsAreFinal) return this; else From f3ddd6365909fd0a0f98fab8373b64cbf84b5d97 Mon Sep 17 00:00:00 2001 From: Jason Malinowski Date: Wed, 6 Oct 2021 15:39:27 -0700 Subject: [PATCH 8/8] Add another test for handling the fork-after-feeze case --- .../SolutionTests/SolutionTestHelpers.cs | 3 ++ .../SolutionWithSourceGeneratorTests.cs | 33 ++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTestHelpers.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTestHelpers.cs index 90b6a860e2409..77921bd37bce3 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTestHelpers.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTestHelpers.cs @@ -32,6 +32,9 @@ public static Workspace CreateWorkspaceWithRecoverableSyntaxTreesAndWeakCompilat return workspace; } + public static Workspace CreateWorkspaceWithPartialSemanticsAndWeakCompilations() + => WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(new[] { typeof(TestProjectCacheService), typeof(TestTemporaryStorageService) }); + #nullable disable public static void TestProperty(T instance, Func factory, Func getter, TValue validNonDefaultValue, bool defaultThrows = false) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index 71692b51d4f7d..e09fb01fd5e1c 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -610,11 +610,42 @@ public async Task ForkAfterFreezeNoLongerRunsGenerators() .AddDocument("RegularDocument.cs", "// Source File", filePath: "RegularDocument.cs").Project; // Ensure generators are ran - _ = await project.GetCompilationAsync(); + var objectReference = await project.GetCompilationAsync(); + + Assert.True(generatorRan); + generatorRan = false; + + var document = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None); + + // And fork with new contents; we'll ensure the contents of this tree are different, but the generator will still not be ran + document = document.WithText(SourceText.From("// Something else")); + + var compilation = await document.Project.GetRequiredCompilationAsync(CancellationToken.None); + Assert.Equal(2, compilation.SyntaxTrees.Count()); + Assert.False(generatorRan); + + Assert.Equal("// Something else", (await document.GetRequiredSyntaxRootAsync(CancellationToken.None)).ToFullString()); + } + + [Fact] + [WorkItem(56702, "https://github.com/dotnet/roslyn/issues/56702")] + public async Task ForkAfterFreezeNoLongerRunsGeneratorsEvenIfCompilationFallsAwayBeforeFreeze() + { + using var workspace = CreateWorkspaceWithPartialSemanticsAndWeakCompilations(); + var generatorRan = false; + var analyzerReference = new TestGeneratorReference(new CallbackGenerator(_ => { }, onExecute: _ => { generatorRan = true; }, source: "// Hello World!")); + var project = AddEmptyProject(workspace.CurrentSolution) + .AddAnalyzerReference(analyzerReference) + .AddDocument("RegularDocument.cs", "// Source File", filePath: "RegularDocument.cs").Project; + + // Ensure generators are ran + var compilationReference = ObjectReference.CreateFromFactory(() => project.GetCompilationAsync().Result); Assert.True(generatorRan); generatorRan = false; + compilationReference.AssertReleased(); + var document = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None); // And fork with new contents; we'll ensure the contents of this tree are different, but the generator will still not be ran