Generators - analyzer using references causing problems CS8785

mgravell commented 4 years ago

I have an idea for an analyzer I'd like to add, reusing functionality that is already packaged in a netstandard2.0 package.

Repro is here: https://github.com/protobuf-net/protobuf-net.Grpc/pull/85/ - the interesting projects are:

src/generators/protobuf-net.Generator (not the Grpc one!!!)
toys/PlayContracts

In my analyzer csproj I have the two expected refs, plus a commented out one for the tool I want to use:

<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="3.6.0-3.20207.2" PrivateAssets="all" />
<PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.0.0-beta2.final" PrivateAssets="all" />
<!--<PackageReference Include="protobuf-net.Reflection" Version="3.0.0-alpha.143" PrivateAssets="all" />-->

The analyzer currently just lists the additional files (I have a <AdditionalFiles Include="test.proto" />). With the above reference commented out, everything kinda works; however, if I uncomment the <PackageReference> and do nothing else, it stops working completely, citing:

Severity Code Description Project File Line Suppression State Warning CS8785 Generator 'Protogen' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. PlayContracts (netstandard2.0) C:\Code\protobuf-net.Grpc\toys\PlayContracts\CSC 1 Active

(the actual "do the thing" code is there but commented out)

This may simply be a packaging thing, but... it isn't obvious how to fix this; I've tried numerous things - with/without the PrivateAssets hint, with <Analyzer> dll references vs project references to the analyzer, etc. It could be as simple as a dll version conflict, but I can't find any way of getting the info out to tell me what exploded.

Any guidance @chsienki ?

jmarolf commented 4 years ago

unfortunately nuget does not know how to pack analyzer dependencies and we've never done to work to call nuget to dynamically resolve analyzer dependencies at runtime. Effectively, you need to place the protobuf-net.Reflection contents next to your analyzer/source generator.

chsienki commented 4 years ago

Sounds like @jmarolf can guess what the error is. We have work to do to actually surface the errors, for now they're just swallowed unless you have a debugger on it.

Current 'best guidance' (very loosely speaking) is to Debugger.Launch() in your initialize method. Then you can attach and see what's actually failing during a compilation.

mgravell commented 4 years ago

@jmarolf I'm not entirely sure what that means in practice as either an author or consumer. When you say nuget doesn't know... so: how does one most correctly ship an analyzer? Is this:

don't have non-inbuilt dependencies, they won't work; inline everything into one assembly
If it can resolve one level; write your own nuspec by hand that ships everything rather than using transitive trees
Some exotic csproj flag I haven't used when packaging
Something else I haven't thought of?

Or am I misunderstanding the problem?

davidfowl commented 4 years ago

I assume 1.

sharwell commented 4 years ago

If your analyzer assembly has unique dependencies (i.e. no other part of the hosting application references the dependency, and no other analyzers reference the dependency), you can place the dependency assembly in the same folder of the NuGet package where the analyzer assembly is.

If the dependency is not unique, this may or may not work, and failures may be non-deterministic. The two cases where I know this has occurred in the past both ended up treating the situation like (1): remove or inline the dependency. https://github.com/DotNetAnalyzers/StyleCopAnalyzers/pull/2351 dotnet/roslyn#41096

davidfowl commented 4 years ago

Once you "do plugins" you have to solve all the same problems we solve today at build time with nuget (unification etc)

jmarolf commented 4 years ago

yeah, we haven't tackled runtime-dependency-resolution for analyzers. As @davidfowl says, solving this would likely need to start with additions to nuget.

mgravell commented 4 years ago

Oof. Right, I guess I'm going to have to forget about transitive package refs for now, and do something horrible in my build, either with some "and include these .cs files", or maybe some IL-merge/fody fun. This could get interesting.

danielcrenna commented 4 years ago

Was about to submit a repro for this because it seems counter-intuitive that you can't simply reference some other otherwise uncontroversial netstandard2.0 assembly in your source generator library, without it failing with an obscure warning.

