Implement a new brokered service for passing project information#65592
Conversation
ghost
added
the
|
@drewnoakes I would appreciate your thoughts on this. |
drewnoakes
left a comment
drewnoakes
left a comment
There was a problem hiding this comment.
Looks good to me, modulo the things you called out. We'll be able to move dotnet/project-system to consume this once available.
What makes it more friendly? |
| _project.RemoveAnalyzerReference(analyzerPath); | ||
| } | ||
|
|
||
| public async Task AddMetadataReferencesAsync(IReadOnlyList<MetadataReferenceInfo> metadataReferences, CancellationToken cancellationToken) |
There was a problem hiding this comment.
why IReadOnlyList out of curiosity?
There was a problem hiding this comment.
This was explicitly mentioned in the RPC contracts best practices:
Use readonly collections interfaces in RPC method signatures (e.g. IReadOnlyList) rather than concrete types (e.g. List or T[]). This allows for potentially more efficient deserialization.
Maybe @AArnott has further thoughts; but the downside of a strongly typed thing here is we're then stuck with that. Also because they're abstract means they don't technically even need an immutable implementation if that has extra memory overhead.
There was a problem hiding this comment.
Besides interfaces leaving the deserializer to do the most efficient thing, using a read only interface makes it more clear to the receiver that they were not given some data structure that they can mutate and expect to be mutating the sender's copy. This guidance applies to more than just collections. We encourage every data type to be immutable so there is never any question that the receiver cannot change it and expect the sender to change as a result.
There was a problem hiding this comment.
Sorry, I meant: why IROL instead on ImmutableArray?
We use the immutable types in the rest of our service huh services. So I was curious why we're not doing the same here.
There was a problem hiding this comment.
This guidance applies to more than just collections. We encourage every data type to be immutable so there is never any question that the receiver cannot change it and expect the sender to change as a result.
I think @CyrusNajmabadi's question might still be "but ImmutableArray works, so why not that here?"
There was a problem hiding this comment.
That's up to you. But a reason not to use ImmutableArray<T> in a deserializer is that if the deserializer doesn't know the length ahead of time, it means an extra copy and allocation of the collection is required.
Now it so happens that if you're using MessagePack as your formatter, it does know the length ahead of time and leverages that to use the optimal builder+MoveToImmutable pattern, so that could serve you well here.
jasonmalinowski
force-pushed
the
introduce-brokered-service-for-project-system
branch
from
000d0e0 to
26bc809
Compare
|
|
||
| namespace Microsoft.CodeAnalysis.Remote.ProjectSystem | ||
| { | ||
| public record WorkspaceProjectCreationInfo(string Language, string DisplayName, string? FilePath, IReadOnlyDictionary<string, string> BuildSystemProperties); |
There was a problem hiding this comment.
@tmat: as it is, it's not RPC friendly since the contract is you derive the type and then there's a chatty calling back and forth for each item. For properties we can easily just gather up all the properties at once and just send them over. Right now we aren't actually using this for items at all:
So this seems a lot easier.
There was a problem hiding this comment.
We are going to use items. See #64892.
You can derive a type that has the two dictionaries in it and is serializable or create another type that has the same shape.
There was a problem hiding this comment.
You can derive a type that has the two dictionaries in it and is serializable or create another type that has the same shape.
@tmat Yeah, the latter is the goal here. The former might work but seems silly to define an RPC interface where you can only use certain derived types to actually meet the implicit contracts of the system.
Do we have any examples of items other than the intermediate output path example? Because that's not a terribly motivating example given I'm not sure why it's not a property in the first place. I'd much rather just teach the project system to convert a single item group to a property when requested rather than have a bunch of infrastructure for that.
There was a problem hiding this comment.
I have other concerns with items specifically, since we have cases today where if we were going to use items more fully we'd also need to pass along some of the other metadata too, so we'd need to spec that properly.
There was a problem hiding this comment.
I'd much rather just teach the project system to convert a single item group to a property when requested rather than have a bunch of infrastructure for that.
Sure, we can do that but what do we do when we need to read items that have multiple values? That's not such a stretch and having support for that isn't that much extra effort. It's one more immutable dict.
msbuild allows setting properties based on other properties and items based on other items during evaluation. It does not support converting items to properties, otherwise we'd just do that. If we need to read metadata of some item group we can transform them to item values. Hence, we can easily get away without supporting item metadata in the project system interface.
There was a problem hiding this comment.
Sure, we can do that but what do we do when we need to read items that have multiple values?
What concrete examples did you have in mind?
If we need to read metadata of some item group we can transform them to item values
Ick, the cases I had in mind where we might need this would be a nightmare for escaping. That doesn't seem like a usable approach.
If we have to ultimately add a Dictionary<string, IEnumerable<Dictionary<string>>> here I'm not strictly against it; it seems however the current pattern we have here isn't generally applicable for the things I'd first imagine needing to use it for. I'd rather design that hen we're actually in a place to consume that versus where we are now.
| --> | ||
| <ItemGroup> | ||
| <InProcService Include="Microsoft.VisualStudio.LanguageServices.SolutionAssetProvider" /> | ||
| <InProcService Include="Microsoft.VisualStudio.LanguageServices.WorkspaceProjectFactoryService" /> |
There was a problem hiding this comment.
For my own curiosity, what does this do and how does it do it? :-)
There was a problem hiding this comment.
It's more or less equivalent to ProvideBrokeredServiceAttribute. Rather than using the pkgdef attributes we generate them in MSBuild because CreatePkgDef has issues with cross-plat builds.
This defines a new brokered service that is pretty similar to IWorkspaceProjectContext, but more friendly for cross-process communication. It's currently implemented atop IWorkspaceProjectContext so that way the behavior is identical otherwise, but hopefully the project system can move to consuming this one soon enough and we can flatten the implementation.
jasonmalinowski
force-pushed
the
introduce-brokered-service-for-project-system
branch
from
26bc809 to
91afef7
Compare
jasonmalinowski
deleted the
introduce-brokered-service-for-project-system
branch
The dotnet/roslyn#65592 the Roslyn team defined a new brokered service for communicating information from the project system to the language service. In effect, the `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` types are being replaced by the very similar `IWorkspaceProject` and `IWorkspaceProjectFactoryService` types, but it isn't happening all at once. In preparation, this commit moves all the code depending on `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` out of our host-agnostic layer (Microsoft.VisualStudio.ProjectSystem.Managed) and into our VS-specific layer (Microsoft.VisualStudio.ProjectSystem.Managed.VS). Specific changes: 1. Any files that actually call the old interfaces are moved as-is to the VS layer. 2. Any further files that depend on those files in 1 are moved as well. 3. Files that refer to the old interfaces only in comments or doc comments have been updated to use phrases like "language service project" instead but have otherwise been left in place. 4. The dependency on the Microsoft.VisualStudio.LanguageServices package has moved from the host-agnostic layer to the VS-specific layer. 5. The `DotNetImportsEnumProvider` has also moved to the VS layer, as it depends on the `VisualStudioWorkspace` type. The host-agnostic layer no longer references the assembly with that type. 6. Unit test files have also been moved around to reflect the new layering. There are a few further dependencies on the Roslyn `Workspace` type in Microsoft.VisualStudio.ProjectSystem.Managed; those will be moved in a future change as direct access to a `Workspace` is very much dependent on VS.
The dotnet/roslyn#65592 the Roslyn team defined a new brokered service for communicating information from the project system to the language service. In effect, the `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` types are being replaced by the very similar `IWorkspaceProject` and `IWorkspaceProjectFactoryService` types, but it isn't happening all at once. Instead, the VS Code-specific version of the project system will adopt the new interfaces first (since the old ones don't work at all in a system where the language service and project system are in different processes) and at a later date the VS for Windows-specific project system will move over, once we've worked out all the bugs. This should prevent regressions in VS for Windows while we get VS Code up and running. However, this means that any code depending on `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` needs to move out of our host-agnostic layer (Microsoft.VisualStudio.ProjectSystem.Managed) and into our VS-for-Windows-specific layer (Microsoft.VisualStudio.ProjectSystem.Managed.VS). This commit does the minimum necessary to accomplish that: 1. Any files that actually call the old interfaces are moved as-is to the VS layer. 2. Any further files that depend on those files in 1 are moved as well. 3. Files that refer to the old interfaces only in comments or doc comments have been updated to use phrases like "language service project" instead but have otherwise been left in place. 4. The dependency on the Microsoft.VisualStudio.LanguageServices package has moved from the host-agnostic layer to the VS-specific layer. 5. The `DotNetImportsEnumProvider` has also moved to the VS layer, as it depends on the `VisualStudioWorkspace` type. The host-agnostic layer no longer references the assembly with that type. 6. Unit test files have also been moved around to reflect the new layering. There are a few further dependencies on the Roslyn `Workspace` type in Microsoft.VisualStudio.ProjectSystem.Managed; those will be moved in a future change as we can't count on being able to MEF-import a `Workspace` in VS Code scenarios; it will generally exist only in the language service process.
Move language service support The dotnet/roslyn#65592 the Roslyn team defined a new brokered service for communicating information from the project system to the language service. In effect, the `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` types are being replaced by the very similar `IWorkspaceProject` and `IWorkspaceProjectFactoryService` types, but it isn't happening all at once. Instead, the VS Code-specific version of the project system will adopt the new interfaces first (since the old ones don't work at all in a system where the language service and project system are in different processes) and at a later date the VS for Windows-specific project system will move over, once we've worked out all the bugs. This should prevent regressions in VS for Windows while we get VS Code up and running. However, this means that any code depending on `IWorkspaceProjectContext` and `IWorkspaceProjectContextFactory` needs to move out of our host-agnostic layer (Microsoft.VisualStudio.ProjectSystem.Managed) and into our VS-for-Windows-specific layer (Microsoft.VisualStudio.ProjectSystem.Managed.VS). This commit does the minimum necessary to accomplish that: 1. Any files that actually call the old interfaces are moved as-is to the VS layer. 2. Any further files that depend on those files in 1 are moved as well. 3. Files that refer to the old interfaces only in comments or doc comments have been updated to use phrases like "language service project" instead but have otherwise been left in place. 4. The dependency on the Microsoft.VisualStudio.LanguageServices package has moved from the host-agnostic layer to the VS-specific layer. 5. The `DotNetImportsEnumProvider` has also moved to the VS layer, as it depends on the `VisualStudioWorkspace` type. The host-agnostic layer no longer references the assembly with that type. 6. Unit test files have also been moved around to reflect the new layering. There are a few further dependencies on the Roslyn `Workspace` type in Microsoft.VisualStudio.ProjectSystem.Managed; those will be moved in a future change as we can't count on being able to MEF-import a `Workspace` in VS Code scenarios; it will generally exist only in the language service process. Related work items: #1632862
8 participants
This defines a new brokered service that is pretty similar to IWorkspaceProjectContext, but more friendly for cross-process communication. It's currently implemented atop IWorkspaceProjectContext so that way the behavior is identical otherwise, but hopefully the project system can move to consuming this one soon enough and we can flatten the implementation.