Support for source generators that add non BCL code

[AArnott]

In VS 16.9.3 (or as new as 16.10 Preview 3, 31206.385.main)...

1. Create a new C# class library:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
    <LangVersion>9</LangVersion>
  </PropertyGroup>
</Project>

2. Use the NuGet Package Manager to install Microsoft.Windows.CsWin32 0.1.422-beta to the project.
3. Add a type reference to Span<byte> somewhere in a .cs file

Expected

No compilation errors.

Actual

Span<byte> produces a compiler error because System.Memory.dll is not a reference that is passed to the compiler.

The nuget package reference was added with:

  <ItemGroup>
    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.1.422-beta">
      <PrivateAssets>all</PrivateAssets>
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
    </PackageReference>
  </ItemGroup>

Note the IncludeAssets metadata is missing the compile value. Adding it manually (or removing the IncludeAssets metadata altogether) fixes the problem.

Note that the nuspec for Microsoft.Windows.CsWin32 includes the System.Memory dependency with include="all":

<dependency id="System.Memory" version="4.5.4" include="All" />

Note also that our nuspec sets:

    <developmentDependency>true</developmentDependency>

The net effect we want is that Microsoft.Windows.CsWin32 itself not be a package dependency expressed in the consuming project's own package, but we do want System.Memory to be expressed.

As originally reported at microsoft/CsWin32#242

[jnm2]

I suspect the answers will be "this is by design" or "it can't be compile because that has other consequences." In that case, please consider extending the design to make development dependency packages capable of adding runtime dependencies for consuming projects, or in some other way make the state of things more friendly for consumers of source generators.

[AraHaan]

The way I work around this is to have the build\<package name>.props file to contain the PackageReference (and as such it can be marked as a implicit reference that could be wildcarded to *-*) instead so that way it will work like how this issue expects. This is due to the fact that all Analyzers automatically are considered development dependencies only and as such are excluded from being reference-able by the main project.

This means all analyzers that list packages under the dependency section in their nuspecs will be excluded from being recognized as runtime dependencies. And I think this is actually by design too, but I think it should change.

Because the System.Memory package is exposed by the nuspec's dependency section on CsWin32, I think the project system tries to resolve it thinking that it is required before trying to run the analyzer (in this case the analyzer is actually a source generator). This idea must be tested to verify what I assume however.

[jnm2]

@AraHaan I've explored the suggestion of adding a package reference in a package's .props for very different reasons and it's not stable last I checked. The .props does not exist on the first restore for the consuming project, and there is no loop to restore over and over until the set of package references stabilizes. It's necessary for a clean build with no prior restores (and an empty NuGet package cache) to restore correctly and compile and run the first time.

[AArnott]

I share @jnm2's concern. The only supported way for one nuget package to bring in another is via nuspec dependency. A PackageReference item added by an msbuild import file brought in by another package does not work in a single-phase restore that our tools assume today.

[nkolev92]

Hey all,
you provided a lot of the context necessary in this discussion!

It's definitely a tricky situation, let me provide some information:

DevelopmentDependency

I suspect the answers will be "this is by design" or "it can't be compile because that has other consequences

@jnm2 You were right on point. Things work as designed, but not ideal in the context of source generators as per the current guidelines.

Development dependency as a concept in PackageReference is mixed with the regular project graph.
That's not ideal. It's something was originally mentioned in the original design of this feature: #4125.

Dev dependencies are relatively pain free as long as the packages are self contained.
When they are not, it completely falls apart like above.
There's no enforcement on whether development dependency marked packages bring in development dependencies themselves.

I think the documentation in https://github.com/dotnet/roslyn/blob/main/docs/features/source-generators.cookbook.md#use-functionality-from-nuget-packages might need some updating, as it doesn't necessarily prevent customers from running into the scenario described here.

I don't think introducing another knob for controlling the metadata is the right solution, as it's been suggested.

I personally think dev dependencies should be self contained and not have dependencies, and apply the same reasoning to the source generators themselves.

Alternatively, development dependencies could be their own item, and not interfere with the PackageReference/compile graph.

cc @chsienki as I imagine you are the source gen owner from roslyn side.
cc @aortiz-msft @zivkan @zkat

[nkolev92]

Another thing that I wanted to add is that development dependencies affecting runtime is not without issues.
It was added as a consideration based on how development dependencies were used in packages.config.

[jnm2]

I personally think dev dependencies should be self contained and not have dependencies, and apply the same reasoning to the source generators themselves.

@nkolev92 To clarify: this is not a runtime dependency for the development dependency tool itself to load as it runs, but for its consuming project to load as it runs.

[nkolev92]

Yep, I think similar idea applies.

If you want to depend on a certain package, you should include it explicitly, not through something that is a development dependency.

[canton7]

To add another data point: I've got a library which generates implementations for user-defined interfaces at runtime, using S.R.E, when the user calls e.g. RestClient.For<IUserInterface>(). I created a source generator backend which generated these interfaces at compile-time, which are then picked up by that same call to RestClient.For. The generated interface implementations can only be used by my library, and they contain code which uses types from my library: the source generated code won't compile if the user isn't referencing my library.

I currently have to have the source generator check whether the user is referencing my library,and what version of it they're referencing, and raise an error diagnostic if necessary saying "please install version XYZ of this library", and also spam this information over the Nuget packet description, and documentation.

This works, but it's not a great user experience.

[AArnott]

If you want to depend on a certain package, you should include it explicitly, not through something that is a development dependency.

We want the app to "just work" after installing the source generator. If I want to add functionality to my project, and I add a package to do this, if the package opts into "disappearing" itself as a dependency because downstream users do not need it, why should that prevent that same package from leaving behind dependencies on other packages that are needed by downstream users?
I think a source generator that emits code that depends on other nuget packages is perfectly normal and should be a first class scenario -- not one where the project owner gets errors until they add a package reference manually.

Another code generation library I own that I am planning to migrate to Roslyn Source Generators (ImmutableObjectGraph) is exactly in the use case @canton7 describes: it provides its own runtime library as well as generated code that uses it.

But all this doesn't address one of my fundamental questions: if removing the IncludeAssets metadata from the PackageReference makes the whole thing work as we intend (which is true), what in VS gaining by explicitly setting the metadata and omitting compile, which breaks it? What scenario breaks if VS includes compile by default for development dependency packages?

[nkolev92]

I think a source generator that emits code that depends on other nuget packages is perfectly normal and should be a first class scenario -- not one where the project owner gets errors until they add a package reference manually.

I misunderstood the part that the generated code would depend on an API from a package.

if removing the IncludeAssets metadata from the PackageReference makes the whole thing work as we intend (which is true), what in VS gaining by explicitly setting the metadata and omitting compile, which breaks it? What scenario breaks if VS includes compile by default for development dependency packages?

I'll try to cover the multiple parts that went into the original design:

• If you can code against a package, then that is counter to the development dependency understanding. It is mostly associated with build time dependencies.
• Include/Exclude flags are applied transitively. Say you have applied exclude compile metadata on a package. Removing that package shouldn't lead to a compile time error due to a missing reference.

The development dependencies feature breaks down the moment there's a dependency for that package, whether it brings compile or other assets is not particularly relevant. It makes generating the dependency graph significantly more difficult because it would potentially require re-walking if a package id has been used as both a development dependency and not a development dependency.

If I'm understanding it correctly, the concerns for source generators seem slightly different in that, these now transitively included packages would need to flow to the parent of the consuming project at runtime, so changing the metadata on the generator packagereference wouldn't be sufficient.

Maybe a concept of related packages/package groups is appropriate here?

[AArnott]

because it would potentially require re-walking if a package id has been used as both a development dependency and not a development dependency.

