Remove RoslynServiceExtensions.GetServiceAsync #77799
Conversation
Fixes dotnet#73636 Remove the RoslynServiceExtensions.GetServiceAsync and instead encourage use of the standard IAsyncServiceProvider* methods. The VS implementation of these methods has been changed to better understand which interfaces require a main thread switch for the cast, as opposed to the roslyn implementations which blindly did a main thread switch.
ghost
added
Area-IDE
untriaged
| M:Microsoft.VisualStudio.Shell.ServiceExtensions.GetService``2(System.IServiceProvider); Use RoslynServiceExtensions instead. This extension internally relies on ThreadHelper, which is incompatible with testing. | ||
| M:Microsoft.VisualStudio.Shell.ServiceExtensions.GetService``2(System.IServiceProvider,System.Boolean); Use RoslynServiceExtensions instead. This extension internally relies on ThreadHelper, which is incompatible with testing |
There was a problem hiding this comment.
Should these be banned too, or the message updated for it?
There was a problem hiding this comment.
I didn't look into these methods, so I'd rather not remove or change their message
| @@ -34,7 +34,7 @@ public StubVsServiceExporter( | |||
| [Import(typeof(SAsyncServiceProvider))] IAsyncServiceProvider2 asyncServiceProvider, | |||
There was a problem hiding this comment.
@davkean's bug specifically calls out IAsyncServiceProvider3, do we need to move to those newer interface here or are we good? His comment makes me think all the methods will eventually call the right thing.
There was a problem hiding this comment.
My understanding is that the implementations on each of the interfaces will do the right thing, and that there is no need to explicitly use the "3" interface.
| await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(_threadingContext.DisposalToken); | ||
| shellService.AdviseSolutionEvents(this, out _); | ||
| shellService!.AdviseSolutionEvents(this, out _); |
There was a problem hiding this comment.
I wonder if we should be putting the ! on the fetching so that way the nullable suppression is closer to the place where the throwOnFailure: true is. Or it's unfortunate if we can't get that nullable annotated in some way to make this simpler.
There was a problem hiding this comment.
Looks like there is a version of GetServiceAsync that only takes in a CancellationToken and assumes throwOnFailure is true and has null annotated the return values. Switched over to use that when possible.
…ust takes in a CancellationToken as that is nullable annotated to indicate the return value isn't null
Fixes #73636
Remove the RoslynServiceExtensions.GetServiceAsync and instead encourage use of the standard IAsyncServiceProvider* methods. The VS implementation of these methods has been changed to better understand which interfaces require a main thread switch for the cast, as opposed to the roslyn implementations which blindly did a main thread switch.