Support multi-targeting for Roslyn components

[eerhardt]

Background

As Roslyn implements more features and APIs, the need to target these features/APIs increases in Roslyn components (i.e. Code Analyzers/Fixers and Source Generators). As the components need to target newer versions of Roslyn, it is becoming clear that some components need to target multiple versions of Roslyn at the same time.

Take, for example, the following issues in dotnet/runtime with the System.Text.Json source generator:

• Very sluggish VS 2019 and VS 2022 performance starting with DotNet 6 Preview 5 related to Source Generators runtime#56702
• Logging Source Generator fails to compile due to CS8795 error with file-scoped namespaces runtime#57880

In both of these issues, the only fix is for the System.Text.Json source generator to take a dependency on version 4.0 of the Microsoft.CodeAnalysis assemblies. The first needs IIncrementalGenerator and the second needs FileScopedNamespaceDeclarationSyntax, both of which were only introduced in 4.0.

However, once a Roslyn component references a version of Microsoft.CodeAnalysis, that Roslyn component can no longer be loaded in an earlier version of the compiler.

This poses two problems:

1. If a Roslyn component still wants to work in a previous version of the compiler, for example in Visual Studio 2019, the component has a problem. It needs to pick which version it targets - either older versions without the new API support, or the new version.
2. If a Roslyn component chooses to target the new version, there is no way to shut off the component when loaded in older versions. Thus, when a developer in VS 2019 references a NuGet package with a component targeting Roslyn 4.0, the developer gets a warning in their build:

Warning CS8032 An instance of analyzer System.Text.Json.SourceGeneration.JsonSourceGenerator cannot be created from C:\Users\eerhardt\.nuget\packages\system.text.json\7.0.0-dev\analyzers\dotnet\cs\System.Text.Json.SourceGeneration.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.

This is not a great experience for our customers.

Proposal

We will update the current logic that resolves the Analyzers from NuGet packages to support an enhanced analyzers folder structure. The enhanced folder structure will allow for the Roslyn version targeted by the component to be expressed. This will allow for multiple versions to be targeted in a single package, as well as being able to express a "minimal" version. This will address the two problems above. When the Analyzers are resolved from the NuGet package, the correct asset(s) will be resolved according to which version of the compiler will be used to compile the assembly.

Current folder structure

The following structure is recognized today by the SDK:

  analyzers[\dotnet][\cs|vb]\component.dll

Proposed folder structure

This proposal adds a new, optional folder to the structure: roslyn{version}:

  analyzers[\dotnet][\roslyn{version}][\cs|vb]\component.dll

Where {version} is the {major}.{minor} version of the Microsoft.CodeAnalysis assemblies used during the Compile task.

When a NuGet package contains folders under analyzers with the pattern, roslyn<major-version>.<minor-version>, the folder with the highest version that is less than or equal to the current Microsoft.CodeAnalysis <major>.<minor> version will be used. This works the same as TFMs work under the lib folder. Except instead of TargetFramework versions being evaluated, the Roslyn version will be evaluated.

Also note, since the same Analyzer logic exists in NuGet's ResolveNuGetPackageAssets Task. Thus, we will need to update the NuGet MSBuild logic with this support. That way developers using non-SDK .NET projects will get the same Analyzers selected as SDK projects.

Earlier SDK versions

Since this proposal is to add support to .NET SDK 6.0, we need to consider what happens when NuGet packages that follow this structure are used in SDK versions before 6.0.

If implemented as indicated above, NuGet packages that support multiple Roslyn versions will have all of their assets loaded in earlier versions of the SDK. To prevent that, we can add a new MSBuild property to the SDK that indicates it supports multi-targeting Roslyn versions.

$(SupportsRoslynComponentVersioning)

NuGet packages can include MSBuild logic that "lights up" when $(SupportsRoslynComponentVersioning) is not present. This indicates that the built-in support isn't there to select the right version. The NuGet package needs to do the selection itself. Typically, NuGet packages will select the component that targets the lowest version of Roslyn version it supports, since newer Roslyn versions can't be guaranteed. Another option would be to opt out of adding a Roslyn component all together.

Additional rules

