vs-threading icon indicating copy to clipboard operation
vs-threading copied to clipboard

Provide a non-net472 alternative to ThrowIfNotOnUIThread to satisfy VSTHRD010

Open matteo-prosperi opened this issue 2 years ago • 3 comments

Is your feature request related to a problem? Please describe.

VSTHRD010 asks to use ThreadHelper.ThrowIfNotOnUIThread() for sync methods. ThreadHelper is only available for the net472 target. Microsoft.VisualStudio.Threading is available for other targets as well.

Describe the solution you'd like

Microsoft.VisualStudio.Threading should provide a ThrowIfNotOnUIThread() method for all platforms for which VSTHRD010 is generated. The VSTHRD010 error message should be updated to instruct using the ThrowIfNotOnUIThread() provided by Microsoft.VisualStudio.Threading.

matteo-prosperi avatar Apr 20 '22 16:04 matteo-prosperi

One place where this would be helpful is in IDE components or extensions that dual target VS for Windows and VS for Mac.

ThreadHelper is only available for the net472 target.

Note that some of these components build .net472. The analyzers currently recommend that devs take a dependency on ThreadHelper, which doesn't exist on Mac. It's difficult explaining to those new to the area to ignore the analyzer.

I'd recommend:

  • JTF.ThrowIfNotOnUIThread() be introduced, potentially as an extension method.
  • Analyzer be updated to recommend this when a JTF instance is in scope in the current class or method.

gundermanc avatar Apr 20 '22 16:04 gundermanc

VSTHRD010 doesn't ask for ThreadHelper at all, does it? It's VS-agnostic and ThreadHelper comes from a VS SDK assembly. The doc for the analyzer suggests that in a sample fix, but the automated code fix you get for VSTHRD010 diagnostics is driven by AdditionalFiles such as this which is VS specific, as it comes from the VSSDK-Analyzers.

So, if you have a non-VS app and want to use this analyzer, that's totally fine. And if you provide your own AdditionalFile like we do in vssdk-analyzer, it will even encourage using your own API.

You could even write your own JTF extension method that calls an app-specific Throw method, and use AdditionalFiles to direct user code to use your extension method.

But yes, it probably makes sense to define the method within this library because it would make app-agnostic code easier to write.

AArnott avatar May 05 '22 22:05 AArnott

I would be inclined from an API standpoint to add the method to JoinableTaskContext, since it knows what the UI thread is. But from a usability standpoint, folks don't usually deal with that class, and they switch to the main thread with JoinableTaskFactory, so adding the asserting method on that class as well is probably more discoverable.

AArnott avatar May 05 '22 22:05 AArnott