Add service which exists to tell OOP about solution events #64127

CyrusNajmabadi · 2022-09-19T20:17:55Z

Note: this is going into a feature branch, as i expect several more PRs necessary here.

The general premise here is that we are moving away from SolutionCrawler and want to have no 1st class roslyn features using it. One snag in this is that Unit-Testing still uses solution-crawler and needs more time than we would prefer to fully move off of it.

To support their longer migration path, while also getting us off of Solution-Crawler, we intend to transfer ownership fo Solution-Crawler to the unit testing team for them to maintain for their single use case, until they've moved onto a new approach for doing test-discovery.

The SolutionCrawler code (ne UnitTestingSolutionCrawler) will live in the Unit-Testing EA layer and will be launched/maintained by them. We can def assist with it at times. But the intent is for this to be entirely legacy, with only tiny changes put into it while UnitTesting moves to a modern solution.

To assist this though, the UNitTestingSolutionCrawler does need to know what's going on in the Workspace itself. As we intend to not have a Workspace in OOP, and we def won't have a RemoteWorkspace issuing WorkspaceEvents in OOP, we need a solution that will allow the new UnitTestingSolutionCrawler to know what's going on so it can decide what crawling steps to take.

To taht end, this PR adds a new service+oop combo that exists purely to hear about host Workspace-Events and remote them to OOP for OOP clients to respond to. This combo is lightweight and doesn't do anything but just send the events over. On the OOP side then, UnitTesting will have a listener for this that then forwards the events to the UnitTestingSolutionCrawler.

This still means we have some code for this scenarios owned and operated by roslyn. HOwever:

This event-remoting code is extremely trivial and required no complex logic or state management.
All complex logic and state management around crawling will be contained entirely in the UnitTesting EA lib and will be owned by them.
THis will let us get off of having a RemoteWorkspace entirely. All OOP calls will simply be functional service calls that run off of Solution snapshots.
This new code can also go away once UnitTesting moves to a new system. For example, one where they "pull" on their test discoverer, or maybe one where they use Source-GEnerators to emit the discovered test information.

CyrusNajmabadi · 2022-09-19T20:19:46Z

src/EditorFeatures/Core/SolutionEvents/HostSolutionEventsWorkspaceEventListener.cs

+namespace Microsoft.CodeAnalysis.SolutionEvents
+{
+    [ExportEventListener(WellKnownEventListeners.Workspace, WorkspaceKind.Host), Shared]
+    internal sealed partial class HostSolutionEventsWorkspaceEventListener : IEventListener<object>


this is the part that runs in VS and connects to the actual VS workspace and hears about changes to it. It is very similar in that regard to the equivalent HostSolutionCrawlerWorkspaceEventListener type.

CyrusNajmabadi · 2022-09-19T20:21:14Z

src/EditorFeatures/Core/SolutionEvents/HostSolutionEventsWorkspaceEventListener.cs

+            SolutionEvent ev,
+            CancellationToken cancellationToken)
+        {
+            if (ev.DocumentOpenArgs != null)


this method/switch corresponds to equivalent code in WorkCoordinator which suscribes to and listens for these events off of hte workspace. Once we move WorkCoordinator to EA layer for unit testing, it will just listen for tehse new events.

CyrusNajmabadi · 2022-09-19T20:22:43Z

src/Features/Core/Portable/SolutionEvents/IRemoteSolutionEventsAggregationService.cs

+        ValueTask OnSolutionChangedAsync(Checksum oldSolutionChecksum, Checksum newSolutionChecksum, CancellationToken cancellationToken);
+        ValueTask OnProjectChangedAsync(Checksum oldSolutionChecksum, Checksum newSolutionChecksum, ProjectId projectId, CancellationToken cancellationToken);
+        ValueTask OnDocumentChangedAsync(Checksum oldSolutionChecksum, Checksum newSolutionChecksum, DocumentId documentId, CancellationToken cancellationToken);
+    }


this defines the api on the remote side which hears about the workspace events and will dispatch them to different listeners.

CyrusNajmabadi · 2022-09-19T20:23:19Z

src/Features/Core/Portable/SolutionEvents/ISolutionEventsAggregationService.cs

+    }
+
+    [ExportWorkspaceService(typeof(ISolutionEventsAggregationService)), Shared]
+    internal class DefaultSolutionEventsAggregationService : ISolutionEventsAggregationService


aggregation service that pulls in the individual listener services and dispatches to all of them.

src/Features/Core/Portable/SolutionEvents/ISolutionEventsService.cs

tmat · 2022-09-19T23:02:42Z

...rkspaces/Remote/ServiceHub/Services/SolutionEvents/RemoteSolutionEventsAggregationService.cs

