-
Notifications
You must be signed in to change notification settings - Fork 28
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
VSSDK003 flags toolwindows using ToolWindowCreationContext.Unspecified #72
Comments
Thanks for the great write-up of the problem you found and suggestions for the resolution. |
@AArnott Happy to do so! And in my case, even just a small doc update or something would have been useful. Honestly, what was the trickiest part was actually finding any docs saying what should be done; if there was simple step-by-step that was "override these methods, do this", you could easily have a note there which is "you need a non-default constructor to make the analyzer happy", and I wouldn't have given it much of a second thought. |
I think I've come across a similar bug, so rather than opening a new issue, I'll add to this one. The The only downside to those helper classes is that the The PR I linked above contains the full sample code, but the two key parts are:
Under the hood, the |
Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 16.7.0 to 16.7.1. - [Release notes](https://github.com/microsoft/vstest/releases) - [Commits](microsoft/vstest@v16.7.0...v16.7.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bug description
VSSDK003 (use async tool windows) still fires if you're using ToolWindowCreationContext.Unspecified, and doesn't hint at what you should do to make it go away then.
To Reproduce
We had an existing class that defined a tool window, and we moved to the newer analyzers which (correctly) suggested to move to an async tool window. I did my first pass overriding the methods of AsyncPackage, but I couldn't get the diagnostic to go away. I was using ToolWindowCreationContext.Unspecified in InitializeToolWindowAsync to specify that my tool window constructor didn't need to get anything -- at least initially while I was trying to get everything else to work -- but the analyzer was still complaining.
I was able to figure out what was going on by finding the analyzer code and reading it, which quickly revealed the problem. But hey, I work on Roslyn, so that's "easy" for me. 😄
Expected behavior
Once I had overridden the methods in AsyncPackage, the diagnostic would go away. I kept assuming that because I was still getting the diagnostic, I had missed some overload.
Actual behavior
The analyzer is actually looking entirely at the constructor of the ToolWindow to see if there's a non-default constructor. Although the presence of one does indicate it's an async tool window, the absence doesn't necessarily. In any case, I ran around in circles trying to figure out what I was doing wrong.
Suggested behavior
In a perfect world the analyzer could see what was happening and realized it was fine, but that's hard. A good heuristic might be that default constructors are OK if InitializeToolWindowAsync somewhere has a reference to ToolWindowCreationContext.Unspecified. Or, if the analyzer sees a reference to Unspecified, consider reporting a different message that tells you it might be OK, and you should just use a constructor to ensure it's safe.
Or, absent of code changes, just update the docs that if you use Unspecified, the analyzer will still be "wrong". That at least wouldn't have sent me down a path of assuming I'm doing something wrong.
The text was updated successfully, but these errors were encountered: