Support automatic transient dependency packaging for source generators

[Turnerj]

Originally raised as part of an issue with Roslyn (dotnet/roslyn#52017), it was suggested that I raise it here to potentially get the ball rolling on this.

Basically from a user's point of view, having private dependencies in your source generator can become unwieldy very fast when factoring in transient dependencies. For example, this is what it looks like to add System.Text.Json as a private dependency while factoring in transient dependencies.

  <ItemGroup Label="Package References">
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.9.0" />
    <PackageReference Include="System.Text.Json" Version="5.0.1" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="5.0.0" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.4" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Text.Encodings.Web" Version="5.0.1" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Buffers" Version="4.5.1" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Memory" Version="4.5.4" GeneratePathProperty="true" PrivateAssets="all" />
    <PackageReference Include="System.Numerics.Vectors" Version="4.4.0" GeneratePathProperty="true" PrivateAssets="all" />
  </ItemGroup>

  <PropertyGroup>
    <GetTargetPathDependsOn>$(GetTargetPathDependsOn);GetDependencyTargetPaths</GetTargetPathDependsOn>
  </PropertyGroup>

  <Target Name="GetDependencyTargetPaths">
    <ItemGroup>
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Text_Json)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGMicrosoft_Bcl_AsyncInterfaces)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Runtime_CompilerServices_Unsafe)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Threading_Tasks_Extensions)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Buffers)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Memory)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Numerics_Vectors)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
      <TargetPathWithTargetPlatformMoniker Include="$(PKGSystem_Text_Encodings_Web)\lib\netstandard2.0\*.dll" IncludeRuntimeDependency="false" />
    </ItemGroup>
  </Target>

Not only am I needing to be more careful about package version updates and their transient dependencies (the linked Roslyn issue came about because upgrading System.Text.Encodings.Web from 5.0.0 to 5.0.1 brought in two more dependencies for .NET Standard 2.0) but it seems like I'm doing the work the compiler/build system should be doing for me.

So far to avoid doing that, I've managed to hack in a semi-automated behaviour with the following:

  <ItemGroup Label="Package References">
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.9.0" />
    <PackageReference Include="System.Text.Json" Version="5.0.1" PrivateAssets="all" />
  </ItemGroup>

  <PropertyGroup>
    <GetTargetPathDependsOn>$(GetTargetPathDependsOn);GetDependencyTargetPaths</GetTargetPathDependsOn>
  </PropertyGroup>

  <Target Name="GetDependencyTargetPaths" AfterTargets="ResolvePackageDependenciesForBuild">
    <ItemGroup>
      <TargetPathWithTargetPlatformMoniker Include="@(ResolvedCompileFileDefinitions)" IncludeRuntimeDependency="false" />
    </ItemGroup>
  </Target>