I guess the least horrible thing to do here would be to post-build copy the package references to the analyzer build folder?

danielcrenna commented 4 years ago

It doesn't look like copying the offending dependency into the same location as the analyzer .DLL itself is working for me, maybe because System.Text.Json is implicitly referenced in the ASP.NET Core FrameworkReference.

Example code:

<ItemGroup>
    <PackageReference Include="System.Text.Json" Version="5.0.0-preview.3.20214.6" PrivateAssets="All" GeneratePathProperty="true" />
</ItemGroup>
<Target Name="CopyPrivateReferences" AfterTargets="Build">
    <Exec Command="echo f | xcopy $(PkgSystem_Text_Json)\lib\$(TargetFramework)\System.Text.Json.dll $(TargetDir)\System.Text.Json.dll /Y"/>
</Target>

danielcrenna commented 4 years ago

And trying to ILMerge that single dependency quickly descended into madness:

<ItemGroup>
    <PackageReference Include="System.Text.Json" Version="5.0.0-preview.3.20214.6" PrivateAssets="All" GeneratePathProperty="true" />
    <PackageReference Include="System.Memory" Version="4.5.4" GeneratePathProperty="true" />
    <PackageReference Include="System.Buffers" Version="4.5.1" GeneratePathProperty="true" />
    <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="5.0.0-preview.3.20214.6" GeneratePathProperty="true" />
    <PackageReference Include="System.Text.Encodings.Web" Version="5.0.0-preview.3.20214.6" GeneratePathProperty="true"  />
    <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.4" GeneratePathProperty="true" />
    <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="1.1.1"  GeneratePathProperty="true" />
</ItemGroup>
<Target Name="ILMerge" AfterTargets="Build">
    <Exec Command="$(ILMergeConsolePath) /out:$(AssemblyName).dll ^
          /lib:$(NuGetPackageRoot)NETStandard.Library\2.0.1\build\netstandard2.0\ref ^
          /lib:$(PkgSystem_Runtime_CompilerServices_Unsafe)\lib\$(TargetFramework) ^
          /lib:$(PkgSystem_Buffers)\lib\$(TargetFramework) ^
          /lib:$(PkgSystem_Memory)\lib\$(TargetFramework) ^
          /lib:$(PkgSystem_Text_Encodings_Web)\lib\$(TargetFramework) ^
          /lib:$(PkgSystem_Threading_Tasks_Extensions)\lib\$(TargetFramework) ^
          $(PkgSystem_Text_Json)\lib\$(TargetFramework)\System.Text.Json.dll ^
          $(PkgMicrosoft_Bcl_AsyncInterfaces)\lib\$(TargetFramework)\Microsoft.Bcl.AsyncInterfaces.dll ^
          "/>
</Target>

tmat commented 4 years ago

Whatever you do, please don't use ILMerge.

tmat commented 4 years ago

I think the only sane solution is for Roslyn to run analyzers and code generators on .NET Core only.

mgravell commented 4 years ago

@tmat right now I'm thinking my best option is to hack a build that imports all the code I need directly as code, but: somewhere in this there is a "the story of package dependencies in analyzers is bad"

danielcrenna commented 4 years ago

@tmat Would love not to; how would you suggest I reference System.Text.Json from my source generator? It's ILMerge or copy-pasting the code off of GitHub. Neither one is good.

jmarolf commented 4 years ago

Currently working on a design to resolve this. Agree it needs to be solved

danielcrenna commented 4 years ago

Currently working on a design to resolve this. Agree it needs to be solved

Ah, good concept. I didn't think of stepping back into Roslyn to create snapshot tests, and wasn't aware of GeneratorDriver. Many thanks.

wieslawsoltes commented 4 years ago

