Fix STJ support for System.Reflection.NullabilityInfoContext.IsSupported = false

[eiriktsarpalis]

Fix #102848

[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.

[jkotas]

I do not think that this is the right fix.

I think the right fix is to throw exception when RespectNullableAnnotations = true and the nullability annotations are trimmed.

[MichalStrehovsky]

I think I know the answer, but just to double check - when using the source generator, this runtime reflection doesn't happen, right? (I.e. wasm is only running into this because it's using trimming in a way that we do not recommend to anyone.)

[eiriktsarpalis]

I do not think that this is the right fix.

I think the right fix is to throw exception when RespectNullableAnnotations = true and the nullability annotations are trimmed.

It's something I considered, although ultimately I settled on the conclusion that it's common for reflection to misreport metadata in the context of trimmed applications. If anything, the current throwing behavior of NullabilityInfoContext is inconsistent with how the rest of reflection works in trimmed apps.

I think I know the answer, but just to double check - when using the source generator, this runtime reflection doesn't happen, right?

That is correct, which is why I'm not particularly concerned about this corner case. There will have been warnings.

[MichalStrehovsky]

It's something I considered, although ultimately I settled on the conclusion that it's common for reflection to misreport metadata in the context of trimmed applications. If anything, the current throwing behavior of NullabilityInfoContext is inconsistent with how the rest of reflection works in trimmed apps.

The current NullabilityInfoContaxt feature switch is simply a bad design. We knew that it would be bad when it was introduced (#55860 (comment)). I was not involved in any of the discussions besides what I posted so I don't know what tradeoff forced going with it anyway, despite being bad.

That is correct, which is why I'm not particularly concerned about this corner case. There will have been warnings.

There will be no warnings: Blazor WASM that hit this turns them off (that's why people are not guided towards using the source generator in the first place). If we are choosing between an exception and silent different behavior, we should choose the exception be default and only very exceptionally silent different behaviors.

[eiriktsarpalis]

OK, I've pushed a change that reinstates the exception when the user has opted into enforcement.

[eiriktsarpalis]

/backport to release/9.0-preview5

[github-actions]

Started backporting to release/9.0-preview5: https://github.com/dotnet/runtime/actions/runs/9302780301