Don't break if we fork a snapshot after freezing partial semantics

[jasonmalinowski]

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 #56702

[genlu]

if we later forked, we'd run generators again and that would result in us trying to add generated trees that already existed.

So if we pass a frozen document to a completion provider, it will trigger the source generator to run at the moment it needs to make additional change and get an update compilation to the frozen document (e.g. trying to figure out the changes for the final commit)?

[jasonmalinowski]

@CyrusNajmabadi @genlu: I've pushed another set of changes to this PR to discuss about risk/reward tradeoffs here.

Background: override completion, when it is producing the generated method, is forking a document to make the code changes. It is doing this on the frozen partial semantics we generally moved completion onto. My original commit fixed the fact that freeze-and-then-fork was just going to break, but it did mean that on the forked solution generators could run again if further semantics were asked about.

Concern: it means we're still potentially going to run generators in a place where we were already concerned about performance. Even if the generator is fast, it's not clear if running it on a partial semantics snapshot is a great idea, since it might be missing trees or cross-project references.

New Change: when we freeze semantics, we ensure we never run generators for that project again, even if there's some later forking of that project. If you look at the last commit first, the core change is pretty simple: we already have a bit which is "we have updated documents for this snapshot, we don't need to re-run" -- I'm adding a second bit that means to not rerun ever.

Concern: the last commit is pretty simple, but commit before that was a bit less so since I needed to ensure we're actually keeping that bit around. There were various places we decomposed the GeneratorInfo structure. Generally, the cleanup commit is a good thing and I want to keep around, but I'm feeling iffy about wanting to land that in 17.0 just because of the amount of churn. If we're going to take this I'd really appreciate some careful eyes on this one.

[RikkiGibson]

@jasonmalinowski @jinujoseph If this intended to go through QB, we should merge it tonight and get things flowing. 🙂

[CyrusNajmabadi]

would prefer this not be optional. callers should have to think about this.

[genlu]

                var finalState = FinalState.Create(

Add an assert documentsAreFinal==true here?

---

Refers to: src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker.cs:882 in d198e82. [](commit_id = d198e82, deletion_comment = False)

[genlu]

Would you consider using a enum here excessive? It seems to me that having something like enum GeneratedDocumentsState with 3 members NotFinal, Final and FinalAndFrozen would make it clearer. i.e. you can only change the state in one direction.

Actually the one-direction comment is false.

[jasonmalinowski]

Not a bad idea, OK to defer just to limit churn here?

[genlu]

Sure

[jasonmalinowski]

Add an assert documentsAreFinal==true here?

Missed this comment; turns out that's unnecessary since even if we did the FinalState constructor itself also forces it to true.