Disable JsonSerializer.IsReflectionEnabledByDefault on PublishTrimmed=true

[eiriktsarpalis]

Have we decided we want this to be defaulted to off only when PublishAot=true? Or do we think we should go as far as turning it off by default when PublishTrimmed=true?

For the ASP.NET feature switch, we had it for both. But I'm a little concerned about making the switch for PublishTrimmed at the bottom of the stack. It is a breaking change. If we do this, we will need to update Maui and Blazor WASM SDKs to turn it back on.

We don't need to block this PR on the decision. For ASP.NET, we can just switch our SDK to change EnsureAspNetCoreJsonTrimmability => JsonSerializerIsReflectionEnabledByDefault once this is merged, and can react as changes happen.

Originally posted by @eerhardt in #83844 (comment)

[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

---

Have we decided we want this to be defaulted to off only when PublishAot=true? Or do we think we should go as far as turning it off by default when PublishTrimmed=true?

For the ASP.NET feature switch, we had it for both. But I'm a little concerned about making the switch for PublishTrimmed at the bottom of the stack. It is a breaking change. If we do this, we will need to update Maui and Blazor WASM SDKs to turn it back on.

We don't need to block this PR on the decision. For ASP.NET, we can just switch our SDK to change EnsureAspNetCoreJsonTrimmability => JsonSerializerIsReflectionEnabledByDefault once this is merged, and can react as changes happen.

Originally posted by @eerhardt in #83844 (comment)

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

[eerhardt]

FYI @Redth @PureWeen @jonathanpeppers @SteveSandersonMS @javiercn @marek-safar @lewing @SamMonoRT @steveisok - (please also tag anyone else you'd think would be interested from a MAUI or Blazor WASM stand point).

We are considering disabling Reflection in System.Text.Json by default in a PublishTrimmed=true application. This will affect all app models that are using trimming today. If we disable it, we will need to update MAUI and Blazor WASM SDKs to turn the reflection feature switch back on in STJ. Tagging you as app model owners that may have feedback on this plan.

[Sergio0694]

I assume this would also affect WASDK (WinUI 3) apps?

[eerhardt]

I assume this would also affect WASDK (WinUI 3) apps?

Yes, if you set PublishTrimmed=true in those apps.

[Sergio0694]

Not sure if this point has already been raised (probably?), but are there concerns about code potentially breaking due to this change and surprising developers? Is the assumption that if you enable trimming you'd already get all trimming warnings anyway, and in order to fix those you would already have to switch to code paths that are guaranteed to be safe when this flag is disabled? As in, just so that people won't encounter unexpected issues that only come up in published builds and not in normal ones? 🤔

I guess what I'm trying to say is that for people wanting to publish their apps with trimming, my expectations would be that this flag should always be disabled to ensure they get a consistent behavior even for normal F5 builds. Is that just not doable and relying on trimming warnings alone is sufficient?

[eerhardt]

See also the discussion here dotnet/sdk#31626 (comment), which explains more of the reasoning why we should turn off Reflection in System.Text.Json when PublishTrimmed=true is set.

[jonathanpeppers]

What project types needs the setting on vs off? If these need it on: wasm, iOS + 3 other Apples, Android.

Do only the platforms running Mono need to keep it on?

[jkotas]

What project types needs the setting on vs off?

Projects that use the fragile <TrimMode>partial</TrimMode> by default. Can we enable it by default when <TrimMode>partial</TrimMode> is set instead of touching each of the affected app-model SDKs?

Do only the platforms running Mono need to keep it on?

This is unrelated to the runtime flavor used.

[eerhardt]

Can we enable it by default when partial is set instead of touching each of the affected app-model SDKs?

That’s a good idea. I would go with the policy based on TrimMode == full though. So when PublishTrimmed == true (which it is for PublishAot as well) && TrimMode == full -> default JsonSerializer.IsReflectionEnabledByDefault = false.

[jkotas]

I would go with the policy based on TrimMode == full though.

Why would it be better option? TrimMode == full is the default for .NET 7+, so empty TrimMode is treated same way as explicitly setting TrimMode to full.

[jkotas]

I guess it can make a difference for <TrimMode>copyused</TrimMode>. We want to keep the reflection fallback enabled for `copyused as well. So it makes sense to base the policy on TrimMode == full, but it needs to be done after we fill in the TrimMode default.

[eerhardt]

Right now TrimMode doesn't get defaulted until the PrepareForILLink MSBuild target:

runtime/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

Lines 217 to 221 in 563408a

	<!-- Set up TrimMode and TrimmerDefaultAction. -->
	<PropertyGroup>
	<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '6.0'))">copyused</TrimMode>
	<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionEquals($(TargetFrameworkVersion), '6.0'))">partial</TrimMode>
	<TrimMode Condition="'$(TrimMode)' == ''">full</TrimMode>

This means to make the above rule based on TrimMode == full, we would also need to check for TrimMode == ''.

However, it looks like the iOS SDK sets its TrimMode in a MSBuild Target:

https://github.com/xamarin/xamarin-macios/blob/f853788a06fe3fef0d0d97a739e175bd2d60f3cd/dotnet/targets/Xamarin.Shared.Sdk.targets#L454-L458.

This means the above rule won't be possible to make without iOS reacting to it. cc @rolfbjarne.

We might be saved that PublishTrimmed also doesn't get defaulted in iOS until a target as well:

https://github.com/xamarin/xamarin-macios/blob/f853788a06fe3fef0d0d97a739e175bd2d60f3cd/dotnet/targets/Xamarin.Shared.Sdk.targets#L261-L265

So our logic of PublishTrimmed == true wouldn't kick in since we would put that in a PropertyGroup which will run before all Targets.

[rolfbjarne]

Right now TrimMode doesn't get defaulted until the PrepareForILLink MSBuild target:

runtime/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets

Lines 217 to 221 in 563408a

	<!-- Set up TrimMode and TrimmerDefaultAction. -->
	<PropertyGroup>
	<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '6.0'))">copyused</TrimMode>
	<TrimMode Condition="'$(TrimMode)' == '' And $([MSBuild]::VersionEquals($(TargetFrameworkVersion), '6.0'))">partial</TrimMode>
	<TrimMode Condition="'$(TrimMode)' == ''">full</TrimMode>

This means to make the above rule based on TrimMode == full, we would also need to check for TrimMode == ''.

However, it looks like the iOS SDK sets its TrimMode in a MSBuild Target:

xamarin/xamarin-macios@f853788/dotnet/targets/Xamarin.Shared.Sdk.targets#L454-L458.

This means the above rule won't be possible to make without iOS reacting to it. cc @rolfbjarne.

We might be saved that PublishTrimmed also doesn't get defaulted in iOS until a target as well:

xamarin/xamarin-macios@f853788/dotnet/targets/Xamarin.Shared.Sdk.targets#L261-L265

So our logic of PublishTrimmed == true wouldn't kick in since we would put that in a PropertyGroup which will run before all Targets.

This could be solved if we set JsonSerializerIsReflectionEnabledByDefault=true for iOS before Microsoft.NET.Sdk.targets is imported (from the discussion it seems we always want the reflection fallback enabled for MAUI targets, so this seems like what we'd want anyway?)

[javiercn]

From the Blazor Webassembly perspective, as long as we can turn it on in the SDK, I think we should be ok.

Reflection support is more important for customers apps and third-party libraries that might be making use of it than the framework. I believe in most cases the framework avoids reflection for interop, except for the JS interop mechanism that apps can use to talk to JS from Blazor. That, we don't have control of.

[jkotas]

long as we can turn it on in the SDK, I think we should be ok.

When you turn it on in the SDK, please make it conditional such the app .csproj can disable it again by overriding the default to get reliable behavior with trimming.

We get a steady stream of issues where people are hitting problems due to bad interaction of System.Text.Json with trimming and AOT (for example, #74141). Our default answer has been to use source generators to get reliable behavior with trimming. Our new default answer should be: use source generator and set JsonSerializerIsReflectionEnabledByDefault to false.

[akoeplinger]

Let's say we turn this off by default for MAUI apps as well, how big of a breaking change do we think it'd be? If the only reliable way is using source generators and JsonSerializerIsReflectionEnabledByDefault=false then maybe we should nudge people into that direction.

[eerhardt]

Let's say we turn this off by default for MAUI apps as well, how big of a breaking change do we think it'd be?

It would break every call to JsonSerializer that isn't using the source generator (which is probably most of them). The reason MAUI and Blazor WASM apps work today is because we don't trim user assemblies, by default. So if someone serializes a simple class with just primitive properties, it "happens" to work. This strategy breaks very easily though, like in the example @jkotas gave above (and numerous other examples as well).

[javiercn]

I agree with @eerhardt. Enabling this feature is not reasonable for Blazor Webassembly apps, as it will cause an ecosystem breaking change that would force every existing library to update their codebase.

In addition to that, for Blazor, we don't give people control over the JSON serialization settings when we are doing JS interop, so there is no way for them to configure this, and is something that we are keen on avoiding, as this contract needs to be implemented both in .NET and JS for interop to work, so we don't want people changing it and breaking framework functionality in subtle/obscure ways.

[eiriktsarpalis]

@eerhardt seems we can close this?

[eerhardt]

@eerhardt seems we can close this?

Have we settled on a rule yet? I don’t think this can be closed until it gets defaulted for all apps in some form (either AOT or trimmed). I’ve only enabled this for aspnet apps in order to replace aspnet’s temporary feature switch.

[eiriktsarpalis]

FYI @Redth @PureWeen @jonathanpeppers @SteveSandersonMS @javiercn @marek-safar @lewing @SamMonoRT @steveisok - (please also tag anyone else you'd think would be interested from a MAUI or Blazor WASM stand point).

FYI we are planning on turning this after Preview 6 development completes.

[ghost]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.