Invalid trimming suppression in AIJsonUtilities

[eerhardt]

We have the following trimming suppression:

extensions/src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.cs

Lines 193 to 198 in 6487428

	#if !NET9_0_OR_GREATER
	[UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access",
	Justification = "Pre STJ-9 schema extraction can fail with a runtime exception if certain reflection metadata have been trimmed. " +
	"The exception message will guide users to turn off 'IlcTrimMetadata' which resolves all issues.")]
	#endif
	private static JsonElement GetJsonSchemaCore(JsonSerializerOptions options, FunctionParameterKey key)

This is an incorrect way to address trimming warnings. The intention of the warnings is so developers now upfront what does and what doesn't work when the build and publish a trimmed or native AOT'd app. Suppressing warnings like this minimizes the principle of "if you don't get warnings during publish, your app is guaranteed to work the same as before you published".

We should remove this suppression and either make the underlying code trim-compatible, or we should annotate this method as RequiresUnreferencedCode.

cc @eiriktsarpalis

[eiriktsarpalis]

It's something we debated when the polyfill got introduced. Ultimately, the desire to avoid viral annotations prevailed. cc @stephentoub

[stephentoub]

Suppressing warnings like this minimizes the principle of "if you don't get warnings during publish, your app is guaranteed to work the same as before you published".

But not suppressing harms the experience for everyone else, by forcing APIs to be surfaced in particularly non-ergonomic ways. It's a balancing act. In this case, you get an exception the moment you hit this in your app that tells you exactly what the problem is and how to fix it.

[eiriktsarpalis]

In this case, you get an exception the moment you hit this in your app that tells you exactly what the problem is and how to fix it.

The runtime exceptions have actually been eliminated with some clever use of reflection Eric proposed. The issue now only manifests as missing custom attributes (nullability annotations, description attributes). The workaround to disable IlcTrimMetadata remains though.

[stephentoub]

The issue now only manifests as missing custom attributes (nullability annotations, description attributes).

Then even more so I'm happier suppressing here, since the impact is minimal but the cost of not suppressing like this is high, IIRC. Though maybe I'm merging cases in my head. Remind me what the impact would be in this case of marking this RUC?

Regardless, it sounds like the justification is now wrong and should be fixed.

[eerhardt]

But not suppressing harms the experience for everyone else, by forcing APIs to be surfaced in particularly non-ergonomic ways.

The same APIs can be surfaced in the same way. You would just need to flow the [RequiresUnreferencedCode] annotations up to all callers of this method for the non-.NET 9 build.

The runtime exceptions have actually been eliminated

This is part of why the suppressions are bad. The static analysis is disabled, so it can't help you anymore.

The workaround to disable IlcTrimMetadata remains though.

@agocke @MichalStrehovsky - I don't see any documentation on that switch on learn.microsoft.com.

[stephentoub]

The same APIs can be surfaced in the same way. You would just need to flow the [RequiresUnreferencedCode] annotations up to all callers of this method for the non-.NET 9 build.

Yes, and then to allow the functionality to be used, you need to expose other APIs that complicate the surface area. At which point you need to re-evaluate everything about how it's exposed. When it's a big issue, this makes sense. When it's minor, it's a gray area, in my opinion.

The runtime exceptions have actually been eliminated

This is part of why the suppressions are bad. The static analysis is disabled, so it can't help you anymore.

How would [RequiresUnreferencedCode] help with that? If the underlying reason it's been annotated becomes safer, are you alerted to the annotation no longer being necessary?

[eerhardt]

Yes, and then to allow the functionality to be used, you need to expose other APIs that complicate the surface area.

How does suppressing the warning change this? Why don't we need these other APIs now?

How would [RequiresUnreferencedCode] help with that?

It turns this case from a possible false-negative into a possible false-postive, which is more desirable. It is more desirable to get a warning, when in fact it is fine, than it is to not get a warning for something that is going to break.

[stephentoub]

Yes, and then to allow the functionality to be used, you need to expose other APIs that complicate the surface area.

How does suppressing the warning change this? Why don't we need these other APIs now?

If we propagate the [RUC] all the way up to public surface area, then for example this needs to show up on AIFunctionFactory.Create, which optionally accepts an AIFunctionFactoryCreateOptions, which optionally contains a JsonSerializerOptions. If that Create is [RUC], then to make the whole factory usable with trimming / NativeAOT, we can't expose it like that, and instead need some other API that forces you to provide a JSO when creating an AIFunctionFactoryCreateOptions and forces you to supply an AIFunctionFactoryCreateOptions to Create.

It turns this case from a possible false-negative into a possible false-postive, which is more desirable.

