WIP: Sync solution contents consistently

[CyrusNajmabadi]

[CyrusNajmabadi]

Need to go over why this is failing with @jasonmalinowski . I believe it's due to a very odd test that itself may be wrong. But it relates to frozen SG docs, so i need to talk to him to understand that better.

[CyrusNajmabadi]

@jasonmalinowski Let me try to explain what's going on wrong (as far as the test is concerned). Note, i do not know if the test is wrong, or if the product behavior is wrong. So i'll def need your input here.

First, let's talk about what teh change is that this PR is making. Previously, the way we would sync with oop would be the following:

1. send checksum over.
2. rehydrate solution-id and solution-filepath and compare that to RemoteWorkspace's CurrentSolution.
3. if different, we'd make a new solution, hydrating it from teh checksum.
4. if teh same same, we'd take the CurrentSolution, get its local checksum, and do a diff/sync of it vs the checksum passed in, syncing over missing/changed data and making the necessary changes.

Now: consider what would happen in tests. In lots of tests, we would never go into the '4' path (even when making local changes to the solution). That's because we never sent the message from host to oop to "set the primary solution" in the first place. So for every call to OOP, we'd always go into the '3' path, always creating a new fresh solution instance and operating on that.

Why does this matter? Well paths '3' and '4' operate in a subtle different fashion. While both (should be) the same vis-a-vis getting the same sets of projects, documents, options, references, and analyzers. '3' differs from '4' in that it does not hydrate the "frozen source generated documents" into the solution it just created.

In the tests in question that fails SolutionWithSourceGeneratorTests.FreezingSourceGeneratedDocumentsWorks this has the impact that there's no prior frozen generated document when freezing the second document, and so only 1 document is found here: https://github.com/dotnet/roslyn/blob/92fb904fb7200ce46c3b07bfe3835a3c955613ca/src/Workspaces/CoreTest/SolutionTests/SolutionWithSourceGeneratorTests.cs#L840

Ok. So that explains what was going on, and why the test passed before. So what happened with my change. Well, with my change, i changed '3' and '4'. Now the new logic is:

1. send checksum over.
2. rehydrate solution-id and solution-filepath and compare that to RemoteWorkspace's CurrentSolution.
3. if different, we'd make a new solution, hydrating it from teh checksum.
4. regardless if same or different, we'd take the existing solution (or freshly hydrated new solution), get its local checksum, and do a diff/sync of it vs the checksum passed in, syncing over missing/changed data and making the necessary changes.

With thsi new logic we always do the diff/sync (to be as sure as possible the oop solution matches the host one). In the test in question, that means that after the initial sync, we see the host side had a "frozen source generated documents", and hten it adds another one (that's jut what the test does). This then means that we think we have two frozen generated documents as we keep the old syntax tree around, and add the new one in. Causing the test to fail.

I'm honestly not sure how the "frozen source generated documents" piece is supposed to work. And i'm worried the test is trying to tell me about something important :) So i don't want to just change it. So i def need your understanding/info here to move forward on this.

Note: i really like the overall concept of the change. I think it's very sound to just make a fresh solution when needed, but otherwise have both codepaths do the same full diff sync in both cases to ensure you always get the same oop snapshot no matter how you started.

Put another way, i view it as a strong bug/consistency-issue that you could ever get different solution-snapshots on the OOP side depending on if it was starting from scratch, or if it was updating something it already had. Thsi PR attempts to fix that, but runs afoul of some of the stuff you know much more about.

Thanks!

[CyrusNajmabadi]

@jasonmalinowski this is still a priority :)

[CyrusNajmabadi]

@jasonmalinowski Def need eyes on https://github.com/dotnet/roslyn/pull/72860#issuecomment-2035274001. Please review soon. Thanks.

[jasonmalinowski]

@CyrusNajmabadi: so if I'm following your question correctly I think this is the bit where things are breaking down:

we see the host side had a "frozen source generated documents", and hten it adds another one (that's jut what the test does). This then means that we think we have two frozen generated documents as we keep the old syntax tree around, and add the new one in. Causing the test to fail.

Note that the AssertFrozen helper there is static and not reassigning back to the "project" local. Instead it's doing a test that the freezing works with a regular snapshot, and then doing a second assert it works with a project that we removed the analyzer from. But the frozen snapshot should have been thrown away since it was just a fork at that point. Put another way, this test is conceptually two tests, just with two asserts to avoid duplicating the setup.

It's not obvious why this is failing to me, but it would indicate there's a bug in our code somewhere. I would expect that at the point of failure that this map here:

https://github.com/dotnet/roslyn/blob/92fb904fb7200ce46c3b07bfe3835a3c955613ca/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs#L44

would contain only a single entry (since in each case where we're doing a test we're calling WithFrozenSourceGeneratedDocument we only had a single document in there. And I'm not sure how there'd be two, since the Ids of both cases would be the same, so the key -> value map really can't have two unless somehow the Ids were different.

What I'm guessing is happening is this: when the first AssertFrozen() is being ran, a compilation is being created, and that fork is being thrown away. But somehow that compilation is making it's way to the second use in AssertFrozen, where the compilation is being taken and "reused" by the generated document being added a second time, somehow. So we're parsing the contents a second time.

[jasonmalinowski]

I had forgotten you had a question here so my signoff was before I saw that (which explained to me why I was asked to review -- the code looks so simple!) I'll leave the signoff since it seems at least what has been written looks fine, and the bug exists elsewhere.

[CyrusNajmabadi]

@jasonmalinowski That helps a lot. Thanks!

[CyrusNajmabadi]

@jasonmalinowski THat helped a bunch. Especially realizing that each fork was independent.

What was happening is that on both the host and oop side we were now CORRECTLY setting up the trackers to have:

Host:
GeneratedSourceReplacingTracker->CompilationTracker

OOP:
GeneratedSourceReplacingTracker->CompilationTracker

Previously, due to the bugs in syncing (that this PR fixes) we weren't doing that and were gettitng:

Host:
GeneratedSourceReplacingTracker->CompilationTracker

OOP:
CompilationTracker

This revealed a bug in OOP source generators. Specifically, the host side's side normal CompilatinoTracker would ask the oop side for generated sources. But the OOP side wouldn't ask its normal CompilationTracker for this, but instead its GeneratedSourceReplacingTracker. So OOP would say "i have one generated document". the host would tehn then go "ok... great, my regular compilation tracker has one generated doc, and i'll now add an additional generated doc on top of that in my GeneratedSourceReplacingTracker".

Basically we had:

Host:                                                         OOP:
GeneratedSourceReplacingTracker->CompilationTracker    ->     GeneratedSourceReplacingTracker->CompilationTracker

Now we have:

Host:                                                         OOP:
                                                   /--------------------->--------------------\
GeneratedSourceReplacingTracker->CompilationTracker           GeneratedSourceReplacingTracker->CompilationTracker

[CyrusNajmabadi]

@jasonmalinowski I know you already signed off. But you should take another look with the additional changes this motivated.

[CyrusNajmabadi]

@ToddGrun ptal.