• When a package contains analyzer assets both inside a roslyn{version} folder and outside, ex. directly under analyzers\dotnet\cs\analyzer.dll, both assets will be selected. The reasoning is that the assets outside of roslyn{version} folders are considered to work everywhere. Existing analyzer resolution rules add both assets inside and outside {language} folders, if present. This follows the same reasoning.
  • If a NuGet package wants to express a component that should be used when none of the roslyn{version} folders apply, it can add that asset to roslyn1.0.

Alternative Solutions

An alternative is to follow the proposal in dotnet/roslyn#54108:

The current plan is to provide authoring and consumption targets that make this easy to do correctly. Over time we'll aim to get NuGet itself updated to support the feature in-box and remove the need for the targets.

Following this approach would mean that each NuGet package that wants to support multiple versions of Roslyn (or a minimum version) would need to ship special MSBuild .targets files with duplicated logic in them.

This approach is not ideal because:

1. If there is a bug in the targets, it needs to be fixed in all NuGet packages that copied the targets file.
2. There could be potential conflicts between the targets in the NuGet packages if they are not named uniquely.
3. The logic to resolve the Roslyn version shouldn't be the responsibility of the NuGet packages that ship Roslyn components.

@chsienki @jaredpar @dsplaisted @jasonmalinowski @jmarolf

[KalleOlaviNiemitalo]

Would the analyzer build primarily run inner builds for the Roslyn versions, and then each of those would have a single target framework?

• Roslyn v. X with framework A
• Roslyn v. Y with framework A
• Roslyn v. Z with framework B

[eerhardt]

I think the authoring experience needs a separate proposal. At least, I didn't give any thought to the authoring experience above. When we do an authoring experience, I would imagine that we would run inner builds for each target Roslyn version the component indicates.

With regards to the framework - I believe all Roslyn components should be targeting netstandard2.0 for now, since it will need to be loaded in both .NET Framework and on .NET 5+ (Core). At this point, I am not proposing to add the Roslyn component's target framework to the folder structure. (Or were you referring to the TFM of the project being analyzed? If so, I'd also consider that a separate proposal that I'm not covering here.)

[KalleOlaviNiemitalo]

I meant, if a later version of Roslyn has APIs that cannot be used in analyzer projects that target netstandard2.0. But I suppose this won't happen as long as Visual Studio needs to be able to load Roslyn on .NET Framework.

[jaredpar]

I meant, if a later version of Roslyn has APIs that cannot be used in analyzer projects that target netstandard2.0

The Roslyn API does not currently differentiate between netstandard2.0 and .NET Core. There are also no plans to do so. Too much of our own infrastructure runs on both .NET Core and Framework that it would very negatively impact our own tooling if we did this.

[KalleOlaviNiemitalo]

Visual Studio 2017 includes Roslyn version-2.10.0, in which csc.exe targets net46 and Microsoft.CodeAnalysis.dll targets netstandard1.3. I wrongly assumed that analyzers intended to be compatible with this version would likewise have to target netstandard1.3, which would complicate the multitargeting. However, now that I try it, an analyzer targeting netstandard2.0 works OK for warnings in Intellisense, for builds in the VS IDE, and for running csc.exe in a terminal. I see Visual Studio 2017 system requirements include .NET Framework 4.7.2, which I think installs netstandard.dll version 2 and thus satisfies the reference at run time.

[eerhardt]

I spent some time implementing a prototype of this proposal. Please take a look if you are interested: eerhardt@c26ae1e.

The drawback is that since the Analyzer asset selection is also duplicated in NuGet.BuildTasks, we would need to duplicate this logic there. Or, since we are keying off the MSBuild property $(SupportsRoslynComponentVersioning) in our NuGet packages, if NuGet.BuildTasks doesn't want to implement this behavior, the NuGet package's logic to select the lowest Roslyn version would kick in, and anyone using non-SDK projects would get the "v1" source generator.

[steveharter]

As the components need to target newer versions of Roslyn, it is becoming clear that some components need to target multiple versions of Roslyn at the same time.

@eerhardt is it an assumption here that we need to support VS 2019 for the inbox generators? i.e. if we only support VS 2021 for inbox generators what (if anything) is necessary?