I am using it like this:

  <ItemGroup>
    <!-- Take a private dependency on Newtonsoft.Json (PrivateAssets=all) Consumers of this generator will not reference it.
         Set GeneratePathProperty=true so we can reference the binaries via the PKGNewtonsoft_Json property -->
    <PackageReference Include="Newtonsoft.Json" Version="12.0.1" PrivateAssets="all" GeneratePathProperty="true" />

    <!-- Package the generator in the analyzer directory of the nuget package -->
    <None Include="$(OutputPath)\$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />

    <!-- Package the Newtonsoft.Json dependency alongside the generator assembly -->
    <None Include="$(PkgNewtonsoft_Json)\lib\netstandard2.0\*.dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />
  </ItemGroup>

wieslawsoltes commented 4 years ago

I am using it like this:

  <ItemGroup>
    <!-- Take a private dependency on Newtonsoft.Json (PrivateAssets=all) Consumers of this generator will not reference it.
         Set GeneratePathProperty=true so we can reference the binaries via the PKGNewtonsoft_Json property -->
    <PackageReference Include="Newtonsoft.Json" Version="12.0.1" PrivateAssets="all" GeneratePathProperty="true" />

    <!-- Package the generator in the analyzer directory of the nuget package -->
    <None Include="$(OutputPath)\$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />

    <!-- Package the Newtonsoft.Json dependency alongside the generator assembly -->
    <None Include="$(PkgNewtonsoft_Json)\lib\netstandard2.0\*.dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />
  </ItemGroup>

And I had include all nuget PackageReference as dll's in analyzers/dotnet/cs.

kjkrum commented 3 years ago

Currently working on a design to resolve this. Agree it needs to be solved

@jmarolf Any movement on this?

sharwell commented 3 years ago

@wieslawsoltes Note that Newtonsoft.Json is one of the dependencies we were never able to get to work reliably. We ended up embedding a JSON serialization layer as source code in the analyzer project itself, and now it works.

@kjkrum Packaging dependencies manually is not especially difficult (there are many examples already available). If this approach works for you, the new design might simplify it. However, if this approach does not work for you (due to dependency conflicts at runtime), the new design will not fix the issue and despite automating the packaging process it will still not execute correctly.

kjkrum commented 3 years ago

@sharwell The dependency conflicts issue is a deal breaker. Not only would it be annoying and fragile to manually add and maintain the recursive dependencies of a library like NSwag, but even if it works for me, I can't guarantee that another user of my generator will not have a conflict with some other analyzer. As currently designed, I don't see non-trivial source generators ever being practical.

jmarolf commented 3 years ago

@kjkrum the only way around this (which is what you need to do for Visual Studio extensions and other .NET extensibility systems) is strong name sign your assemblies. Unless you are willing to do that we cannot guarantee the runtime will not fail to load some mis-matched version from another assembly.

jmarolf commented 3 years ago

Currently working on a design to resolve this. Agree it needs to be solved

@jmarolf Any movement on this?

I'll put up a proposal next week in dotnet/designs

kjkrum commented 3 years ago

@jmarolf Source generators that take the form of console applications packaged with build targets do not need to be strong name signed and AFAIK do not suffer from dependency conflicts. Those remain my go-to because the issue with zombie MSBuild tasks locking files was either never actually fixed, or was fixed after I gave up hoping for a fix, which was some time after it was claimed to be fixed. I'll watch for your proposal.

jmarolf commented 3 years ago

Source generators that take the form of console applications packaged with build targets do not need to be strong name signed and AFAIK do not suffer from dependency conflicts.

Right, if you do the entire build again (in order to create the compilation context) then you do not need to load the assemblies into the same process at all. They can all live in separate processed. Unfortunately this kills build performance (this build twice model was already possible without a roslyn api). If a console app called from a build target meets your need today I do not think you should move to the roslyn source generators API. The only advantage the roslyn API gives is being able to introspect into the compilation in order to make decisions about what should be generated.

We could have separate processes for all source generators in order to isolate them. This would either require us to rebuild the compiler to allow all of its internal state to be serializable (unclear what the performance implications would be here), or have each driver for source generators re-create the compilation context massively degrading build performance.

