Annotate for trim/AOT (in)compatibility

[sbomer]

Adds annotations to propagate trim and AOT compatibility warnings to callers. This way, a trimmed/AOT-compiled app referencing Newtonsoft.Json will get warnings at the places where it calls into Newtonsoft.Json, instead of warnings that point at Newtonsoft.Json internals.

Adds a net8.0 target, because the AOT analyzer requires net7.0+.

I'm not setting up a test app because I don't want to be responsible for some ad-hoc infrastructure or script that does the validation - but I did check this change against a test app locally. I think relying on the analyzer is good enough in this case because the goal isn't to make the code trim-compatible, but to surface existing trim incompatibilities earlier in the development cycle for consumers. If we do miss some warnings it's not a big problem since the problems will still be caught when publishing an app, as they are today.

@eerhardt @agocke @MichalStrehovsky @jtschuster

[JamesNK]

Why add these annotations to types like JsonTextReader? I don't think it uses reflection or other problem APIs.

Same with reader, Newtonsoft.Json.Linq types, etc.

[sbomer]

My approach was to annotate warnings at the type level. I'm working under the assumption that we want to direct folks away from using Newtonsoft.Json entirely in trimmed apps. If we think there's value in keeping some part of Newtonsoft.Json trim-compatible, I can look into making the annotations more granular.

The one on JsonTextReader came from references to various other types where I took the same approach. For example, one path is:

• JsonTextReader derives from JsonReader, which has some usages of JsonTypeReflector, which has some code that calls into trim-incompatible ComponentModel APIs.

[JamesNK]

I would like to tell people not to use anything to do with the serializer in AOT. But some Newtonsoft.Json things work and don't need warnings on them.

• Reader/writer should be fine in AOT.
• Almost all of JObject, JArray, JValue, etc should almost completely work. Those types have an ToObject<T>() method that invokes the serializer and should be annotated individually.

[sbomer]

Updated with more granular annotations. This time I annotated methods instead of types when possible, only bubbling up to the type level when there were warnings in an implementation/override of a virtual method, where I didn't want to annotate the base/interface virtual.

I kept most of JObject, etc. trim-compatible by introducing some feature switches that can be set to change out the implementation of some methods to throw. For example, see JToken.ToString which calls the annotated JToken.WriteTo.

[sbomer]

A couple notes:

• There's no build logic to disable the new feature switches by default in trimmed apps, so those callsites will produce trim warnings by default.
• This is using the feature switch attributes added in .NET 9, so disabling the features in an app will be possible only when the app targets .NET 9 or above.

[JamesNK]

Sorry, it's been a while since I've thought about trimming and AOT. I don't know what the feature switches mean. Maybe we should have a call sometime and you can talk through this PR and show me.

[eerhardt]

My suggestion on these names is to name them like public .NET APIs. That way if we ever needed to expose this as a public property, we could match the signature to the name.

For example https://github.com/dotnet/runtime/blob/128fdfd1fd982e277e969d5c7ca1710345f4f649/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs#L27

[sbomer]

I struggled at first to find a good place to put them, but looking again maybe JToken is an ok place, although it makes the feature names a little more unwieldy. Fixed.

[eerhardt]

Should this (and similar) be:

Suggested change

	#if HAVE_APPCONTEXT
	if (!MiscellaneousUtils.DynamicIsSupported)
	#if HAVE_DYNAMIC
	if (!MiscellaneousUtils.DynamicIsSupported)

?

[sbomer]

This is already inside a #if HAVE_DYNAMIC - the #if HAVE_APPCONTEXT check is there because I only defined the feature switches under #if HAVE_APPCONTEXT (since they call TryGetSwitch).

[eerhardt]

I don't think these feature switches are going to work like you want them to. What you are doing is disabling the static analysis at build time, so users don't get warnings up front. But they will still get warnings at publish time. And their app will fail at runtime becuase no one defaults ComponentModelIsSupported = false in the build.

[sbomer]

The behavior you described is what I'm going for - I want to prevent users from getting static analysis warnings (just for calls to those particular methods where I'm introducing the feature switch checks) and make that a publish warning and runtime failure instead. That's in response to the feedback that some parts of Newtonsoft.Json should be left "trim-compatible".

If the switches were defined in dotnet/runtime I would add a default that disables the switches in trimmed apps, but we don't have a great model for doing so in third-party libraries (I didn't want to add props/targets that ship as part of Newtonsoft.Json). At least this makes it possible to set the switches manually via RuntimeHostConfigurationOption.

I think that is acceptable because it's an improvement over the current state where Newtonsoft.Json internals will produce warnings and unpredictable runtime failures. With these changes you'll get better warnings, and the option of making the failure modes more predictable via the RuntimeHostConfigurationOption. But overall the goal is to guide people away from using the trim-incompatible parts of Newtonsoft.Json in the first place.

[sbomer]

It's the same idea as the approach I took in dotnet/runtime#107713