[jaredpar]

Why are we using the Roslyn version here instead of the .NET SDK version? The version of Roslyn is tied to the .NET SDK version and our overall customer message is around .NET SDK, not about individual component versions. This would seem to be another place where we should lean into that and use the SDK version vs. teaching customers about individual component versions.

Note: scenarios like Microsoft.Net.Compilers.Toolset shouldn't be considered here. Those aren't supported scenarios .

[eerhardt]

is it an assumption here that we need to support VS 2019 for the inbox generators? i.e. if we only support VS 2021 for inbox generators what (if anything) is necessary?

When the Logging and STJ source generators are used from the net6.0 "ref-pack", we don't need to support VS 2019. net6.0 is only supported in VS 2022.

However, both of these source generators also ship in the corresponding NuGet packages. These NuGet packages are supported on netstandard2.0 and above TFMs, which are supported in VS 2019. So we need to define some sort of behavior for what happens when developers use the 6.0 NuGet packages on netstandard2.0. Options I see are:

1. Do nothing. If a developer uses the 6.0 NuGet in VS 2019, they get the source gen warning. At least this way they know the source generator won't work
2. Turn the SG off in VS 2019. They don't get a warning. But the generator doesn't run either. So at best their code doesn't compile. At worst in the logging case, the code compiles, but it doesn't log at runtime (since the partial methods aren't filled).
3. This proposal: "multi-target" the SGs so that in VS 2019 they get a version that works, but may not be the best performing. In VS 2022 they get the latest and greatest version.

Why are we using the Roslyn version here instead of the .NET SDK version?

Partially because there are scenarios where the .NET SDK isn't involved. For example, PackageReference in a non-SDK, .NET Framework .csproj.

But mostly because this is the version of Microsoft.CodeAnalysis the component took a dependency on. It would make a lot more sense to tell component authors to match up the version of their Microsoft.CodeAnalysis reference to the folder name in their NuGet package:

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="3.8.0" />
  </ItemGroup>

    <None Include="..\InterfaceStubGenerator.Roslyn38\bin\$(Configuration)\netstandard2.0\InterfaceStubGeneratorV1.dll"
          PackagePath="analyzers\roslyn3.8\cs"
          Pack="true"
          Visible="false" />

The two versions match. If we used the .NET SDK version in the PackagePath, component authors would need to do this mapping themselves.

The version of Roslyn is tied to the .NET SDK version and our overall customer message is around .NET SDK, not about individual component versions. This would seem to be another place where we should lean into that and use the SDK version vs. teaching customers about individual component versions.

Turning the question back around: why isn't the version of Microsoft.CodeAnalysis tied to the .NET SDK version?

[chsienki]

@jaredpar

Why are we using the Roslyn version here instead of the .NET SDK version?

We spoke about this pretty early on in the design that you, me and @jmarolf talked about. There are I think a couple of compelling reasons not to use the SDK version:

• There are potentially multiple SDK versions for a given Roslyn version, and we'd need to somehow handle that mapping or educate users that they need to select the lowest version of the SDK that a particular Roslyn shipped in
• Generally users want to know that it'll work on the command line and in VS. Using the SDK would require the user knowing another mapping of what VS version supports that.
• As @eerhardt notes above its odd to reference one version, then have to select a different one (we could potentially alleviate this with a custom SDK or something that handles both, but that is more work)
• I think the most compelling reason is that we actively list our APIs by which roslyn version they ship in see: https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.generatorinitializationcontext.registerforpostinitialization?view=roslyn-dotnet-3.11.0#applies-to We either have to update the documentation to list the SDK version, or again, ask users to somehow map that back.

I think given the fairly limited expected audience that are going to do this, along with the fact that they really want to reason about roslyn itself, we should use the API version here. In the future if we want to expand it out to more users we can go the SDK / targets path where we still use it but as an implementation detail hidden from the user.

[jaredpar]

Turning the question back around: why isn't the version of Microsoft.CodeAnalysis tied to the .NET SDK version?

The compiler ships in a number of products of which the .NET SDK is not the most significant driver. The .NET SDK though for a given version is tied to a version of Roslyn. That is the explicit design of the SDK.

There are I think a couple of compelling reasons not to use the SDK version:

Gotcha.

Given that we're changing the loading behavior here to make it more customizable, should we consider also allowing the user to customize based on the runtime the compiler will execute on? Essentially should we allow customers to deploy analyzers / generators that are multi-targeted to .NET Framework and .NET Core?

That has been a semi-frequent ask of customers. I've been skeptical that it alone was enough to warrant changing our loading rules. But if we're already changing them is it cheap / timely to solve that problem too?

[jmarolf]

Essentially should we allow customers to deploy analyzers / generators that are multi-targeted to .NET Framework and .NET Core?

I think whatever design we pick for multi-targeting roslyn apis needs to be able to accommodate multi-targeting based on runtime in the future. Though that does ask some wild questions. Is it NxM number of builds that we need to do for TFM and roslyn api version?

[chsienki]

I think runtime targeting is a nice to have, but it isn't blocking the inbox generators right now. I think we should keep this scoped to just the Roslyn version (given how far through the cycle we are) and look at doing runtime multitargeting in the NET 7 timeframe. I think the designs naturally stack together, so we don't have to do both at once.

[jmarolf]

totally agree we should not design framework multi-targeting now. Just want to make sure the design we come up with will not make it impossible in the future

[dsplaisted]

I have some reservations about this.

We’ve said that targeting .NET 6 in VS 2019 is not supported, and we know there are ways it breaks. I know System.Text.Json supports other target frameworks, but it seems reasonable to me that if you want to use the System.Text.Json library that ships with .NET 6, you would have the same tooling requirements as .NET 6 itself.

It also seems like adding this multi-targeting logic to the .NET 6 SDK doesn’t actually help with multi-targeting between VS 2019 and VS 2022, because VS 2019 doesn’t have the multi-targeting logic. So to get the scenario to work you would still need custom logic in .targets files in the NuGet packages. Adding analyzer multi-targeting support to .NET 6 might help for future versions of Roslyn, but even then the spec currently says that the multi-targeting will be based on the major/minor versions of the Microsoft.CodeAnalysis DLLs. Since an incomplete version of C# 10 is shipping in VS 16.11, wouldn’t that mean that even if we had the multitargeting logic there, the C# 10 versions of the analyzers / source generators would be picked up, and they might or might not work at all?

I would also prefer to avoid adding more analyzer-convention-logic to the .NET SDK. The right place for that logic is in NuGet, which would then write the analyzers which applied to the project to the assets file for the SDK to consume: NuGet/Home#6279

I know there are probably other scenarios for multi-targeting analyzers and source generators besides just System.Text.Json, but that seems to be the reason we would need to do this now. I’d prefer to first figure out how to get NuGet to handle the analyzers, and then as part of that we could add support for multi-targeting.

What do you think?

[eerhardt]

it seems reasonable to me that if you want to use the System.Text.Json library that ships with .NET 6, you would have the same tooling requirements as .NET 6 itself.

To me, this isn't a reasonable approach because System.Text.Json also ships as a NuGet package. This allows developers to use the new functionality without forcing them to move to .NET 6. That's the whole reason we ship it in a NuGet package and support older TFMs.
Imagine the scenario where you have a .NET Framework application that uses System.Text.Json v5.0 NuGet package. We ship the new STJ v6.0 NuGet package. Visual Studio is going to lead you to update your NuGet package references to the latest versions (there is an "Updates" section in the NuGet package manager UI). If you click that button, you now get a warning in your build that "the System.Text.Json source generator could not be loaded".

There is a big difference between targeting .NET 6 and updating NuGet package dependencies. It's reasonable to say "If you want to target .NET 6, use VS 2022". I don't think it is reasonable to say "if you want to update this NuGet package version, you need to also update your VS".

It also seems like adding this multi-targeting logic to the .NET 6 SDK doesn’t actually help with multi-targeting between VS 2019 and VS 2022, because VS 2019 doesn’t have the multi-targeting logic. So to get the scenario to work you would still need custom logic in .targets files in the NuGet packages

Agreed that it doesn't fully solve multi-targeting between VS 2019 and VS 2022. We will still need to ship custom logic in our NuGet packages. But that custom logic can be much simpler if this feature is in the .NET 6 SDK.

If this feature is in the .NET 6 SDK, the custom NuGet logic would logically look like:

if ('$(SupportsRoslynComponentVersioning)' != 'true')
{
    <Analyzer Include="select the analyzer that references the lowest Roslyn version" />
}

Because if the NuGet package is being used in an environment that doesn't have $(SupportsRoslynComponentVersioning), we can just use the component that works with the oldest Roslyn.

But if this feature isn't in the .NET 6 SDK, the custom NuGet logic needs to get much more complicated. It basically needs to do the multi-targeting logic itself: figure out the Roslyn version, select the most appropriate analyzer assembly, etc.

the spec currently says that the multi-targeting will be based on the major/minor versions of the Microsoft.CodeAnalysis DLLs. Since an incomplete version of C# 10 is shipping in VS 16.11, wouldn’t that mean that even if we had the multitargeting logic there, the C# 10 versions of the analyzers / source generators would be picked up, and they might or might not work at all?

I'm not familiar with "an incomplete version of C# 10 is shipping in VS 16.11", but my understanding is that the Microsoft.CodeAnalysis version in VS 16.11 will always be 3.11.*. This proposal is for the Roslyn components to be selected based on which version of Microsoft.CodeAnalysis they reference. Not based on the LangVersion.

I would also prefer to avoid adding more analyzer-convention-logic to the .NET SDK

I agree that this isn't the ideal long-term location for this logic, but this is the place that is doing the language (cs vs. vb) selection logic. Given that logic lives here, if we were to implement this proposal, it makes the most sense to put this new multi-target logic in the same place as the language selection logic.

The right place for that logic is in NuGet

I'm not sure I'm convinced of that myself. Having NuGet need to understand all of the conventions used in NuGet packages is a limiting design IMO. We have proposals all the time for new conventions, and forcing NuGet to understand all of them forces too much burden in the package manager.

(aside: extensions to .NET Interactive are loaded based on a convention based location in the NuGet package. I don't think NuGet should need to understand them.)

IMO, the ideal long-term location for this logic would be in the Compiler/Roslyn targets. Roslyn owns the scenario of why people put /analyzers in their NuGet packages. Roslyn should be the one that defines the conventions used in the NuGet package.

I know there are probably other scenarios for multi-targeting analyzers and source generators besides just System.Text.Json

dotnet/runtime has 2 source generators that both would use this functionality: System.Text.Json and Microsoft.Extensions.Logging.

There is also reactiveui/refit#1216 which has already done its own multi-targeting for the same reasons as the dotnet/runtime generators.

[dsplaisted]

I don't want to block you from going ahead with this if you think it's important and your team is willing to own the implementation and possible bug tail.

I do want to comment on some of your points:

I don't think it is reasonable to say "if you want to update this NuGet package version, you need to also update your VS".

To me, it depends on the NuGet package. In this case the NuGet package depends on tooling features. If source generators were an entirely new feature in C# 10, then we wouldn't be trying to make them work with older versions of the compiler. We'd try to mitigate the experience issues where NuGet offers you a package update which won't work with your project.

If you are using older releases of the tooling, or targeting older versions of the Framework, then you will get security fixes (if what you're using / targeting is still supported), but you aren't guaranteed to be able to use new features or new libraries. If you can, that's great, but I don't think we should jump through hoops to enable it.

The right place for that logic is in NuGet

I supposed that if an asset doesn't interact with the dependency graph, then it would be OK to implement the convention outside of NuGet. But if you want to control how an asset flows across edges in the dependency graph (ie via PrivateAssets or ExcludeAssets), then I think NuGet really needs to be the one handling it.

It's been a long-standing bug that PrivateAssets and ExcludeAssets don't work with analyzers, but given that we haven't fixed it yet maybe it's not important. On the other hand maybe it is going to get more and more important now that analyzers and especially source generators are getting very popular.

[eerhardt]

Fixed by #20793.