I currently do not think this large re-write is worth it for the use case source generators were intended for: code generated in conjunction with the user typing. Especially since its unclear to me how often conflicting dependencies are going to happen in the wild. For now I only plan to have us show a useful diagnostic so that if this situation ever happens its clear to the developer what is going on.

TorreyGarland commented 2 years ago

<ItemGroup>
    <!-- Take a private dependency on Newtonsoft.Json (PrivateAssets=all) Consumers of this generator will not reference it.
         Set GeneratePathProperty=true so we can reference the binaries via the PKGNewtonsoft_Json property -->
    <PackageReference Include="Newtonsoft.Json" Version="12.0.1" PrivateAssets="all" GeneratePathProperty="true" />

    <!-- Package the generator in the analyzer directory of the nuget package -->
    <None Include="$(OutputPath)\$(AssemblyName).dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />

    <!-- Package the Newtonsoft.Json dependency alongside the generator assembly -->
    <None Include="$(PkgNewtonsoft_Json)\lib\netstandard2.0\*.dll" Pack="true" PackagePath="analyzers/dotnet/cs" Visible="false" />
  </ItemGroup>

using VS 2022 v 17.0.4 with this no longer works:

2>CSC : warning CS8785: Generator 'IncrementalGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'FileNotFoundException' with message 'Could not load file or assembly 'Newtonsoft.Json, Version=12.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed' or one of its dependencies. The system cannot find the file specified.'

pluraltouch commented 2 years ago

It gets weird...

I am completely rebuilding the whole solution which produces the CS8785. The warning is there, with the explicit assembly name, source generator does not produce the output (as expected :-( ), but monitoring all processes with procmon64 no process seems to be even looking for that particular .dll. (No surprise it does not find it)

if I monitor for the sourcegenerator package itself I can see VBCSCompiler is opening the dll from ..nuget\packages...........\analyzers\dotnet\cs.... As a crosscheck, if I monitor for only 'analyzers\dotnet\cs' path fragment, I see only the sourcegenerator package is searched (and successfully found there)

So having the referenced assembly there is completely obsolete, and it is a puzzler why I does not see with procmon64 any process to looking for the "missing" assembly...

sharwell commented 2 years ago

Take a private dependency on Newtonsoft.Json

StyleCop Analyzers never found a way to make Newtonsoft.Json a reliable dependency of an analyzer (or source generator). We ended up embedding a different JSON library as source.

pluraltouch commented 2 years ago

this issue is more generic and not about how to use json in a source generator...

mgravell commented 2 years ago

fair enough... hopefully this second version makes things more generic...

~StyleCop Analyzers~ protobuf-net.BuildTools never found a way to make ~Newtonsoft.Json~ anything a reliable dependency of an analyzer (or source generator). We ended up embedding ~a different JSON library~ everything we needed as source.

michael-hawker commented 2 years ago

I've been facing this same issue with a similar CS8784 warning this past week as well trying to include YamlDotNet.

It seems to work in a stand-alone setup with the suggested setup like this:

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

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.0.1" Pack="false" PrivateAssets="all" />
    <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="3.3.3" Pack="false" PrivateAssets="all" />
    <PackageReference Include="YamlDotNet" Version="11.2.1" PrivateAssets="all" Pack="false" GeneratePathProperty="true" />
  </ItemGroup>

  <Target Name="GetDependencyTargetPaths">
    <ItemGroup>
      <TargetPathWithTargetPlatformMoniker Include="$(PKGYamlDotNet)\lib\netstandard1.3\YamlDotNet.dll" IncludeRuntimeDependency="false" />
    </ItemGroup>
  </Target>

But is failing when I try and use this setup in our larger project.

WoLfulus commented 2 years ago

This is still a thing apparently. The suggestions here haven't worked for me (referencing the analyzer in the same solution).

kjkrum commented 1 year ago

Generators need to run out of process. Performance takes a back seat to actually working and being useful.

sharwell commented 1 year ago

@kjkrum The performance penalty associated with process isolation for source generators isn't viable. In some instances today, we do run source generators in a separate process from Visual Studio. However, this is not sufficient to resolve the problems because more than one source generator end up running in that process, and their dependencies can conflict. Eventually we'll be able to separate them using AssemblyLoadContext in .NET Core, but that will only work once the remaining tools using .NET Framework move over.

kjkrum commented 1 year ago

@sharwell Would it be practical to make it optional and let users decide if it's viable?

kkukshtel commented 9 months ago

Was this ever resolved? I'm also looking at figuring out how to just add a System.Text.Json reference to my generator and unclear on how it's meant to work.

kjkrum commented 8 months ago

@sharwell If the "performance penalty" of console app source generators is viable (which it is) then running incremental source generators with process isolation is also viable. Do not make performance decisions for users. I'd rather wait minutes for a build than years for a minimum viable product.

CyrusNajmabadi commented 8 months ago

@kjkrum Just an FYI that Jonathan has sadly passed away. We're continuing to look at this space. But it is frought with challenges.

CyrusNajmabadi commented 8 months ago

than years for a minimum viable product.

We are continuing to work in this space. But we're a small team with a ton of responsibilities. I do have it as a goal to make it so that:

people can just rebuild their SGs and have them picked up.
SGs can run in a moden core environment, not just standard2
the IDE experience can be entirely insulated from poorly behaved SGs (the norm unfortunately).

However, i would not expect this to come soon as each of these is a substantial amount of work, and we are often constrained by global requirements around compatibility.

kjkrum commented 8 months ago

@CyrusNajmabadi Sorry to hear about Jonathon. FWIW, I don't blame your team at all. I blame Microsoft for releasing a product with such incredible potential in a half-finished state, and then leaving its completion to a small team with a ton of responsibilities.

CyrusNajmabadi commented 8 months ago

@CyrusNajmabadi FWIW, I don't blame your team at all. I blame Microsoft for releasing a product with such incredible potential in a half-finished state, and then leaving its completion to a small team with a ton of responsibilities.

For every person who wants us waiting before releasing to get all these things done, we have just as many asking to release early in whatever state things are in just so they can use it :-)

That very thing happened today already :-D

kjkrum commented 8 months ago

@CyrusNajmabadi The early release is less of a problem than seemingly abandoning it for years afterward. My assumption is that someone in management is under the mistaken impression that it's "done" and has assigned resources elsewhere.

CyrusNajmabadi commented 8 months ago

@CyrusNajmabadi The early release is less of a problem than seemingly abandoning it for years afterward. My assumption is that someone in management is under the mistaken impression that it's "done" and has assigned resources elsewhere.

It has not been abandoned. Several people, including myself are working in this space. However, the costs are enormous here. Especially given the dramatic impact SGs have on every single other layer that sits above them.

sharwell commented 8 months ago

If the "performance penalty" of console app source generators is viable (which it is) then running incremental source generators with process isolation is also viable. Do not make performance decisions for users.

There are different ways that we can end up making performance decisions for users, and not all of them are bad. For example:

"We shouldn't expose this API because a user might accidentally cause performance to be bad." This is a fairly common theme in a product with an extensibility layer as rich as Visual Studio, but I always fight back hard against making decisions in this manner. If the user makes performance as bad as is feared, they will notice and fix it. If they don't make performance as bad as is feared, then there wasn't really a problem to start with.
"We shouldn't expose this API because it will be impossible for any user (internal or external), even with deep understanding of the performance of the entire system, to use the API in even the simplest form without catastrophic performance impact." When this situation occurs, it makes no sense to proceed in that direction since it gives a mistaken appearance of a new feature when in reality the feature cannot be used.

To date, running generators in an isolated process runs afoul of the second case. In other words, we are not withholding a feature here; the requested feature simply does not exist.

dotnet / roslyn

Generators - analyzer using references causing problems CS8785 #43903