[System.Text.Json] Consider removing `RequiresUnreferenceCode`/`RequiresDynamicCode` annotations from certain `JsonSerializer` APIs

[eiriktsarpalis]

Today all JsonSerializer methods accepting JsonSerializerOptions are marked as RequiresUnreferencedCode/RequiresDynamicCode on account of their behavior in a couple of cases:

1. If no JsonSerializerOptions is specified, it will be defaulted to JsonSerializerOptions.Default.
2. If a JsonSerializerOptions is specified that doesn't explicitly set a TypeInfoResolver, then that field will be silently populated using JsonSerializerOptions.Default.TypeInfoResolver.

The pervasiveness of the annotations in some of the most commonly used APIs in STJ is creating substantial usability issues for library and Native AOT application authors, who often have to come up with elaborate workarounds to evade warnings.

What appears to be true for both cases is that code paths accessing unreferenced/dynamic code are guarded by the JsonSerializer.IsRefectionEnabledByDefault feature switch:

1. Here's the JsonSerializerOptions.Default constructor and
2. here is the TypeInfoResolver populating behavior.

It seems to me that this protection should suffice to remove the annotation; the IsRefectionEnabledByDefault feature switch is turned off by default for every application setting PublishTrimmed to true. Granted, a trimmed/Native AOT application could explicitly turn on IsRefectionEnabledByDefault but this is usually a case of misconfiguration. In any case, this is the tactic that has been employed by ASP.NET core since .NET 8 and one that just got rolled out in Microsoft.Extensions.AI so it makes sense that this a tried approach that we could try moving down the stack.

[eiriktsarpalis]

cc @eerhardt @stephentoub @sbomer

[dotnet-policy-service]

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

[eiriktsarpalis]

Here's a draft of the proposed change: #108782

Adding just three suppressions removed the bulk of the RUC/RDC annotations from the STJ API surface.

[eerhardt]

We've discussed this in the past, and the reasoning of leaving these attributes as-is is to inform developers that their code is going to break when trimming/native AOT'ing. They should be using the STJ source generator and passing in a JsonTypeInfo instead. Without these attributes, the Roslyn analyzers and the trimming tools can't do a good job of pointing you to where your code is going to break.

The ASP.NET and Microsoft.Extensions.AI cases are in the minority here. Adding a suppressions in those cases is (marginally) acceptable, since they have mechanisms for adding the JsonTypeInfo globally - once for the entire app.

cc @vitek-karas @MichalStrehovsky @agocke

[eiriktsarpalis]

the reasoning of leaving these attributes as-is is to inform developers that their code is going to break when trimming/native AOT'ing.

Depends on what is meant by "break" in this case. There are two failure modes present when contemplating these APIs:

1. throwing an exception because the contract for a type could not be found and
2. nondeterministic runtime behaviour related to trimmed members or dynamic code generation.

Generally speaking we want to annotate APIs that open up the possibility of 2) happening but not 1). But because we're guarding all unsafe code with IsRefectionEnabledByDefault checks only 1) is possible in the context of AOT (unless IsRefectionEnabledByDefault has been explicitly enabled by the user).

They should be using the STJ source generator and passing in a JsonTypeInfo instead.

This is precisely where the usability issues come in. Any scenario looking to customize serialization beyond defining a basic source generated context will have to craft their own JsonSerializerOptions and extracting a JsonTypeInfo from that has its own challenges. We need to improve things which will come either by introducing new API or making a change like the one proposed here.

Adding a suppressions in those cases is (marginally) acceptable, since they have mechanisms for adding the JsonTypeInfo globally - once for the entire app.

Not sure I entirely follow -- all three (ASP.NET, M.E.AI and STJ) define a singleton that is used by components if the user-supplied JSO is left unconfigured.

[eerhardt]

Depends on what is meant by "break" in this case.

The whole point of trimming warnings is to "shift left" the experience from runtime to build-time. These APIs aren't guaranteed to work the same before and after trimming (they obviously don't work the same), so they have warnings on them when you call them. The IsRefectionEnabledByDefault feature switch is in a supportive role here. The switch isn't the primary way of indicating behaviorial differences.

We want build/publish-time warnings to be what drives the experience, not runtime errors. For ASP.NET 8, we had to concede that we couldn't keep to this principle, so we did the next best thing we could. We shouldn't be sacrificing the principle in other, lower areas like System.Text.Json.

We need to improve things which will come either by #74492 or making a change like the one proposed here.

My opinion is that we introduce a new API.

[MichalStrehovsky]

+100 to what @eerhardt said

Feature switches are kind of last ditch effort to make a legacy API "trim friendly". Whatever is behind the switch is not actually trim friendly, only sort of. It will break the same at F5 debug time and deploy time if you're an app developer, so it's at least consistent, but it's not trim friendly.

If you're developing a library, you're never going to see the behavior with the switch defined. Not in unit testing, not when E2E testing, never (unless you go out of your way to add a PublishTrimmed project). You'll put IsTrimmable in your library, nothing gets flagged, and you ship it. Actual end users who have PublishTrimmed in their project will in an ideal be the first person to see the behavior with the feature switch enabled. In the non-ideal case, the person to first see it is the end user.

Feature switches are actively harmful to our "no warnings means no trouble at runtime" effort. We added them as an escape hatch, but ended up with too many of them. We should not rely on them for trim correctness. If we were able to introduce a new API that is 100% safe without any feature switches, the old API should be "deprecated" by marking it trim/AOT unfriendly.

[MichalStrehovsky]

System.Text.Json devs should still have in fresh memory the fiasco from nullable reflection APIs that were also "trim friendly", except if you enabled a feature switch they weren't. Nothing got flagged anywhere because the theory was "you get the same behavior with F5 debug and deployment time, so what's the problem" and then we had a firedrill in integration tests because S.T.Json took advantage of them and Blazor enabled the feature switch. Feature switches are horrible, we should strive not to expose customers to them.