Extract compilation-tracker/SG-production code out of SolutionState to a new owner. #71257

CyrusNajmabadi · 2023-12-14T00:51:21Z

Implements initial work around cleaning up how SG production works. As part of the effort outlined here: #67441 (comment)

Major design changes in this PR:

SolutionState is broken up into two types: SolutionState and SolutionCompilationState
SolutionState is now a very simple green tree storing the basic information about the solution, its projects, and the documents therein (including the texts/loaders for those documents). Only when information about those values changes would you expect to get a new SolutionState instance.
SolutionCompilationState has a pointer to a SolutionState. So any time a new SolutionState is produced, a new SolutionCompilationState must be produced.
SolutionCompilationState then has all the information used for producing compilations (the 'compilation trackers'), generated docs (part of the compilation tracking system) and the ability to do things like get 'frozen partial semantics' (also related to compilations).
Solution now points at SolutionCompilationState, though it still exposes as a convenience the SolutionState value as many consumers still read that.

Initially, i tried to not do '3'. I wanted to keep these entities entirely independent. That way you could even update a SolutionState and not update a SolutionCompilationState. However, in the process of doing this, i ended up finding this very arduous. Lots of codepaths now needed to pass both entities around, and there was a very real concern about doing this incorrectly and ending up with inconsistencies. There are so many commits here because i went down that route, ran into enough subtle cases, and eventually felt it was not worth it.

Several nice things fell out of this PR imo. First, there were downstream impacts in that a bunch of places that were accessing Solution.State moved to accessing Solution.CompilationState. This is nice as it strongly calls out the dependency that code actually has on compilatino-info, and not just solution-structure. This also revealed that "oop checksum computation" is dependent on SG production today. Something we may want to see about resolving. Specifically, now that generators run in OOP anyways, it would be nice to just have all OOP syncing jsut be based on solution-structure.

--

Remainder items:

Remove Solution._state.
Rename Solution.State to Solution.SolutionState.
Audit SolutionCompilationState methods to see if they actually have to fork projects.
Some methods throw after seeing hte state didn't change. Can they just do a more up front data check?
rename files.
rename SolutionCompilationState Solution to COmpilationState. same with SolutionCompilationState solution
update ComputeNewGeneratorInfoInCurrentProcessAsync to only take solutionstate
try to make GetState(ProjectId/DocumentId) not allow nulls.

… objects

CyrusNajmabadi · 2023-12-19T04:22:13Z

Considering adding a TODO here, even if it's going away in a follow-up.

Done.

CyrusNajmabadi · 2023-12-19T04:29:29Z

Why are we requesting to fork trackers though in these situations?

So, a core goal here was to preserve semantics with the existing code. The existing code does this:

roslyn/src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.cs

Lines 634 to 645 in 95f4a35

    
           public SolutionState WithProjectOutputFilePath(ProjectId projectId, string? outputFilePath) 
        
           { 
        
               var oldProject = GetRequiredProjectState(projectId); 
        
               var newProject = oldProject.WithOutputFilePath(outputFilePath); 
        
               if (oldProject == newProject) 
        
               { 
        
                   return this; 
        
               } 
        
               return ForkProject(newProject); 
        
           }

As can be seen, if the project doesn't actually change, then the same solution is returned. If it does change, we do fork the project.

--

We can always cahnge this. And i'm happy to do an audit with you on this after the PR is done. But it's def safest to keep the semnatics the same.

CyrusNajmabadi · 2023-12-19T04:31:15Z

Should this just check the project reference existing more direclty rather than having the different pattern here?

Happy to do that. Marked for followup.

git push

CyrusNajmabadi · 2023-12-19T04:38:12Z

Do you have a follow-up list actually going somewhere for these?

Yup. DOing it in the first post of this PR.

CyrusNajmabadi · 2023-12-19T04:40:00Z

Frankly we should just inline it...there's more lines of parameters than there are actual code

Done!

jasonmalinowski

Questions in this batch are mostly around "do you imagine this changing once we do further work?" and don't really indicate problems here, I'm just trying to mentally match our conversations with this code.

jasonmalinowski · 2023-12-20T01:00:46Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker_Generators.cs

@@ -85,12 +85,13 @@ private partial class CompilationTracker : ICompilationTracker
        }

        private async Task<(Compilation compilationWithGeneratedFiles, CompilationTrackerGeneratorInfo generatorInfo)?> TryComputeNewGeneratorInfoInRemoteProcessAsync(
-            SolutionState solution,
+            SolutionCompilationState compilationState,


So this seems a bit strange: if we're computing new generator results, we don't actually need to sync over the existing CompilationState, right? Is this just because right now we don't have a sync method that takes only the "new" SolutionState? If I recall our conversation we figured this was potentially the one "special" place that can safely ignore the generated state since that's the whole point of this method.

I of course am worried if it means we have to fork the sync system to support either kind of state, unless we just can easily introduce some interface for either kind of state if all it needs is a checksum in the end or something.

Is this just because right now we don't have a sync method that takes only the "new" SolutionState? If I recall our conversation we figured this was potentially the one "special" place that can safely ignore the generated state since that's the whole point of this method.

That's exactly right. And this is something i want to do in a followup. It's unclear though how best to layer that and use the type system properly. Especially since everyone else will not want this path.

I of course am worried if it means we have to fork the sync system to support either kind of state, unless we just can easily introduce some interface for either kind of state if all it needs is a checksum in the end or something.

Yup. I don't have a great answer yet. What i'm tentatively thinking is:

have all the existing 'Invoke' methods (which take the full compilation state). And then expose the literal: "get me the generator results" APIs which will take only the solution-state. So basically, no one else would call these. And this code here would only call that API.

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.CompilationTracker_Generators.cs

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SymbolToProjectId.cs

jasonmalinowski · 2023-12-20T01:14:04Z

src/Workspaces/Core/Portable/Workspace/Solution/StateChecksums.cs

@@ -70,7 +70,7 @@ public static SolutionStateChecksums Deserialize(ObjectReader reader)
    }

    public async Task FindAsync(
-        SolutionState state,
+        SolutionCompilationState compilationState,


At some point do you imagine we might have two FindAsync() methods, one that handles just the SolutionState for the run-generator case (since we're not going to sync generators over) and one that takes the SolutionCompiltionState that delegates to the former?

Yes. That's exactly how i see it.

Yes. That's precisely what i want to do.

jasonmalinowski · 2023-12-20T01:15:30Z

src/Workspaces/Core/Portable/Workspace/Solution/TextDocumentStates.cs

+        public TState? GetState(DocumentId? documentId)
+            => documentId != null && _map.TryGetValue(documentId, out var state) ? state : null;


Ugh seeing the place we did this for ProjectId terrified me; can we do something else instead? What needed this?

i will add this to the fallout list.

jasonmalinowski · 2023-12-20T01:22:09Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState_Checksum.cs

@@ -100,7 +109,7 @@ void AddReferencedProjects(HashSet<ProjectId> result, ProjectId projectId)
                    // host to remove the reference.  We do not expose this through the full Solution/Project which
                    // filters out this case already (in Project.ProjectReferences). However, becausde we're at the
                    // ProjectState level it cannot do that filtering unless examined through us (the SolutionState).


Suggested change

// ProjectState level it cannot do that filtering unless examined through us (the SolutionState).

// ProjectState level it cannot do that filtering unless examined through us (the SolutionCompilationState).

Although I'm not really sure the difference matters here.

jasonmalinowski · 2023-12-20T01:24:45Z