Remove SolutionServices in favor of HostWorkspaceServices #62909
Conversation
| var services = workspace != _solutionServices.Workspace | ||
| ? new SolutionServices(workspace) | ||
| : _solutionServices; | ||
|
|
||
| // Note: this will potentially have problems if the workspace services are different, as some services | ||
| // get locked-in by document states and project states when first constructed. |
There was a problem hiding this comment.
this comment is outright terrifying (and true). the workspace services are passed deeply down the solution. but when we do WithNewWorkspace, we only have a shallow pointer to the new services here. and all our children still point to the old services =-o
| @@ -238,7 +240,7 @@ public async Task<MutableConflictResolution> ResolveConflictsAsync() | |||
| { | |||
| var definitionLocations = _renameLocationSet.Symbol.Locations; | |||
| var definitionDocuments = definitionLocations | |||
| .Select(l => conflictResolution.OldSolution.GetRequiredDocument(l.SourceTree)) | |||
| .Select(l => conflictResolution.OldSolution.GetRequiredDocument(l.SourceTree!)) | |||
There was a problem hiding this comment.
i Have no idea why, but changing line 121 caused an NRT warning here...
There was a problem hiding this comment.
@CyrusNajmabadi So I had (not yet submitted in a PR) a change that was also touching SolutionServices and I think I saw this behavior too.
There was a problem hiding this comment.
Did you figure out what it was?
ghost
added this to the 
| @@ -14,7 +14,7 @@ internal sealed class AdditionalDocumentState : TextDocumentState | |||
| private readonly AdditionalText _additionalText; | |||
|
|
|||
| private AdditionalDocumentState( | |||
| SolutionServices solutionServices, | |||
| HostWorkspaceServices solutionServices, | |||
There was a problem hiding this comment.
Did you want to rename the parameters here?
| @@ -142,7 +142,7 @@ public async Task<Checksum> GetChecksumAsync(ProjectId projectId, CancellationTo | |||
| }) | |||
| .ToArray(); | |||
|
|
|||
| var serializer = _solutionServices.Workspace.Services.GetRequiredService<ISerializerService>(); | |||
There was a problem hiding this comment.
Oh that old code was hilarious.
There was a problem hiding this comment.
@CyrusNajmabadi Thanks for the cleanup. Was looking only since I had a PR adding something to SolutionServices so just wanted to see if there was anything interesting.
We intend to add
solutionInstance.Workspaceto the banned api list for roslyn itself. It's highly problematic for OOP, as we have to try to replicate that concept, which isn't something sensical in that world. First, OOP cannot mutation things, so exposing the mutable workspace doesn't make much sense. Second, in VS you can have many workspaces, but in OOP we map that all back to one, which means we don't have a proper representation of VS state (for example, where you might have the VSWorkspace and the Metadata-as-source workspace). This means those features either cannot use OOP, or they try to use OOP and can end up with non-sensical results.This is a series of PRs to get us off of that API so we can move to a much cleaner and simpler OOP model for the solution and services.