diff --git a/src/Workspaces/Core/Portable/SemanticModelReuse/AbstractSemanticModelReuseLanguageService.cs b/src/Workspaces/Core/Portable/SemanticModelReuse/AbstractSemanticModelReuseLanguageService.cs index 8c54739a41db8..8f2cf482d46ef 100644 --- a/src/Workspaces/Core/Portable/SemanticModelReuse/AbstractSemanticModelReuseLanguageService.cs +++ b/src/Workspaces/Core/Portable/SemanticModelReuse/AbstractSemanticModelReuseLanguageService.cs @@ -6,7 +6,7 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; -using Microsoft.CodeAnalysis.ErrorReporting; +using Microsoft.CodeAnalysis.Internal.Log; using Microsoft.CodeAnalysis.LanguageService; namespace Microsoft.CodeAnalysis.SemanticModelReuse @@ -14,15 +14,12 @@ namespace Microsoft.CodeAnalysis.SemanticModelReuse internal abstract class AbstractSemanticModelReuseLanguageService< TMemberDeclarationSyntax, TBasePropertyDeclarationSyntax, - TAccessorDeclarationSyntax> : ISemanticModelReuseLanguageService + TAccessorDeclarationSyntax> : ISemanticModelReuseLanguageService, IDisposable where TMemberDeclarationSyntax : SyntaxNode where TBasePropertyDeclarationSyntax : TMemberDeclarationSyntax where TAccessorDeclarationSyntax : SyntaxNode { - /// - /// Used to make sure we only report one watson per sessoin here. - /// - private static bool s_watsonReported; + private readonly CountLogAggregator _logAggregator = new(); protected abstract ISyntaxFacts SyntaxFacts { get; } @@ -32,6 +29,15 @@ internal abstract class AbstractSemanticModelReuseLanguageService< protected abstract SyntaxList GetAccessors(TBasePropertyDeclarationSyntax baseProperty); protected abstract TBasePropertyDeclarationSyntax GetBasePropertyDeclaration(TAccessorDeclarationSyntax accessor); + public void Dispose() + { + Logger.Log(FunctionId.SemanticModelReuseLanguageService_TryGetSpeculativeSemanticModelAsync_Equivalent, KeyValueLogMessage.Create(m => + { + foreach (var kv in _logAggregator) + m[kv.Key.ToString()] = kv.Value.GetCount(); + })); + } + public async Task TryGetSpeculativeSemanticModelAsync(SemanticModel previousSemanticModel, SyntaxNode currentBodyNode, CancellationToken cancellationToken) { var previousSyntaxTree = previousSemanticModel.SyntaxTree; @@ -39,37 +45,18 @@ internal abstract class AbstractSemanticModelReuseLanguageService< // This operation is only valid if top-level equivalent trees were passed in. If they're not equivalent // then something very bad happened as we did that document.Project.GetDependentSemanticVersionAsync was - // still the same. So somehow we don't have top-level equivalence, but we do have the same semantic version. - // - // log a NFW to help diagnose what the source looks like as it may help us determine what sort of edit is - // causing this. - if (!previousSyntaxTree.IsEquivalentTo(currentSyntaxTree, topLevel: true)) - { - if (!s_watsonReported) - { - s_watsonReported = true; - - try - { - // Avoid including tree contents in exception message for privacy compliance. Instead, include - // in exception type for dump analysis. - throw new NonEquivalentTreeException( - "Syntax trees should have been equivalent.", previousSyntaxTree, currentSyntaxTree); - - } - catch (Exception e) when (FatalError.ReportAndCatch(e)) - { - } - } + // still the same. Log information so we can be alerted if this isn't being as successful as we expect. + var isEquivalentTo = previousSyntaxTree.IsEquivalentTo(currentSyntaxTree, topLevel: true); + _logAggregator.IncreaseCount(isEquivalentTo); + if (!isEquivalentTo) return null; - } var previousRoot = await previousSemanticModel.SyntaxTree.GetRootAsync(cancellationToken).ConfigureAwait(false); var currentRoot = await currentBodyNode.SyntaxTree.GetRootAsync(cancellationToken).ConfigureAwait(false); var previousBodyNode = GetPreviousBodyNode(previousRoot, currentRoot, currentBodyNode); - // Trivia is ignore when comparing two trees for quivalence at top level, since it has no effect to API shape + // Trivia is ignore when comparing two trees for equivalence at top level, since it has no effect to API shape // and it'd be safe to drop in the new method body as long as the shape doesn't change. However, trivia changes // around the method do make it tricky to decide whether a position is safe for speculation. @@ -81,8 +68,8 @@ internal abstract class AbstractSemanticModelReuseLanguageService< // method body and after OriginalPositionForSpeculation. // Given that the common use case for us is continuously editing/typing inside a method body, we believe we can be conservative - // in creating speculative model with those kind of trivia change, by requring the method body block not to shift position, - // w/o sacrificing performance in those common sceanrios. + // in creating speculative model with those kind of trivia change, by requiring the method body block not to shift position, + // w/o sacrificing performance in those common scenarios. if (previousBodyNode.SpanStart != currentBodyNode.SpanStart) return null; diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs index 3f61606992d7b..e182920003789 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs @@ -872,7 +872,7 @@ public ProjectState RemoveAllDocuments() analyzerConfigSet: ComputeAnalyzerConfigOptionsValueSource(AnalyzerConfigDocumentStates)); } - public ProjectState UpdateDocument(DocumentState newDocument, bool textChanged, bool recalculateDependentVersions) + public ProjectState UpdateDocument(DocumentState newDocument, bool contentChanged) { var oldDocument = DocumentStates.GetRequiredState(newDocument.Id); if (oldDocument == newDocument) @@ -882,7 +882,7 @@ public ProjectState UpdateDocument(DocumentState newDocument, bool textChanged, var newDocumentStates = DocumentStates.SetState(newDocument.Id, newDocument); GetLatestDependentVersions( - newDocumentStates, AdditionalDocumentStates, oldDocument, newDocument, recalculateDependentVersions, textChanged, + newDocumentStates, AdditionalDocumentStates, oldDocument, newDocument, contentChanged, out var dependentDocumentVersion, out var dependentSemanticVersion); return With( @@ -891,7 +891,7 @@ public ProjectState UpdateDocument(DocumentState newDocument, bool textChanged, latestDocumentTopLevelChangeVersion: dependentSemanticVersion); } - public ProjectState UpdateAdditionalDocument(AdditionalDocumentState newDocument, bool textChanged, bool recalculateDependentVersions) + public ProjectState UpdateAdditionalDocument(AdditionalDocumentState newDocument, bool contentChanged) { var oldDocument = AdditionalDocumentStates.GetRequiredState(newDocument.Id); if (oldDocument == newDocument) @@ -901,7 +901,7 @@ public ProjectState UpdateAdditionalDocument(AdditionalDocumentState newDocument var newDocumentStates = AdditionalDocumentStates.SetState(newDocument.Id, newDocument); GetLatestDependentVersions( - DocumentStates, newDocumentStates, oldDocument, newDocument, recalculateDependentVersions, textChanged, + DocumentStates, newDocumentStates, oldDocument, newDocument, contentChanged, out var dependentDocumentVersion, out var dependentSemanticVersion); return this.With( @@ -939,13 +939,13 @@ private void GetLatestDependentVersions( TextDocumentStates newDocumentStates, TextDocumentStates newAdditionalDocumentStates, TextDocumentState oldDocument, TextDocumentState newDocument, - bool recalculateDependentVersions, bool textChanged, + bool contentChanged, out AsyncLazy dependentDocumentVersion, out AsyncLazy dependentSemanticVersion) { var recalculateDocumentVersion = false; var recalculateSemanticVersion = false; - if (recalculateDependentVersions) + if (contentChanged) { if (oldDocument.TryGetTextVersion(out var oldVersion)) { @@ -963,13 +963,13 @@ private void GetLatestDependentVersions( dependentDocumentVersion = recalculateDocumentVersion ? new AsyncLazy(c => ComputeLatestDocumentVersionAsync(newDocumentStates, newAdditionalDocumentStates, c), cacheResult: true) - : textChanged + : contentChanged ? new AsyncLazy(newDocument.GetTextVersionAsync, cacheResult: true) : _lazyLatestDocumentVersion; dependentSemanticVersion = recalculateSemanticVersion ? new AsyncLazy(c => ComputeLatestDocumentTopLevelChangeVersionAsync(newDocumentStates, newAdditionalDocumentStates, c), cacheResult: true) - : textChanged + : contentChanged ? CreateLazyLatestDocumentTopLevelChangeVersion(newDocument, newDocumentStates, newAdditionalDocumentStates) : _lazyLatestDocumentTopLevelChangeVersion; } diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs index 29fd9bf8ab229..a13bedfe56dce 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs @@ -201,7 +201,7 @@ public ICompilationTracker FreezePartialStateWithTree(SolutionState solution, Do var oldTree = oldState.GetSyntaxTree(cancellationToken); compilationPair = compilationPair.ReplaceSyntaxTree(oldTree, tree); - inProgressProject = inProgressProject.UpdateDocument(docState, textChanged: false, recalculateDependentVersions: false); + inProgressProject = inProgressProject.UpdateDocument(docState, contentChanged: true); } else { diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs index 63a4065825dee..d891fd758a071 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs @@ -1231,7 +1231,7 @@ public SolutionState WithDocumentName(DocumentId documentId, string name) return this; } - return UpdateDocumentState(oldDocument.UpdateName(name)); + return UpdateDocumentState(oldDocument.UpdateName(name), contentChanged: false); } /// @@ -1246,7 +1246,7 @@ public SolutionState WithDocumentFolders(DocumentId documentId, IReadOnlyList @@ -1260,7 +1260,7 @@ public SolutionState WithDocumentFilePath(DocumentId documentId, string? filePat return this; } - return UpdateDocumentState(oldDocument.UpdateFilePath(filePath)); + return UpdateDocumentState(oldDocument.UpdateFilePath(filePath), contentChanged: false); } /// @@ -1275,7 +1275,7 @@ public SolutionState WithDocumentText(DocumentId documentId, SourceText text, Pr return this; } - return UpdateDocumentState(oldDocument.UpdateText(text, mode), textChanged: true); + return UpdateDocumentState(oldDocument.UpdateText(text, mode), contentChanged: true); } /// @@ -1290,7 +1290,7 @@ public SolutionState WithAdditionalDocumentText(DocumentId documentId, SourceTex return this; } - return UpdateAdditionalDocumentState(oldDocument.UpdateText(text, mode), textChanged: true); + return UpdateAdditionalDocumentState(oldDocument.UpdateText(text, mode), contentChanged: true); } /// @@ -1320,7 +1320,7 @@ public SolutionState WithDocumentText(DocumentId documentId, TextAndVersion text return this; } - return UpdateDocumentState(oldDocument.UpdateText(textAndVersion, mode), textChanged: true); + return UpdateDocumentState(oldDocument.UpdateText(textAndVersion, mode), contentChanged: true); } /// @@ -1335,7 +1335,7 @@ public SolutionState WithAdditionalDocumentText(DocumentId documentId, TextAndVe return this; } - return UpdateAdditionalDocumentState(oldDocument.UpdateText(textAndVersion, mode), textChanged: true); + return UpdateAdditionalDocumentState(oldDocument.UpdateText(textAndVersion, mode), contentChanged: true); } /// @@ -1367,7 +1367,7 @@ public SolutionState WithDocumentSyntaxRoot(DocumentId documentId, SyntaxNode ro return this; } - return UpdateDocumentState(oldDocument.UpdateTree(root, mode), textChanged: true); + return UpdateDocumentState(oldDocument.UpdateTree(root, mode), contentChanged: true); } private static async Task UpdateDocumentInCompilationAsync( @@ -1393,16 +1393,16 @@ public SolutionState WithDocumentSourceCodeKind(DocumentId documentId, SourceCod return this; } - return UpdateDocumentState(oldDocument.UpdateSourceCodeKind(sourceCodeKind), textChanged: true); + return UpdateDocumentState(oldDocument.UpdateSourceCodeKind(sourceCodeKind), contentChanged: true); } public SolutionState UpdateDocumentTextLoader(DocumentId documentId, TextLoader loader, PreservationMode mode) { var oldDocument = GetRequiredDocumentState(documentId); - // Assumes that text has changed. User could have closed a doc without saving and we are loading text from closed file with - // old content. Also this should make sure we don't re-use latest doc version with data associated with opened document. - return UpdateDocumentState(oldDocument.UpdateText(loader, mode), textChanged: true, recalculateDependentVersions: true); + // Assumes that content has changed. User could have closed a doc without saving and we are loading text + // from closed file with old content. + return UpdateDocumentState(oldDocument.UpdateText(loader, mode), contentChanged: true); } /// @@ -1413,9 +1413,9 @@ public SolutionState UpdateAdditionalDocumentTextLoader(DocumentId documentId, T { var oldDocument = GetRequiredAdditionalDocumentState(documentId); - // Assumes that text has changed. User could have closed a doc without saving and we are loading text from closed file with - // old content. Also this should make sure we don't re-use latest doc version with data associated with opened document. - return UpdateAdditionalDocumentState(oldDocument.UpdateText(loader, mode), textChanged: true, recalculateDependentVersions: true); + // Assumes that content has changed. User could have closed a doc without saving and we are loading text + // from closed file with old content. + return UpdateAdditionalDocumentState(oldDocument.UpdateText(loader, mode), contentChanged: true); } /// @@ -1431,10 +1431,10 @@ public SolutionState UpdateAnalyzerConfigDocumentTextLoader(DocumentId documentI return UpdateAnalyzerConfigDocumentState(oldDocument.UpdateText(loader, mode)); } - private SolutionState UpdateDocumentState(DocumentState newDocument, bool textChanged = false, bool recalculateDependentVersions = false) + private SolutionState UpdateDocumentState(DocumentState newDocument, bool contentChanged) { var oldProject = GetProjectState(newDocument.Id.ProjectId)!; - var newProject = oldProject.UpdateDocument(newDocument, textChanged, recalculateDependentVersions); + var newProject = oldProject.UpdateDocument(newDocument, contentChanged); // This method shouldn't have been called if the document has not changed. Debug.Assert(oldProject != newProject); @@ -1448,10 +1448,10 @@ private SolutionState UpdateDocumentState(DocumentState newDocument, bool textCh newFilePathToDocumentIdsMap: newFilePathToDocumentIdsMap); } - private SolutionState UpdateAdditionalDocumentState(AdditionalDocumentState newDocument, bool textChanged = false, bool recalculateDependentVersions = false) + private SolutionState UpdateAdditionalDocumentState(AdditionalDocumentState newDocument, bool contentChanged) { var oldProject = GetProjectState(newDocument.Id.ProjectId)!; - var newProject = oldProject.UpdateAdditionalDocument(newDocument, textChanged, recalculateDependentVersions); + var newProject = oldProject.UpdateAdditionalDocument(newDocument, contentChanged); // This method shouldn't have been called if the document has not changed. Debug.Assert(oldProject != newProject); diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index cba27c3c465f4..fa0a5cbac8c15 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -2962,7 +2962,7 @@ public void TestProjectWithBrokenCrossLanguageReferenceHasIncompleteReferences() } [Fact] - public async Task TestFrozenPartialProjectHasDifferentSemanticVersions() + public async Task TestFrozenPartialProjectHasDifferentSemanticVersions_AddedDoc() { using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); var project = workspace.CurrentSolution.AddProject("CSharpProject", "CSharpProject", LanguageNames.CSharp); @@ -2986,6 +2986,97 @@ await documentToFreeze.Project.GetSemanticVersionAsync(), await frozenDocument.Project.GetSemanticVersionAsync()); } + [Fact] + public async Task TestFrozenPartialProjectHasDifferentSemanticVersions_ChangedDoc1() + { + using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); + var project = workspace.CurrentSolution.AddProject("CSharpProject", "CSharpProject", LanguageNames.CSharp); + project = project.AddDocument("Extra.cs", SourceText.From("class Extra { }")).Project; + + var documentToFreezeOriginal = project.AddDocument("DocumentToFreeze.cs", SourceText.From("class DocumentToFreeze { void M() { } }")); + project = documentToFreezeOriginal.Project; + var compilation = await project.GetCompilationAsync(); + + var solution = project.Solution.WithDocumentText(documentToFreezeOriginal.Id, SourceText.From("class DocumentToFreeze { void M() { /*no top level change*/ } }")); + var documentToFreezeChanged = solution.GetDocument(documentToFreezeOriginal.Id); + var tree = await documentToFreezeChanged.GetSyntaxTreeAsync(); + + var frozenDocument = documentToFreezeChanged.WithFrozenPartialSemantics(CancellationToken.None); + + Assert.NotSame(frozenDocument, documentToFreezeChanged); + + // Versions should the same since there wasn't a top level change different + Assert.Equal( + await documentToFreezeOriginal.GetTopLevelChangeTextVersionAsync(), + await frozenDocument.GetTopLevelChangeTextVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.GetTopLevelChangeTextVersionAsync(), + await frozenDocument.GetTopLevelChangeTextVersionAsync()); + + Assert.Equal( + await documentToFreezeOriginal.Project.GetDependentSemanticVersionAsync(), + await frozenDocument.Project.GetDependentSemanticVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.Project.GetDependentSemanticVersionAsync(), + await frozenDocument.Project.GetDependentSemanticVersionAsync()); + + Assert.Equal( + await documentToFreezeOriginal.Project.GetSemanticVersionAsync(), + await frozenDocument.Project.GetSemanticVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.Project.GetSemanticVersionAsync(), + await frozenDocument.Project.GetSemanticVersionAsync()); + } + + [Fact] + public async Task TestFrozenPartialProjectHasDifferentSemanticVersions_ChangedDoc2() + { + using var workspace = WorkspaceTestUtilities.CreateWorkspaceWithPartialSemantics(); + var project = workspace.CurrentSolution.AddProject("CSharpProject", "CSharpProject", LanguageNames.CSharp); + project = project.AddDocument("Extra.cs", SourceText.From("class Extra { }")).Project; + + var documentToFreezeOriginal = project.AddDocument("DocumentToFreeze.cs", SourceText.From("class DocumentToFreeze { void M() { } }")); + project = documentToFreezeOriginal.Project; + var compilation = await project.GetCompilationAsync(); + + var solution = project.Solution.WithDocumentText(documentToFreezeOriginal.Id, SourceText.From("class DocumentToFreeze { void M() { } public void NewMethod() { } }")); + var documentToFreezeChanged = solution.GetDocument(documentToFreezeOriginal.Id); + var tree = await documentToFreezeChanged.GetSyntaxTreeAsync(); + + var frozenDocument = documentToFreezeChanged.WithFrozenPartialSemantics(CancellationToken.None); + + Assert.NotSame(frozenDocument, documentToFreezeChanged); + + // Before/after the change must always result in a top level change. + // After the change, we should get the same version between the doc and its frozen version. + Assert.NotEqual( + await documentToFreezeOriginal.GetTopLevelChangeTextVersionAsync(), + await frozenDocument.GetTopLevelChangeTextVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.GetTopLevelChangeTextVersionAsync(), + await frozenDocument.GetTopLevelChangeTextVersionAsync()); + + Assert.NotEqual( + await documentToFreezeOriginal.Project.GetDependentSemanticVersionAsync(), + await frozenDocument.Project.GetDependentSemanticVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.Project.GetDependentSemanticVersionAsync(), + await frozenDocument.Project.GetDependentSemanticVersionAsync()); + + Assert.NotEqual( + await documentToFreezeOriginal.Project.GetSemanticVersionAsync(), + await frozenDocument.Project.GetSemanticVersionAsync()); + + Assert.Equal( + await documentToFreezeChanged.Project.GetSemanticVersionAsync(), + await frozenDocument.Project.GetSemanticVersionAsync()); + } + [Fact] public void TestFrozenPartialProjectAlwaysIsIncomplete() { diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs index ba4a68ea277ed..81e8d465e9a15 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Log/FunctionId.cs @@ -583,5 +583,8 @@ internal enum FunctionId // 650-660 for diagnostic/fix related ids. Diagnostics_AnalyzerPerformanceInfo = 651, + + // 660-670 for semantic model reuse service. + SemanticModelReuseLanguageService_TryGetSpeculativeSemanticModelAsync_Equivalent = 660, } }