Freeze semantics for all branches when asked#55910
Freeze semantics for all branches when asked#55910jasonmalinowski merged 5 commits intodotnet:mainfrom
Conversation
We had logic in Document.WithFrozenPartialSemantics which meant we would only freeze semantics if the Soluion in question was the "primary" branch, that is the actual instance from some Workspace.CurrentSolution. This seems terribly unwise: features like completion call GetOpenDocumentInCurrentContextWithChanges when doing things like computing entries or committing; if the text snapshots were out of sync then that would always be forking. In that case Freeze did nothing at all -- it would return the original Solution instance, so asking for semantics would build the full compilation like before. This is even worse with source generators, since we weren't freezing the generator state either and would rerun generators when later asked. The comment before the code to me isn't really explaining why this behavior is desirable: it's correct that if you have a forked Solution there is no background compiler pulling it forward. But it'll still have the state that was computed when it was forked, which should be enough for anybody opting into partial semantics in the first place. While writing a test, I also discovered our existing tests around frozen partial semantics and generators weren't actually testing anything, since the workspace didn't have partial semantics on in the first place. Worse off, they still didn't work unless we also remove the BranchId check.
| // as partial semantics don't make sense otherwise. | ||
| if (solution.BranchId == workspace.PrimaryBranchId && | ||
| workspace.PartialSemanticsEnabled && | ||
| if (workspace.PartialSemanticsEnabled && |
There was a problem hiding this comment.
If we felt we still wanted to keep the BranchId check, then the alternative fix for #55578 might be to instead ensure that on the "else" branch we freeze generators while still letting the rest of the compilation generation continue. We can do that, although it's a bit awkward.
There was a problem hiding this comment.
personally, i find the branchid stuff a nightmare. i don't know why it exists, and i often can't reason about the impact it will have because of how it changes random logic in random places. i would love being able to remove it.
afaict, all our systems (except maybe diagnostics) are all snapshot based, and should work perfectly well no matter what snapshot (or branch-id of that snapshot) it's running against.
There was a problem hiding this comment.
@CyrusNajmabadi Agreed, I hate it too. I fully admit to checking what was still using it to see if I could delete it as a part of this PR, assuming this actually does what we want.
|
I tested this with the change in #55745, and it seems to fixed the UI freeze caused by running SG during commit. |
We had logic in Document.WithFrozenPartialSemantics which meant we would only freeze semantics if the Soluion in question was the "primary" branch, that is the actual instance from some Workspace.CurrentSolution. This seems terribly unwise: features like completion call GetOpenDocumentInCurrentContextWithChanges when doing things like computing entries or committing; if the text snapshots were out of sync then that would always be forking. In that case Freeze did nothing at all -- it would return the original Solution instance, so asking for semantics would build the full compilation like before. This is even worse with source generators, since we weren't freezing the generator state either and would rerun generators when later asked.
The comment before the code to me isn't really explaining why this behavior is desirable: it's correct that if you have a forked Solution there is no background compiler pulling it forward. But it'll still have the state that was computed when it was forked, which should be enough for anybody opting into partial semantics in the first place.
While writing a test, I also discovered our existing tests around frozen partial semantics and generators weren't actually testing
anything, since the workspace didn't have partial semantics on in the first place. Worse off, they still didn't work unless we also
remove the BranchId check.