Multi-Targeting Roslyn analyzers with support for pre-SDK format projects

[Tornhoof]

xUnit recently added for their xunit.analyzers nuget package the folder 'analyzers/dotnet/cs' with a roslyn 3.11 analyzer in parallel to all the other versioned roslyn analyzers (analyzers/dotnet/roslyn4.4/cs for example). Otherwise the roslyn analyzer would not work in pre-SDK format projects anymore. The outcome of that is, that while the analyzer now works in pre-SDK format projects, the build process is several times slower per test project as apparently the compilation server needs to be restarted, because it loads the wrong dlls on a VS 17.10 installation.

CompilerServer: server failed - server rejected the request due to analyzer / generator issues 'analyzer assembly 'C:\Users\BuildUser\.nuget\packages\xunit.analyzers\1.14.0\analyzers\dotnet\roslyn4.8\cs\xunit.analyzers.dll' has MVID '3e2596a5-a0c9-41a0-b5cf-9fe156e7ea5f' 
  but loaded assembly 'C:\Users\BuildUser\AppData\Local\Temp\VBCSCompiler\AnalyzerAssemblyLoader\c7412f87926d47a2a5eb7eda0608e533\10\xunit.analyzers.dll' has MVID 'b1a42a47-0126-4f64-b679-249d385e60a7', 
  analyzer assembly 'C:\Users\BuildUser\.nuget\packages\xunit.analyzers\1.14.0\analyzers\dotnet\roslyn4.8\cs\xunit.analyzers.fixes.dll' has MVID '2dec3629-615b-446a-b634-e65e8cb5dffb' 
  but loaded assembly 'C:\Users\BuildUser\AppData\Local\Temp\VBCSCompiler\AnalyzerAssemblyLoader\c7412f87926d47a2a5eb7eda0608e533\11\xunit.analyzers.fixes.dll' has MVID 'bfdb98a4-35e9-4e80-97d9-33b9157e3ce3'' - ec95a20c-48f8-44d0-a766-e8629738d172

I looked through the comments and solutions in #20355, but the examples linked there, e.g. the one for refit by @sharwell or by @eerhardt for S.T.J apply for the other way round, i.e. preventing an older SDK Version from loading a 'too new' src gen, I tried that approach anyway, but couldn't get it working, the analyzers node in the solution tree in VS is empty, admittingly I do not understand enough about MsBuild targets ;)

This is sufficently annoying, as per test project the build now takes roughly 4s longer, in my solution the build went up from 50s to roughly 150s (which fits, there are around ~30 test projects).

The original issue in xUnit is https://github.com/xunit/xunit/issues/2943

Repro: As for repro, a pretty much empty Test Project in VS does the trick

using Xunit;

namespace TestProject1
{
    public class UnitTest1
    {
        [Fact]
        public void Test1()
        {

        }
    }
}

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeCoverage" Version="17.10.0" />
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.10.0" />
    <PackageReference Include="xunit" Version="2.8.1" />
    <PackageReference Include="xunit.assert" Version="2.8.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.8.1">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>
</Project>

Then run from command line:

• dotnet restore
• msbuild TestProject1.sln /p:configuration="release"

The output will now contain the compiler server error message, at the end of step 'CoreCompile' You also see a noticeable pause after the error message. If you downgrade to 2.8.0, both the error message and pause are gone.

[baronfel]

@jaredpar is this something that you'd need to look at? I think the SDK has some influence on what analyzer DLLs are loaded for a given build, so it could be a bug on our side as well, but IIRC you usually like to take a poke at analyzer-loading-related issues first.

[jaredpar]

The good news is that I'm glad I invested in making the consistency check error message clearer cause it really makes this situation easier to understand. The bad news is that it won't be in the compiler until 17.11. I'll run this on a 17.11 build tomorrow and see what the message is.

The very likely cause though is that there are two xunit.analyzers.dll instances in the build graph now with the same identity (assembly name + version) but different MVID. The compiler server is properly shutting down because it can't load both of them into the same process.

Haven't looked at the build yet but I'm guessing one of these package is bringing along a copy of xunit.analyzers.dll which is conflicting with the official one.

[jaredpar]

Can see from the binlog here that the xunit.analyzers.dll and xunit.analyzers.fixes.dll is being passed twice to the compiler. That leads to an immediate conflict and will shut down the server.