Right, you're still disabling static analysis, and if something changes, you may never know. For significant issues, yes, knowing about the possibility of an issue is more desirable. For a minor issue, I don't believe that's law. There are similar gray areas everywhere: you accept a JsonSerializerOptions, but the JSO doesn't contain the type information necessary, everything compiles fine, no analyzer warnings, no publish errors, and you may or may not blow up at run-time depending on how the application is written to look for and consume the type information from the JSO.

[eerhardt]

Here's all the public API that would need to be annotated 2b17dff

[eerhardt]

If that Create is [RUC], then to make the whole factory usable with trimming / NativeAOT, we can't expose it like that

Note it is only [RUC] on pre-.NET 9 builds, where you aren't using System.Text.Json v9.

[eiriktsarpalis]

Here's all the public API that would need to be annotated 2b17dff

The problem with these annotations is that they're not actionable. There is no alternative API users can call that gets rid of it. They have to either propagate the annotation on their code or suppress it.

[eerhardt]

AFAICT - these are the 2 places that are problematic:

extensions/src/Shared/JsonSchemaExporter/JsonSchemaExporter.cs

Lines 261 to 276 in 6487428

	Func<JsonPropertyInfo, ParameterInfo?>? parameterInfoMapper =
	ReflectionHelpers.ResolveJsonConstructorParameterMapper(typeInfo.Type, typeInfo);
	state.PushSchemaNode(JsonSchemaConstants.PropertiesPropertyName);
	foreach (JsonPropertyInfo property in typeInfo.Properties)
	{
	if (property is { Get: null, Set: null } or { IsExtensionData: true })
	{
	continue; // Skip JsonIgnored properties and extension data
	}
	JsonNumberHandling? propertyNumberHandling = property.NumberHandling ?? effectiveNumberHandling;
	JsonTypeInfo propertyTypeInfo = typeInfo.Options.GetTypeInfo(property.PropertyType);
	// Resolve the attribute provider for the property.
	ICustomAttributeProvider? attributeProvider = ReflectionHelpers.ResolveAttributeProvider(typeInfo.Type, property);

Doing a visual inspection, I don't see any runtime error that will happen if either the constructors (1st case) or the property/field metadata (2nd case) are trimmed. So what will happen is that when you try using these APIs in a trimmed .NET 8 app, you can get a different JSON schema depending on what reflection info was available. If no one else is reflecting on the constructor or properties, the metadata won't be there, and the JSON schema will be missing that information. The property attributes appear to be used for nullability and description info, as @eiriktsarpalis points out above. The constructor is used to know which ctor parameter maps to which property.

This is the bad scenario - your app "kinda works" like it did before trimming. But it works in a different way now, because the JSON schema is different. There is no way for you to know that your app is going to behave differently other than by exercising every line after trimming.

The problem with these annotations is that they're not actionable. There is no alternative API users can call that gets rid of it. They have to either propagate the annotation on their code or suppress it.

The action would be to target net9.0 if you want to trim/native AOT your app. We can add that to the RUC message. The above commit was just to show which APIs would need to be annotated.

[eiriktsarpalis]

Related to eiriktsarpalis/stj-schema-mapper#7

[eiriktsarpalis]

Doing a visual inspection, I don't see any runtime error that will happen if either the constructors (1st case) or the property/field metadata (2nd case) are trimmed. So what will happen is that when you try using these APIs in a trimmed .NET 8 app, you can get a different JSON schema depending on what reflection info was available.

Your analysis is correct. In practice, this translates to three specific issues:

1. Non-nullable properties are reported as nullable,
2. Required properties do not map to required schemas,
3. DescriptionAttribute specified on constructor parameters will not be incorporated in the schema.

[MichalStrehovsky]

We guarantee to users that either there will be trimming warnings, or the app will behave the same before/after trimming. If you suppress a warning and this doesn't hold, the suppression is simply a bug. It would be better to do nothing (no suppression, just turn off the trimming analyzer on the AIJsonUtilities project).

We don't ask users to re-test the entire app after trimming. By suppressing a warning you put them into a position where they would need to re-test entire app every time they publish. Minor changes in completely unrelated parts of the codebase could make it so that this throws or doesn't throw. Whether something is visible from reflection or isn't can be affected by butterfly effects in unrelated parts of the app.

@agocke @MichalStrehovsky - I don't see any documentation on that switch on learn.microsoft.com.

That's because it's an undocumented, and unsupported switch. We only have it due to testing in the dotnet/runtime repo and trying to phase it out (dotnet/runtime#91774). There's no intention to ship it. It doesn't "solve" trimming.