Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a non-net472 alternative to ThrowIfNotOnUIThread to satisfy VSTHRD010 #1025

Open
matteo-prosperi opened this issue Apr 20, 2022 · 3 comments

Comments

@matteo-prosperi
Copy link
Member

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.

@gundermanc
Copy link
Member

gundermanc commented Apr 20, 2022

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.

@AArnott
Copy link
Member

AArnott commented May 5, 2022

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
Copy link
Member

AArnott commented May 5, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants