Ensure we recompute top level semantic versions, even when making frozen documents #65820
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
merged these to one value. if hte content changed we need to recalculateDependentVersions, and if it didn't, we don't.
| // We're changing the text of this document to something else entirely. So we need to pass | ||
| // `textChanged: true, recalculateDependentVersions: true` so that any interested parties that | ||
| // compute anything that is dependent on changed text get the right values. | ||
| inProgressProject = inProgressProject.UpdateDocument(docState, contentChanged: true); |
There was a problem hiding this comment.
this is the crux of the fix. it was possible to take a prior syntax tree for this doc and update ourselves with it, but then say there was no change, so top-level-versions might be incorrect. We have a supposition this is what's leading to the nfw.
| @@ -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) | |||
There was a problem hiding this comment.
reason 10 that optional parameters can be really bad.
src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs
Outdated
Show resolved
Hide resolved
| 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); |
There was a problem hiding this comment.
What does contentChanged control here in the case of additional files? I wouldn't have imagined it mattered, but if it does, why isn't there one in the analyzer config method right below this one?
There was a problem hiding this comment.
so, in the end, it shouldn't matter as any attempt to get things like top-level versions will bail out immediately and return the normal text-version since these docs don't 'SupportSyntax'. But i wanted to be consistent here around the concept that is happening. There are doc changes that change contents, and doc changes that don't. We should flow that along, and have any smarts happen later.
There was a problem hiding this comment.
Specifically, this will end up bottoming out here:
if (_treeSource == null)
{
return await GetTextVersionAsync(cancellationToken).ConfigureAwait(false);
}So it's all ok. But, for example, if we ever stored info with these docs, we'd want to know when they actually chnaged so we could properly dump it and recompute.
ghost
added this to the 
Potential fix for an NFW we're seeing in speedometer. And, even if it doesn't help there, it certainly makes this code saner and properly in line with invariants we expect.