+
+namespace Microsoft.CodeAnalysis.Remote
+{
+    internal sealed class RemoteSolutionEventsAggregationService : BrokeredServiceBase, IRemoteSolutionEventsAggregationService


RemoteSolutionEventsAggregationService

Nit: Consider renaming the containing dir to SolutionEventsAggregation to match the service name (w/o Remote- prefix and -Service suffix).

I personally think this is fine as teh general component is the 'solution events' component. This is just the remote side of one particular interface of that component. keeping the component name (i.e. the folder) consistent across all the projects this touches is desirable to me (and matches what we do with our other components).

tmat · 2022-09-19T23:08:17Z

src/Workspaces/Remote/Core/ServiceDescriptors.cs

@@ -24,6 +24,7 @@
 using Microsoft.CodeAnalysis.NavigationBar;
 using Microsoft.CodeAnalysis.Rename;
 using Microsoft.CodeAnalysis.Shared.TestHooks;
+using Microsoft.CodeAnalysis.SolutionEvents;


SolutionEvents

Do we need a new namespace? Workspace event related types are in Microsoft.CodeAnalysis

I'd like a new namespace here to keep this isolated.

tmat · 2022-09-19T23:13:19Z

src/Workspaces/Core/Portable/SolutionCrawler/InvocationReasons.cs


 namespace Microsoft.CodeAnalysis.SolutionCrawler
 {
+    [DataContract]
    internal partial struct InvocationReasons : IEnumerable<string>


InvocationReasons

Can this simply be a flags enum?

the goal here is not change anything, but to use the existing concepts that solution-crawler is already doing.

In-proc we seem to have pre-allocated instances of this, but OOP deserialization will allocate a new one each call.

ok. will move to enum in followup :)

tmat · 2022-09-19T23:14:18Z

src/Features/Core/Portable/SolutionEvents/ISolutionEventsAggregationService.cs

+
+namespace Microsoft.CodeAnalysis.SolutionEvents
+{
+    internal interface ISolutionEventsAggregationService : IWorkspaceService


ISolutionEventsAggregationService

ILegacySolutionEventAggregator?

sure, i can add Legacy to the name.

tmat · 2022-09-19T23:15:42Z

src/Workspaces/Core/Portable/SolutionCrawler/InvocationReasons.cs


 namespace Microsoft.CodeAnalysis.SolutionCrawler
 {
+    [DataContract]
    internal partial struct InvocationReasons : IEnumerable<string>


Move out of SolutionCrawler namespace to avoid dependency of the new interfaces on SolutionCrawler

the next PR does that.

CyrusNajmabadi · 2022-09-20T00:08:50Z

@tmat i can roll the feedback into the next PR. note that this is all going into a feature branch currently.

CyrusNajmabadi added 14 commits September 19, 2022 10:35

Initial stubs to report solution events to OOP

5f6c752

In progress

c9bb965

in progress

5f52883

in progress

813dc11

in progress

3375fe8

Make static

5da7e9e

In progress

3566e8f

Call into notification service

5def02f

IN progress

c0b1085

Renames

e5e1bca

Renames

77e8315

remote side

f358af8

make workspace service

f53250f

End to end

97f12e8

CyrusNajmabadi requested review from tmat and shyamnamboodiripad September 19, 2022 20:17

CyrusNajmabadi requested review from a team as code owners September 19, 2022 20:17

dotnet-issue-labeler bot added the Area-IDE label Sep 19, 2022

CyrusNajmabadi commented Sep 19, 2022

View reviewed changes

CyrusNajmabadi added 3 commits September 19, 2022 13:21

Formatting

f9b59c3

Delete

cfc5ab6

Cleanup

360284f

CyrusNajmabadi commented Sep 19, 2022

View reviewed changes

src/Features/Core/Portable/SolutionEvents/ISolutionEventsService.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits September 19, 2022 13:24

nrt

3f11d77

lint

2f120fb

tmat reviewed Sep 19, 2022

View reviewed changes

CyrusNajmabadi requested a review from tmat September 19, 2022 23:11

tmat reviewed Sep 19, 2022

View reviewed changes

CyrusNajmabadi requested a review from tmat September 20, 2022 00:08

tmat approved these changes Sep 20, 2022

View reviewed changes

CyrusNajmabadi merged commit e5d1e48 into dotnet:features/solutionEvents Sep 20, 2022

CyrusNajmabadi deleted the features/solutionEvents branch September 20, 2022 04:49

allisonchou mentioned this pull request Oct 11, 2022

[Automated] PRs inserted in VS build main-33011.173 #64652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service which exists to tell OOP about solution events #64127

Add service which exists to tell OOP about solution events #64127

CyrusNajmabadi commented Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

tmat Sep 19, 2022

CyrusNajmabadi Sep 19, 2022

CyrusNajmabadi commented Sep 20, 2022

Add service which exists to tell OOP about solution events #64127

Add service which exists to tell OOP about solution events #64127

Conversation

CyrusNajmabadi commented Sep 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 20, 2022