This is being added by the ResolvePackageAssets target

I think this is a bug in how xunit is packaging here. I believe that rather than putting the fallback DLLs in dotnet/cs they should be put into dotnet/cs/roslyn3.3. At the same time though ... that strategy doesn't always work because ResolveNuGetPackageAssets doesn't have the same logic as ResolvePackageAssets.

At the compiler layer we can't do operations like de-dupe that needs to be done at a higher level.

[Tornhoof]

they should be put into dotnet/cs/roslyn3.3.

Yeah, then the analyzer does not work in the old csproj (XML) format, that was the initial reason why Brad added the 'dotnet/cs' folder. I tried a few variations of the same theme, incl. a 'roslyn1.0' folder (read that in the original issue). No variation, I tried, except Brad's original method, worked with the old project format.

[baronfel]

Is there a binlog available of 'the older format' and its results? I'd like to diff the behaviors here. It sounds like the SDK itself might need to add some compat/fallback behavior compensation for packages like these.

[Tornhoof]

I don't have one readily available, I can produce one as soon as I'm back on a PC.

[jaredpar]

It sounds like the SDK itself might need to add some compat/fallback behavior compensation for packages like these.

The SDK could do this. Essentially recognize that when there is the following pattern:

dotnet/cs
  - analyzer.dll
  - roslyn4.4/analyzer.dll

Then don't auto add the dotnet/cs/analyzer.dll because it's just trying to compensate for behavior of ResolveNuGetPackageAssets.

That approach though feels like we're just digging the hole deeper. Essentially we have a behavior gap between ResolvePackageAssets and ResolveNuGetPackageAssets that leads to confusion. To compensate we're adding more differentiation. Should we consider instead fixing ResolveNuGetPackageAssets?

[baronfel]

Because I always forget this, that target + Task implementation live over at https://github.com/dotnet/NuGet.BuildTasks.

[eerhardt]

I think this is a bug in how xunit is packaging here. I believe that rather than putting the fallback DLLs in dotnet/cs they should be put into dotnet/cs/roslyn3.3.

Correct. From https://github.com/dotnet/sdk/issues/20355 Additional rules:

When a package contains analyzer assets both inside a roslyn{version} folder and outside, ex. directly under analyzers\dotnet\cs\analyzer.dll, both assets will be selected. The reasoning is that the assets outside of roslyn{version} folders are considered to work everywhere. Existing analyzer resolution rules add both assets inside and outside {language} folders, if present. This follows the same reasoning. If a NuGet package wants to express a component that should be used when none of the roslyn{version} folders apply, it can add that asset to roslyn1.0.

[Tornhoof]

The 'roslyn1.0' trick does not appear to work for the old csproj format.

[eerhardt]

The way we made "old csproj format" projects work for dotnet/runtime packages is to add an MSBuild targets file in the buildTransitive folder of the NuGet package that does:

• Checks for SupportsRoslynComponentVersioning. If the current project doesn't support the feature (i.e. it is an "old project format"):
  • Removes all but the "fallback" targetted analyzer (in our case it was roslyn3.11)
• Adds a switch to completely remove all the analyzers, in case someone needs to turn them off for whatever reason.

This is what it looks like for the System.Text.Json nuget package:

<Project>
  <Target Name="_System_Text_JsonGatherAnalyzers">

    <ItemGroup>
      <_System_Text_JsonAnalyzer Include="@(Analyzer)" Condition="'%(Analyzer.NuGetPackageId)' == 'System.Text.Json'" />
    </ItemGroup>
  </Target>

  <Target Name="_System_Text_JsonAnalyzerMultiTargeting" 
          Condition="'$(SupportsRoslynComponentVersioning)' != 'true'" 
          AfterTargets="ResolvePackageDependenciesForBuild;ResolveNuGetPackageAssets"
          DependsOnTargets="_System_Text_JsonGatherAnalyzers">

    <ItemGroup>
      <!-- Remove our analyzers targeting roslyn4.x -->
      <Analyzer Remove="@(_System_Text_JsonAnalyzer)"
                Condition="$([System.String]::Copy('%(_System_Text_JsonAnalyzer.Identity)').IndexOf('roslyn4')) &gt;= 0"/>
    </ItemGroup>
  </Target>

  <Target Name="_System_Text_JsonRemoveAnalyzers" 
          Condition="'$(DisableSystemTextJsonSourceGenerator)' == 'true'"
          AfterTargets="ResolvePackageDependenciesForBuild;ResolveNuGetPackageAssets"
          DependsOnTargets="_System_Text_JsonGatherAnalyzers">

    <!-- Remove all our analyzers -->
    <ItemGroup>
      <Analyzer Remove="@(_System_Text_JsonAnalyzer)" />
    </ItemGroup>
  </Target>