It is not great for multiple reasons (ResolvedCompileFileDefinitions isn't always available & includes more than the dependencies that we actually want to bundle) however this still seems like a more practical solution than manually specifying transient dependencies.

From my point of view, the ideal scenario is simply specifying PrivateAssets="all" and the build system takes care of the rest.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[sfoslund]

@ericstj are you the right person to comment here?

[ericstj]

@ericstj are you the right person to comment here?

I don't know, depends on what you want in the way of comments. Here's some thoughts. cc @jaredpar @chsienki @KathleenDollard

I would have expected that CopyLocalLockFileAssemblies would be enough to make Roslyn happy, but it doesn't seem to probe next to the analyzer assembly. You have to pass it into the compiler. Here's @Turnerj's repro with that change that demonstrates that (seems like a fix to Roslyn could make this better). ericstj/SourceGeneratorDependencyTest@e80fbd3
There's probably a better sample than the manual one shared in the original post that can do the right thing based on what the SDK would have copied. I don't have time to sketch that out but might get back to it later. Perhaps such an example should be added to the source generator cookbook?

Related to this is how analyzers pack. Ideally the same mechanism that decides what should be passed via a project reference to an analyzer also determines what's included in a an analyzers package.

Next issue: source generators and analyzers are netstandard2.0 libraries. Creating a netstandard2.0 runnable layout with dependencies is not recommended, since netstandard2.0 isn't a runnable framework. By choosing netstandard2.0 assets from package dependencies you may be missing functionality, missing platform-specific loading, or be missing type unification. Something to consider around the cost of having dependencies.

Next issue: dependencies for plugins that run on desktop don't really work well unless the plugin model supports bindingRedirects. Analyzers do not as far as I know. @jaredpar @chsienki how does this work when dependencies are passed to the compiler via the /analyzer flag, will roslyn automatically unify everyone to the set of dependencies passed in (deduping as well)?

[jaredpar]

Analyzers do not as far as I know. @jaredpar @chsienki how does this work when dependencies are passed to the compiler via the /analyzer flag, will roslyn automatically unify everyone to the set of dependencies passed in (deduping as well)?

At a high level the compiler approach is to call Assembly.Load(string path) on the analyzer and then let the runtime deal with the dependencies. The details are more complicated but at a high level that is what we are doing.

Most analyzers / generators that have dependencies tend to be very simple, one layer deep dependencies. Likely because that is all that will work due to the issues you're outlining above. There also isn't a lot of room for us to do binding redirects. In the command line compiler / server yes we could think about doing this. But it would be custom implemented because we can't use AppDomain isolation. Other hosts though like Visual Studio or VS Code we can't safely implement binding redirects without possibly compromising the integrity of the host process.

Overall though the solution here is getting to a point where the compiler is always running on .NET Core. In that mode we use AssemblyLoadContext and the dependency loading story is more flexible and has a lot more options.

[ericstj]

At a high level the compiler approach is to call Assembly.Load(string path) on the analyzer and then let the runtime deal with the dependencies. The details are more complicated but at a high level that is what we are doing.

That's what I would have expected, in which case the runtime would probe next to the analyzer assembly, but that doesn't seem to be the case. Looks to me like roslyn makes a copy of the analyzer before loading it and misses copying any adjacent files. This is from a run with /analyzer:C:\scratch\SourceGeneratorDependencyTest\SourceGeneratorProject\bin\Debug\netstandard2.0\SourceGeneratorProject.dll where all the dependencies were copied next to the dll.

LOG: Attempting download of new URL file:///C:/Program Files (x86)/Microsoft Visual Studio/2019/Preview/MSBuild/Current/Bin/Roslyn/System.Text.Json.DLL.
LOG: Attempting download of new URL file:///C:/Program Files (x86)/Microsoft Visual Studio/2019/Preview/MSBuild/Current/Bin/Roslyn/System.Text.Json/System.Text.Json.DLL.
LOG: Attempting download of new URL file:///C:/Program Files (x86)/Microsoft Visual Studio/2019/Preview/MSBuild/Current/Bin/Roslyn/System.Text.Json.EXE.
LOG: Attempting download of new URL file:///C:/Program Files (x86)/Microsoft Visual Studio/2019/Preview/MSBuild/Current/Bin/Roslyn/System.Text.Json/System.Text.Json.EXE.
LOG: Attempting download of new URL file:///C:/Users/erics/AppData/Local/Temp/VBCSCompiler/AnalyzerAssemblyLoader/fbe76030cce94d90a0cf41943286fe57/3/System.Text.Json.DLL.
LOG: Attempting download of new URL file:///C:/Users/erics/AppData/Local/Temp/VBCSCompiler/AnalyzerAssemblyLoader/fbe76030cce94d90a0cf41943286fe57/3/System.Text.Json/System.Text.Json.DLL.
LOG: Attempting download of new URL file:///C:/Users/erics/AppData/Local/Temp/VBCSCompiler/AnalyzerAssemblyLoader/fbe76030cce94d90a0cf41943286fe57/3/System.Text.Json.EXE.
LOG: Attempting download of new URL file:///C:/Users/erics/AppData/Local/Temp/VBCSCompiler/AnalyzerAssemblyLoader/fbe76030cce94d90a0cf41943286fe57/3/System.Text.Json/System.Text.Json.EXE.
LOG: All probing URLs attempted and failed.

This was when loading analyzer from a p2p reference. I don't see how this would ever work since Roslyn is copying each analyzer to its own directory.
I notice different behavior when running the compiler directly and avoiding the compiler server, in that case no copy happens, so the runtime will probe next to the analyzer assembly. This works correctly so long as no dependency requires a bindingRedirect.

The repro required a bindingRedirect for System.Text.Encodings.Web so it fails no matter what.

In both cases it seems like roslyn shares the same AppDomain for analyzer loads so compiler server path will work if you happen to pass all dependencies to the compiler and dependencies happen to be passed in breadth first order from lowest in the stack up.

All of these were tested using .NETFramework. I see no failure in .NETCore, where I believe this case happens to work because the dependencies all happen to exist in the shared framework.

[jaredpar]

That's what I would have expected, in which case the runtime would probe next to the analyzer assembly, but that doesn't seem to be the case. Looks to me like roslyn makes a copy of the analyzer before loading it and misses copying any adjacent files

Yep that is the "details are more complicated part". 😄

How you invoke the compiler changes the details here. The command line will just use Assembly.Load in place. It's a short lived process hence there are no locking concerns. The server will copy all of the values passed via /analyzer into a temp directory and load from there. That prevents file locking in the dev work space.

The end result is roughly the same though as the shadow copy directory is meant to mirror the original. Hence at a high level it should still be Assembly.Load

Looks to me like roslyn makes a copy of the analyzer before loading it and misses copying any adjacent files.

This is by explicit design. The compiler will only copy the arguments to /analyzer as those are the only tracked input to the compilers. This means that dependencies to analyzers must be passed with /analyzer. This is actually quite natural though. The build rules default into passing all DLLs in an analyzer NuPkg via /analyzer hence you essentially have to go out of your way for this to not happen.

[ericstj]

I see. So in order to have dependencies passed to the compiler you need to pass everything as an /analyzer, this happens automatically when you package everything into a nuget package, but doesn't happen when you use ProjectReferences. This issue is presumably asking the SDK to make better support for Analyzers/Source-generators as ProjectReferences to address this.

You can't rely on assemblies being next to the analyzer, though this works for csc command, it doesn't work for the compiler server due to the copying done by the compiler (this means that in general analyzers can't rely on doing any file relative loading, good thing to remember).

I was noticing what I thought was an ordering issue with loading, but I think this was actually a bindingRedirect issue. It seems the compiler loads all analyzer assemblies before it attempts to get types from those assemblies. This is what allows passing dependencies as analyzers to work, since the compiler loads them up front. The lack of bindingRedirect handling seems like a pretty significant limitation and seems like something folks writing analyzers/generators with dependencies need to consider up front.

It is not great for multiple reasons (ResolvedCompileFileDefinitions isn't always available

Solve this by adding DependsOnTargets="ResolveReferences"

includes more than the dependencies that we actually want to bundle)

You're using the wrong item (compile assets, instead of runtime) and you're using an earlier stage of it (before conflict resolution and RAR). Try setting CopyLocalLockFileAssemblies=true and using ReferenceCopyLocalPaths item instead. This will have all the runtime assemblies which might be copied as if you had an application.

[Turnerj]

Thanks @ericstj , I'll try that out when I can as it would be a good workaround until the SDK does this automatically. I mean if it works well, could the SDK have this target built-in so myself (and others) wouldn't need that target for every source generator? Would that simplify things on your end or actually make it more complicated?