As a framework developer, I would like to enable FxCop Analyzers on the Bot Builder libraries so it is easier to catch common issues during code reviews

[gabog]

I had to do several reviews on community PR and found that several of them miss some design guidelines we follow for the product and it is hard to spot them manually and we sometimes miss many of them (especially in large PRs).

Describe the solution you'd like
I would like to enable FxCopstatic code analyzers on the product so common code issues as shown as warnings during compile time.

This should make our PR review job easier going forward and help us make sure the code is consistent and correct.

Some common rules that I find to be useful are:

• Not disposing IDisposable objects
• Missing ConfigureAwait(false) in async calls
• Not passing CultureInfo to string conversions
• Swallowing exceptions or rethrowing them using throw ex
• Make methods that don't access member variables static to improve perf
• Some naming conventions not caught by StyleCop (like using _ in names)
• Remove unused parameters and unused fields
• and many others that are hard to catch if you are just reading code and don't have super powers 😊

Implementation approach
Applying all these rules to the code base in one go would be too much since we have to fix several violations that are already in the product.
We can fix the non-breaking one but we will need to explicitly exclude breaking ones or the ones that we decide not to adhere to.

I did some research on the use of Directory.Build.props file and found that it is possible to configure it in a hierarchical way.

This will allow us to light up the analyzer one project at a time, fix the issues in different PRs and remove the Directory.Build.props as we move up the hierarchy.

We can event have different rules for the product (under Libraries folder) and leave the Tests folder out if we want.

The Directory.Build.props file would look as follows (I tested this with some PRs I am reviewing and it works as expected):

<Project>
  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.9.4">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

  <Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>

Packages to be updated:

• Adapters folder (This folder just need to be updated to FxCop 3.0.0)
• Microsoft.Bot.Builder.Testing
• Microsoft.Bot.Schema
• Microsoft.Bot.Connector
• Microsoft.Bot.Builder
• Microsoft.Bot.Streaming
• Microsoft.Bot.Builder.Dialogs
• Integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.Core
• Integration/Microsoft.Bot.Builder.Integration.AspNet.Core
• Integration/Microsoft.Bot.Builder.Integration.ApplicationInsights.WebApi
• Integration/Microsoft.Bot.Builder.Integration.AspNet.WebApi
• AI/Microsoft.Bot.Builder.AI.Luis
• AI/Microsoft.Bot.Builder.AI.QnA
• AdaptiveExpressions
• Microsoft.Bot.Builder.Dialogs.Adaptive
• Microsoft.Bot.Builder.Dialogs.Declarative
• Microsoft.Bot.Builder.Dialogs.Debugging
• Microsoft.Bot.Builder.Dialogs.Adaptive.Testing
• Microsoft.Bot.Builder.LanguageGeneration
• Microsoft.Bot.Builder.Dialogs.Adaptive.Teams (the csproj for this one needs to also be updated for warnings as errors and other settings that we updated on the other csproj files)
• Microsoft.Bot.Builder.Azure
• Microsoft.Bot.Builder.ApplicationInsights (no changes were needed here, I didn't add FxCop to it, it will inherit it once I create the buildprops for the libraries project)
• Microsoft.Bot.Configuration
• Microsoft.Bot.Builder.TemplateManager
• Create .dirprops for libraries folder with warnings as errors, FxCop and AsyncAnalyzer and remove specific instances from the projects under libraries.

[enhancement]

[gabog]

@cleemullins , @johnataylor , @tomlm , @sgellock : would like to hear your thoughts on this enhancement, I think it will make our PR review process easier and more consistent.

We will need to invest on fixing existing issues before lighting up across the board but the hierarchical approach to using Directory.Build.props files in multiple levels would allow us to do this one project at a time.

Gabo

[garypretty]

@gabog I think this is a good idea. Anything that can be done to assist with catching those sort of violations earlier and automatically feels like a win.

[tomlm]

I thought we had these turned on already? I know I'm getting warnings on ConfigureAwait() along with a ton of dubious style guidelines. (No trailing spaces...seriously?)

[gabog]

@tomlm, do you have the FxCop add in for VS installed? That may be why you are getting the ConfigureAwait() warnings today (I don't have FxCop and I don't get them).

This approach enables the analyzers at the solution level regardless of what addins you have installed. The Roslyn rules start with CAXXXX. The stylecop ones start with SAXXXX.

We have StyleCop at the solution level in Directory.Build.props today, that's why you are getting the trailing spaces warning. The StyleCop warnings are more about coding style, not perf, security, and things like that (those come in Roslyn).

[garypretty]

Yes, I don't believe I get any warnings about ConfigureAwait right now. I do get the StyleCop rules.

I have to agree about the trailing spaces and a few of the other rules :) on the whole though it's good to have in place.

[cleemullins]

I walked through this in detail with Gabo. What we have today is:

1. The StyleCop Analyzers are enabled on everything. This is good.
2. The Async Usage Analyzers are only on the BotBuilder project, and are now officially deprecated. We need to move off this package, and on to a different one.
3. The FxCop and other Rosyln analyzers are not enabled, and Gabo will begin enabling them for the core libraries.

In terms of approach, we're going to grandfather in the old libraries and try to hold the line on the newer ones, such as the Twilio Adapter.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[gabog]

Reopening for R10