Skip to content

Commit

Permalink
Merge pull request #65820 from CyrusNajmabadi/frozenChange
Browse files Browse the repository at this point in the history 
Ensure we recompute top level semantic versions, even when making frozen documents
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi authored Dec 7, 2022
2 parents c22bb89 + 5989da7 commit 369f361
Show file tree
Hide file tree
Showing 6 changed files with 142 additions and 61 deletions.
51 changes: 19 additions & 32 deletions src/Workspaces/Core/Portable/SemanticModelReuse/AbstractSemanticModelReuseLanguageService.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,20 @@
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
{
internal abstract class AbstractSemanticModelReuseLanguageService<
TMemberDeclarationSyntax,
TBasePropertyDeclarationSyntax,
TAccessorDeclarationSyntax> : ISemanticModelReuseLanguageService
TAccessorDeclarationSyntax> : ISemanticModelReuseLanguageService, IDisposable
where TMemberDeclarationSyntax : SyntaxNode
where TBasePropertyDeclarationSyntax : TMemberDeclarationSyntax
where TAccessorDeclarationSyntax : SyntaxNode
{
/// <summary>
/// Used to make sure we only report one watson per sessoin here.
/// </summary>
private static bool s_watsonReported;
private readonly CountLogAggregator<bool> _logAggregator = new();

protected abstract ISyntaxFacts SyntaxFacts { get; }

Expand All @@ -32,44 +29,34 @@ internal abstract class AbstractSemanticModelReuseLanguageService<
protected abstract SyntaxList<TAccessorDeclarationSyntax> 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<SemanticModel?> TryGetSpeculativeSemanticModelAsync(SemanticModel previousSemanticModel, SyntaxNode currentBodyNode, CancellationToken cancellationToken)
{
var previousSyntaxTree = previousSemanticModel.SyntaxTree;
var currentSyntaxTree = currentBodyNode.SyntaxTree;

// 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.

Expand All @@ -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;

Expand Down
16 changes: 8 additions & 8 deletions src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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(
Expand All @@ -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)
Expand All @@ -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(
Expand Down Expand Up @@ -939,13 +939,13 @@ private void GetLatestDependentVersions(
TextDocumentStates<DocumentState> newDocumentStates,
TextDocumentStates<AdditionalDocumentState> newAdditionalDocumentStates,
TextDocumentState oldDocument, TextDocumentState newDocument,
bool recalculateDependentVersions, bool textChanged,
bool contentChanged,
out AsyncLazy<VersionStamp> dependentDocumentVersion, out AsyncLazy<VersionStamp> dependentSemanticVersion)
{
var recalculateDocumentVersion = false;
var recalculateSemanticVersion = false;

if (recalculateDependentVersions)
if (contentChanged)
{
if (oldDocument.TryGetTextVersion(out var oldVersion))
{
Expand All @@ -963,13 +963,13 @@ private void GetLatestDependentVersions(

dependentDocumentVersion = recalculateDocumentVersion
? new AsyncLazy<VersionStamp>(c => ComputeLatestDocumentVersionAsync(newDocumentStates, newAdditionalDocumentStates, c), cacheResult: true)
: textChanged
: contentChanged
? new AsyncLazy<VersionStamp>(newDocument.GetTextVersionAsync, cacheResult: true)
: _lazyLatestDocumentVersion;

dependentSemanticVersion = recalculateSemanticVersion
? new AsyncLazy<VersionStamp>(c => ComputeLatestDocumentTopLevelChangeVersionAsync(newDocumentStates, newAdditionalDocumentStates, c), cacheResult: true)
: textChanged
: contentChanged
? CreateLazyLatestDocumentTopLevelChangeVersion(newDocument, newDocumentStates, newAdditionalDocumentStates)
: _lazyLatestDocumentTopLevelChangeVersion;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
38 changes: 19 additions & 19 deletions src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/// <summary>
Expand All @@ -1246,7 +1246,7 @@ public SolutionState WithDocumentFolders(DocumentId documentId, IReadOnlyList<st
return this;
}

return UpdateDocumentState(oldDocument.UpdateFolders(folders));
return UpdateDocumentState(oldDocument.UpdateFolders(folders), contentChanged: false);
}

/// <summary>
Expand All @@ -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);
}

/// <summary>
Expand All @@ -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);
}

/// <summary>
Expand All @@ -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);
}

/// <summary>
Expand Down Expand Up @@ -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);
}

/// <summary>
Expand All @@ -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);
}

/// <summary>
Expand Down Expand Up @@ -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<Compilation> UpdateDocumentInCompilationAsync(
Expand All @@ -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);
}

/// <summary>
Expand All @@ -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);
}

/// <summary>
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Loading

0 comments on commit 369f361

Please sign in to comment.