Add tests for disabling JsonSerializer.IsReflectionEnabledByDefault on PublishTrimmed=true

[eiriktsarpalis]

Fix dotnet/runtime#84378

cc @eerhardt @rolfbjarne @jonathanpeppers

[eerhardt]

The current feature switches that get set when PublishTrimmed=true is actually contained in dotnet/runtime:

https://github.com/dotnet/runtime/blob/d52b3038cd19aab5b95be4f1c76010b91dd07429/src/tools/illink/src/ILLink.Tasks/build/Microsoft.NET.ILLink.targets#L34-L47

Thoughts on putting this one there instead to keep them all in one place?

I think the reason they are in dotnet/runtime is because they version with the runtime version. So if you target net7.0, you get a different set than if you target net8.0.

cc @sbomer @vitek-karas

[eiriktsarpalis]

Does Microsoft.NET.ILLink.targets get included when you run build targets?

[sbomer]

Yes - the import is here:

sdk/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

Line 1265 in d3607fb

<Import Project="$(ILLinkTargetsPath)" Condition="'$(ILLinkTargetsPath)' != '' and '$(Language)' != 'C++'" />

[eiriktsarpalis]

One more question -- can we set up testing in runtime or should I just make the change there and merge this PR once the sdk repo has absorbed the changes?

[eiriktsarpalis]

Given dotnet/runtime#88480 was merged I've updated this PR to only include the tests. Will merge once an up-to-date built is absorbed and tests are passing.

[eerhardt]

Can you remove this line as part of this change?

sdk/src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets

Lines 49 to 52 in 9a98699

	<!-- Default settings for trimmed and aot'd apps. -->
	<PropertyGroup Condition="'$(PublishTrimmed)' == 'true' Or '$(PublishAot)' == 'true'">
	<JsonSerializerIsReflectionEnabledByDefault Condition="'$(JsonSerializerIsReflectionEnabledByDefault)' == ''">false</JsonSerializerIsReflectionEnabledByDefault>
	</PropertyGroup>

[eiriktsarpalis]

I could -- just to confirm, is it guaranteed PublishAot == true always implies PublishTrimmed == true?

[eerhardt]

I could -- just to confirm, is it guaranteed PublishAot == true always implies PublishTrimmed == true?

Currently, yes I believe this is true. @MichalStrehovsky and @agocke can confirm. If we ever change that, we would change all the feature switches we default when PublishTrimmed=true to also check for PublishAot.

[vitek-karas]

I could -- just to confirm, is it guaranteed PublishAot == true always implies PublishTrimmed == true

Currently it probably is... but please don't rely on it. We're discussing setting PublishAot=true even on mono AOTed apps, and I think mono has some cases where it can do AOT without trimming, in which case it should probably not set PublishTrimmed=true. Regardless, you should not make that assumption anyway. If the feature is incompatible with trimming (for example has RequiresUnreferencedCode somewhere) and independently also incompatible with AOT (for example has RequiresDynamicCode somewhere) then you should condition it on both.

[eiriktsarpalis]

@vitek-karas this suggests to me that we should update Microsoft.NET.ILLink.targets in runtime to also condition the feature switch on PublishAot == true. @eerhardt thoughts?

[vitek-karas]

I think the answer is "yes it should be", but the question is where. Currently the ILLink targets only handle trimming. We have a different set of targets for AOT. So maybe this should also be in the AOT targets as well. It would then probably also belong here: https://github.com/dotnet/runtime/blob/36481bfd079093fe7d29ef024b1fb801e6c787fe/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L36C7-L42
@MichalStrehovsky ?

[eerhardt]

I'm fine with defaulting JsonSerializerIsReflectionEnabledByDefault to false for both PublishTrimmed and PublishAot. This switch affects both trimming and AOT - it guards code that is both RequiresUnreferencedCode and RequiresDynamicCode.

[eerhardt]

LGTM