</Project>

https://github.com/dotnet/NuGet.BuildTasks/issues/152 is tracking supporting it in the "old project" NuGet sources, but that hasn't received much traction.

[Tornhoof]

Yeah, i tried to adapt that one, but failed, as written in the original Post. All the examples I could find were removing 'too new', not the inverse, but as stated, I'm not really proficient with msbuild syntax

[jaredpar]

The way we made "old csproj format" projects work for dotnet/runtime packages is to add an MSBuild targets file in the buildTransitive folder of the NuGet package that does:

I appreciate the work put into getting that working but it feels like patching over the problem. Is there a reason we didn't push the issue with ResolveNuGetPackageAssets to have the same support? Caues right now it seems like multiple people are working around the same issue.

[eerhardt]

Is there a reason we didn't push the issue with ResolveNuGetPackageAssets to have the same support?

I believe it was a combination of:

• resource constraints
• the hope that more people will move to SDK style projects, which makes it less of a priority

Do we know who owns https://github.com/dotnet/NuGet.BuildTasks/?

[eerhardt]

Yeah, i tried to adapt that one, but failed, as written in the original Post. All the examples I could find were removing 'too new', not the inverse, but as stated, I'm not really proficient with msbuild syntax

If you can send me a binlog of what you tried, I can try to spot the problem.

I don't really understand the "All the examples I could find were removing 'too new', not the inverse" comment. If you are in an "old format" project, how do you know what version of Roslyn the project is using? That's why you need to load the oldest one possible - because you don't know which one will work.

[Tornhoof]

I don't really understand the "All the examples I could find were removing 'too new', not the inverse" comment.

I apologize if I was too obtuse. The 'dotnet/cs' path nuspec entry works with the old format, no other attempt of modifying the nuspec kept it working, so my attempt was to do the inverse of the .target files you posted, i.e. to remove that analyzer from that path from projects where 'SupportsRoslynComponentVersioning' is true, i.e., which support the versioned roslyn approach and that's where I failed. Admittingly I did not spend too much time on that approach, because I don't even know how to debug msbuild targets and the only evidence if it worked was if the node in the solution tree was populated.

That's why you need to load the oldest one possible - because you don't know which one will work.

The goal here is not to load the oldest one in VS with newer roslyn support, but to load the newest one the VS version can support, in line with your original PR which added versioned roslyn support. From my POV the current loading behavior should consider that non-versioned folder simply as 'roslyn0.9', if other versioned analyzer folders exist.

[eerhardt]

The goal here is not to load the oldest one in VS with newer roslyn support, but to load the newest one the VS version can support

I'm saying that unless you know what version Roslyn is being used, you don't know which one the VS version can support.

From my POV the current loading behavior should consider that non-versioned folder simply as 'roslyn0.9', if other versioned analyzer folders exist.

See the reasoning above.

[Tornhoof]

@baronfel Here are the two binlogs in a zip file,as github apparently does not like the binlog extension.

binlogs.zip

The old file format project is a plain vanilla .NET 4.7.2 project, I put the content in the details below.

