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?