What does this mean? A package self-identifies as a development dependency -- it is not designated as one by the one referencing it. So how could the same package appear as one and not as one in the same dependency graph?

If you can code against a package, then that is counter to the development dependency understanding

Sure, if you're only thinking about the content of the package itself (excluding its respective dependencies). But then, if that made sense, the dev dependency package wouldn't include any content for the compile asset anyway. So if it doesn't (because we all agree that makes sense), then why block doing what no one will do, in such a way as to also block its dependencies from doing what it wants them to do?

Maybe a concept of related packages/package groups is appropriate here?

If you're asking for a concrete example, here's one:
CsWin32 is a source generator package, and the code it emits often ends up with a dependency on the System.Memory package. We therefore want the consuming project to reference System.Memory.dll as well as consume the source generator.
If the project is itself packed into a nuget package, we do not want CsWin32 to show up as a nuget package dependency, but we do want System.Memory to show up as a nuget package dependency (including its compile and runtime assets).

Today, this compiles fine, since it includes the System.Memory reference to the compiler:

    <PackageReference Include="Microsoft.Windows.CsWin32" Version="0.1.422-beta">
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>

But unfortunately the packed result omits the System.Memory package dependency. And of course as the issue description describes, the IDE like to add this metadata when the reference is added:

<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>

And this breaks even the compilation.

So what we want (in this concrete example) is a way for System.Memory to survive as a dependency even without the dev dependency intermediary, and for compile in be in the IncludeAssets metadata (or for the metadata to be removed).

[AraHaan]

Perhaps the way of using source generator packages should change,

Perhaps something like this could work for source generators then:

<SourceGeneratorReference Include="SourceGeneratorName" Version="Version" />

And then any dependencies of them are then automatically marked as runtime dependencies, note the fact that IncludeAssets would and must not be enforced on this then so then source generators can be special cased, without actually breaking anything (then the way SourceGeneratorReference works would be similar to PackageReference except that it would ignore the Development Dependency bit for explicit references added to the nuspec of the source generator and assume it's a runtime dependency on actual generated code, fixing this issue then.

[chsienki]

Tagging in @jmarolf as he's been experimenting with some potential solutions to this problem.

Unfortunately analyzers and NuGet have never played particularly well together and this has been really exacerbated with the addition of source generators. It's not really a conscious decision, but just an outcome of the two teams working at very different levels in the stack and there being impedance mismatches across those levels.

I suspect we need some 'joined up thinking' from both teams to really solve this rather than just patching holes as they come up.

[nkolev92]

I suspect we need some 'joined up thinking' from both teams to really solve this rather than just patching holes as they come up.

I agree. There are some pending designs for analyzers that may extend to source generators.
I think using dev dependency is doing the best that was possible with the currently available features, but that's not ideal in some of these scenarios.

[HavenDV]

Fody/Equals#356
A similar problem also applies to all projects from Fody

[ltrzesniewski]

To clarify: Fody weaver packages include a library which provides code that is needed to configure the desired weaving. The references to the library are then removed by Fody during its processing.

These packages can't set developmentDependency to true since it removes compile from IncludeAssets, and the library would become unusable.

I know it's a very specific use case, but ideally we'd like to have a way to tell NuGet to only add PrivateAssets="all" to the PackageReference, without changing IncludeAssets. Currently, we emit a warning which asks the user to do so themselves.

[jeffward01]

Perhaps the way of using source generator packages should change,

Perhaps something like this could work for source generators then:

<SourceGeneratorReference Include="SourceGeneratorName" Version="Version" />

And then any dependencies of them are then automatically marked as runtime dependencies, note the fact that IncludeAssets would and must not be enforced on this then so then source generators can be special cased, without actually breaking anything (then the way SourceGeneratorReference works would be similar to PackageReference except that it would ignore the Development Dependency bit for explicit references added to the nuspec of the source generator and assume it's a runtime dependency on actual generated code, fixing this issue then.

I think this is a good solution - any updates on this topic?