Generators for different roslyn versions need different strong names

jaredpar commented 6 months ago

Description

The runtime ships versioned analyzers in NuPkg. Essentially different versions of an analyzer / generator for different versions of the Roslyn API. This results in NuPkg that have the following file layout:

analyzers\dotnet\roslyn3.11\cs\Microsoft.Extensions.Logging.Abstractions.dll
analyzers\dotnet\roslyn4.0\cs\Microsoft.Extensions.Logging.Abstractions.dll

These analyzers are functionally very different but maintain the same strong name. This creates problems in build because if both are passed to the C# compiler, particularly on desktop, then it forces a VBCSCompiler restart. There is no way for the compiler to load both into the same AppDomain, there is an inherent conflict so the server must restart.

Reproduction Steps

Use a <PackageReference> to Microsoft.Extensions.Logging.Abstractions in an old style project.

Expected behavior

The expected behavior is that the build system should never send both of these to the compiler. The design of our SDK essentially guarantees we'll pick only one of the versions.

However that is only true for new style projects. For projects running on the traditional MSBuild SDK versioned analyzers are not supported. Our analyzer packages compensate for this by shipping .targets files that detect older SDKs and do some post processing that effectively pick the analyzer targeting the lower version of roslyn. In this case that is the roslyn3.11 version.

That is a problem when a solution has a mix of old and new style projects. The new style projects will send the roslyn4.0 version while the old style projects will send the roslyn3.11 version of analyzers to the compiler. That creates conflicts which cause the server to restart and result in a significant increase in build times.

Actual behavior

The references with different functionality should have different strong names.

Regression?

Maybe

Known Workarounds

The known work arounds:

Customers convert all their projects to new style SDK projects. This is not always feasible.
Customers should use dotnet build as AssemblyLoadContext allows us to avoid this problem.
MSBuild magic to mess with analyzers

Configuration

No response

Other information

No response

dotnet-policy-service[bot] commented 6 months ago

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries See info in area-owners.md if you want to be subscribed.

ericstj commented 3 months ago

@eiriktsarpalis, @tarekgh - this applies to JSON, Logging, and Options generators. I think the suggestion here would be that they get different file names per roslyn version or perhaps different versions somehow.

I don't think this applies to our framework source generators, since those will have a different major version per framework. @jaredpar can you confirm?

jaredpar commented 3 months ago

I don't think this applies to our framework source generators, since those will have a different major version per framework.

Are they strong named signed? If so then diff major versions is enough.

ViktorHofer commented 3 months ago

FWIW here's the code path in the sdk that parses analyzer assemblies from nuget packages: https://github.com/dotnet/sdk/blob/1a84d18ec99eec5c48830bdee2be3476789fb123/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs#L935-L1111

Looks like different assembly names would be fine. Different version should be possible as well, i.e. 9.3.11.0 for roslyn3.11.

jaredpar commented 3 months ago

FWIW here's the code path in the sdk that parses analyzer assemblies from nuget packages

That is only one of the implementations. There are others

https://github.com/dotnet/NuGet.BuildTasks/blob/2342ea5526c954d8cab77c30a290a82bd5cc6b44/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs#L360

dotnet / runtime