Best practice analyzers

[madskristensen]

I think it might be helpful to create an analyzer package that checks for best practices and other patterns that make it easier to write extensions. Here are some ideas:

1. Flag the use of Task.Forget() and Task.FileAndForget() to use the Toolkit's Task.ForgetAndLogOnFailure() instead
2. Don't use IVsOutputWindowPane.OutputStringThreadSafe() but use OutputString() instead

What other analyzers would be helpful?

[reduckted]

Love it! Here's a few I can think of:

1. Inherit from ToolkitPackage instead of AsyncPackage.
2. Detect a ToolWindowBase class that isn't initialized in a package.
3. Detect a command that isn't initialized in a package.

[yannduran]

@madskristensen

Huh?? Don't use OutputStringThreadSafe()? I learned of that method through you and/or your code! I didn't even know it existed, and I started using it instead of OutputString() because of that.

[AArnott]

(edited) WARNING the below advice has the two methods mixed up. The thread-safety comment below is correct, but see my later comment for the fix in method names.

Ya, don't use OutputStringThreadSafe. The name doesn't even make sense because the method appears on an STA COM object, so both that method and its shorter-named sibling OutputString will only execute on the main thread, and calling the object on another thread will result in a blocking RPC call which can deadlock. So the object is not free-threaded, and both are "thread safe" in the sense that they can only execute on the main thread.

The difference between these two methods is that OutputStringThreadSafe runs an open message pump (briefly) after printing the text to the output window whereas OutputString does not. We don't want open message pumps running on top of this stack generally, so the guidance is to never use OutputStringThreadSafe.

[yannduran]

Thanks Andrew!

[AArnott]

I got the methods above backwards! OutputString is the one that runs the wide-open (UI input processing) message pump and OutputStringThreadSafe is the preferred one because the message pump it runs is more limited (and thus less prone to causing unwanted side effects). Some implementations of panes implement an interface that avoids the pump altogether.

The best code would be something like this:

void Write(IVsOutputWindowPane pane, string message)
{
  if (pane is IVsOutputWindowPaneNoPump nopump)
  {
    nopump.OutputStringNoPump(message);
  }
  else
  {
    ErrorHandler.ThrowOnFailure(pane.OutputStringThreadSafe(message));
  }
}

[madskristensen]

Thanks @AArnott. I'll make the change

[yannduran]

[bluetarpmedia]

Actually I believe it was Mads who originally wrote the extension method named FireAndForget and he renamed to ForgetAndLogOnFailure after the code review in #24.

My pull request only added an overload.

[madskristensen]

It was @AArnott that suggested renaming it to more accurately reflect what the method was doing. It's not just "forgetting" a task, it handles the errors and logs to the output window. I like the idea of adding an optional parameter that allows you to disable the output window logging. I'm personally OK with the name FireAndForget(), but there are various opinions on this. Perhaps we can come up with a list of names to choose from

[StevenBonePgh]

@yannduran I think the appropriate thing to do if you are unhappy with the code is to raise a PR and it can be discussed there. There are now three threads discussing the same thing. For this discussion item in particular, I believe this is off-topic.

[reduckted]

As @StevenBonePgh says, renaming that method is off-topic for this discussion. If you think it should be renamed, please create an issue.

[yannduran]

@StevenBonePgh , @reduckted

This was Mads's issue, and he even engaged in the discussion. It really should have been Mads who suggested that my comment was off-topic here.

But you're right I guess, so I've created this issue and deleted my comments here, and I suggest you do the same. If you can point me to where else you're saying my comments are off-topic, I'll do the same there. Please feel free to ping me privately.

[bluetarpmedia]

Yeah this would be great!

Coincidentally I've actually been working on some VSIX threading analyzers. Despite its name, the Microsoft.VisualStudio.Threading library is general-purpose and used by a variety of applications, not just Visual Studio. So its analyzers (as awesome as they are) are also quite general in nature and don't include all specific requirements for Visual Studio extensions.

My repo is still private at the moment but I was intending to make it public. @madskristensen I've just sent you a link to it if you'd like to take a look. I'd be happy to merge my analyzers into a new project, or you're welcome to take my project and rename it as a starting point.

[AArnott]

@bluetarpmedia Consider contributing your analyzers to the microsoft/vssdk-analyzers repo. That brings in the vs-threading analyzers and adds AdditionalFiles to make them more VS SDK aware, and then adds a few more analyzers too.

[bluetarpmedia]

Will do!

[yannduran]

Some suggestions:

• Detect a ToolWindow that doesn't have both a ProvideOptionPage and ProvideProfile attribute on the package
• Detect a ToolWindow that doesn't inherit from ToolWindowBase and suggest that it does so (for older code)
• Detect a PackageRegistration attribute that doesn't have AllowsBackgroundLoading = true (for older code)
  • I've also been meaning to write a pull request that adds this directly to ToolkitPackage as I can't think of when it wouldn't be required
  • possibly also [ProvideMenuResource("Menus.ctmenu", 1)] (unless someone knows of a use case that would not want it?)
• Detect await base.InitializeAsync(cancellationToken, progress) in InitializeAsync overrides (I've checked, and it actually does nothing, so there's no need to call it)
• Detect when Vsix.Name etc isn't being used instead of hard-coded strings
• Detect GetService(TService) as TInterface instead of using VS.GetServiceAsync<TService, TInterface> (or one of the ones added to VS)
• Detect ThreadHelper.xxx calls and suggest ToolkitThreadHelper.xxx instead
• Detect calls to WPF MessageBox and suggest Notificatons.MessageBox instead

[AArnott]

Don't use IVsOutputWindowPane.OutputStringThreadSafe() but use OutputString() instead

This does not require its own analyzer. Your analyzer package can depend on https://www.nuget.org/packages/Microsoft.CodeAnalysis.BannedApiAnalyzers/ and then add the method to avoid to the AdditionalFiles.
And we should do this in vssdk-analyzers as it transcends this toolkit.

[AArnott]

microsoft/VSSDK-Analyzers#135

[reduckted]

Regarding where this analyzer project lives, from past experience, having the analyzers project in the same repository as the toolkit would be much easier to manage.

If you make changes to the toolkit that requires changes to an analyzer, it can all be done in one pull request. Plus the version of the toolkit and analyzer can stay in sync.

[yannduran]

Absolutely!

[reduckted]

I've just created #172 which adds a code analysis project and an initial code analyzer. The code analyzer is one that wasn't mentioned here, but I thought it would be really useful and a nice simple one to start things off. It detects calls to any VS.Services....Async() methods that return a Task<object> and produces a warning if it hasn't been cast to the correct type, and (more importantly) provides a code fix to add the cast.