Analyzer host process fails to load analyzers correctly when building in parallel

[jnm2]

Version Used: SDK 5.0.201

This has always worked when running msbuild, but it does not work using dotnet build. The only workaround I have at the moment is to put /p:BuildInParallel=false in the dotnet build line which is not great because I want people to be able to walk up and type dotnet build multiple times. <BuildInParallel>false</BuildInParallel> didn't seem to have any effect on the repro.

What would it take to get from here to things just working for consumers of analyzers that have dependencies?

Steps to Reproduce:

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

  <PropertyGroup>
    <TargetFramework>netcoreapp3.1</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="PropertyChangedAnalyzers" Version="3.2.1" PrivateAssets="all" />
    <PackageReference Include="ReflectionAnalyzers" Version="0.1.22-dev" PrivateAssets="all" />
  </ItemGroup>

</Project>

using System.ComponentModel;
using System.Runtime.CompilerServices;

public sealed class C : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    private string property1;
    public string Property1 { get => property1; set => Set(ref property1, value); }

    private bool Set<T>(ref T field, T value, [CallerMemberName] string propertyName = null)
    {
        if (RuntimeHelpers.Equals(field, value)) return false;
        field = value;
        OnPropertyChanged(propertyName);
        return true;
    }

    private void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    }
}

Run this (rebuild is only to simulate edit + build during development):

for ($i = 0; $i -lt 10; $i++) { dotnet build /t:rebuild }

Expected Behavior:

No warnings.

Actual Behavior:

37 warnings appear, all analyzer crashes due to not being able to load an assembly:

CSC : warning AD0001: Analyzer 'PropertyChangedAnalyzers.PropertyDeclarationAnalyzer' threw an exception of type 'System.IO.FileLoadException' with message 'Could not load file or assembly 'Gu.Roslyn.Extensions, Version=0.12.9.0, Culture=neutral, PublicKeyToken=4b04740f2fd5868f'. Could not find or load a specific file.

[sharwell]

.NET Core relies on Assembly Load Context (ALC) to load more than one version of the same assembly side-by-side. Currently the compiler doesn't support ALC for analyzer references, so if more than one analyzer in the project is providing the same dependency, they all need to have the same version. This applies even to the case where strong names can distinguish between the assemblies, which would work side-by-side for .NET Framework builds.

[jnm2]

Does that make this a duplicate of work that is already tracked, or is new work still needed to achieve a "just working" analyzer ecosystem?

[jaredpar]

Think we need to just follow the pattern that the MSBuild team laid out here for how they isolated Task in different AssemblyLoadContext:

• Load plugins in an AssemblyLoadContext msbuild#4916

One complication we have to consider though is that analyzers and there dependencies are passed with the /analyzer: flag. Essentially consider if you have a NuPkg which has the following layout:

• analyzers
  • cs
    • Analyer.dll
    • Util.dll

The standard tasks and targets will cause the compiler command line to include both /analyzer:Analyzer.dll and /analyzer:Util.dll. There are a couple of reasons for this but the simplest one is because these are all inputs to the compiler and hence should be expressed on the command line.

This means that for the purpose of isolating in an AssemblyLoadContext we can't just isolate every single /analyzer argument. Instead we need to group them based on the containing directory path. Essentially put all of the analyzer and generator DLLs from the same directory into the same AssemblyLoadContext.

Details on how AssemblyLoadContext works are here

[AraHaan]

I think I know what could work.

When trying to load an anaylzer / source generator:

• get the file's absolute path (minus file name to use it to recursively find dll's at the root of it)
• take that absolute path of the folder of which the dlls to that analyzers are in and use it to find the absolute paths of every file (recursively) that is within that path and list them out
• do an foreach loop to load each needed dlls into the ALC (filtering out the *.resources.dll files).
• any assemblies for localized resources (if needed).

Using this would have benefits:

• less chance of breakage.
• There would need to be an list lookup and would be done before actually loading anything when it comes to analyzers / sourcegenerators and makes them easier to self-contain without having issues on if every analyzer / sourcegenerator always have the same version of any common dependencies of them.

Cons of this:

• list lookups and iteration might be slow.

[jaredpar]

@AraHaan

That approach was considered in 1.0 and rejected for the following reasons:

• It causes non-determinism in the compiler because inputs to the compiler are not a part of the compiler command line. Hence subtle changes to the directories around the analyzers (adding or deleting dlls) could change the output of the compiler.
• This can cause hard to track down slow downs in compilation. Consider the case where developers are loading analyzers from their product build and their product build drops all binary outputs into the same directory. In that case every compilation is copying every output of the build into the shadow directory.

In the end we decided to stick with first principals here and force all analyzers and their dependencies to be listed with /analyzer. Further we invested in the tooling to make this automatic such that it's hard for consumers to get this wrong. If the dependencies are present it will just work. In the time since we made this decision we've only seen it go wrong in very small number of cases. Basically where developers hand authored build files that loaded analyzers and didn't follow the patterns set out in the standard targets.

[AraHaan]

I guess another option is for the compiler to check every analyzer first before it runs them for dependencies inside the metadata references to them (like what ILSpy can do for that part), then only load the newest version of each dependency based on the analyzers / sourcegenerators needing to be used then do an implicit runtime "binding redirect" for the rest.

[jnm2]

@AraHaan That still falls foul of Jared's first point:

It causes non-determinism in the compiler because inputs to the compiler are not a part of the compiler command line

[jaredpar]

@jnm2 has it right here, still runs afoul of the first pooint.

Overall though I want to stress there really isn't a problem with our existing design and making it work with AssemblyLoadContext. My explanation is mostly meant to shed light on the details here because I'm guessing it doesn't match the expectations of a lot of people. Particularly if you look at the linked PR where they isolate on a per Task basis. It would be natural to say we should isolate on a per Analyzer / Generator basis. That doesn't map directly though due to the different way in which our plugin models are expressed. Instead I think we need to focus on the containing directory as the unit of isolation here

[AraHaan]

I think there could be another option, look for a json file (like what it generates when you target net5.0 for the dependencies and where they are located at) which is located right beside the analyzer/sourcegenerator dll and then use that for loading generators / analyzers.

Also it would come with benefits as well:

• the json could be told that the dependencies are in the folder of the analyzer / sourcegenerator dll file.
• the compiler would look at that json file before trying to load them and then load them all into an ALC.
• if the json file does not exist, fallback to the current behavior.

Cons:

• msbuild would need a property to force generating that json file when it is told that the project is an analyzer like project.

[jaredpar]

@AraHaan overall though you're proposing a solution for a behavior that is not deemed as a problem by us. The build flow here has remain unchanged for ~5 years now. It's not that the existing flow doesn't have the data that would allow us to be successful, it's just that we haven't fully leveraged the data in the .NET Core scenarios to provide maximum flexibility.

[RikkiGibson]

I was able to manually verify that this scenario works with the changes in #55098. I manually referenced an "old" compiler and got it to exhibit the bad behavior, then updated the reference and things started working.

[jnm2]

I've verified that both the minimal repro at the top and the real-world project begin to build successfully under SDK 5.0.302 but not under 5.0.205. If 5.0.302 doesn't have your changes, I guess that means that this particular repro is lost but the underlying problem may show itself in another combination of analyzers and source files.