Add nullable reference type annotations to System.Text.Json source gen

[eiriktsarpalis]

NRE's have been a frequent occurrence when making changes to the STJ source generator. This PR enables nullable reference type annotations to the STJ source generator components. Because source generator projects need to target netstandard2.0, this is done by adding a new project, System.Text.Json.SourceGenerator.NetCoreApp that targets net8.0 as an unused class library.

Additionally, this PR makes the sourcegen test suite target the Roslyn v4.4 instead of v4.0. The changes have been factored into a separate commit fe78570 for easier reviewing.

These infrastructural changes are in anticipation of implementing #58770, which needs to make use of APIs only available in Roslyn v4.4.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

NRE's have been a frequent occurrence when making changes to the STJ source generator. This PR enables nullable reference type annotations to the STJ source generator components. Because source generator projects need to target netstandard2.0, this is done by adding a new project, System.Text.Json.SourceGenerator.NetCoreApp that targets net8.0 as an unused class library.

Additionally, this PR makes the sourcegen test suite target the Roslyn v4.4 instead of v4.0. The changes have been factored into a separate commit fe78570 for easier reviewing.

Author:	eiriktsarpalis
Assignees:	eiriktsarpalis
Labels:	area-System.Text.Json
Milestone:	-

[ViktorHofer]

We should use msbuild's multi-targeting feature here which was designed for such use-cases. I don't think that introducing new projects per target framework for source generators is scalable. Every project adds to the overall build graph and restore time.

Because source generator projects need to target netstandard2.0, this is done by adding a new project, System.Text.Json.SourceGenerator.NetCoreApp that targets net8.0 as an unused class library.

Why can't you multi-target the existing roslyn4.4 project and add net8.0 there? Did you hit this error?

runtime/src/libraries/Directory.Build.targets

Lines 234 to 235 in ea44671

	<Error Condition="'%(_AnalyzerPackFile.TargetFramework)' != 'netstandard2.0'"
	Text="Analyzers must only target netstandard2.0 since they run in the compiler which targets netstandard2.0. The following files were found to target '%(_AnalyzerPackFile.TargetFramework)': @(_AnalyzerPackFile)" />

. That's our own validation that can be changed to make multi-targeting possible (of course without shipping the assembly).

[eiriktsarpalis]

Why can't you multi-target the existing roslyn4.4 project and add net8.0 there? Did you hit this error?

Multi-targeting the sourcegen project themselves produced a bit of msbuild weirdness which I tried to address, but ultimately I couldn't fix it. That particular error you're referencing wasn't hit actually, it was primarily complaining that v8 version of System.Runtime could not be found (I'm missing precise error logs right now).

I should add that the Directory.Build.targets logic can be evaded by defining the AnalyzerRoslynVersion property conditionally only for netstandard2.0. This doesn't solve the problem though -- it would seem that multi-targeting and source generators don't work together well.

[ViktorHofer]

I think we should make multi-targeting work here. I can take a look tomorrow morning.

[ViktorHofer]

@eiriktsarpalis I just pushed a commit to your branch to allow our source-generator projects to multi-target. Note that this comes with a number of pitfalls:

• With a lot of experimentation, I got targeting the live shared framework working but this is fragile to say the least. I.e. I had to reference a ton of shim projects like System.Runtime.Extensions as the Roslyn nuget packages often compile against netcoreapp3.1 (and these assemblies still contain references to these now shim assemblies). Ideally, we would ask Roslyn to include the very latest TFM in their packages to avoid these unnecessary shim project builds.
• Now that a source generators targets the live shared framework, we suddenly need to think about layering which wasn't a big concern with the prebuilt .NET Standard TFM. While today, we have a split between reference and source projects, we eventually want to eliminate that which would implicate that a source generator's dependency must not use that source generator in the corresponding source project. Also, source generators outside of the shared framework must not be referenced from inside the shared framework as that would cause a circular dependency in the graph.

Before merging, I would like @ericstj to review my changes as this is an entirely new approach with possible unknown implications and/or regressions.

[eiriktsarpalis]

With a lot of experimentation, I got targeting the live shared framework working but this is fragile to say the least. I.e. I had to reference a ton of shim projects like System.Runtime.Extensions as the Roslyn nuget packages often compile against netcoreapp3.1 (and these assemblies still contain references to these now shim assemblies). Ideally, we would ask Roslyn to include the very latest TFM in their packages to avoid these unnecessary shim project builds.

Would using a standalone project be more reliable in that case? It achieves the same end goal with the inconvenience of one placeholder project being included in the solution.

[ViktorHofer]

Would using a standalone project be more reliable in that case? It achieves the same end goal with the inconvenience of one placeholder project being included in the solution.

No, that doesn't make any difference. It's mostly about using the live shared framework vs. using a prebuilt. Using the live shared framework is a good goal to have but slightly more work. I would hope that with time, .NETCoreApp targeting source generators will become more natural.

[krwq]

.cs files changes look good to me

[ViktorHofer]

The PR is now ready and I added some documentation around multi-targeting source generators and what to watch out for (when consuming a live built source generator).

[layomia]

This is awesome.

[eiriktsarpalis]

Test failures are unrelated.