[release/6.0] Multi-target Json and Logger Source Generators between Roslyn v3.11 and v4.0 (#58446)

[eerhardt]

Backport of #58446 to release/6.0

/cc @eerhardt

Customer Impact

With the latest 6.0 NuGet packages of System.Text.Json and Microsoft.Extensions.Logging.Abstractions, if a developer is using VS 2019 and upgrades their <PackageReference to the 6.0.0-rc1 versions, they start seeing build warnings:

Warning CS8032 An instance of analyzer System.Text.Json.SourceGeneration.JsonSourceGenerator cannot be created from C:\Users\eerhardt\.nuget\packages\system.text.json\6.0.0-rc1\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 confusing to the customer. I've even seen reports of customers trying to reference Microsoft.CodeAnalysis, Version=4.0.0.0 to fix the issue, which won't help.

This fix changes our source generators to target both Roslyn versions (VS 2019 and VS 2022), and selects the correct asset based on which version of the tooling is being used.

Testing

I have tested in both VS 2019 update 11 and VS 2022 that the correct source generator is picked from the NuGet package.

Risk

We run the risk of the wrong (or both) source generators getting selected and causing design time issues. The source generators could also cause performance issues (especially in VS 2019). To mitigate this risk, I've added a "disable" switch for our NuGet package source generators. Customers can disable these source generators if they are causing problems.

Original commit message

• Multi-target LoggerMessageGenerator between Roslyn v3.11 and v4.0

• Include a .targets file in NuGet packages which will select the correct analyzer assembly depending on which Roslyn version will be used to compile.

• Multi-target JsonSourceGenerator between Roslyn v3.11 and v4.0

• Fix restore

• Update NuGet package MSBuild logic to detect when SupportsRoslynComponentVersioning is not available, and use the lowest analyzer available.

• Handle non-SDK projects by running after ResolveNuGetPackageAssets

• Respond to PR feedback

• Name .cs and .csproj files with Roslyn in the name
• Upgrade to Roslyn 3.11 so IsExplicitlyNamedTupleElement API is available
• Fix some references to the test projects
• Fix incremental pack of the analyzer targets

[danmoseley]

Approved. As previously discussed, this is necessary to get acceptable perf in VS2022 and avoid breaking customers in VS2019.

[danmoseley]

Someone should review to make sure all the right stuff is ported.
@ericstj are you comfortable with this also?

[ericstj]

Looks reasonable to me and I support this change. I would like someone from the crew involved in reviewing the diff if it is not a simple cherry-pick.

[ericstj]

Not blocking, but one clarification of the way this works is only for this package's analyzers, it does not have any effect if the analyzer from the shared framework would win. It might make sense to put package in the name of the property but that gets a little long. I guess if we document them we should make it clear it only applies to package.

[eerhardt]

it does not have any effect if the analyzer from the shared framework would win

For the analyzers in the shared fx ref-pack, I was relying on dotnet/roslyn#55518 to provide this functionality, if needed.

I wouldn't object to putting Package in the property name, if we think that is the best option.

DisableSystemTextJsonSourceGenerator => DisableSystemTextJsonPackageSourceGenerator

It expresses intent.

Do you have a strong opinion either way?

[ericstj]

nit: Not for this PR but maybe 7.0. It seems unusual to special case this centrally. I would have made this property public and let the project define what it wants this to be called.

[eerhardt]

I will make this change in main.

[danmoseley]

No need to rush this, we can take to tactics if it doesn't make it in in time. I'm confident that it will be approved.

[eerhardt]

if it is not a simple cherry-pick.

it wasn't 😞. And I already have a merge conflict with other JSON source generator changes. So yes getting an extra review would help.

[danmoseley]

@lewing the Windows all configs build failed with some ?timing issue: "2021-09-15T01:03:58.1379129Z CSC : error CS2012: Cannot open 'D:\a_work\2\s\artifacts\obj\WasmAppBuilder\Debug\net472\WasmAppBuilder.dll' for writing -- 'The process cannot access the file 'D:\a_work\2\s\artifacts\obj\WasmAppBuilder\Debug\net472\WasmAppBuilder.dll' because it is being used by another process.' [D:\a_work\2\s\src\tasks\WasmAppBuilder\WasmAppBuilder.csproj]"

[eerhardt]

the Windows all configs build failed

I also see similar errors here: #58904 (comment)

cc @radical

[danmoseley]

Oh - nuts - I didn't notice the extra review didn't happen. @eerhardt we can revert if you like, or ask folks to review after the fact, as you choose.

[eerhardt]

the extra review didn't happen.

I did a line-by-line comparison between the main PR and this. I didn't find any differences. I'd be glad if someone else would do the diff as well, just in case I missed something, but I don't think we need to revert this.

@ericstj - if you think we should change the MSBuild property name, let me know and I can make a PR for it.

[ericstj]

I'm OK with it. If we do document it, make it clear in the docs.