From 36042737e005c25b486cd680ff7a9f1809864f8b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 6 Nov 2024 17:17:32 -0800 Subject: [PATCH 1/3] Ensure frozen snapshots don't return less data if solution already has compilatino data in progress in progress Revert in progress Return existing full semantic info for frozen document if already available Fix --- .../Portable/Debugging/DataTipInfoGetter.cs | 10 ++-- .../Portable/Completion/CompletionService.cs | 6 ++- .../CompletionService_GetCompletions.cs | 11 ++-- ...stractMemberInsertingCompletionProvider.cs | 9 ++-- .../AbstractSnippetCompletionProvider.cs | 7 +-- .../Shared/Extensions/DocumentExtensions.cs | 23 --------- .../Portable/Workspace/Solution/Document.cs | 31 +++++++++--- .../Solution/SourceGeneratedDocument.cs | 2 +- .../SolutionWithSourceGeneratorTests.cs | 50 ++++++++++++++++--- 9 files changed, 93 insertions(+), 56 deletions(-) diff --git a/src/Features/CSharp/Portable/Debugging/DataTipInfoGetter.cs b/src/Features/CSharp/Portable/Debugging/DataTipInfoGetter.cs index 86b1ba4a94e65..7b82550248997 100644 --- a/src/Features/CSharp/Portable/Debugging/DataTipInfoGetter.cs +++ b/src/Features/CSharp/Portable/Debugging/DataTipInfoGetter.cs @@ -39,11 +39,11 @@ internal static async Task GetInfoAsync(Document document, int if (expression is LiteralExpressionSyntax) { - // If the user hovers over a literal, give them a DataTip for the type of the - // literal they're hovering over. - // Partial semantics should always be sufficient because the (unconverted) type - // of a literal can always easily be determined. - var (_, semanticModel) = await document.GetFullOrPartialSemanticModelAsync(cancellationToken).ConfigureAwait(false); + // If the user hovers over a literal, give them a DataTip for the type of the literal they're hovering + // over. Partial semantics should always be sufficient because the (unconverted) type of a literal can + // always easily be determined. + document = document.WithFrozenPartialSemantics(cancellationToken); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var type = semanticModel.GetTypeInfo(expression, cancellationToken).Type; return type == null ? default diff --git a/src/Features/Core/Portable/Completion/CompletionService.cs b/src/Features/Core/Portable/Completion/CompletionService.cs index 6ecbfa300daf4..ad5395fb889f2 100644 --- a/src/Features/Core/Portable/Completion/CompletionService.cs +++ b/src/Features/Core/Portable/Completion/CompletionService.cs @@ -215,7 +215,8 @@ public virtual TextSpan GetDefaultCompletionListSpan(SourceText text, int caretP var extensionManager = document.Project.Solution.Workspace.Services.GetRequiredService(); // We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it. - (document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false); + document = GetDocumentWithFrozenPartialSemantics(document, cancellationToken); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var description = await extensionManager.PerformFunctionAsync( provider, cancellationToken => provider.GetDescriptionAsync(document, item, options, displayOptions, cancellationToken), @@ -246,7 +247,8 @@ public virtual async Task GetChangeAsync( var extensionManager = document.Project.Solution.Workspace.Services.GetRequiredService(); // We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it. - (document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false); + document = GetDocumentWithFrozenPartialSemantics(document, cancellationToken); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var change = await extensionManager.PerformFunctionAsync( provider, diff --git a/src/Features/Core/Portable/Completion/CompletionService_GetCompletions.cs b/src/Features/Core/Portable/Completion/CompletionService_GetCompletions.cs index 178d6758b87e3..e3a809d931a7e 100644 --- a/src/Features/Core/Portable/Completion/CompletionService_GetCompletions.cs +++ b/src/Features/Core/Portable/Completion/CompletionService_GetCompletions.cs @@ -69,7 +69,8 @@ internal virtual async Task GetCompletionsAsync( CancellationToken cancellationToken = default) { // We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it. - (document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false); + document = GetDocumentWithFrozenPartialSemantics(document, cancellationToken); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); var text = await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false); var completionListSpan = GetDefaultCompletionListSpan(text, caretPosition); @@ -177,14 +178,12 @@ static async Task> GetAugmentingProvidersAsyn /// In most cases we'd still end up with complete document, but we'd consider it an acceptable trade-off even when /// we get into this transient state. /// - private async Task<(Document document, SemanticModel? semanticModel)> GetDocumentWithFrozenPartialSemanticsAsync(Document document, CancellationToken cancellationToken) + private Document GetDocumentWithFrozenPartialSemantics(Document document, CancellationToken cancellationToken) { if (_suppressPartialSemantics) - { - return (document, await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false)); - } + return document; - return await document.GetFullOrPartialSemanticModelAsync(cancellationToken).ConfigureAwait(false); + return document.WithFrozenPartialSemantics(cancellationToken); } private static bool ValidatePossibleTriggerCharacterSet(CompletionTriggerKind completionTriggerKind, IEnumerable triggeredProviders, diff --git a/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs index 25e3bb8a6e34d..d3533100ba5a9 100644 --- a/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs @@ -4,6 +4,7 @@ using System.Collections.Immutable; using System.Linq; +using System.Linq.Expressions; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeActions; @@ -80,9 +81,11 @@ private async Task DetermineNewDocumentAsync( var tree = await document.GetRequiredSyntaxTreeAsync(cancellationToken).ConfigureAwait(false); var token = GetToken(completionItem, tree, cancellationToken); var annotatedRoot = tree.GetRoot(cancellationToken).ReplaceToken(token, token.WithAdditionalAnnotations(_otherAnnotation)); - // Make sure the new document is frozen before we try to get the semantic model. This is to - // avoid trigger source generator, which is expensive and not needed for calculating the change. - document = document.WithSyntaxRoot(annotatedRoot).WithFrozenPartialSemantics(cancellationToken); + + // Make sure the new document is frozen before we try to get the semantic model. This is to avoid trigger source + // generator, which is expensive and not needed for calculating the change. Pass in 'forceFreeze: true' to + // ensure all further transformations we make do not run generators either. + document = document.WithSyntaxRoot(annotatedRoot).WithFrozenPartialSemantics(forceFreeze: true, cancellationToken); var memberContainingDocument = await GenerateMemberAndUsingsAsync(document, completionItem, line, cancellationToken).ConfigureAwait(false); if (memberContainingDocument == null) diff --git a/src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs index ecc4b1fedba12..6be105ab1e421 100644 --- a/src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/Snippets/AbstractSnippetCompletionProvider.cs @@ -128,9 +128,10 @@ public override async Task ProvideCompletionsAsync(CompletionContext context) var textChange = new TextChange(span, string.Empty); originalText = originalText.WithChanges(textChange); - // The document might not be frozen, so make sure we freeze it here to avoid triggering source generator - // which is not needed for snippet completion and will cause perf issue. - var newDocument = document.WithText(originalText).WithFrozenPartialSemantics(cancellationToken); + // The document might not be frozen, so make sure we freeze it here to avoid triggering source generator which + // is not needed for snippet completion and will cause perf issue. Pass in 'forceFreeze: true' to ensure all + // further transformations we make do not run generators either. + var newDocument = document.WithText(originalText).WithFrozenPartialSemantics(forceFreeze: true, cancellationToken); return (newDocument, span.Start); } } diff --git a/src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs b/src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs index 6a41bf456a50a..dc01cc80dc577 100644 --- a/src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs +++ b/src/Workspaces/Core/Portable/Shared/Extensions/DocumentExtensions.cs @@ -2,7 +2,6 @@ // 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.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.FindSymbols; @@ -23,28 +22,6 @@ public static async ValueTask GetSyntaxTreeIndexAsync(this Docu public static ValueTask GetSyntaxTreeIndexAsync(this Document document, bool loadOnly, CancellationToken cancellationToken) => SyntaxTreeIndex.GetIndexAsync(document, loadOnly, cancellationToken); - /// - /// Returns the semantic model for this document that may be produced from partial semantics. The semantic model - /// is only guaranteed to contain the syntax tree for and nothing else. - /// - public static async Task<(Document document, SemanticModel? semanticModel)> GetFullOrPartialSemanticModelAsync(this Document document, CancellationToken cancellationToken) - { - if (document.Project.TryGetCompilation(out var compilation)) - { - // We already have a compilation, so at this point it's fastest to just get a SemanticModel - var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); - - // Make sure the compilation is kept alive so that GetSemanticModelAsync() doesn't become expensive - GC.KeepAlive(compilation); - return (document, semanticModel); - } - else - { - var frozenDocument = document.WithFrozenPartialSemantics(cancellationToken); - return (frozenDocument, await frozenDocument.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false)); - } - } - internal static Document WithSolutionOptions(this Document document, OptionSet options) => document.Project.Solution.WithOptions(options).GetRequiredDocument(document.Id); } diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/Document.cs b/src/Workspaces/Core/Portable/Workspace/Solution/Document.cs index 068122537e3e3..2e77e388f6c74 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/Document.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/Document.cs @@ -487,18 +487,35 @@ public ImmutableArray GetLinkedDocumentIds() return filteredDocumentIds.Remove(this.Id); } + /// + internal Document WithFrozenPartialSemantics(CancellationToken cancellationToken) + => WithFrozenPartialSemantics(forceFreeze: false, cancellationToken); + /// - /// Creates a branched version of this document that has its semantic model frozen in whatever state it is available at the time, - /// assuming a background process is constructing the semantics asynchronously. Repeated calls to this method may return - /// documents with increasingly more complete semantics. + /// Creates a branched version of this document that has its semantic model frozen in whatever state it is available + /// at the time, assuming a background process is constructing the semantics asynchronously. Repeated calls to this + /// method may return documents with increasingly more complete semantics. /// /// Use this method to gain access to potentially incomplete semantics quickly. - /// Note: this will give back a solution where this 's project will not run - /// generators when getting its compilation. However, all other projects will still run generators when their - /// compilations are requested. + /// Note: this will give back a solution where this 's project will not run generators + /// when getting its compilation. However, all other projects will still run generators when their compilations are + /// requested. /// - internal virtual Document WithFrozenPartialSemantics(CancellationToken cancellationToken) + /// If then a forked document will be returned no matter what. This + /// should be used when the caller wants to ensure that further forks of that document will remain frozen and will + /// not run generators/skeletons. For example, if it is about to transform the document many times, and is fine with + /// the original semantic information they started with. If then this same document may be + /// returned if the compilation for its was already produced. In this case, generators and + /// skeletons will already have been run, so returning the same instance will be fast when getting semantics. + /// However, this does mean that future forks of this instance will continue running generators/skeletons. This + /// should be used for most clients that intend to just query for semantic information and do not intend to make any + /// further changes. + /// + internal virtual Document WithFrozenPartialSemantics(bool forceFreeze, CancellationToken cancellationToken) { + if (!forceFreeze && this.Project.TryGetCompilation(out _)) + return this; + var solution = this.Project.Solution; // only produce doc with frozen semantics if this workspace has support for that, as without diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocument.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocument.cs index 2a2a9d97a6668..ba4bb1339eff8 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocument.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocument.cs @@ -26,7 +26,7 @@ internal SourceGeneratedDocument(Project project, SourceGeneratedDocumentState s internal DateTime GenerationDateTime => State.GenerationDateTime; - internal override Document WithFrozenPartialSemantics(CancellationToken cancellationToken) + internal override Document WithFrozenPartialSemantics(bool forceFreeze, CancellationToken cancellationToken) { // For us to implement frozen partial semantics here with a source generated document, // we'd need to potentially deal with the combination where that happens on a snapshot that was already diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index b686a59887b39..d58c3f07f27bf 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -318,7 +318,8 @@ static async Task AssertCompilationContainsGeneratedFilesAsync(Project project, } [Theory, CombinatorialData] - public async Task PartialCompilationsIncludeGeneratedFilesAfterFullGeneration(TestHost testHost) + public async Task PartialCompilationsIncludeGeneratedFilesAfterFullGeneration( + TestHost testHost, bool forceFreeze) { using var workspace = CreateWorkspaceWithPartialSemantics(testHost); var analyzerReference = new TestGeneratorReference(new GenerateFileForEachAdditionalFileWithContentsCommented()); @@ -331,8 +332,15 @@ public async Task PartialCompilationsIncludeGeneratedFilesAfterFullGeneration(Te Assert.Equal(2, fullCompilation.SyntaxTrees.Count()); - var partialProject = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None).Project; - Assert.NotSame(partialProject, project); + var partialProject = project.Documents.Single().WithFrozenPartialSemantics(forceFreeze, CancellationToken.None).Project; + + // If we're forcing the freeze, we must get a difference project instance. If we're not, we'll get the same + // project since the compilation was already available. + if (forceFreeze) + Assert.NotSame(partialProject, project); + else + Assert.Same(partialProject, project); + var partialCompilation = await partialProject.GetRequiredCompilationAsync(CancellationToken.None); Assert.Same(fullCompilation, partialCompilation); @@ -721,7 +729,7 @@ public async Task FreezingSolutionEnsuresGeneratorsDoNotRun(bool forkBeforeFreez } [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/56702")] - public async Task ForkAfterFreezeNoLongerRunsGenerators(TestHost testHost) + public async Task ForkAfterForceFreezeNoLongerRunsGenerators(TestHost testHost) { using var workspace = CreateWorkspaceWithPartialSemantics(testHost); var generatorRan = false; @@ -736,9 +744,10 @@ public async Task ForkAfterFreezeNoLongerRunsGenerators(TestHost testHost) Assert.True(generatorRan); generatorRan = false; - var document = project.Documents.Single().WithFrozenPartialSemantics(CancellationToken.None); + var document = project.Documents.Single().WithFrozenPartialSemantics(forceFreeze: true, 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 + // And fork with new contents; we'll ensure the contents of this tree are different, but the generator will not + // run since we explicitly force froze. document = document.WithText(SourceText.From("// Something else")); var compilation = await document.Project.GetRequiredCompilationAsync(CancellationToken.None); @@ -748,6 +757,35 @@ public async Task ForkAfterFreezeNoLongerRunsGenerators(TestHost testHost) Assert.Equal("// Something else", (await document.GetRequiredSyntaxRootAsync(CancellationToken.None)).ToFullString()); } + [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/56702")] + public async Task ForkAfterFreezeOfCompletedDocumentStillRunsGenerators(TestHost testHost) + { + using var workspace = CreateWorkspaceWithPartialSemantics(testHost); + 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 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 run + // since we didn't force freeze, and we got the frozen document after its compilation was already computed. + document = document.WithText(SourceText.From("// Something else")); + + var compilation = await document.Project.GetRequiredCompilationAsync(CancellationToken.None); + Assert.Equal(2, compilation.SyntaxTrees.Count()); + Assert.True(generatorRan); + + Assert.Equal("// Something else", (await document.GetRequiredSyntaxRootAsync(CancellationToken.None)).ToFullString()); + } + [Theory, CombinatorialData] public async Task LinkedDocumentOfFrozenShouldNotRunSourceGenerator(TestHost testHost) { From 64854238670cc21f3346ebf8e07c9726e1f3c45a Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 7 Nov 2024 08:41:54 -0800 Subject: [PATCH 2/3] Update src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs --- .../Providers/AbstractMemberInsertingCompletionProvider.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs b/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs index d3533100ba5a9..aed35da65eafb 100644 --- a/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs +++ b/src/Features/Core/Portable/Completion/Providers/AbstractMemberInsertingCompletionProvider.cs @@ -4,7 +4,6 @@ using System.Collections.Immutable; using System.Linq; -using System.Linq.Expressions; using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.CodeActions; From b5683a37bed9a0d1694be154459cccd719709c9c Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 7 Nov 2024 09:09:22 -0800 Subject: [PATCH 3/3] Update src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs --- .../CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs index d58c3f07f27bf..ce71143a2df2d 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs @@ -334,7 +334,7 @@ public async Task PartialCompilationsIncludeGeneratedFilesAfterFullGeneration( var partialProject = project.Documents.Single().WithFrozenPartialSemantics(forceFreeze, CancellationToken.None).Project; - // If we're forcing the freeze, we must get a difference project instance. If we're not, we'll get the same + // If we're forcing the freeze, we must get a different project instance. If we're not, we'll get the same // project since the compilation was already available. if (forceFreeze) Assert.NotSame(partialProject, project);