Fix broken warning (it never appears if any .NET analyzers are present)

[ErikEJ]

fixes #640

[jmezach]

@ErikEJ None of the "default" analyzers really make sense since they are targeted at C# code. So I'm wondering whether we should "reset" the analyzers to an empty list in the Sdk and then adding the analyzers that were explicitly referenced by the project, rather than trying to fix this in the tool?

[ErikEJ]

@jmezach That would be great (if you can help with that) 🙏

[jmezach]

I guess we'll have to figure out where the Analyzers item group is being created. A binary log could probably give us some insight into how that works. From there we can figure out where we need to "reset" it.

[ErikEJ]

@jmezach I tried adding this to the .targets file, but I could not make it work, I guess because of the wildcard (*.dll) - any pro tips?

<!--
  Resolves package references to analyzer libraries by enumerating all package references, resolving their associated Pkg<package-id> property to get the physical
  location of the package and then checking if a that package contains .dll files inside of the analyzers/dotnet/cs folder.
-->
<Target Name="ResolveAnalyzerReferences">
  <ItemGroup>
    <!-- Resolve all package references to their physical location first -->
    <_ResolvedAnalyzerReference Include="%(PackageReference.Identity)">
      <!-- Determine technical name of package (ie. Foo.Bar.Database -> Foo_Bar_Database) -->
      <PackageName>@(PackageReference->'%(Identity)'->Replace('.', '_'))</PackageName>
      <!-- Prepend Pkg to technical name from above and resolve variable (ie. Foo_Bar_Database -> %home%/.nuget/packages/foo.bar.database/<version> -->
      <PhysicalLocation>$(Pkg%(_ResolvedAnalyzerReference.PackageName))</PhysicalLocation>
      <!--
        If no Pkg<package-id> property is available, fall back to deriving the physical location from several other properties.
        This isn't guaranteed to be correct, particularly when floating versions are used, but will successfully resolve most of the time.
      -->
      <PhysicalLocation Condition="'%(_ResolvedAnalyzerReference.PhysicalLocation)'==''">$([System.String]::new('$(NuGetPackageRoot)%(PackageReference.Identity)/%(PackageReference.Version)').ToLower())</PhysicalLocation>

      <AnalyzerFile>%(_ResolvedAnalyzerReference.PhysicalLocation)/analyzers/dotnet/cs/*.dll</AnalyzerFile>
      <AnalyzerFile Condition="Exists('%(_ResolvedAnalyzerReference.PhysicalLocation)/analyzers/dotnet/cs/*.dll')">%(_ResolvedAnalyzerReference.PhysicalLocation)/analyzers/dotnet/cs/*.dll</AnalyzerFile>
    </_ResolvedAnalyzerReference>

    <!-- Build a list of .dll references that include .dll files in the analyzers/dotnet/cs folder -->
    <AnalyzerReference Include="@(_ResolvedAnalyzerReference)" Condition="Exists(%(AnalyzerFile))" />
  </ItemGroup>

  <Message Importance="normal" Text="Resolved analyzer references: @(AnalyzerReference)" />
</Target>

[jmezach]

Hmm, doesn't the .NET SDK already do this out of the box? When you add the rules packages as a <PackageReference> we are already getting the paths to the correct assemblies into the DacpacTool right?

[ErikEJ]

It does, issue is the array contains all analyzers not just ours.

[jmezach]

Yeah, that's why I was wondering how the other analyzers end-up in that array. Have you tried generating a binary log?

[ErikEJ]

In my case they are injected via a Directory.Build.Props file as PackageReferences

[ErikEJ]

I can try generating a binary log...

[ErikEJ]

I have this in a Directory.Build.Props file in my solution tree, I am sure that is why the additional analyzers are added:

 <ItemGroup Label="Code Analyzers">
    <PackageReference Include="AsyncFixer" Version="1.6.0" PrivateAssets="All" />
    <PackageReference Include="Meziantou.Analyzer" Version="2.0.169" PrivateAssets="All" />
    <PackageReference Include="SecurityCodeScan.VS2019" Version="5.6.7" PrivateAssets="All" />
    <PackageReference Include="StyleCop.Analyzers" Version="1.2.0-beta.556" PrivateAssets="All" />
    <PackageReference Include="SonarAnalyzer.CSharp" Version="9.32.0.97167" PrivateAssets="All" />
 </ItemGroup>

[ErikEJ]

I have done some digging. We could use the MSBuild property $(ImportDirectoryBuildProps) = false in the SDK and maybe also add to.the template to make it more visible?

And then revert the check in the CLI tool?

[ErikEJ]

ImportDirectoryBuildProps had no effect...

[jmezach]

I guess our main question here is how we are going to differentiate between C# analyzers and SQL analyzers? I don't think we have a good way to determine that currently. Perhaps if we were to move the analyzers into the dotnet/sql folder within the package. Then they wouldn't be picked up by the built-in logic, but we could write additional logic in the SDK to include them for SQL projects?

[ErikEJ]

@jmezach If we do that then the analyzers I publish cannot be used with Microsoft.Build.Sql, and I would have to publish two sets of packages or similar. I prefer to avoid that.

[jmezach]

Then how does Microsoft.Build.Sql differentiate between the two?

[ErikEJ]

Not sure that is does!
(From .nuget\packages\microsoft.sqlserver.dacfx\162.4.92\lib\netstandard2.1\Microsoft.Data.Tools.Schema.SqlTasks.targets

  <SqlStaticCodeAnalysisTask CodeAnalysisRules="$(SqlCodeAnalysisRules)"
                             SqlCodeAnalysisAssemblyPaths="$(SqlCodeAnalysisAssemblyPaths);@(Analyzer);"

And the DacFX code (disassembled)

private bool ExecuteLoadServiceStep()
    {
      CodeAnalysisRuleSettings ruleSettings = this.CreateRuleSettings();
      CodeAnalysisServiceSettings settings = new CodeAnalysisServiceSettings()
      {
        CodeAnalysisSucceededFile = this.StaticCodeAnalysisSucceededFile,
        ResultsFile = this.ResultsFile,
        RuleSettings = ruleSettings,
        AssemblyLookupPath = this.SqlCodeAnalysisAssemblyPaths
      };
      if (this._problemSuppressor != null)
        settings.ShouldSuppressProblem = this._problemSuppressor.ShouldSuppressProblem;
      this._service = new CodeAnalysisServiceFactory().CreateAnalysisService(this._publicModel, settings);
      return this._service != null;
    }

And it looks like the implementation just loads all listed dll files.

[jmezach]

Hmm, then why do we care? Only because we want to notify users of the fact that they don't have any rules configured? Could we get the set of applied rules from DacFx somehow maybe?

[ErikEJ]

I care because this is a breaking change, previously all three rules sets were enabled, now the only set that is enabled is the Microsoft set if you upgrade from 2.8 to 2.9. And I think this is the best way to convey the breaking change, I think very few users read release notes. Could also combine this PR with a clearer message in the readme perhaps?

[jmezach]

Looks like the service object returned from the CreateAnalysisService() method has a GetRules() method that should return the set of loaded rules. Can we base our warning on that?

[ErikEJ]

@jmezach Let me give that a try!

[ErikEJ]

@jmezach Thanks, this looks a lot better!

[jmezach]

Cool, this looks good to me

[ErikEJ]

@jmezach @jeffrosenberg added a small readme note

[ErikEJ]

@jeffrosenberg Hope you had a nice holiday. Would be grateful if you could have a look at this 🙏