```xml Debug AnyCPU {EEFC6F63-DB8C-43DE-919B-E2A58D7F23C8} Library Project1 Project1 v4.7.2 512 true true AnyCPU true full false bin\Debug\ DEBUG;TRACE prompt 4 AnyCPU pdbonly true bin\Release\ TRACE prompt 4 packages\Microsoft.Bcl.AsyncInterfaces.8.0.0\lib\net462\Microsoft.Bcl.AsyncInterfaces.dll packages\System.Buffers.4.5.1\lib\net461\System.Buffers.dll packages\System.Memory.4.5.5\lib\net461\System.Memory.dll packages\System.Numerics.Vectors.4.5.0\lib\net46\System.Numerics.Vectors.dll packages\System.Runtime.CompilerServices.Unsafe.6.0.0\lib\net461\System.Runtime.CompilerServices.Unsafe.dll packages\System.Text.Encodings.Web.8.0.0\lib\net462\System.Text.Encodings.Web.dll packages\System.Text.Json.8.0.3\lib\net462\System.Text.Json.dll packages\System.Threading.Tasks.Extensions.4.5.4\lib\net461\System.Threading.Tasks.Extensions.dll packages\System.ValueTuple.4.5.0\lib\net47\System.ValueTuple.dll packages\xunit.abstractions.2.0.3\lib\net35\xunit.abstractions.dll packages\xunit.assert.2.8.1\lib\netstandard1.1\xunit.assert.dll packages\xunit.extensibility.core.2.8.1\lib\net452\xunit.core.dll packages\xunit.extensibility.execution.2.8.1\lib\net452\xunit.execution.desktop.dll This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. ``` app.config ```xml ``` packages.config ```xml ```

[Tornhoof]

I'm saying that unless you know what version Roslyn is being used, you don't know which one the VS version can support.

I understand that perfectly fine, that's why the idea was to identify the non-versioned analyzer, remove that one and hopefully let the changes from your PR do the task of selecting the correct one. The goal of my attempts was basically to modify the nuget import logic to ignore/remove the 'dotnet/cs' folder and let everything else intact and that's exactly where I failed, how to identify the identity of the analyzer from the 'dotnet/cs' folder, I kinda hoped to find the path in there somewhere or something similar.

See the reasoning https://github.com/dotnet/sdk/issues/41352#issuecomment-2148254074.

Yes, that's why I tried roslyn1.0, as that was written exactly there in the case you need it If a NuGet package wants to express a component that should be used when none of the roslyn{version} folders apply, it can add that asset to roslyn1.0. Unfortunately that did not work.

In any case we deviate from the problem here, If you say this an packaging issue in xUnit, Brad already pushed a pre-release build to the pre-release feed which removes everything except roslyn3.11, based on a suggestion by @sharwell.

Unfortunately the build time issue due to restarting the compilation server was only found after the release nuget package was done, this means a lot of people will encounter the slowdown, which is unfortunately quite noticeable and xunit.analyzers is a rather common nuget as it is a default part of xunit. Internally I put that xunit package on a local azure devops feed and updated all my projects in all solutions and got rid of the problem.

I have no skin in the game here, I just found the build time issue, tracked it down to the xunit.analyzer package, Brad said he does not believe it's an xUnit issue, I agreed with that and opened the issue here, which from my pov is the least I can do as a community member.

Now you can decide if you want to pass the ball back to Brad, fix it here or pass the ball to someone else. I understand perfectly fine, that it's an resource issue, that was even pointed out in the original issue, but back then probably nobody imagined that by now a lot of nuget packages either contain source generators (some requiring the msbuild target workaround you posted) or roslyn analyzers.

But reality has caught up with us and 352.674 times the affected nuget package has been downloaded by now, so if you happen to have telemetry data for that, please look up how often the compilation server restarted due to the issue from the beginning, my estimation is a number of somewhere in the tens of millions. (My projects alone are probably already in the range of 100 thousand times). And that adds up to a lot of wasted build time.

[jaredpar]

Unfortunately the build time issue due to restarting the compilation server was only found after the release nuget package was done, this means a lot of people will encounter the slowdown, which is unfortunately quite noticeable and xunit.analyzers is a rather common nuget as it is a default part of xunit.

This is very problematic. For example: roslyn won't be able to take this version of the NuPkg because it would be blocked by our PR system for this reason.

[jaredpar]

As much as i dislike bifurcating the targets further, I think we should consider the structure of the xunit package as the standard for how you have an unconditional fallback. Basically use roslynX.Y if it matches else if there is somethnig in the root pick that.

[Aaronontheweb]

Question - we've run into this issue with our analyzers too and have users asking about it. Should we wait for a tooling solution from the SDK team or adopt the same work-around xUnit has?

[baronfel]

do the workaround @Aaronontheweb - we don't have a timeline to make this change yet.